Date: Wed, 24 Jan 2018 12:23:48 -0500 From: Pedro Giffuni <pfg@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: cem@freebsd.org, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: 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/... Message-ID: <a6b8fe83-5b1c-0202-828e-28dcf185b785@FreeBSD.org> In-Reply-To: <20180124182548.X1063@besplex.bde.org> References: <201801211542.w0LFgbsp005980@repo.freebsd.org> <CAG6CVpXxuFyHS11rF=NF6bSSkC2=xnDh=WnbK-aWp4sOomrZ7w@mail.gmail.com> <51ff8aef-5660-7857-e4d5-12cdc77bc071@FreeBSD.org> <20180124182548.X1063@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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 >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?a6b8fe83-5b1c-0202-828e-28dcf185b785>