From owner-freebsd-fs@FreeBSD.ORG Tue Sep 24 01:17:27 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 DA8586E1; Tue, 24 Sep 2013 01:17:27 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail104.syd.optusnet.com.au (mail104.syd.optusnet.com.au [211.29.132.246]) by mx1.freebsd.org (Postfix) with ESMTP id 3ED942660; Tue, 24 Sep 2013 01:17:27 +0000 (UTC) Received: from c122-106-156-23.carlnfd1.nsw.optusnet.com.au (c122-106-156-23.carlnfd1.nsw.optusnet.com.au [122.106.156.23]) by mail104.syd.optusnet.com.au (Postfix) with ESMTPS id DE69B42184D; Tue, 24 Sep 2013 10:48:57 +1000 (EST) Date: Tue, 24 Sep 2013 10:48:45 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: sbruno@FreeBSD.org Subject: Re: kern/vfs_mount.c vfs_donmount() checks of MFSNAMELEN In-Reply-To: <1379975599.1593.10.camel@localhost> Message-ID: <20130924092642.P954@besplex.bde.org> References: <201309231802.r8NI2KmF083133@chez.mckusick.com> <1379975599.1593.10.camel@localhost> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=bpB1Wiqi c=1 sm=1 tr=0 a=ebeQFi2P/qHVC0Yw9JDJ4g==:117 a=PO7r1zJSAAAA:8 a=oauJmmNaXYEA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=kMyxM1bEWncA:10 a=rCNU_45lQ6v1koXlp8QA:9 a=QhwgJvBDTykYGVhM:21 a=QYbaFIYWtEo_1KnJ:21 a=CjuIK1q_8ugA:10 Cc: Kirk McKusick , 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 01:17:27 -0000 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