Date: Fri, 9 Mar 2012 18:18:12 +0100 From: Peter Holm <peter@holm.cc> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r232692 - head/sys/ufs/ffs Message-ID: <20120309171812.GA52767@x2.osted.lan> In-Reply-To: <20120310012432.Y3108@besplex.bde.org> References: <201203081249.q28Cn9ed045648@svn.freebsd.org> <20120309145110.D1365@besplex.bde.org> <20120309064036.GA40029@x2.osted.lan> <20120310012432.Y3108@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120309171812.GA52767>