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 Message-ID: <20130924092642.P954@besplex.bde.org> In-Reply-To: <1379975599.1593.10.camel@localhost> References: <201309231802.r8NI2KmF083133@chez.mckusick.com> <1379975599.1593.10.camel@localhost>
next in thread | previous in thread | raw e-mail | index | archive | help
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 they 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 implenting 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130924092642.P954>