Otherwise cd9660's VOP_VPTOFH can overflow the output buffer.
Reported by: Kevin Miller <mas@0x194.net>
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| sys/fs/cd9660/iso.h | ||
|---|---|---|
| 270 | Why add packed? | |
| sys/fs/cd9660/iso.h | ||
|---|---|---|
| 270 | Because otherwise there's an gap between ifid_pad and ifid_ino, violating the added assertion. | |
Comment Actions
I think more relevant question there are compilers generate proper code to access unaligned ifid_ino and ifid_start on sensitive arches.
It might be that the better fix is to re-order members without introducing __packed. There is no ABI involved.
Comment Actions
They have to, else the packed attribute would just be useless. Bugs should be filed against those that don't.
It might be that the better fix is to re-order members without introducing __packed. There is no ABI involved.
That doesn't seem possible as len and pad must stay first.
Comment Actions
I think more relevant question there are compilers generate proper code to access unaligned ifid_ino and ifid_start on sensitive arches.
I mentioned in a secteam chat that we should have a comment about unaligned access in arch(7). (The only comment we have right now is Most ILP32 ABIs, except arm, require only 4-byte alignment for 64-bit integers.)
ifid starting with a u_short ifid_len matches the fid_len at the beginning of struct fid; does anything make use of that?
Comment Actions
Yes, that would be a major compiler bug, I hope..
It might be that the better fix is to re-order members without introducing __packed. There is no ABI involved.
That doesn't seem possible as len and pad must stay first.
As far as I can see, this isn't true - the layout is up to the filesystem. The filesystem just needs to guarantee that fh1 == fh2 implies that the two FIDs refer to the same file.
I can't see any NFS code which peeks into the fid structure. Perhaps @rmacklem can confirm?
But, it doesn't make much sense to have len be anywhere other than the first field.
Comment Actions
As far as I know, the NFS server code does not look inside the fid structure.
I will note that the NFS server code zeros out the fh_fid before making VFS/VOP calls,
so that a fs specific "struct Xfid" can be smaller that "struct fid", but not larger, or course.
rick
ps: Maybe we should add a compile time assert to each file system to check that the
structure's size doesn't exceed sizeof(struct fid)?
But, it doesn't make much sense to have len be anywhere other than the first field.
Comment Actions
Btw, ext2fs and tarfs both are broken w.r.t. *fid size.
Adding __packed fixes both of them.
Is it ok to commit an __packed change for both or would
you rather see a separate review?
Comment Actions
Oh, thank you for going through the others. I had noticed tarfs but not ext2fs.
I think you can just go ahead with those patches. The static_asserts are fine verification.
It would be nice to have a basic regression test for each filesystem type wherein we create a filesystem, export it over NFS, and check that basic file accesses work as expected. With KASAN enabled, that would have been sufficient to catch these bugs. I'll see if I can find time to try and write a test like that.
Comment Actions
That would be great. Wouldn't it be worthwhile to add to it that compilers already handle relative alignment in structures correctly themselves (perhaps with caveats, as kib@'s comment hinted to; a propos, has someone experienced any compiler problem on "sensitive" arches? recently?), and that developers should watch for base alignments of structures when doing pointer conversions?
Comment Actions
Conceptually that could become the case (provided we remove the fid_len field), but in practice it's not currently also because tmpfs and zfs make use of fid_len and the latter then aliases struct fid to its own struct zfid_short or struct zfid_long, whose zf_len field would then need to be moved to match.
Leaving the layout completely to the filesystem means we could get rid of fid_len. I'm not completely sure if we really want to be doing so. fid_len could serve the purpose of limiting which bytes are actually compared to determine which handles are the same (which needs a slight amendment to tmpfs or zfs, as they are not consistent with what they store into fid_len), possibly avoiding problems with uninitialized bits. It could also be handy if we decided to switch to dynamic allocation (just saying; I'm not aware of an actual need to do that).
Unless you think it would be preferable to remove fid_len entirely, I'll draft some concrete changes giving fid_len a standard meaning.
Comment Actions
Right, some mechanical changes would be needed there.
Leaving the layout completely to the filesystem means we could get rid of fid_len. I'm not completely sure if we really want to be doing so. fid_len could serve the purpose of limiting which bytes are actually compared to determine which handles are the same (which needs a slight amendment to tmpfs or zfs, as they are not consistent with what they store into fid_len), possibly avoiding problems with uninitialized bits. It could also be handy if we decided to switch to dynamic allocation (just saying; I'm not aware of an actual need to do that).
Unless you think it would be preferable to remove fid_len entirely, I'll draft some concrete changes giving fid_len a standard meaning.
What changes do you propose?
Comment Actions
I've started with some field re-ordering to avoid __packed where possible, see D47954, D47955, D47956 (for tmpfs, packing was in fact done "by hand"). I was planning to perhaps change NFSVNO_CMPFH() to bcmp() with size only fid_len, but I'm not sure if there is much value in doing that.
After more pondering, I'd propose removing fid_len entirely in a second step (this one not MFCable, I guess), although the precise mean is not clear (just fill struct fid with an array of bytes?). That would be enough to remove the remaining __packed. Ideally, I would have also grown struct fid in the process (to cater to future needs, as ZFS is currently very near the limit for snapshots) but that part isn't completely trivial as struct fid is embedded in struct fandle which is part of the ABI, so I would need to provide ABI backwards compatibility (so it's probably best to postpone until there's an actual plan/need to provide more bytes).
Comment Actions
Just fyi, whenever the "struct fid" changes, the file handles
change. As such, an upgrade of the NFS server requires that
all clients be unmounted/remounted.
This isn't much of an issue for cd9660, ext2fs, tarfs, since
those are rarely mounted, although maybe an UPDATING or
RELNOTES entry is needed.
However, if you change the generic one such that all file
handles change, that will have more impact, I think?
Although I wouldn't exactly recommend it, I suspect
some upgrade their NFS servers without unmounting
and remounting all the client mounts.
Comment Actions
Yes. Not sure if it's worth it for these filesystems. Mark didn't do that.
However, if you change the generic one such that all file
handles change, that will have more impact, I think?
Although I wouldn't exactly recommend it, I suspect
some upgrade their NFS servers without unmounting
and remounting all the client mounts.
Sure, noted. This would be for a second round of changes.