From owner-svn-src-head@freebsd.org Wed Jan 24 17:34:02 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 7BA18EB7438 for ; Wed, 24 Jan 2018 17:34:02 +0000 (UTC) (envelope-from pfg@FreeBSD.org) Received: from sonic312-36.consmr.mail.ne1.yahoo.com (sonic312-36.consmr.mail.ne1.yahoo.com [66.163.191.217]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 194AD6B387 for ; Wed, 24 Jan 2018 17:34:01 +0000 (UTC) (envelope-from pfg@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1516815241; bh=0tnM+4WK3RTsvPgUpZE3DFXXd+UGALjCIZIbiffWBI8=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From:Subject; b=L3JIC5WZozabcaPv9WIONiaQLuMBvUduYDBPTosE2FnsXPdhpN6bjOypVMw01t+zYKx3mJeLNwn5HS/SFSDeOpO+ysHJcF83Ucci95Vt5amlLnAb+RELFXdus4R7IiMUm3ztCuxD8XNrxeD/c92IvEizeColTOJIQqweMlMheV13QzHTbjANWrat56CimeMIbSHHbrF0lFs9HvcvxveRKuu0PzEU9jcZeAagfvWlmv+9u+a69zsR1xJSXAJERV7QR/BWie1LKPTMi7IKFKtE/b+d5kLIvLGb2ZS84LC9B3q1+mW+42yHdAUk95pVFkIUt7Nx6BzMnXv5TZJci62VOw== X-YMail-OSG: rwcX2osVM1ljHhovnUsIIGV6dfasEjuF8Rby2vQtOQOdytyhKFaA3FfEKDm4IsU 8gTSqS2RN5u9Rw3vDgqoyp2ft2wWoboXE7QFM420rxuFtrL9LZic6ZgB_trdrPl5mcGthUNYajQs EYwlqG.ZFXEgfKcHPqT3ft56DddVk5zUBg.NwnNRd8jvQ6Ju3dd9_fe15Oubt2aFeXCBgWuFwci2 arojLNZTFbBOJJi_PqHQ2GTCDbcfWD9CHVhvObUqRVSSSXDec4WR_QHTstfoRFM40.6b7f6Pq5.N 9uhX1VCOepGJ7Yivb2Q6dFiU6NUoTdya_tihvnU75Bd91z9w5nh8mztKR8niNw1pSPa06ukm0Kzz 80SKBuVO7kkS0bYzjGPw.FErWde4JZ_gnFIGGaOAu1oUg.FyMnv_Douy8z_hCiM4_LFq82OBwLjX cGmjAF8_Fp4ixZOh0oK_qcylZnjsWYdun5y5JDhAhn49QUGAiptZ0TrfslXLR5gS1TXLj5ESfpLu KQrq4mtayTQ-- Received: from sonic.gate.mail.ne1.yahoo.com by sonic312.consmr.mail.ne1.yahoo.com with HTTP; Wed, 24 Jan 2018 17:34:01 +0000 Received: from smtp235.mail.ne1.yahoo.com (EHLO [192.168.0.8]) ([10.218.253.206]) by smtp409.mail.ne1.yahoo.com (JAMES SMTP Server ) with ESMTPA ID 59128b338738b6cab9c8ce7b42745176; Wed, 24 Jan 2018 17:23:49 +0000 (UTC) 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/... To: Bruce Evans Cc: cem@freebsd.org, src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <201801211542.w0LFgbsp005980@repo.freebsd.org> <51ff8aef-5660-7857-e4d5-12cdc77bc071@FreeBSD.org> <20180124182548.X1063@besplex.bde.org> From: Pedro Giffuni Message-ID: Date: Wed, 24 Jan 2018 12:23:48 -0500 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180124182548.X1063@besplex.bde.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 24 Jan 2018 17:34:02 -0000 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 >>> 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 >