>Number:         25226
>Category:       kern
>Synopsis:       serious compat problem when mount_msdos calls mount(2)
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Apr 18 13:37:00 UTC 2004
>Closed-Date:
>Last-Modified:
>Originator:     Todd Whitesel
>Release:        NetBSD 2.0_BETA 20040417
>Organization:
	NetBSD Developer
>Environment:
System: NetBSD uni.toddpw.net 2.0_BETA NetBSD 2.0_BETA (GENERIC) #2: Sun Apr 18 05:18:30 PDT 2004  toddpw@yuri.toddpw.net:/yuri/rnetbsd-2-0/src/sys/arch/i386/compile/obj/GENERIC i386
Architecture: i386
Machine: i386
>Description:
	mount_msdos suffers from a serious compat issue going from -1-6 to -2-0
	because of sys/fs/msdosfs/msdosfsmount.h version 1.4 and related edits.
	A 'gmtoff' field was added to struct msdosfs_args, causing the flags
	and version fields in this structure to move. Running a -2-0 kernel on
	a -1-6 userland would result in mount_msdos putting flags where the
	kernel expected 'gmtoff', and garbage where the kernel expected 'flags'.
	Even if this issue is fixed, the versioning in this structure (added by
	msdosfsmount.h version 1.3) is risky because it reads off the end of a
	user-supplied structure (in msdosfs_vfsops.c:msdosfs_mount); mount(2)
	is therefore vulnerable to returning EFAULT when it should not.
>How-To-Repeat:
	Boot a -2-0 kernel on a 1.6.2 userland.
	mount -t msdos -o ro,-l /dev/sd0h /d
>Fix:
	The following patch is against -current but was smoke-tested in -2-0
	(the -current kernel uvm faulted during autoconfig).

Index: msdosfs_vfsops.c
===================================================================
RCS file: /cvsroot/src/sys/fs/msdosfs/msdosfs_vfsops.c,v
retrieving revision 1.13
diff -u -r1.13 msdosfs_vfsops.c
--- msdosfs_vfsops.c	24 Mar 2004 15:34:52 -0000	1.13
+++ msdosfs_vfsops.c	18 Apr 2004 13:08:09 -0000
@@ -246,6 +246,7 @@
 {
 	struct vnode *devvp;	  /* vnode for blk device to mount */
 	struct msdosfs_args args; /* will hold data from mount request */
+	int voff;		  /* offset to args.version */
 	/* msdosfs specific mount control block */
 	struct msdosfsmount *pmp = NULL;
 	int error, flags;
@@ -265,19 +266,50 @@
 		vfs_showexport(mp, &args.export, &pmp->pm_export);
 		return copyout(&args, data, sizeof(args));
 	}
-	error = copyin(data, &args, sizeof(struct msdosfs_args));
+
+	/*
+	 * Only copyin the unversioned structure fields until we know
+	 * whether this is a newer, versioned structure or an older one.
+	 * XXX - unversioned check is technically COMPAT_anythingbefore2.0
+	 */
+	memset(&args, 0, sizeof args);
+	voff = offsetof(struct msdosfs_args, version);
+	error = copyin(data, &args, voff);
 	if (error)
 		return (error);
 
 	/*
-	 * If not versioned (i.e. using old mount_msdos(8)), fill in
-	 * the additional structure items with suitable defaults.
+	 * If not versioned (i.e. using old mount_msdos(8)), at least make
+	 * sure the version field is appropriate so it can be tested later.
+	 * Otherwise, use the version (which is actually the true size of the
+	 * structure to avoid a table lookup here) to copyin the final size.
+	 * We go through all this effort to avoid reading off the end of the
+	 * struct (where there may be invalid memory lurking).
 	 */
 	if ((args.flags & MSDOSFSMNT_VERSIONED) == 0) {
-		args.version = 1;
+		args.version = voff + sizeof(args.version);
+	} else {
+		error = copyin(data, &args, voff + sizeof(args.version));
+		if (error)
+			return (error);
+		/* Now use the user's args.version value to finish copyin. */
+		error = copyin(data, &args, args.version);
+		if (error)
+			return (error);
+	}
+
+	/* Default args.dirmask if it was not provided by the user. */
+	if (args.version < offsetof(struct msdosfs_args, dirmask)
+			   + (sizeof(args.dirmask))) {
 		args.dirmask = args.mask;
 	}
 
+	/* Default args.gmtoff if it was not provided by the user. */
+	if (args.version < offsetof(struct msdosfs_args, gmtoff)
+			   + (sizeof(args.gmtoff))) {
+		args.gmtoff = 0;	/* XXX - should be local timezone? */
+	}
+
 	/*
 	 * If updating, check whether changing from read-only to
 	 * read/write; if there is no device name, that's all we do.
Index: msdosfsmount.h
===================================================================
RCS file: /cvsroot/src/sys/fs/msdosfs/msdosfsmount.h,v
retrieving revision 1.5
diff -u -r1.5 msdosfsmount.h
--- msdosfsmount.h	3 Oct 2003 16:34:31 -0000	1.5
+++ msdosfsmount.h	18 Apr 2004 13:08:12 -0000
@@ -48,7 +48,7 @@
  */
 
 /*
- *  Arguments to mount MSDOS filesystems.
+ *  Arguments to mount MSDOS filesystems. SEE VERSIONING INFORMATION BELOW.
  */
 struct msdosfs_args {
 	char	*fspec;		/* blocks special holding the fs to mount */
@@ -56,13 +56,21 @@
 	uid_t	uid;		/* uid that owns msdosfs files */
 	gid_t	gid;		/* gid that owns msdosfs files */
 	mode_t  mask;		/* mask to be applied for msdosfs perms */
-	int	gmtoff;		/* offset from UTC in seconds */
 	int	flags;		/* see below */
-
-	/* Following items added after versioning support */
-	int	version;	/* version of the struct */
-#define MSDOSFSMNT_VERSION	2
+	/* DO NOT CHANGE the OFFSETOF() any field above, or of 'version' !! */
+	/* If you do, you will break msdosfs_vfsops.c:msdosfs_mount()	    */
+	/* badly and with no good COMPAT_xx recourse. Just don't do it.	    */
+
+	/* The following items exist IFF (flags & MSDOSFSMNT_VERSIONED) */
+	int	version;	/* version of the struct (true length) */
+#define MSDOSFSMNT_VERSION	(sizeof(struct msdosfs_args))
 	mode_t  dirmask;	/* v2: mask to be applied for msdosfs perms */
+	int	gmtoff;		/* v3: offset from UTC in seconds */
+
+	/* If you add fields here, you need to check to see if the version
+	 * is large enough to cover the field, otherwise you must supply a
+	 * default in msdosfs_vfsops.c:msdosfs_mount() somewhere.
+	 */
 };
 
 /*
>Release-Note:
>Audit-Trail:
>Unformatted: