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

[Pedro Giffuni]

On 25/01/2018 14:24, Bruce Evans wrote:
> 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.
>
Mailer agents ... they all suck :(
>> 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.
>

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.

>> 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