Re: overflow in libsa dosfs, feature for efiboot (patches provided)

[Rob Newberry <robthedude%mac.com@localhost>]

Thanks for your feedback!

>> Below are patches -- one to "dosfs.c" to fix the overflow problem,
>> one to "efiblock.c" to add the "deal with no disklabel" feature.  I
>> don't know the proper way to propose or advocate for these, but I'm
>> sharing them here in the hopes that they'll eventually make it in --
>> though surely after some feedback from others.  Thanks for any advice!
> 
> I think I dimly recall there was a magic way to just use the default
> device efiboot was loaded from w/o any partition magic - but I can't
> remember details. Maybe the non-disklabel part is not needed?

I don't think so, though I will admit I struggled for a long time trying to figure out how efiboot worked :-).

In order to correctly find a file, efi_block_parse needs not only a default device, but a default device with a partition on it.

And the code in efi_block_probe will only identify partitions if:

	- it's GPT (mine isn't)

	- it's MBR (mine is) and it's a NETBSD partition type (mine isn't)

Basically all my code does is add a the MSDOSFS partition to the list in the second case -- where there's no NetBSD partition (and also no disklabel).  However, once my code is in place, it is possible to specify this DOS partition as default, and then the following EFI boot commands work:

	dev hd0a
	initrd ramdisk.fs
	dtb dtbfile.dtb
	boot netbsd

> Can you try the below patch instead of your first one?

It thought it would work, but it didn't.

Specifically, when I got to line ~322:

		if ((err = ioread(f->fs, (c ? blkoff(f->fs, c) :
				secbyt(f->fs->lsndir)) + off,
			    buf, n)))
			goto out;

the code would fail for my disk (a 64GB SD card with a single FAT partition), which has these parameters at this point in the code:

	fs->lndta = 30470
	c = 1663485
	fs->bshift = 15

With those parameters, the blkoff macro (with your change) still returns 0xb1eb8c00, truncating the high bits.  The inline function returns the correct value of 0xcb1eb8c00.

I think the problem is that the blkbyt macro (used in the second part of the blkoff macro) is truncating to the u_int range before being added to the (casted up to uint64_t) portion in the blkoff macro.  (I'm not at all an expert with how the compiler handles automatic promotions/truncations/etc, which is why I turned to an inline function.)

With your change AND the following change to blkbyt, I was able to get thing to work with this disk:

	-#define blkbyt(fs, b)  ((b) << (fs)->bshift)
	+#define blkbyt(fs, b)  ((uint64_t)(b) << (fs)->bshift)

Does that make sense?  It's certainly a simpler set of patches to dosfs.c (though I have to swallow my angst over all the casting :-).

Thank you again for looking at this.

Rob