From owner-svn-src-head@FreeBSD.ORG Fri Mar 9 14:48:37 2012 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 BC2C1106566C; Fri, 9 Mar 2012 14:48:37 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail26.syd.optusnet.com.au (mail26.syd.optusnet.com.au [211.29.133.167]) by mx1.freebsd.org (Postfix) with ESMTP id 5394D8FC16; Fri, 9 Mar 2012 14:48:36 +0000 (UTC) Received: from c211-30-171-136.carlnfd1.nsw.optusnet.com.au (c211-30-171-136.carlnfd1.nsw.optusnet.com.au [211.30.171.136]) by mail26.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q29EmPHS006290 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 10 Mar 2012 01:48:28 +1100 Date: Sat, 10 Mar 2012 01:48:20 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Peter Holm In-Reply-To: <20120309064036.GA40029@x2.osted.lan> Message-ID: <20120310012432.Y3108@besplex.bde.org> References: <201203081249.q28Cn9ed045648@svn.freebsd.org> <20120309145110.D1365@besplex.bde.org> <20120309064036.GA40029@x2.osted.lan> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r232692 - head/sys/ufs/ffs 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: Fri, 09 Mar 2012 14:48:37 -0000 On Fri, 9 Mar 2012, Peter Holm wrote: > On Fri, Mar 09, 2012 at 03:51:30PM +1100, Bruce Evans wrote: >> On Thu, 8 Mar 2012, Peter Holm wrote: >> >>> Log: >>> syscall() fuzzing can trigger this panic. Return EINVAL instead. >>> >>> MFC after: 1 week >> >> If so, then, this is not the place to hide the bug. > > OK > > This specific problem here was discover by: > > for (i = 0; i < 2000; i++) { > if ((dirp = opendir(path)) == NULL) > break; > bcopy(dirp, &fuzz, sizeof(fuzz)); > fuzz.dd_len = arc4random(); > readdir(&fuzz); > closedir(dirp); > } Try this fix. You already fixed getdirentries() in 2008, but it was broken recently. % Index: vfs_syscalls.c % =================================================================== % RCS file: /home/ncvs/src/sys/kern/vfs_syscalls.c,v % retrieving revision 1.522 % diff -u -2 -r1.522 vfs_syscalls.c % --- vfs_syscalls.c 21 Feb 2012 01:05:12 -0000 1.522 % +++ vfs_syscalls.c 9 Mar 2012 14:22:57 -0000 % @@ -4154,7 +4154,7 @@ % % AUDIT_ARG_FD(fd); % - auio.uio_resid = count; % - if (auio.uio_resid > IOSIZE_MAX) % + if (count > IOSIZE_MAX) % return (EINVAL); % + auio.uio_resid = count; % if ((error = getvnode(td->td_proc->p_fd, fd, CAP_READ | CAP_SEEK, % &fp)) != 0) The broken version checked `count' only after possibly clobbering it. Clobbering occurs in practice on 32-bit arches, since then uio_resid has type int so it cannot hold a value of INT_MAX + 1U. The behaviour was undefined. The actual behaviour was normally to store a negative value in uio_resid. Comparing this with IOSIZE_MAX then always passed, since IOSIZE_MAX is a large int. `count' only has type u_int, not the usual size_t. It would be safer to keep comparing it with INT_MAX. Supporting directory reads of more than INT_MAX bytes is even more useless bloat than supporting regular files reads of more than INT_MAX bytes. But it seems safe enough. The limit is still INT_MAX on 32-bit arches. On 64-bit arches, it is now UINT_MAX = 2**32-1, not actually IOSIZE_MAX = 2**63-1 like the code checks for, since u_int can't go nearly as high as IOSIZE_MAX. At least after the above change, the compiler should see that the check always passes, and not generate any code for it, and maybe complain about it. Perhaps this is why it was broken. However, the compiler can't see this now, since IOSIZE_MAX is actually a macro that depends on a configuration variable, so it isn't always > UINT_MAX on 64-bit arches. Without the dynamic configuration, the broken check would obviously always pass on 32-bit arches, and the compiler complaining about it would be just what was required to avoid the bug. And even with dynamic configuration, it is almost obvious that the check always passed, since IOSIZE_MAX is (var ? INT_MAX : SSIZE_MAX), which is always INT_MAX. Bruce