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
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
