tmpfs: Preserve alignment of struct fid fields

[freqlabs]

Summary

On 64-bit platforms, the two short fields in struct tmpfs_fid are padded to the 64-bit alignment of the long field. This pushes the offsets of the subsequent fields by 4 bytes and makes struct tmpfs_fid bigger than struct fid. tmpfs_vptofh() casts a struct fid * to struct tmpfs_fid *, causing 4 bytes of adjacent memory to be overwritten when the struct fields are set. Through several layers of indirection and embedded structs, the adjacent memory for one particular call to tmpfs_vptofh() happens to be the stack canary for nfsrvd_compound(). Half of the canary ends up being clobbered, going unnoticed until eventually the stack check fails when nfsrvd_compound() returns and a panic is triggered.

Instead of duplicating fields of struct fid in struct tmpfs_fid, narrow the struct to cover only the unique fields for tmpfs and assert at compile time that the struct fits in the allotted space. This way we don't have to replicate the offsets of struct fid fields, we just use them directly.

Test Plan

Tested as follows:

mount -t tmpfs tmpfs /mnt
truncate -s 1g d0
zpool create p0 $PWD/d0
zfs set mountpoint=/mnt/nfs p0
sysrc nfsv4_server_enable=YES
cat >/etc/exports <<EOF
V4: /
/mnt/nfs
EOF
service nfsd onestart

Before the fix, observe the server panics when a client mounts the export:

mount -t nfs -o vers=4 ${server}:/mnt/nfs /mnt

After the fix, observe the mount succeeds.

[mav]

While its a good catch to find the overflow, your solution will not work on architectures with strict alignment. The problem is that struct tmpfs_fid_data require 64-bit alignment due to its fields, while fid_data offset within struct fid is 32 bits. So either you need some alignment-agnostic access to the fid_data, such as bcopy(), or split field(s) to 32-bit parts to fit into the struct fid alignment, or struct fid needs some padding to align fid_data to 64bits.

[kib]

In D25077#552326, @mav wrote:

While its a good catch to find the overflow, your solution will not work on architectures with strict alignment. The problem is that struct tmpfs_fid_data require 64-bit alignment due to its fields, while fid_data offset within struct fid is 32 bits. So either you need some alignment-agnostic access to the fid_data, such as bcopy(), or split field(s) to 32-bit parts to fit into the struct fid alignment, or struct fid needs some padding to align fid_data to 64bits.

In fact it might break even on arches with hw allowing unaligned access, because de-referencing unaligned pointer is UB, which modern compilers start to exploit.
I agree with mav, memcpy is the best fix.

[freqlabs]

Use memcpy to avoid unaligned field access.

[kib]

sys/fs/tmpfs/tmpfs.h

400 ↗

(On Diff #72482)

Use _Static_assert.

sys/fs/tmpfs/tmpfs_vnops.c

1454 ↗

(On Diff #72482)

'On some CPUs' is excessive, dereferencing unaligned pointer is always UB. On 32bit arches the member is aligned.

[freqlabs]

Improve comment and replace deprecated CTASSERT with C11 _Static_assert.

[freqlabs]

sys/fs/tmpfs/tmpfs_vnops.c

1454 ↗

(On Diff #72482)

I suppose I could still clarify that the access is only unaligned on 64-bit arches. (we don't have any others to consider do we?)

[kib]

sys/fs/tmpfs/tmpfs_vnops.c

1454 ↗

(On Diff #72482)

You can. But I suggest to add this comment at the place of the struct fid definition, so that it is visible to everybody who looks at it. This could be a follow-up change.

[freqlabs]

Might as well get the comments right. Thanks for the guidance, @kib and @mav.

[kib]

sys/sys/mount.h

61 ↗

(On Diff #72487)

I would say '4 bytes' instead of 32-bit.

65 ↗

(On Diff #72487)

I would shrink the text, it explains too basic things about alignment and then it is imprecise. For instance, on arm 32bit 64bit values are 8-bytes aligned (and I suspect on everything else except i386).

[freqlabs]

Trim the comment on fid_data to a concise hint.

[rmacklem]

Looks fine to me, too.
I assume this fixes your crashes, good work.