Date: Tue, 24 Sep 2013 02:42:20 -0700 From: Kirk McKusick <mckusick@mckusick.com> To: Bruce Evans <brde@optusnet.com.au> Cc: freebsd-fs <freebsd-fs@FreeBSD.org> Subject: Re: kern/vfs_mount.c vfs_donmount() checks of MFSNAMELEN Message-ID: <201309240942.r8O9gKna000550@chez.mckusick.com> In-Reply-To: <20130924092642.P954@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> Date: Tue, 24 Sep 2013 10:48:45 +1000 (EST)
> From: Bruce Evans <brde@optusnet.com.au>
> To: sbruno@FreeBSD.org
> cc: Kirk McKusick <mckusick@mckusick.com>, freebsd-fs <freebsd-fs@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;
}
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201309240942.r8O9gKna000550>