Date: Thu, 25 Jan 2018 15:01:15 -0500 From: Pedro Giffuni <pfg@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs Message-ID: <21b6bdda-65b7-89da-4dd6-bed64978eba8@FreeBSD.org> In-Reply-To: <20180126053133.R3207@besplex.bde.org> References: <201801241758.w0OHwm26063524@repo.freebsd.org> <20180126020540.B2181@besplex.bde.org> <8d5ddd06-14b2-e7e1-14dd-5e9d42f9b33c@FreeBSD.org> <20180126053133.R3207@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?21b6bdda-65b7-89da-4dd6-bed64978eba8>