svn commit: r328218 - in head/sys: amd64/amd64 arm/xscale/ixp425 arm64/arm64 cam cam/ctl compat/ndis dev/aacraid dev/advansys dev/ath dev/beri/virtio dev/bnxt dev/bwn dev/ciss dev/cxgbe/crypto dev/...

Wed Jan 24 17:44:10 UTC 2018

On 01/24/18 03:50, Bruce Evans wrote:
> On Tue, 23 Jan 2018, Pedro Giffuni wrote:
>
>> On 23/01/2018 14:08, Conrad Meyer wrote:
>>> Hi Pedro,
>>>
>>> On Sun, Jan 21, 2018 at 7:42 AM, Pedro F. Giffuni <pfg at freebsd.org> 
>>> wrote:
>>>> Author: pfg
>>>> Date: Sun Jan 21 15:42:36 2018
>>>> New Revision: 328218
>>>> URL: https://svnweb.freebsd.org/changeset/base/328218
>>>>
>>>> Log:
>>>>    Revert r327828, r327949, r327953, r328016-r328026, r328041:
>>>>    Uses of mallocarray(9).
>>>>
>>>>    The use of mallocarray(9) has rocketed the required swap to 
>>>> build FreeBSD.
>>>>    This is likely caused by the allocation size attributes which 
>>>> put extra pressure
>>>>    on the compiler.
>>> I'm confused about this change.  Wouldn't it be better to remove the
>>> annotation/attributes from mallocarray() than to remove the protection
>>> against overflow?
>
> It would be better to remove mallocarray().
>
I am fine with that: Its probably better to stop the bug now before it 
propagates.
This said, several drivers, including drm2, have #defines to do the same.

>> Not in my opinion: it would be better to detect such overflows at 
>> compile time (or through a static analyzer) than to have late 
>> notification though panics. The blind use of mallocarray(9) is 
>> probably a mistake also: we shouldn't use it unless there is some 
>> real risk of overflow.
>
> Excellent!  There is no real risk of overflow except in cases that are
> broken anyway.  The overflow checks in mallocarray() are insufficient 
> even
> for detecting overflow and don't detect most broken cases when they are
> sufficient.  This leaves no cases where even non-blind use of it does any
> good.
>
>>>    (If the compiler is fixed in the future to not use
>>> excessive memory with these attributes, they can be conditionalized on
>>> compiler version, of course.)
>>
>> All in all, the compiler is not provably wrong: it's just using more 
>> swap space, which is rather inconvenient for small platforms but not 
>> necessarily wrong.
>
> I don't know why the compiler uses more swap space, but the general 
> brokenness
> of mallocarray() is obvious.
>
> Callers must somehow ensure that the allocation size is not too large.
> Too large is closer to 64K than SIZE_MAX for most allocations. Some
> bloatware allocates a few MB, but malloc() is rarely used for that.
> vmstat -m has been broken to not show a summary of sizes at the end,
> but on freefall now it seems to shows a maximum malloc() size of 64K,
> and vmstat -z confirms this by not showing any malloc() bucket sizes
> larger than 64K, and in fact kern_malloc.c only has statically allocatd
> bucket sizes up to 64K.  (vmstat -z never had a summary at the end to
> break).  Much larger allocations are mostly done using k*alloc(), and
> slightly larger allocations are usually done using UMA.  zfs does
> zillions of allocations using UMA, and the largest one seems to be 16M.
>
> If the caller doesn't check, this gives a Denial of Service security hole
> if the size and count are controlled by userland.  If the size and count
> are controlled by the kernel, then the overflow check is not very useful.
> It is less useful than most KASSERT()s which are left out of production
> kernels.  The caller must keep its allocation sizes not much more than
> 64K, and once it does that it is unlikely to make them larger than 
> SIZE_MAX
> sometimes.
>
> The overflow checks in mallocarray have many type errors so they don't
> always work:
>
> X Modified: head/share/man/man9/malloc.9
> X 
> ==============================================================================
> X --- head/share/man/man9/malloc.9    Sun Jan  7 10:29:15 2018 (r327673)
> X +++ head/share/man/man9/malloc.9    Sun Jan  7 13:21:01 2018 (r327674)
> X @@ -45,6 +45,8 @@
> X  .In sys/malloc.h
> X  .Ft void *
> X  .Fn malloc "unsigned long size" "struct malloc_type *type" "int flags"
> X +.Ft void *
> X +.Fn mallocarray "size_t nmemb" "size_t size" "struct malloc_type 
> *type" "int flags"
>
> One type error is already obvious.  malloc(9)'s arg doesn't have type 
> size_t
> like malloc(3)'s arg.  The arg type is u_long in the kernel. 
> mallocarray()'s
> types are inconsistent with this.
>

