Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 10 Mar 2012 01:48:20 +1100 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Peter Holm <peter@holm.cc>
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:  <20120310012432.Y3108@besplex.bde.org>
In-Reply-To: <20120309064036.GA40029@x2.osted.lan>
References:  <201203081249.q28Cn9ed045648@svn.freebsd.org> <20120309145110.D1365@besplex.bde.org> <20120309064036.GA40029@x2.osted.lan>

next in thread | previous in thread | raw e-mail | index | archive | help
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



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120310012432.Y3108>