kern/vfs_mount.c vfs_donmount() checks of MFSNAMELEN

[Kirk McKusick]

> Date: Tue, 24 Sep 2013 10:48:45 +1000 (EST)
> From: Bruce Evans <brde at optusnet.com.au>
> To: sbruno at FreeBSD.org
> cc: Kirk McKusick <mckusick at mckusick.com>, freebsd-fs <freebsd-fs at FreeBSD.org>
> Subject: Re: kern/vfs_mount.c vfs_donmount() checks of MFSNAMELEN
> 
> On Mon, 23 Sep 2013, Sean Bruno wrote:
> 
>> On Mon, 2013-09-23 at 11:02 -0700, Kirk McKusick wrote:
>>>> So, I'm confused by this check:
>>>>
>>>>         if (fstypelen >= MFSNAMELEN - 1 || fspathlen >= MNAMELEN -
>>> 1) {
>>>>                 error = ENAMETOOLONG;
>>>>                 goto bail;
>>>>         }
>>>>
>>>> MFSNAMELEN is 16, why do we check against >= MFSNAMELEN - 1?  Why
>>> dont
>>>> we check against (> MFSNAMELEN - 1) or (>= MFSNAMELEN)?  Is a 14
>>>> character fstypelen with a "\0" at the end considered too long?
>>>>
>>>> Sean
>>>>
>>>> p.s. e.g. mount -t fuse.glusterfs ...
>>>
>>> I agree with you. It should either be (> MFSNAMELEN - 1) or (>=
>>> MFSNAMELEN).
> 
> Apart from the existence of this check being a bug, it is still off by
> 1 for fstypename and off by 2 for fspathlen.  It used to be off by 2
> for both.
> 
> fstypelen and fsnamelen are not string lengths; they are array sizes
> where the array includes space for the terminating NUL (you can see
> this most easily a few lines earlier where it is checked that the byte
> at the end is NUL, where the end is determined by these variables).
> MFSNAMELEN and MNAMELEN are also not string lengths; they are also array
> sizes where the array includes space for the terminating NUL.  Thus
> fstypelen == MFSNAMELEN and fspathlen == MNAMELEN are maximal non-lengths,
> not errors.
> 
> "fuse.glusterfs" happens to have length 14 and size 15, so it is is maximal
> for the off-by-1 limit.
> 
>> Not sure if we should adjust MNAMELEN or not too while we're at it, I
>> need to do a bit more of a code audit before thunking that one.
> 
> The existence of the MNAMELEN check is a bug.  It breaks mounting of file
> systems when fspathlen is too long to fit in the copy of the pathname
> that will be returned by statfs() if statfs() is ever called (except with
> the off-by-2 bug it breaks even for pathnames that fit).  mount() worked
> correctly in old versions of FreeBSD -- pathnames were only limited by
> MAXPATHLEN, except where they are copied to struct statfs and possibly
> to superblocks (where the array reserved for the pathname is typically
> also too short, but not as short as MNAMELEN).  Utilities using statfs()
> couldn't report correct pathnames for truncated ones, and the full
> pathname needed to be remembered somewhere if unmount() needs to be by
> pathname, but that is better than not allowing the mount at all.
> 
> The MFSNAMELEN check doesn't break anything, but is just silly.  fstype
> just points to a full copy of a mount arg and isn't copied to any array
> of limited length MFSNAMELEN.  It is only used to look up the file
> system in vfs_byname() or vfs_byname_kld().  If the caller is silly
> enough to try to mount a file system whose name length (plus 1) exceeds
> MFSNAMELEN, then the lookup obviously fails since the name is longer
> than the name of any file system that can be supported.  The relevant
> bounds check is in vfs_buildopts() where the arg is copied in.
> 
> vfs_mount.c now has internal dependencies on the buggy up-front MNAMELEN
> limit, but these should be easy to fix by using the correct limit
> MAXPATHLEN everywhere except where the pathname is copied to the statfs
> struct.  This copying used to be repeated in individual file systems,
> but is now centralized in vfs_mount_alloc(), and it still explicitly
> truncates although truncation is now always null due to the up-front
> check.
> 
>> Propsed patch to set fstyplen check:
>> Index: sys/kern/vfs_mount.c
>> ===================================================================
>> --- sys/kern/vfs_mount.c	(revision 255831)
>> +++ sys/kern/vfs_mount.c	(working copy)
>> @@ -656,7 +656,7 @@
>> 	 * variables will fit in our mp buffers, including the
>> 	 * terminating NUL.
>> 	 */
>> -	if (fstypelen >= MFSNAMELEN - 1 || fspathlen >= MNAMELEN - 1) {
>> +	if (fstypelen >= MFSNAMELEN || fspathlen >= MNAMELEN - 1) {
>> 		error = ENAMETOOLONG;
>> 		goto bail;
>> 	}
> 
> The correct checks for implementing this bug are simpler and more natural:
> 
>  	if (fstypelen > MFSNAMELEN || fspathlen > MNAMELEN) {
> 
> These buggy checks are repeated at the start of vfs_domount(), except
> the checks there use strlen() and are naturally missing the off-by-[12]
> errors:
> 
>  	if (strlen(fstype) >= MFSNAMELEN || strlen(fspath) >= MNAMELEN)
> 
> Bruce

I agree with Bruce's arguments and his proposed solution. Notably the
change should be:

Index: sys/kern/vfs_mount.c
===================================================================
--- sys/kern/vfs_mount.c	(revision 255831)
+++ sys/kern/vfs_mount.c	(working copy)
@@ -656,7 +656,7 @@
	 * variables will fit in our mp buffers, including the
	 * terminating NUL.
	 */
-	if (fstypelen >= MFSNAMELEN - 1 || fspathlen >= MNAMELEN - 1) {
+	if (fstypelen > MFSNAMELEN || fspathlen > MNAMELEN) {
		error = ENAMETOOLONG;
		goto bail;
	}