msdosfs and small sectors

[Maxime Villard]

Hi,
some days ago when reading msdosfs_vfsops.c I saw this:

if ((error = bread(devvp, 0, secsize, NOCRED, 0, &bp)) != 0)
goto error_exit;
bsp = (union bootsector *)bp->b_data;
b33 = (struct byte_bpb33 *)bsp->bs33.bsBPB;
b50 = (struct byte_bpb50 *)bsp->bs50.bsBPB;
b710 = (struct byte_bpb710 *)bsp->bs710.bsBPB;

'secsize' is retrieved through getdisksize(), via an ioctl on the device.

I have a doubt, isn't there a risk that the kernel overflows memory if
secsize is too low? If I plug an USB key with only 2 bytes per sector, only
two bytes will be read by this bread(), and 'bp->b_data' will be accessed
outside the requested area.

Not sure though, does someone have an idea? If I'm right, which limit
should we put?

Thanks,
Maxime

[Martin Husemann]

Post by Maxime Villard
'secsize' is retrieved through getdisksize(), via an ioctl on the device.

force it to be 512 bytes minimum?

Martin

[Maxime Villard]

Post by Martin Husemann

Post by Maxime Villard
'secsize' is retrieved through getdisksize(), via an ioctl on the device.

force it to be 512 bytes minimum?
Martin

I thought about that. I haven't found a clear spec on this, but it is
implicitly suggested that 512 is the minimal size (from what I've seen
here and there). And the smallest BytesPerSec allowed for fat devices
is 512. But still, nothing really clear.

Index: msdosfs_vfsops.c
===================================================================
RCS file: /cvsroot/src/sys/fs/msdosfs/msdosfs_vfsops.c,v
retrieving revision 1.113
diff -u -r1.113 msdosfs_vfsops.c
--- msdosfs_vfsops.c 15 Jul 2014 11:43:54 -0000 1.113
+++ msdosfs_vfsops.c 16 Jul 2014 12:58:52 -0000
@@ -493,6 +493,13 @@
psize = 0;
error = 0;
}
+ if (secsize < DEV_BSIZE) {
+#ifdef DIAGNOSTIC
+ printf("Invalid block secsize %d\n", secsize);
+#endif
+ error = EINVAL;
+ goto error_exit;
+ }

if (argp->flags & MSDOSFSMNT_GEMDOSFS) {
if (secsize != GEMDOSFS_BSIZE) {

[Greg Troxel]

Post by Maxime Villard

Post by Martin Husemann

Post by Maxime Villard
'secsize' is retrieved through getdisksize(), via an ioctl on the device.

force it to be 512 bytes minimum?
Martin

I thought about that. I haven't found a clear spec on this, but it is
implicitly suggested that 512 is the minimal size (from what I've seen
here and there). And the smallest BytesPerSec allowed for fat devices
is 512. But still, nothing really clear.
Index: msdosfs_vfsops.c
===================================================================
RCS file: /cvsroot/src/sys/fs/msdosfs/msdosfs_vfsops.c,v
retrieving revision 1.113
diff -u -r1.113 msdosfs_vfsops.c
--- msdosfs_vfsops.c 15 Jul 2014 11:43:54 -0000 1.113
+++ msdosfs_vfsops.c 16 Jul 2014 12:58:52 -0000
@@ -493,6 +493,13 @@
psize = 0;
error = 0;
}
+ if (secsize < DEV_BSIZE) {
+#ifdef DIAGNOSTIC
+ printf("Invalid block secsize %d\n", secsize);
+#endif
+ error = EINVAL;
+ goto error_exit;
+ }
if (argp->flags & MSDOSFSMNT_GEMDOSFS) {
if (secsize != GEMDOSFS_BSIZE) {

That's an abuse of DIAGNOSTIC. It should only be used to control
whether to assert on things that cannot happen. (Bad data on a mount
point is not a software invariant.)

I don't object to having an error message, but it might be nice to have
the device included, and to say that it's in msdosfs_mount.

I also agree that a value < 512 is almost certainly wrong and it's fine
to error out until there's a valid use case and spec saying smaller is
ok.

[matthew green]

Post by Greg Troxel

Post by Maxime Villard
+ if (secsize < DEV_BSIZE) {
+#ifdef DIAGNOSTIC
+ printf("Invalid block secsize %d\n", secsize);
+#endif
+ error = EINVAL;
+ goto error_exit;
+ }
if (argp->flags & MSDOSFSMNT_GEMDOSFS) {
if (secsize != GEMDOSFS_BSIZE) {

That's an abuse of DIAGNOSTIC. It should only be used to control
whether to assert on things that cannot happen. (Bad data on a mount
point is not a software invariant.)

agreed -- these sorts of messages should be under DEBUG.


.mrg.

[Maxime Villard]

Post by matthew green

Post by Greg Troxel

Post by Maxime Villard
+ if (secsize < DEV_BSIZE) {
+#ifdef DIAGNOSTIC
+ printf("Invalid block secsize %d\n", secsize);
+#endif
+ error = EINVAL;
+ goto error_exit;
+ }
if (argp->flags & MSDOSFSMNT_GEMDOSFS) {
if (secsize != GEMDOSFS_BSIZE) {

That's an abuse of DIAGNOSTIC. It should only be used to control
whether to assert on things that cannot happen. (Bad data on a mount
point is not a software invariant.)

agreed -- these sorts of messages should be under DEBUG.

I don't see what's wrong with that; on -current at least if it
breaks something people will immediately get a clear message, so
that if we someone says "I get invalid ..." we can quickly revert
the change.

I think my commit was clear on that:

"Put the printf under DIAGNOSTIC temporarily to see if someone complains."
^^^^^^^^^^^

[Greg Troxel]

Post by Maxime Villard

Post by matthew green

Post by Greg Troxel

Post by Maxime Villard
+ if (secsize < DEV_BSIZE) {
+#ifdef DIAGNOSTIC
+ printf("Invalid block secsize %d\n", secsize);
+#endif
+ error = EINVAL;
+ goto error_exit;
+ }
if (argp->flags & MSDOSFSMNT_GEMDOSFS) {
if (secsize != GEMDOSFS_BSIZE) {

That's an abuse of DIAGNOSTIC. It should only be used to control
whether to assert on things that cannot happen. (Bad data on a mount
point is not a software invariant.)

agreed -- these sorts of messages should be under DEBUG.

I don't see what's wrong with that; on -current at least if it
breaks something people will immediately get a clear message, so
that if we someone says "I get invalid ..." we can quickly revert
the change.
"Put the printf under DIAGNOSTIC temporarily to see if someone complains."
^^^^^^^^^^^

We are not objecting to the message. The rule for DIAGNOSTIC is that it
only adds asserts. So you are breaking that rule, and doing it for no
good reason. If you want to see if this hits and people see the mesage,
then make the printf unconditional, and put enough context in it so that
someone who isn't looking for this might figure it out.

Longer term, putting it under DEBUG makes sense, as users would be
flooded with messages for every.

So I really did mean to object to even a temporary misuses of
DIAGNOSTIC.

[David Holland]

Post by Maxime Villard
I thought about that. I haven't found a clear spec on this, but it is
implicitly suggested that 512 is the minimal size (from what I've seen
here and there). And the smallest BytesPerSec allowed for fat devices
is 512. But still, nothing really clear.

If you're afraid some real device might turn up with 128-byte sectors
or something, complain if it's less than 64. Or 32. It doesn't really
matter.

The error message should always print, though.

[Maxime Villard]

I've put 512. It's the least magic limit.

-----------------------------------------------------------------------------
Module Name: src
Committed By: maxv
Date: Wed Jul 16 20:09:00 UTC 2014

Modified Files:
src/sys/fs/msdosfs: msdosfs_vfsops.c

Log Message:
Limit the minimum size of a disk sector to 512 bytes, to prevent memory
overflow on extremely low secsize. This normally conforms to the old standard
(for which there doesn't seem to be a clear spec). Since 2011, IDEMA's Advanced
Format standardizes it to 4k, so this change won't cause any trouble on
new devices.

Put the printf under DIAGNOSTIC temporarily to see if someone complains.

after a quick discussion on tech-kern


To generate a diff of this commit:
cvs rdiff -u -r1.113 -r1.114 src/sys/fs/msdosfs/msdosfs_vfsops.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
-----------------------------------------------------------------------------

Thanks,
Maxime

[David Laight]

Post by David Holland

Post by Maxime Villard
I thought about that. I haven't found a clear spec on this, but it is
implicitly suggested that 512 is the minimal size (from what I've seen
here and there). And the smallest BytesPerSec allowed for fat devices
is 512. But still, nothing really clear.

If you're afraid some real device might turn up with 128-byte sectors
or something, complain if it's less than 64. Or 32. It doesn't really
matter.

Real floppies certainly had 128 byte sectors.
Some even had 128 byte ones on track 0 but 256 byte ones on the rest
of the disk!

Is there a check that the sector size is a power of two?
That might depend on where it comes from.

Real devices with 'unusual' sector sizes do exist (like audio CD),
but they won't have a FAT fs on them.
(and ICL system25 whcih wanted 100 byte sectors).

David

[Rhialto]

Post by David Laight
(and ICL system25 whcih wanted 100 byte sectors).

Or IBM's CMS which used to have 800 byte sectors.

-Olaf.

[Roger Brooks]

Post by Rhialto

Post by David Laight
(and ICL system25 whcih wanted 100 byte sectors).

Or IBM's CMS which used to have 800 byte sectors.
-Olaf.

And there were DEC disks which could be used on both DEC20 (where they
were formatted with 9-bit bytes) and VAXen (where they were formatted
with 8-bit bytes). If i remember correctly, this mean that while
sectors were 512 x 9-bit bytes on a DEC20, on a VAX the same disk would
have sectors of 576 x 8 bit bytes.


Roger