This would be relatively easy to "fix", at least so that the types match 
malloc(9).
The rest is likely more painful, if it has solution at all.

> size_t happens to have the same representation as u_long on all supported
> arches, so this doesn't cause many problems now.  On 32-bit arches, 
> size_t
> hs type u_int.  u_int has smaller rank than u_long, so compilers could
> reasonably complain that converting a u_long to a size_t is a downcast.
> They shouldn't complain that converting a size_t to a u_long to pass it
> to malloc() is an upcast, so there is no problem in typical sloppy code
> that uses size_t for sizes and passes these to malloc().  More careful
> ciode would use u_long for size to pass to malloc() and compiler's might
> complain about downcasting to pass to mallocarray() instead.
>
> Otherwise (on exotic machines with size_t larger than u_long), passing
> size_t's to malloc() is a bug unless they values have been checked to
> be <= ULONG_MAX, and mallocarrary()'s inconsistent API expands this bug.
>
> X Modified: head/sys/kern/kern_malloc.c
> X 
> ==============================================================================
> X --- head/sys/kern/kern_malloc.c    Sun Jan  7 10:29:15 2018 (r327673)
> X +++ head/sys/kern/kern_malloc.c    Sun Jan  7 13:21:01 2018 (r327674)
> X @@ -532,6 +533,22 @@ malloc(unsigned long size, struct malloc_type 
> *mtp, in
> X          va = redzone_setup(va, osize);
> X  #endif
> X      return ((void *) va);
> X +}
> X +
> X +/*
> X + * This is sqrt(SIZE_MAX+1), as s1*s2 <= SIZE_MAX
> X + * if both s1 < MUL_NO_OVERFLOW and s2 < MUL_NO_OVERFLOW
> X + */
> X +#define MUL_NO_OVERFLOW        (1UL << (sizeof(size_t) * 8 / 2))
>
> SIZE_MAX and its square root are not the thing to check here. malloc()'s
> limit is ULONG_MAX.
>
> Using 1UL here assumes hat u_long is no smaller than size_t. Otherwise,
> there the shift overflows when size_t is more than twice as large as
> u_long so that even the square root doesn't fit. That would be very
> exotic.  More likely, size_t is only slightly larger than u_long. Then
> the shift doesn't overflow, and MUL_NO_OVERFLOW has type u_long instead
> of the intended size_t.  Since we don't square it, there are problems
> with the square overflowing.
>
> X +void *
> X +mallocarray(size_t nmemb, size_t size, struct malloc_type *type, 
> int flags)
>
> No function API can work for bounds checking, since function args have 
> some
> type and converting to that type may overflow before any overflow check
> can be done.  Even uintmax_t doesn't quite work for the arg types, since
> perverse callers might start with floating point values. uintmax_t is
> enough in the kernel since the kernel doesn't support FP.
>
> X +{
> X +
> X +    if ((nmemb >= MUL_NO_OVERFLOW || size >= MUL_NO_OVERFLOW) &&
> X +        nmemb > 0 && SIZE_MAX / nmemb < size)
> X +        return (NULL);
> X +
> X +    return (malloc(size * nmemb, type, flags));
>
> [This is the original version that returns instead of panicing.)
>
> Except for overflow in the shift in MUL_NO_OVERFLOW, this gives a non-
> overflowing (size * member) with value <= SIZE_MAX, but it needs to be
> <= ULONG_MAX.  Overflow in the product gives undefined behaviour, but
> overflow in the arg passing (either to pass size and nmemb here or the
> product to malloc()) only gives implementation-defined behaviour (usually
> a truncated value).
>
> The bugs are only latent, but in some cases the buggy call to mallocarray
> replaces correct code that does something like:
>
>     u_long nmemb, size;
>
>     size = size(struct foo);    /* small, so doesn't need check */
>     nmemb = whatever();        /* API must ensure it fits in u_long */
>     if (nmemb >= NMEMB_LIMIT)
>         return (EINVAL);    /* not ENOMEM */
>     p = malloc(nmemb * size, M_FOO, M_WAITOK);
>
> The non-hard-coded limit(s) make it non-obvious that the product is 
> small,
> but this code has to trust that the limits are small enough.
>
> If this is converted to mallocarray(), it remains correct, but only 
> because
> the buggy and incomplete range checking in mallocarray has no effect 
> since
> this already did complete checking.  (The checking here guarantees:
> - that size and nmemb are small, so they can be passed to mallocarray()
>   despite it having inconsistent arg types. - that the product can't 
> overflow, so we return EINVAL here instead of
>   panicing later.  (The original version returns an errror, which breaks
>   the M_WAITOK case.  The current version panics, which breaks the 
> M_NOWAIT
>   case.)
> - that the product is small.  mallocarray() just doesn't check for this.
>
> Bruce
>