Bug: nmount(2) lacks parameter checking.

Hi,
Looking over the code for nmount(), I think I noticed a few bugs.

(tried send-pr, but the lack of a web-front-end at freebsd.org,
and a decent mail system locally means that's not a runner)

nmount() calls vfs_nmount() pretty much directly after copying in
the io vector from userland.

vfs_nmount() then calls vfs_buildopts() as the first thing it does.

There's a couple of problems here. Firstly, there's no check up to
this point that the user passing the options in is indeed root. So,
all the bugs mentioned can be tickled by a non-root user.

vfs_buildopts doesn't ensure that the option's "name" is a null
terminated string, but, it later calls vfs_sanitizeopts(), which
assumes this. By passing in strings just at and just over the
pagesize, a non-root user can cause a crash in vfs_buildopts
reasonably reliably when strcmp to hit an unmappped page. (Program
available on request)

vfs_buildopts also leaks memory if it jumps to "bad": anything in
the current option is lost to the woods.

There's also no checking on how much memory is actually aquired by
vfs_buildopts(): it can be passed up to MAX_IOVCOUNT (1024) elements
in the iovec, and each of these can be up to 64K in size. That's
64M of memory, plus some overhead for option structures, which would
be a lot to start chewing up in the kernel.

The source I based these observations on is from today, while my
kernel is a few weeks old, and I no longer have source for it.
Given the traffic on the list recently, I figure now is Not A Good
Time to install a fresh kernel, so the patch attached is tested to
the point that it compiles, but I think something like it is
required.




Index: kern/vfs_mount.c
===================================================================
RCS file: /pub/FreeBSD/development/FreeBSD-CVS/src/sys/kern/vfs_mount.c,v
retrieving revision 1.111
diff -u -r1.111 vfs_mount.c
--- kern/vfs_mount.c	26 Sep 2003 09:07:27 -0000	1.111
+++ kern/vfs_mount.c	4 Nov 2003 21:46:44 -0000
_at__at_ -246,6 +246,8 _at__at_
 	struct vfsopt *opt;
 	unsigned int i, iovcnt;
 	int error, namelen, optlen;
+	size_t memTotal = 0;
+	static const size_t maxMemTotal = 1024 * 64;
 
 	iovcnt = auio->uio_iovcnt;
 	opts = malloc(sizeof(struct vfsoptlist), M_MOUNT, M_WAITOK);
_at__at_ -256,6 +258,26 _at__at_
 		optlen = auio->uio_iov[i + 1].iov_len;
 		opt->name = malloc(namelen, M_MOUNT, M_WAITOK);
 		opt->value = NULL;
+		opt->len = optlen;
+
+		/*
+		 * Do this early, so jumps to "bad" will free the current
+		 * option
+		 */
+		TAILQ_INSERT_TAIL(opts, opt, link);
+		memTotal += sizeof (struct vfsopt) + optlen + namelen;
+
+		/*
+		 * Avoid consuming too much memory, and attempts to overflow
+		 * memTotal
+		 */
+		if (memTotal > maxMemTotal ||
+		    optlen > maxMemTotal ||
+		    namelen > maxMemTotal) {
+			error = EINVAL;
+			goto bad;
+		}
+
 		if (auio->uio_segflg == UIO_SYSSPACE) {
 			bcopy(auio->uio_iov[i].iov_base, opt->name, namelen);
 		} else {
_at__at_ -264,7 +286,11 _at__at_
 			if (error)
 				goto bad;
 		}
-		opt->len = optlen;
+		/* Ensure names are null-terminated strings */
+		if (opt->name[namelen - 1] != '\0') {
+		    error = EINVAL;
+		    goto bad;
+		}
 		if (optlen != 0) {
 			opt->value = malloc(optlen, M_MOUNT, M_WAITOK);
 			if (auio->uio_segflg == UIO_SYSSPACE) {
_at__at_ -277,7 +303,6 _at__at_
 					goto bad;
 			}
 		}
-		TAILQ_INSERT_TAIL(opts, opt, link);
 	}
 	vfs_sanitizeopts(opts);
 	*options = opts;