[Bug 243178] newfs_msdos: wrong FAT type determination

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=243178

--- Comment #5 from Damjan Jovanovic <[email protected]> ---
There are several problems here.

The code is from /usr/src/sbin/newfs_msdos/mkfs_msdos.c on FreeBSD 15.

=========
ISSUE 1
=========

The fundamental cause of this bug is that the FAT bitness selection code here:

    457     if (!fat) {
    458         if (...)
...
    468             fat = 12;
    469         else if (...)
...
    477             fat = 16;
    478         else
    479             fat = 32;
    480     }

wants to select the minimum FAT bitness that spans the whole volume.

This is in direct contradiction to the FAT bitness minimum cluster check later:

    593     if (cls < mincls(fat)) {
    594         warnx("%u clusters too few clusters for FAT%u, need %u", cls,
fat,
    595             mincls(fat));
    596         goto done;
    597     }

which cannot allow FAT32 to be below 65525 clusters, as per its
Microsoft-mandated limit in
http://download.microsoft.com/download/1/6/1/161ba512-40e2-4cc9-843a-923143f3456c/fatgen103.doc

This cannot be. One of the rules MUST have priority over the other. Either
spanning the whole disk must have priority, violating Microsoft's min/max
clusters rule, or clusters must be in range of Microsoft's min/max clusters
rule for that FAT bitness, but waste some disk space in clusters that are
beyond the addressable range.

In particular, for the given example, we have:

2,147,483,648 (== 2^31) bytes
    4,194,304           512-byte sectors

Now a FAT16 filesystem would have this before its data region:
           32           reserved sectors
          512           FAT region sectors
           32           root directory sectors

The data region would have the remaining sectors:
  4,194,304 - (32 + 512 + 32)
= 4,193,728 sectors
At 64 sectors per cluster, this is:
  4,193,728 / 64
= 65,527 clusters in the data region

But the maximum addressable number of clusters for FAT16 is: 
#define MAXCLS16  0xfff4U       /* maximum FAT16 clusters */
= 65,524 maximum addressable clusters

Which means 65,527 - 65,524 = 3 clusters can't be addressed, the FAT16
filesystem cannot completely span this volume. The last 3 clusters (== 192
sectors == 98,304 bytes of disk space), cannot be addressed, and would be
wasted.

What should newfs_msdos do? It can either use FAT16 and waste a little space,
or use FAT32 which will violate Microsoft's rule on the min number of FAT32
clusters.  Currently it does the latter, and errors out when checking that
rule. We should probably allow wasting some space, but satisfy the cluster
limit rules.

=========
ISSUE 2
=========

In the calculation for the number of clusters, the size of the FAT region is
incorrectly added *TWICE*!!

Note how lines 559 and 568 *BOTH* add "bpbBigFATsecs * bpb.bpbFATs" to "x1":

    554         x = bpb.bpbBigFATsecs ? bpb.bpbBigFATsecs : 1;
    555         if (x1 + (u_int64_t)x * bpb.bpbFATs > bpb.bpbHugeSectors) {
    556             warnx("meta data exceeds file system size");
    557             goto done;
    558         }
    559         x1 += x * bpb.bpbFATs;
    560         x = (u_int64_t)(bpb.bpbHugeSectors - x1) * bpb.bpbBytesPerSec *
NPB /
    561             (bpb.bpbSecPerClust * bpb.bpbBytesPerSec * NPB +
    562             fat / BPN * bpb.bpbFATs);
    563         x2 = howmany((RESFTE + MIN(x, maxcls(fat))) * (fat / BPN),
    564             bpb.bpbBytesPerSec * NPB);
    565         if (set_spf) {
    566             if (bpb.bpbBigFATsecs == 0)
    567                 bpb.bpbBigFATsecs = x2;
    568             x1 += (bpb.bpbBigFATsecs - 1) * bpb.bpbFATs;
    569         }
...
    581     } while (alignment != 0 && attempts < 2);

I am not sure how to fix this (this is an incredibly complicated section of
code - no wonder it's buggy), but since in this particular example I know the
loop runs twice, I commented out the bottom one.

With the FAT region half its previous size, this allow the data region to be
slightly bigger, causing the following check to fail on a smaller range of disk
sizes:

    593     if (cls < mincls(fat)) {
    594         warnx("%u clusters too few clusters for FAT%u, need %u", cls,
fat,
    595             mincls(fat));
    596         goto done;
    597     }

but it does not fully fix the bug, due to "issue 1" I previously described.

-- 
You are receiving this mail because:
You are the assignee for the bug.