From owner-freebsd-fs@FreeBSD.ORG Tue Sep 24 09:42:24 2013 Return-Path: Delivered-To: freebsd-fs@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id 36B12276; Tue, 24 Sep 2013 09:42:24 +0000 (UTC) (envelope-from mckusick@mckusick.com) Received: from chez.mckusick.com (chez.mckusick.com [IPv6:2001:5a8:4:7e72:4a5b:39ff:fe12:452]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id F13BC2E97; Tue, 24 Sep 2013 09:42:23 +0000 (UTC) Received: from chez.mckusick.com (localhost [127.0.0.1]) by chez.mckusick.com (8.14.3/8.14.3) with ESMTP id r8O9gKna000550; Tue, 24 Sep 2013 02:42:20 -0700 (PDT) (envelope-from mckusick@chez.mckusick.com) Message-Id: <201309240942.r8O9gKna000550@chez.mckusick.com> To: Bruce Evans Subject: Re: kern/vfs_mount.c vfs_donmount() checks of MFSNAMELEN In-reply-to: <20130924092642.P954@besplex.bde.org> Date: Tue, 24 Sep 2013 02:42:20 -0700 From: Kirk McKusick Cc: freebsd-fs X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 24 Sep 2013 09:42:24 -0000 > Date: Tue, 24 Sep 2013 10:48:45 +1000 (EST) > From: Bruce Evans > To: sbruno@FreeBSD.org > cc: Kirk McKusick , freebsd-fs > 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; }