svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Bruce Evans]

On Fri, 26 Jan 2018, Pedro Giffuni wrote:

> On 01/26/18 06:36, Bruce Evans wrote:
>> On Thu, 25 Jan 2018, Pedro Giffuni wrote:
>> 
>>> On 25/01/2018 14:24, Bruce Evans wrote:
>>>> ...
>>>> This code only works because (if?) nfs is the only caller and nfs never
>>>> passes insane values.
>>>> 
>>> 
>>> I am starting to think that we should simply match uio_resid and set it to 
>>> ssize_t.
>>> Returning the value to int is certainly not the solution.
>> 
>> Of course using the correct type (int) is part of the solution.
>> 
> int *was* the correct type, now it doesn't cover all the range.

int is the correct type.  The range is small after range checking.

u_int is an incorrect type.  It can only hold SSIZE_MAX on __LP32_ arches,
and that only after half-baked range checking for the lower limit only.

ssize_t is an incorrect type.  It can hold SSIZE_MAX on all arches, but
that doesn't prevent various overflow bugs or requests to malloc()
SSIZE_MAX (plus a little more for rounding up) bytes unless there is
some range checking.

> Our problem is not really uio_*, our problem is ncookies and we only test 
> that it is >0.

Our problem is lack of understanding of lectures on C types and range checking.

> I think the attached patch, still keeping the unsigned ncookies, is 
> sufficient.

X Index: sys/fs/ext2fs/ext2_lookup.c
X ===================================================================
X --- sys/fs/ext2fs/ext2_lookup.c	(revision 328443)
X +++ sys/fs/ext2fs/ext2_lookup.c	(working copy)
X @@ -153,7 +153,10 @@
X  		return (EINVAL);
X  	ip = VTOI(vp);
X  	if (ap->a_ncookies != NULL) {
X -		ncookies = uio->uio_resid;
X +		if (uio->uio_resid < 0)
X +			ncookies = 0;
X +		else
X +			ncookies = uio->uio_resid;

This only avoids overflow for negative values.  It doesn't limit uio_resid
from above, so overflow on this assignment can occur on __LP64__ arches.
Even on __LP32__ systems, SSIZE_MAX = 2G is too large to work.  My patch
limits uio_resid to avoid this and many other bugs.

When overflow occurs, this gives the same bugs as before, but they are
larger than I noticed before.  E.g., uio->uio_resid == 1ULL << 32, then
the overflow is to 0.  There is no problem allocating a cookies array
with 0 elements, but since you didn't adjust uio_resid it is inconsistent
with ncookies.  Without INVARIANTS, buffer overrun occurs when the first
dirent is read and cookies[0] is modified to point to the dirent; then
ncookies was decremented from 0 to -1, but after you broke its type it
is decremented from 0 to UINT_MAX.  With INVARIANTS, the bug is detected
by a KASSERT() that ncookies > 0.

X  		if (uio->uio_offset >= ip->i_size)
X  			ncookies = 0;
X  		else if (ip->i_size - uio->uio_offset < ncookies)

When uio_resid is huge, that isn't usually a problem unless assigning it
to ncookies gives a small value.  Otherwise, ncookies is at least large
here.  Then it is usually larger than the residual file size, so it gets
replaced by the residual file size.  This is only too large to fit in
memory if the residual file size is large.

>> The bounds checking is something like:

>> [... context lost to corruption of spaces to binary (UTF-8))

>> This checks for negative values for all cases and converts to 0 (EOF) to
>> preserve historical behaviour for the syscall case and to avoid overflow
>> for the cookies case (in case the caller is buggy).  The correct handling
>> is to return EINVAL, but EOF is good enough.
>> 
> This also touches uio_resid which is probably not good as it is used later 
> on.
> Our job is not to "fix" the caller but only to apply a reasonable behavior.

This indeed too fragile.  I had throught that syscalls adjusted uio_resid
at the end (so could adjust it at the beginning too).  They actually just
calculate nbytes = initial_uio_resid - final_uio_resid.

But if you don't adjust uio_resid, then you get buffer overruns when there
are more dirents than cookies.  dirents are not malloc()ed, so there is no
limit on them except uio_resid, the size that can be malloc()ed for cookies,
and the residual file size.

>> In the syscall case, uio_resid can be up to SSIZE_MAX, so don't check it
>> or corrupt it by assigning it to an int or u_int.
>
> The minimal type conversion does not really involve corruption: ncookies is 
> local
> and the caller will not perceive any change.

The value is corrupted by blind assignmemt to an unrelated type.

Bruce