Date: Mon, 16 Nov 2015 02:29:09 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> Cc: freebsd-fs@freebsd.org Subject: Re: [Bug 127270] fsck_msdosfs(8) may crash if BytesPerSec is zero Message-ID: <20151116021615.X932@besplex.bde.org> In-Reply-To: <bug-127270-3630-F4tWNSF3Fx@https.bugs.freebsd.org/bugzilla/> References: <bug-127270-3630@https.bugs.freebsd.org/bugzilla/> <bug-127270-3630-F4tWNSF3Fx@https.bugs.freebsd.org/bugzilla/>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 15 Nov 2015 bugzilla-noreply@freebsd.org wrote: > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=127270 > > NGie Cooper <ngie@FreeBSD.org> changed: > > What |Removed |Added > ---------------------------------------------------------------------------- > Assignee|freebsd-fs@FreeBSD.org |ngie@FreeBSD.org > CC| |ngie@FreeBSD.org > > --- Comment #2 from NGie Cooper <ngie@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20151116021615.X932>