svn commit: r361748 - in head/sys: fs/tmpfs sys

[Ryan Moeller]

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