msdosfs: verify that the BPB media descriptor and FAT ID match

[guest-svmhdvn]

Summary

Fixes bug 221501. In this bug, a valid msdosfs image was corrupted by changing the bytes per sector from 512 to 2048. This patch checks to see if the FAT is valid and in the correct sector based on the bytes per sector value.

[guest-svmhdvn]

Accidentally had extra whitespace in previous diff

[kib]

1. You should put return argument into braces, as in return (EINVAL);.
2. brelse() is not needed after failed bread(), bp is set to NULL consistently on error. But if bp was indeed read, it must be brelse()d. So I would structure the code differently:

error = bread();
if (error == 0 && bp->b_data[0] != bpbMedia)
    error = EINVAL;
brelse(bp);
if (error != 0)
  return (error);

3. According to the FAT spec, not only the byte 0 should have the specific value bpbMedia, but the whole FAT cluster is specified, other bits must be all 1's. So it might be easier to start the loop in fillinusemap() from 0 and not from CLUST_FIRST, but then ensure that FAT[0] readcn is not added to usemap, but only checked for the right value. Then additional bread() is not needed at all.

[guest-svmhdvn]

Thanks for the feedback Kostik! Updated to reflect the changes.

[kib]

My only comments are about minor style(9) issues. The change itself looks good.

sys/fs/msdosfs/msdosfs_fat.c

911

The empty line is wrong. Previous block comment spans until the vertical space, in this case it covers the loop.

920

if (error != 0)

[guest-svmhdvn]

Fix style and semantics issues

[kib]

I will commit this shortly. That said, I do not believe that the change really fixes the panic reported in the PR 221501. If FAT 0 cluster is corrected in the corrupted image, the failure should still happen. This does not invalidate the change, just that it is somewhat different from what is described in the review.

[guest-svmhdvn]

That makes sense, I will continue working on an actual solution to the panic. Thanks for the clarification.