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

[Bruce Evans]

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