From owner-svn-src-head@FreeBSD.ORG Fri Mar 9 17:18:21 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 F3C5F106566B for ; Fri, 9 Mar 2012 17:18:20 +0000 (UTC) (envelope-from pho@holm.cc) Received: from relay00.pair.com (relay00.pair.com [209.68.5.9]) by mx1.freebsd.org (Postfix) with SMTP id B34FF8FC16 for ; Fri, 9 Mar 2012 17:18:20 +0000 (UTC) Received: (qmail 25424 invoked from network); 9 Mar 2012 17:18:13 -0000 Received: from 87.58.144.241 (HELO x2.osted.lan) (87.58.144.241) by relay00.pair.com with SMTP; 9 Mar 2012 17:18:13 -0000 X-pair-Authenticated: 87.58.144.241 Received: from x2.osted.lan (localhost [127.0.0.1]) by x2.osted.lan (8.14.4/8.14.4) with ESMTP id q29HICwh052814; Fri, 9 Mar 2012 18:18:12 +0100 (CET) (envelope-from pho@x2.osted.lan) Received: (from pho@localhost) by x2.osted.lan (8.14.4/8.14.4/Submit) id q29HICgU052813; Fri, 9 Mar 2012 18:18:12 +0100 (CET) (envelope-from pho) Date: Fri, 9 Mar 2012 18:18:12 +0100 From: Peter Holm To: Bruce Evans Message-ID: <20120309171812.GA52767@x2.osted.lan> References: <201203081249.q28Cn9ed045648@svn.freebsd.org> <20120309145110.D1365@besplex.bde.org> <20120309064036.GA40029@x2.osted.lan> <20120310012432.Y3108@besplex.bde.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120310012432.Y3108@besplex.bde.org> User-Agent: Mutt/1.4.2.3i 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 17:18:21 -0000 On Sat, Mar 10, 2012 at 01:48:20AM +1100, Bruce Evans wrote: > 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 Thank you for the analysis. I have tested your patch, without r232692 and it works as expected. I'll back out r232692. Thank you. -- Peter