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

[Bruce Evans]

On Thu, 25 Jan 2018, Pedro Giffuni wrote:

This is almost unreadable due to hard-coded UTF-8 (mostly for tabs corrupted
to spaces) even in previously-literally quoted C code.

> On 01/25/18 11:28, Bruce Evans wrote:
>> On Wed, 24 Jan 2018, Pedro F. Giffuni wrote:

[... Most unreadable lines deleted]

>> XÂ Â Â Â  ncookies = uio->uio_resid;
>> 
>> This has a more serious type error and consequent overflow bugs. uio_resid
>> used to have type int, and cookies had to have type int to match that, else
>> there overflow occurs before the bounds checks.  Now uio_resid has type
>> ssize_t, which is excessively large on 64-bit arches (64 bits then), so the
>> assignment overflowed when ncookies had type int and uio_resid > INT_MAX.
>> Now it overflows differently when uio_resid > UINT_MAX, and unsign 
>> extension
>> causes overflow when uio_resid < 0.  There might be a sanity check on
>> uio_resid at higher levels, but I can only find a check related to EOF in
>> vfs_read_dirent().
>> 
> I will argue that none of the code in this function is prepared for the 
> eventually of
> uio->uio_resid < 0

All of it except the cookies code has to be prepared for that, and seems
to handle it OK, since this userland can set uio_resid.  The other code
is not broken by either the ssize_t expansion or the unsigned bugs, since
it mostly doesn't truncate uio_resid by assigning it to a variable of the
wrong type (it uses uio->uio_resid in-place, except for copying its initial
value to startresid, and startresid is not missing the ssize_t expansion).
It mostly does comparisons of the form (uio->uio_resid > 0), where it is
0 in uio_resid means EOF, negative is treated as EOF, and strictly positive
means more to do.

There is a clear up-front check that uio_offset >= 0 (return EINVAL if
uio_offset < 0).  This is not needed for the trusted nfs caller, but is
needed for syscalls and is done for both.

> In that case we would have a rather spectacular failure in malloc.
> Unsigning ncookies is a theoretical, although likely impractical, improvement 
> here.

No, it increases the bug slightly.  E.g., if uio_resid is -1, ncookies
was -1 / (offsetof(...) + 4) + 1 = 0 + 1 after rounding.  This might even
work (1 cookie at a time, just like if the caller asked for that).  Now
ncookies is -1U / (offsetof(...) + 4) + 1 = a large value.  However, if
uio_resid was slightly more negative than -2 * (offsetof(...) + 4), then
ncookies was -1 and in the multiplication this overflows to -1U = a large
value and the result is much the same as for earlier overflow on assignment
to u_int ncookies.

This code only works because (if?) nfs is the only caller and nfs never
passes insane values.

> It is not clear to me that using int or u_int makes a difference given it is 
> a local variable
> and in this scope the signedness of the variable is basically irrelevant.

It is clear to me that overflow bugs occur with both if untrusted callers are
allowed.

> From a logical point of view .. we can't really have a negative number of 
> cookies.

Malicious and buggy callers do illogical things to get through holes in
your logic.

It is also illogical to have a zero number of cookies, but ncookies can
be 0 in various ways.  First, ncookies is 0 in the EOF case (and cookies
are requested).  We depend on 0 not being an invalid size for malloc()
so that we malloc() nothing and later do more nothings in the main loop.
This is a standard use for 0.  If you don't like negative numbers, then
you also shouldn't like 0.  Both exist to make some calculations easier.
Later, ncookies is abused as a residual count, so it becomes 0 at the end.
This is another standard use for 0.

Bruce