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

[Pedro Giffuni]

On 01/25/18 11:28, Bruce Evans wrote:
> 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.
>
Yup, my error.

>>
>>     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().
>
I will argue that none of the code in this function is prepared for the 
eventually of
uio->uio_resid < 0

In that case we would have a rather spectacular failure in malloc.
Unsigning ncookies is a theoretical, although likely impractical, 
improvement here.

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.

 From a logical point of view .. we can't really have a negative number 
of cookies.

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