cd9660: Make sure that struct ifid fits in generic filehandle structure

[markj]

Summary

Otherwise cd9660's VOP_VPTOFH can overflow the output buffer.

Reported by: Kevin Miller <[e-mail]>

[imp]

sys/fs/cd9660/iso.h

270

Why add packed?

[markj]

sys/fs/cd9660/iso.h

270

Because otherwise there's an gap between ifid_pad and ifid_ino, violating the added assertion.

[olce]

I've checked that nobody takes the address of the unaligned members.

[kib]

In D47879#1091770, @olce wrote:

I've checked that nobody takes the address of the unaligned members.

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.

[olce]

In D47879#1091920, @kib wrote:

I think more relevant question there are compilers generate proper code to access unaligned ifid_ino and ifid_start on sensitive arches.

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.

[emaste]

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?

[markj]

In D47879#1091930, @olce wrote:

In D47879#1091920, @kib wrote:

I think more relevant question there are compilers generate proper code to access unaligned ifid_ino and ifid_start on sensitive arches.

They have to, else the packed attribute would just be useless. Bugs should be filed against those that don't.

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.

[rmacklem]

In D47879#1091962, @markj wrote:

In D47879#1091930, @olce wrote:

In D47879#1091920, @kib wrote:

I think more relevant question there are compilers generate proper code to access unaligned ifid_ino and ifid_start on sensitive arches.

They have to, else the packed attribute would just be useless. Bugs should be filed against those that don't.

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?

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.