[Bug 127270] fsck_msdosfs(8) may crash if BytesPerSec is zero

[Bruce Evans]

On Sun, 15 Nov 2015 bugzilla-noreply at freebsd.org wrote:

> https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=127270
>
> NGie Cooper <ngie at FreeBSD.org> changed:
>
>           What    |Removed                     |Added
> ----------------------------------------------------------------------------
>           Assignee|freebsd-fs at FreeBSD.org      |ngie at FreeBSD.org
>                 CC|                            |ngie at FreeBSD.org
>
> --- Comment #2 from NGie Cooper <ngie at FreeBSD.org> ---
> This should avoid the divide by 0, but I'd need to verify that the behavior is
> correct:
> https://people.freebsd.org/~ngie/bug127270.patch
>
> This situation should occur if and when boot blocks 12 and 13 are 0, but there
> might need to be some additional conditions that need to be tripped in order
> for the divide by 0 to occur:
>
> 66         boot->bpbBytesPerSec = block[11] + (block[12] << 8);

This is fixed as a side effect of other fixes in my version.  (The sector
size needs to be read from the media.  Instead, a hard-coded sector size
or a larger size is used in all fsck utilities and kernel code.  Sometimes
this size is too small or too large to work.  ffs probes for some things
but not the size.)

X Index: boot.c
X ===================================================================
X RCS file: /home/ncvs/src/sbin/fsck_msdosfs/boot.c,v
X retrieving revision 1.6
X diff -u -2 -r1.6 boot.c
X --- boot.c	31 Jan 2008 13:16:29 -0000	1.6
X +++ boot.c	3 Jul 2010 16:53:33 -0000
X @@ -48,4 +48,6 @@
X  #include "fsutil.h"
X 
X +#define	IOSIZE	65536
X +
X  int
X  readboot(dosfs, boot)
X @@ -53,18 +55,48 @@
X  	struct bootblock *boot;
X  {
X +	u_char ioblock[IOSIZE];
X +	u_char iofsinfo[IOSIZE];
X +	u_char iobackup[IOSIZE];
X  	u_char block[DOSBOOTBLOCKSIZE];
X  	u_char fsinfo[2 * DOSBOOTBLOCKSIZE];
X  	u_char backup[DOSBOOTBLOCKSIZE];
X +	u_char *infop;
X +	size_t iosize;
X +	u_int secsize;
X  	int ret = FSOK;
X 
X -	if (read(dosfs, block, sizeof block) < sizeof block) {
X +	/* Search for an i/o size that works. */
X +	for (iosize = IOSIZE; iosize >= DOSBOOTBLOCKSIZE; iosize >>= 1) {
X +		if (lseek(dosfs, (off_t)0, SEEK_SET) == 0 &&
X +		    read(dosfs, ioblock, iosize) == (ssize_t)iosize)
X +			break;
X +	}
X +	if (iosize < DOSBOOTBLOCKSIZE) {
X  		perror("could not read boot block");
X  		return FSFATAL;
X  	}
X +	memcpy(block, ioblock, sizeof block);
X 
X -	if (block[510] != 0x55 || block[511] != 0xaa) {
X -		pfatal("Invalid signature in boot block: %02x%02x", block[511], block[510]);
X +	/*
X +	 * Preliminary decode to determine where the signature might be.
X +	 * It is supposed to be at the end of a 512-block, but we used to
X +	 * put it at the end of a sector.  Accept the latter so as to fix
X +	 * it someday.
X +	 */
X +	secsize = block[11] + (block[12] << 8);
X +	if (secsize < sizeof block || secsize > IOSIZE) {
X +		perror("Preposterous or unsupported sector size");
X  		return FSFATAL;
X  	}

A sector size of 0 and some other preposterous sizes are rejected here.
Sizes that are not multiples of 512 are still allowed.  The signature
checks weed out most garbage.

X +	if (block[510] != 0x55 || block[511] != 0xaa) {
X +		if (ioblock[secsize - 2] != 0x55 ||
X +		    ioblock[secsize - 1] != 0xaa) {
X +			pfatal("Invalid signature in boot block: %02x%02x",
X +			    block[511], block[510]);
X +			return FSFATAL;
X +		}
X +		pwarn(
X +	"Invalid primary signature in boot block -- using secondary\n");
X +	}
X 
X  	memset(boot, 0, sizeof *boot);
X [... remainder of patch not included]

Bruce