From owner-svn-src-head@FreeBSD.ORG Thu Dec 18 20:16:13 2008 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id A8424106567F; Thu, 18 Dec 2008 20:16:13 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from harmony.bsdimp.com (bsdimp.com [199.45.160.85]) by mx1.freebsd.org (Postfix) with ESMTP id 4381A8FC27; Thu, 18 Dec 2008 20:16:13 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from localhost (localhost [127.0.0.1]) by harmony.bsdimp.com (8.14.2/8.14.1) with ESMTP id mBIKDRIn005746; Thu, 18 Dec 2008 13:13:27 -0700 (MST) (envelope-from imp@bsdimp.com) Date: Thu, 18 Dec 2008 13:13:30 -0700 (MST) Message-Id: <20081218.131330.63052780.imp@bsdimp.com> To: obrien@FreeBSD.org From: "M. Warner Losh" In-Reply-To: <200812181844.mBIIikVF049455@svn.freebsd.org> References: <200812181844.mBIIikVF049455@svn.freebsd.org> X-Mailer: Mew version 5.2 on Emacs 21.3 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r186291 - head/sbin/mount X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 18 Dec 2008 20:16:13 -0000 In message: <200812181844.mBIIikVF049455@svn.freebsd.org> "David E. O'Brien" writes: : Author: obrien : Date: Thu Dec 18 18:44:46 2008 : New Revision: 186291 : URL: http://svn.freebsd.org/changeset/base/186291 : : Log: : Be a little bit more pestimistic in argument handling - check if we've : overflown our internal buffer (though after the fact), and s/strncpy/strlcpy/ : : Reviewed by: rodrigc : Obtained from: Juniper Networks : : Modified: : head/sbin/mount/mount.c : head/sbin/mount/mount_fs.c : : Modified: head/sbin/mount/mount.c : ============================================================================== : --- head/sbin/mount/mount.c Thu Dec 18 18:29:15 2008 (r186290) : +++ head/sbin/mount/mount.c Thu Dec 18 18:44:46 2008 (r186291) : @@ -68,6 +68,8 @@ static const char rcsid[] = : #define MOUNT_META_OPTION_FSTAB "fstab" : #define MOUNT_META_OPTION_CURRENT "current" : : +#define MAX_ARGS 100 : + : int debug, fstab_style, verbose; : : char *catopt(char *, const char *); : @@ -501,7 +503,7 @@ int : mountfs(const char *vfstype, const char *spec, const char *name, int flags, : const char *options, const char *mntopts) : { : - char *argv[100]; : + char *argv[MAX_ARGS]; : struct statfs sf; : int argc, i, ret; : char *optbuf, execname[PATH_MAX], mntpath[PATH_MAX]; : @@ -546,6 +548,10 @@ mountfs(const char *vfstype, const char : argv[argc++] = strdup(name); : argv[argc] = NULL; : : + if (MAX_ARGS <= argc ) : + errx(1, "Cannot process more than %d mount arguments", : + MAX_ARGS); : + This is useless. Once you've overflowed the buffer, your stack is potentially shot, and all kinds of fun can happen downstream from there. In particular, there's no guarantee that argc isn't corrupted as well... Also, the style of the check is inconsistent with other parts of the system: : + if (argc > MAX_ARGS) would be more consistent, and not suffer from the space before the paren problem as well :) Warner : if (debug) { : if (use_mountprog(vfstype)) : printf("exec: mount_%s", vfstype); : : Modified: head/sbin/mount/mount_fs.c : ============================================================================== : --- head/sbin/mount/mount_fs.c Thu Dec 18 18:29:15 2008 (r186290) : +++ head/sbin/mount/mount_fs.c Thu Dec 18 18:44:46 2008 (r186291) : @@ -88,7 +88,7 @@ mount_fs(const char *vfstype, int argc, : char *p, *val; : int ret; : : - strncpy(fstype, vfstype, sizeof(fstype)); : + strlcpy(fstype, vfstype, sizeof(fstype)); : memset(errmsg, 0, sizeof(errmsg)); : : getmnt_silent = 1; :