Date: Fri, 26 Jan 2018 03:28:22 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: "Pedro F. Giffuni" <pfg@freebsd.org> 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: <20180126020540.B2181@besplex.bde.org> In-Reply-To: <201801241758.w0OHwm26063524@repo.freebsd.org> References: <201801241758.w0OHwm26063524@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 24 Jan 2018, Pedro F. Giffuni wrote: > Log: > ext2fs|ufs:Unsign some values related to allocation. > > When allocating memory through malloc(9), we always expect the amount of > memory requested to be unsigned as a negative value would either stand for > an error or an overflow. > Unsign some values, found when considering the use of mallocarray(9), to > avoid unnecessary casting. Also consider that indexes should be of > at least the same size/type as the upper limit they pretend to index. This might not break much, but it adds many more type errors and bogus (implicit) casts than it fixes. It actually changes the brokenness of the first variable touched: > Modified: head/sys/fs/ext2fs/ext2_lookup.c > ============================================================================== > --- head/sys/fs/ext2fs/ext2_lookup.c Wed Jan 24 17:52:06 2018 (r328345) > +++ head/sys/fs/ext2fs/ext2_lookup.c Wed Jan 24 17:58:48 2018 (r328346) > @@ -145,9 +145,9 @@ ext2_readdir(struct vop_readdir_args *ap) > off_t offset, startoffset; > size_t readcnt, skipcnt; > ssize_t startresid; > - int ncookies; > int DIRBLKSIZ = VTOI(ap->a_vp)->i_e2fs->e2fs_bsize; > int error; > + u_int ncookies; The first bug is only a style bug (unsorting the u_int after the ints even when its variable was accidentally already before the ints. > > if (uio->uio_offset < 0) > return (EINVAL); That is the only change in this file. No comment on other changes. ncookies is mostly used in contexts other than for multiplying by it before calling malloc(), and its type is now inconsistent with most other places. The other places are: 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(). Next, we do some bounds checking which seems to be correct modulo previous overflows, and show the care needed for unsigned variables (i_size is unsigned and must be compared with uio_offset which is signed). Next, we assign ncookies, to ap_ncookies which still has the correct type (plain signed int). If ncookies were actually large enough to need a u_int, or worse a 64-bit ssize_t, then this would overflow. Later, we KASSERT() that ncookies > 0. This might cause a compiler warning "unsigned comparison with 0" now that ncookies is unsigned. We count down ncookies, but the loop termination condition is complicated and we don't get any benefits from the possible micro-optimization of using ncookies as a loop counter that counts down to 0. To fix the problem that mallocarray() wanted a size_t arg, ncookies could be cast to size_t, but that would be silly. The prototype does the same cast automatically even for cases with sign mismatches. Now malloc() wants a type of size_t (after further breakage to change malloc()s arg type). Many conversions are still involved, and casting would at most limit compiler warnings: the code is now: X malloc(ncookies * sizeof(*cookies), ...) First, ncookies and sizeof(...) are promoted to a common type. When ncookies was int, usually it was promoted to size_t and sizeof(...) was not promoted. But on exotic arches with size_t smaller than int, sizeof(...) is promoted to int and ncookies is not promoted. Next, the type of the result of the multiplication is the common type. Finally, the prototype used to convert to u_long, but now converts to size_t. When the common type is size_t, then the conversion is now null. When the common type was int, the conversion was promotion to u_long, but it is now demotion to size_t. All this only obviously works in practice because all the variables and the product are small, so they are smaller than all of INT_MAX, SIZE_MAX and ULONG_MAX on all supported and unsupported arches. However, it is hard to write bounds checks that obviously handle all cases, except by using simple small bounds on the variables. It is now obvious that the bounds checking is broken for general use. There is no obvious limit on the malloc() except the file size. However, I think cookies are only used by nfs, so this is not a user-serving bug. nfs just has to limit its cookie size. It seems to use a limit of something like NFS_SRVMAXIO (128K). This also makes the overflow from the ssize_t type error unreachable. Note that it So all of this actually works very unobviously in practice. The sizes are only small because nfs is the only (?) caller and it never asks for large sizes. ffs has almost the same code, so has the same fragility. This fragility goes back to at least FreeBSD-3, but for ffs it was only in the big-endian case then (this is the case which always had to copy and convert the on-disk layout). This changed relatively recently, because the ffs on-disk layout only matches struct dirent for 32-bit ino_t. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180126020540.B2181>