fstyp looks for a 0x55 0xAA signature at offset 510, but this is not required by specifications and is not proivded by the Raspberry Pi Nano bootloader. We should really remove the signature check and implement a more comprehensive BPB validation instead, but it will require more investigation and testing. For now add a special case for the Raspberry Pi Nano bootloader. PR: 262896 MFC after: 3 days Sponsored by: The FreeBSD Foundation
Details
- Reviewers
trasz probono_puredarwin.org delphij - Commits
- rG7b03cfcb24c2: fstyp: detect Raspberry Pi Pico boot filesystem as FAT
rG1318b1ee9c4e: fstyp: detect Raspberry Pi Pico boot filesystem as FAT
rGe06ce938ddc0: fstyp: detect Raspberry Pi Pico boot filesystem as FAT
rG868c1b8431f2: fstyp: detect Raspberry Pi Pico boot filesystem as FAT
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
usr.sbin/fstyp/msdosfs.c | ||
---|---|---|
50–51 | I decided to be conservative for now and look for this specific set of bytes to be confident we can MFC without introducing false positives, but we could update this later on. |
I don't think this is right...
0xaa55 at the end of 512-byte sector was the signature for boot sector indicating that this is a valid boot sector; specification mentioned that and demands the values has to be 0xaa55, but nobody is enforcing it (Okay, we did check:
#ifndef MSDOSFS_NOCHECKSIG if (bsp->bs50.bsBootSectSig0 != BOOTSIG0 || bsp->bs50.bsBootSectSig1 != BOOTSIG1) { error = EINVAL; goto error_exit; } #endif
but it's not really required by specifications; I'm pretty sure that Windows would happily mount and silently fix the 0xaa55 signature, Linux would just ignore it).
A more reasonable check would be to perform some sanity check the BPB, basically something like the following, adapted from boot.c from fsck_msdosfs:
// Valid jmp; from specification. if (sector0[0] != 0xe9 && (sector0[0] != 0xeb || sector0[2] != 0x90)) return (false); // Bytes per sector must be power of two, and has to be at least 512 and at most 4096 per specification bpbBytesPerSec = sector0[11] + (sector0[12] << 8); if (bpbBytesPerSec < 512 || bpbBytesPerSec > 4096 || !powerof2(bpbBytesPerSec)) return (false); // Bytes per cluster must be power of two and non-zero bpbSecPerClust = sector0[13]; if (bpbSecPerClust == 0 || !powerof2(bpbSecPerClust)) return (false); // Reserved sectors must be greater than 1 bpbResSectors = sector0[14] + (sector0[15] << 8); if (bpbResSectors < 1) return (false); // Must have FAT bpbFATs = sector0[16]; if (bpbFATs == 0) return (false);
and return (true) after that, as we are somewhat confident that this is a valid BPB. We could potentially check if it's a valid FAT32 BPB, if media type was valid, etc. as well.
The patch works for me. Tested with actual Raspberry Pi Pico hardware. With this patch, /usr/local/sbin/automount will mount it. Thank you very much.
From the proposed test
// Valid jmp; from specification. if (sector0[0] != 0xe9 && (sector0[0] != 0xeb || sector0[2] != 0x90)) return (false);
I don't see this in boot.c now?
My goal here was to introduce a minimal change that I can have high confidence will not introduce a regression or a false positive. I agree that we need to use an approach like you suggest, but would like to do so on a slightly longer timescale (so that I can collect a variety of FAT images, etc.)
It's not, but I'm pretty sure that Windows does look at it (they don't look at the 0xaa55 signature, despite they would fix it silently upon dismount). If you feel uncomfortable with it we can omit it for now.
My goal here was to introduce a minimal change that I can have high confidence will not introduce a regression or a false positive. I agree that we need to use an approach like you suggest, but would like to do so on a slightly longer timescale (so that I can collect a variety of FAT images, etc.)
Ok
usr.sbin/fstyp/msdosfs.c | ||
---|---|---|
50 | I think you mean 511 here? |