Date: Wed, 24 Jan 2018 19:50:33 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Pedro Giffuni <pfg@freebsd.org> 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: <20180124182548.X1063@besplex.bde.org> In-Reply-To: <51ff8aef-5660-7857-e4d5-12cdc77bc071@FreeBSD.org> References: <201801211542.w0LFgbsp005980@repo.freebsd.org> <CAG6CVpXxuFyHS11rF=NF6bSSkC2=xnDh=WnbK-aWp4sOomrZ7w@mail.gmail.com> <51ff8aef-5660-7857-e4d5-12cdc77bc071@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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(). > 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. 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?20180124182548.X1063>