Fix MNAMELEN or reimplement struct statfs

[Nathanael Hoyle]

On Thu, 21 Nov 2013 09:40:28 -0800
Doug Ambrisko <ambrisko at ambrisko.com> wrote:

> On Wed, Nov 20, 2013 at 09:55:31AM +0200, Konstantin Belousov wrote:
> | On Tue, Nov 19, 2013 at 09:42:16AM -0800, Doug Ambrisko wrote:
> | > I was talking about the more general case since the system tries
> to keep | > the path in the stat structure.  My prior approach which
> had more issues | > was to modify the stat structure of which I was
> pointed to NetBSD and their | > change to statvfs which doesn't
> really solve the problem.  They don't | > have the check to see if
> the mount is longer then VFS_MNAMELEN (in their case) | > and just
> truncate things. | > 
> | > If we are just talking about adding it to the mount structure that
> | > would be okay since it isn't exposed to user land.  I can add
> that. |
> | Yes, this is exactly what I mean.  Add a struct mount field, and use
> | it for kernel only.  In fact, it only matters for sys_unmount() and
> | kern_jail.c, other locations in kernel use the path for warnings,
> and | this could be postponed if you prefer to minimize the patch.
> 
> Okay, I went through all of the occurances and compile tested (except
> for #DEBUG).  I united a few things but should do more once I get
> consensus on the approach.  I found a few spots that should be
> updated as well and made the length check more consistant.  Some were
> doing >= and others
> >.  So this should be better, however, a lot larger.  On the plus side
> when we figure out how to return the longer path length to user land
> that can be more flexible since the kernel is tracking the longer
> length. Probably things to note are changes in:
> 	ZFS to mount snapshot
> 	cd9660 for symlinks
> 	fuse to return full path
> 	jail to check statfs and mount
> 	mount/umount to save and check full path
> 	mountroot to save new field for full path
> 	
> Just in case it doesn't make it in email the full patch is at:
> 	http://people.freebsd.org/~ambrisko/mount_bigger.patch
> 
> Thanks,
> 
> Doug A.
> 

Hey, long-time lurker, don't normally post, but I think this introduces
a boundary error. It certainly appears to make the code not match the
comments.

> Index: cddl/compat/opensolaris/kern/opensolaris_vfs.c
> ===================================================================
> --- cddl/compat/opensolaris/kern/opensolaris_vfs.c	(revision
> 257489) +++ cddl/compat/opensolaris/kern/opensolaris_vfs.c
> (working copy) @@ -126,7 +126,7 @@
>  	 * variables will fit in our mp buffers, including the
>  	 * terminating NUL.
>  	 */
> -	if (strlen(fstype) >= MFSNAMELEN || strlen(fspath) >=
> MNAMELEN)
> +	if (strlen(fstype) > MFSNAMELEN || strlen(fspath) >
> MAXPATHLEN) return (ENAMETOOLONG);
>  
>  	vfsp = vfs_byname_kld(fstype, td, &error);

The change from >= to > in this comparison means that where
strlen(fspath)==MAXPATHLEN, this guard is passed and no error is thrown.

> ===================================================================
> --- kern/vfs_mount.c	(revision 257489)
> +++ kern/vfs_mount.c	(working copy)
> @@ -473,6 +473,7 @@
>  	mp->mnt_cred = crdup(cred);
>  	mp->mnt_stat.f_owner = cred->cr_uid;
>  	strlcpy(mp->mnt_stat.f_mntonname, fspath, MNAMELEN);
> +	strlcpy((char *)mp->mnt_path, fspath, MAXPATHLEN);
>  	mp->mnt_iosize_max = DFLTPHYS;
>  #ifdef MAC
>  	mac_mount_init(mp);
> @@ -656,7 +657,7 @@
>  	 * variables will fit in our mp buffers, including the
>  	 * terminating NUL.
>  	 */
> -	if (fstypelen > MFSNAMELEN || fspathlen > MNAMELEN) {
> +	if (fstypelen > MFSNAMELEN || fspathlen > MAXPATHLEN) {
>  		error = ENAMETOOLONG;
>  		goto bail;

Same logic is used here, so it doesn't fail here.

>  	}
> @@ -748,8 +749,8 @@
>  		return (EOPNOTSUPP);
>  	}
>  
> -	ma = mount_argsu(ma, "fstype", uap->type, MNAMELEN);
> -	ma = mount_argsu(ma, "fspath", uap->path, MNAMELEN);
> +	ma = mount_argsu(ma, "fstype", uap->type, MFSNAMELEN);
> +	ma = mount_argsu(ma, "fspath", uap->path, MAXPATHLEN);
>  	ma = mount_argb(ma, flags & MNT_RDONLY, "noro");
>  	ma = mount_argb(ma, !(flags & MNT_NOSUID), "nosuid");
>  	ma = mount_argb(ma, !(flags & MNT_NOEXEC), "noexec");
> @@ -1040,7 +1041,7 @@
>  	 * variables will fit in our mp buffers, including the
>  	 * terminating NUL.
>  	 */
> -	if (strlen(fstype) >= MFSNAMELEN || strlen(fspath) >=
> MNAMELEN)
> +	if (strlen(fstype) > MFSNAMELEN || strlen(fspath) >
> MAXPATHLEN) return (ENAMETOOLONG);


Adding the rest of the comment from the sources here:
        /*
         * Be ultra-paranoid about making sure the type and fspath
         * variables will fit in our mp buffers, including the
         * terminating NUL.
         */

Ok, so intent is to ensure that the provided mount path can fully fit,
*including* terminating NULL character. For this to be true,
strlen(fspath)+1 must be <= MAXPATHLEN. This is not true when
strlen(fspath) == MAXPATHLEN, but is not detected in this check. At the
very least, here the code and the comments differ now.


> Index: kern/vfs_mountroot.c
> ===================================================================
> --- kern/vfs_mountroot.c	(revision 257489)
> +++ kern/vfs_mountroot.c	(working copy)
> @@ -307,6 +307,8 @@
>  				vp->v_mountedhere = mporoot;
>  				strlcpy(mporoot->mnt_stat.f_mntonname,
>  				    fspath, MNAMELEN);
> +				strlcpy((char *)mporoot->mnt_path,
> +				    fspath, MAXPATHLEN);
>  				VOP_UNLOCK(vp, 0);
>  			} else
>  				vput(vp);

Here strlcpy is used safely. However, in cases where
strlen(fspath)==MAXPATHLEN, the result is to truncate the last
character in fspath and replace it with a NULL byte to ensure proper
termination. Now mnt_path will contain not the "actual path", but the
actual path truncated by 1 character, and back to not matching what was
in the original call (same problem in earlier thread discussion).

Also, two different paths which differed in only their last character
would get truncated to the same path name (hilarity ensues).


Hopefully my understanding is correct and I'm not completely off track
and being unhelpful. I'm certain I have less experience with this code
than others on the thread, but wanted to note what I believe is an
issue.

Regards,
-Nathanael Hoyle