Author: freqlabs Date: Wed Jun 3 09:38:51 2020 New Revision: 361748 URL: https://svnweb.freebsd.org/changeset/base/361748
Log: tmpfs: Preserve alignment of struct fid fields 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. Reviewed by: kib, mav, rmacklem Approved by: mav (mentor) MFC after: 1 week Sponsored by: iXsystems, Inc. Differential Revision: https://reviews.freebsd.org/D25077 Modified: head/sys/fs/tmpfs/tmpfs.h head/sys/fs/tmpfs/tmpfs_vfsops.c head/sys/fs/tmpfs/tmpfs_vnops.c head/sys/sys/mount.h Modified: head/sys/fs/tmpfs/tmpfs.h ============================================================================== --- head/sys/fs/tmpfs/tmpfs.h Wed Jun 3 05:49:19 2020 (r361747) +++ head/sys/fs/tmpfs/tmpfs.h Wed Jun 3 09:38:51 2020 (r361748) @@ -37,6 +37,7 @@ #ifndef _FS_TMPFS_TMPFS_H_ #define _FS_TMPFS_TMPFS_H_ +#include <sys/cdefs.h> #include <sys/queue.h> #include <sys/tree.h> @@ -393,12 +394,12 @@ struct tmpfs_mount { * This structure maps a file identifier to a tmpfs node. Used by the * NFS code. */ -struct tmpfs_fid { - uint16_t tf_len; - uint16_t tf_pad; - ino_t tf_id; - unsigned long tf_gen; +struct tmpfs_fid_data { + ino_t tfd_id; + unsigned long tfd_gen; }; +_Static_assert(sizeof(struct tmpfs_fid_data) <= MAXFIDSZ, + "(struct tmpfs_fid_data) is larger than (struct fid).fid_data"); struct tmpfs_dir_cursor { struct tmpfs_dirent *tdc_current; Modified: head/sys/fs/tmpfs/tmpfs_vfsops.c ============================================================================== --- head/sys/fs/tmpfs/tmpfs_vfsops.c Wed Jun 3 05:49:19 2020 (r361747) +++ head/sys/fs/tmpfs/tmpfs_vfsops.c Wed Jun 3 09:38:51 2020 (r361748) @@ -566,24 +566,29 @@ static int tmpfs_fhtovp(struct mount *mp, struct fid *fhp, int flags, struct vnode **vpp) { - struct tmpfs_fid *tfhp; + struct tmpfs_fid_data tfd; struct tmpfs_mount *tmp; struct tmpfs_node *node; int error; + if (fhp->fid_len != sizeof(tfd)) + return (EINVAL); + + /* + * Copy from fid_data onto the stack to avoid unaligned pointer use. + * See the comment in sys/mount.h on struct fid for details. + */ + memcpy(&tfd, fhp->fid_data, fhp->fid_len); + tmp = VFS_TO_TMPFS(mp); - tfhp = (struct tmpfs_fid *)fhp; - if (tfhp->tf_len != sizeof(struct tmpfs_fid)) + if (tfd.tfd_id >= tmp->tm_nodes_max) return (EINVAL); - if (tfhp->tf_id >= tmp->tm_nodes_max) - return (EINVAL); - TMPFS_LOCK(tmp); LIST_FOREACH(node, &tmp->tm_nodes_used, tn_entries) { - if (node->tn_id == tfhp->tf_id && - node->tn_gen == tfhp->tf_gen) { + if (node->tn_id == tfd.tfd_id && + node->tn_gen == tfd.tfd_gen) { tmpfs_ref_node(node); break; } Modified: head/sys/fs/tmpfs/tmpfs_vnops.c ============================================================================== --- head/sys/fs/tmpfs/tmpfs_vnops.c Wed Jun 3 05:49:19 2020 (r361747) +++ head/sys/fs/tmpfs/tmpfs_vnops.c Wed Jun 3 09:38:51 2020 (r361748) @@ -1435,16 +1435,28 @@ tmpfs_pathconf(struct vop_pathconf_args *v) static int tmpfs_vptofh(struct vop_vptofh_args *ap) +/* +vop_vptofh { + IN struct vnode *a_vp; + IN struct fid *a_fhp; +}; +*/ { - struct tmpfs_fid *tfhp; + struct tmpfs_fid_data tfd; struct tmpfs_node *node; + struct fid *fhp; - tfhp = (struct tmpfs_fid *)ap->a_fhp; node = VP_TO_TMPFS_NODE(ap->a_vp); + fhp = ap->a_fhp; + fhp->fid_len = sizeof(tfd); - tfhp->tf_len = sizeof(struct tmpfs_fid); - tfhp->tf_id = node->tn_id; - tfhp->tf_gen = node->tn_gen; + /* + * Copy into fid_data from the stack to avoid unaligned pointer use. + * See the comment in sys/mount.h on struct fid for details. + */ + tfd.tfd_id = node->tn_id; + tfd.tfd_gen = node->tn_gen; + memcpy(fhp->fid_data, &tfd, fhp->fid_len); return (0); } Modified: head/sys/sys/mount.h ============================================================================== --- head/sys/sys/mount.h Wed Jun 3 05:49:19 2020 (r361747) +++ head/sys/sys/mount.h Wed Jun 3 09:38:51 2020 (r361748) @@ -57,6 +57,9 @@ typedef struct fsid { int32_t val[2]; } fsid_t; /* fil /* * File identifier. * These are unique per filesystem on a single machine. + * + * Note that the offset of fid_data is 4 bytes, so care must be taken to avoid + * undefined behavior accessing unaligned fields within an embedded struct. */ #define MAXFIDSZ 16 _______________________________________________ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
