From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 109927 invoked by alias); 17 Jan 2018 22:01:24 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 109803 invoked by uid 89); 17 Jan 2018 22:01:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy= X-HELO: mail-qt0-f172.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=D9teuDsIqPYRQzXmEBEcLc5xSoFL6HiVzxvTNhKCbME=; b=nh8LreGylPKUBSUBrbqz/UNpJhuMWzbjU9a9Res8vu8yQwPsYj8Z5tjjENgvWR9vJx QYwok7Kp7rF64XUOV/yOUSh3zdsObUm1d1qJUxns+h0cVsWgjQPOUG45iO9roMgkNtIX /ceMSvel59yTJ5QGHKEgyOifHXukFgYDSWZlodmcR7UNYUV5FHL0xHQBmqaKapWjLDSY q8gCIK8lZjAvEROuhJVaa8md6Q4UiyjrXzLKfBY4dWd3N+HacMBb/0OcTWcFRG4hHveh ZQFoK3zb8hXphZzM8xjnacLdK8ib/auE3q/Enoe5hXVjseRzYc4I4k4Uv5pDSNqZzZLm Qtlw== X-Gm-Message-State: AKwxytd8GYauKO9GegzoPQIAiruqDQEyAY1hD5ZhpwvnJG+ruPwDR4Ll +OBWzBF/Nxu1Q2gfaYD05cxxRA== X-Google-Smtp-Source: ACJfBosrRy8F8krgkwty0iWWTnKG6II9D+gENaz+Pmhh9aDKQPoZp0HNmfdkK1/4Xo6OBZVkbFD86w== X-Received: by 10.55.182.2 with SMTP id g2mr57319725qkf.273.1516226465341; Wed, 17 Jan 2018 14:01:05 -0800 (PST) Subject: Re: [PATCH] Fix integer overflows in internal memalign and malloc functions [BZ #22343] To: Arjun Shankar , libc-alpha@sourceware.org Cc: Paul Eggert References: <20180117202641.GA58783@aloka.lostca.se> From: Carlos O'Donell Message-ID: Date: Wed, 17 Jan 2018 22:01:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <20180117202641.GA58783@aloka.lostca.se> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-01/txt/msg00585.txt.bz2 On 01/17/2018 12:26 PM, Arjun Shankar wrote: > When posix_memalign is called with an alignment less than MALLOC_ALIGNMENT > and a requested size close to SIZE_MAX, it falls back to malloc code > (because the alignment of a block returned by malloc is sufficient to > satisfy the call). In this case, an integer overflow in _int_malloc leads > to posix_memalign incorrectly returning successfully. > > Upon fixing this and writing a somewhat thorough regression test, it was > discovered that when posix_memalign is called with an alignment larger than > MALLOC_ALIGNMENT (so it uses _int_memalign instead) and a requested size > close to SIZE_MAX, a different integer overflow in _int_memalign leads to > posix_memalign incorrectly returning successfully. > > Both integer overflows affect other memory allocation functions that use > _int_malloc (one affected malloc in x86) or _int_memalign as well. > > This commit fixes both integer overflows. In addition to this, it adds a > regression test to guard against false successful allocations by the > following memory allocation functions when called with too-large allocation > sizes and, where relevant, various valid alignments: > malloc, realloc, calloc, reallocarray, memalign, posix_memalign, > aligned_alloc, valloc, and pvalloc. OK with the removal of the timeout (new support framework has 20s default timeout which I didn't know about!), and formatting adjustment. Reviewed-by: Carlos O'Donell > ChangeLog: > > 2018-01-16 Arjun Shankar > > [BZ #22343] > * malloc/malloc.c (checked_request2size): call REQUEST_OUT_OF_RANGE > after padding. > (_int_memalign): check for integer overflow before calling > _int_malloc. > * malloc/tst-malloc-too-large.c: New test. > * malloc/Makefile: Add tst-malloc-too-large. > --- > v1 discussion: https://sourceware.org/ml/libc-alpha/2018-01/msg00133.html > > v2: > * uses braces nested within parentheses for checked_request2size > * increases timeout for tst-malloc-too-large to 5 seconds; I realized that > the test runs for 1.4s on my fairly modern laptop) OK. > > malloc/Makefile | 1 + > malloc/malloc.c | 30 +++-- > malloc/tst-malloc-too-large.c | 254 ++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 277 insertions(+), 8 deletions(-) > create mode 100644 malloc/tst-malloc-too-large.c > > diff --git a/malloc/Makefile b/malloc/Makefile > index 4266c2b66b..17873e67c4 100644 > --- a/malloc/Makefile > +++ b/malloc/Makefile > @@ -36,6 +36,7 @@ tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \ > tst-alloc_buffer \ > tst-malloc-tcache-leak \ > tst-malloc_info \ > + tst-malloc-too-large \ OK. > > tests-static := \ > tst-interpose-static-nothread \ > diff --git a/malloc/malloc.c b/malloc/malloc.c > index f5aafd2c05..740bb16799 100644 > --- a/malloc/malloc.c > +++ b/malloc/malloc.c > @@ -1224,14 +1224,21 @@ nextchunk-> +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > MINSIZE : \ > ((req) + SIZE_SZ + MALLOC_ALIGN_MASK) & ~MALLOC_ALIGN_MASK) > > -/* Same, except also perform argument check */ > - > -#define checked_request2size(req, sz) \ > - if (REQUEST_OUT_OF_RANGE (req)) { \ > - __set_errno (ENOMEM); \ > - return 0; \ > - } \ > - (sz) = request2size (req); > +/* Same, except also perform an argument and result check. First, we check > + that the padding done by request2size didn't result in an integer > + overflow. Then we check (using REQUEST_OUT_OF_RANGE) that the resulting > + size isn't so large that a later alignment would lead to another integer > + overflow. */ > +#define checked_request2size(req, sz) \ > + ({ \ > + (sz) = request2size (req); \ > + if (((sz) < (req)) \ > + || REQUEST_OUT_OF_RANGE (sz)) \ > + { \ > + __set_errno (ENOMEM); \ > + return 0; \ > + } \ > + }) My apologies, let me clarify again what I was looking for. ({ start on the first char as-if it were a function. e.g. #define checked_request2size(req, sz) \ ({ \ (sz) = request2size (req); \ if (((sz) < (req)) \ || REQUEST_OUT_OF_RANGE (sz)) \ { \ __set_errno (ENOMEM); \ return 0; \ } \ }) \'s tabbed+spaced out to column 79, but first \ is right beside the function (IMO the best style). OK with those changes. > > /* > --------------- Physical chunk operations --------------- > @@ -4678,6 +4685,13 @@ _int_memalign (mstate av, size_t alignment, size_t bytes) > */ > > > + /* Check for overflow. */ > + if (nb > SIZE_MAX - alignment - MINSIZE) > + { > + __set_errno (ENOMEM); > + return 0; > + } OK. > + > /* Call malloc with worst case padding to hit alignment. */ > > m = (char *) (_int_malloc (av, nb + alignment + MINSIZE)); > diff --git a/malloc/tst-malloc-too-large.c b/malloc/tst-malloc-too-large.c > new file mode 100644 > index 0000000000..1ab3ef1764 > --- /dev/null > +++ b/malloc/tst-malloc-too-large.c > @@ -0,0 +1,254 @@ > +/* Test and verify that too-large memory allocations fail with ENOMEM. OK. > + Copyright (C) 2018 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + . */ > + > +/* Bug 22375 reported a regression in malloc where if after malloc'ing then > + free'ing a small block of memory, malloc is then called with a really > + large size argument (close to SIZE_MAX): instead of returning NULL and > + setting errno to ENOMEM, malloc incorrectly returns the previously > + allocated block instead. Bug 22343 reported a similar case where > + posix_memalign incorrectly returns successfully when called with an with > + a really large size argument. > + > + Both of these were caused by integer overflows in the allocator when it > + was trying to pad the requested size to allow for book-keeping or > + alignment. This test guards against such bugs by repeatedly allocating > + and freeing small blocks of memory then trying to allocate various block > + sizes larger than the memory bus width of 64-bit targets, or almost > + as large as SIZE_MAX on 32-bit targets supported by glibc. In each case, > + it verifies that such impossibly large allocations correctly fail. */ > + > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > + > +/* This function prepares for each 'too-large memory allocation' test by > + performing a small successful malloc/free and resetting errno prior to > + the actual test. */ > +static void > +test_setup (void) > +{ > + void *volatile ptr = malloc (16); > + TEST_VERIFY_EXIT (ptr != NULL); > + free (ptr); > + errno = 0; > +} > + > + > +/* This function tests each of: > + - malloc (SIZE) > + - realloc (PTR_FOR_REALLOC, SIZE) > + - for various values of NMEMB: > + - calloc (NMEMB, SIZE/NMEMB) > + - calloc (SIZE/NMEMB, NMEMB) > + - reallocarray (PTR_FOR_REALLOC, NMEMB, SIZE/NMEMB) > + - reallocarray (PTR_FOR_REALLOC, SIZE/NMEMB, NMEMB) > + and precedes each of these tests with a small malloc/free before it. */ > +static void > +test_large_allocations (size_t size) > +{ > + void * ptr_to_realloc; > + > + test_setup (); > + TEST_VERIFY (malloc (size) == NULL); > + TEST_VERIFY (errno == ENOMEM); > + > + ptr_to_realloc = malloc (16); > + TEST_VERIFY_EXIT (ptr_to_realloc != NULL); > + test_setup (); > + TEST_VERIFY (realloc (ptr_to_realloc, size) == NULL); > + TEST_VERIFY (errno == ENOMEM); > + free (ptr_to_realloc); > + > + for (size_t nmemb = 1; nmemb <= 8; nmemb *= 2) > + if ((size % nmemb) == 0) > + { > + test_setup (); > + TEST_VERIFY (calloc (nmemb, size / nmemb) == NULL); > + TEST_VERIFY (errno == ENOMEM); > + > + test_setup (); > + TEST_VERIFY (calloc (size / nmemb, nmemb) == NULL); > + TEST_VERIFY (errno == ENOMEM); > + > + ptr_to_realloc = malloc (16); > + TEST_VERIFY_EXIT (ptr_to_realloc != NULL); > + test_setup (); > + TEST_VERIFY (reallocarray (ptr_to_realloc, nmemb, size / nmemb) == NULL); > + TEST_VERIFY (errno == ENOMEM); > + free (ptr_to_realloc); > + > + ptr_to_realloc = malloc (16); > + TEST_VERIFY_EXIT (ptr_to_realloc != NULL); > + test_setup (); > + TEST_VERIFY (reallocarray (ptr_to_realloc, size / nmemb, nmemb) == NULL); > + TEST_VERIFY (errno == ENOMEM); > + free (ptr_to_realloc); > + } > + else > + break; > +} OK. > + > + > +static long pagesize; > + > +/* This function tests the following aligned memory allocation functions > + using several valid alignments and precedes each allocation test with a > + small malloc/free before it: > + memalign, posix_memalign, aligned_alloc, valloc, pvalloc. */ > +static void > +test_large_aligned_allocations (size_t size) > +{ > + /* PTR stores the result of posix_memalign but since all those calls s/PTR/ptr/g When writing 'PTR' you are referring to the value of ptr, and since here you speak about the pointer itself you use just the name ptr. Please see: https://www.gnu.org/prep/standards/standards.html#Comments > + should fail, posix_memalign should never touch PTR. We set it to s/tough PTR/change ptr/g > + NULL here and later on we check that it remains NULL after each > + posix_memalign call. */ > + void * ptr = NULL; > + > + size_t align; > + > + /* All aligned memory allocation functions expect an alignment that is a > + power of 2. Given this, we test each of them with every valid > + alignment from 1 thru PAGESIZE. */ OK. > + for (align = 1; align <= pagesize; align *= 2) > + { > + test_setup (); > + TEST_VERIFY (memalign (align, size) == NULL); > + TEST_VERIFY (errno == ENOMEM); > + > + /* posix_memalign expects an alignment that is a power of 2 *and* a > + multiple of sizeof (void *). */ > + if ((align % sizeof (void *)) == 0) > + { > + test_setup (); > + TEST_VERIFY (posix_memalign (&ptr, align, size) == ENOMEM); > + TEST_VERIFY (ptr == NULL); > + } > + > + /* aligned_alloc expects a size that is a multiple of alignment. */ > + if ((size % align) == 0) > + { > + test_setup (); > + TEST_VERIFY (aligned_alloc (align, size) == NULL); > + TEST_VERIFY (errno == ENOMEM); > + } > + } > + > + /* Both valloc and pvalloc return page-aligned memory. */ > + > + test_setup (); > + TEST_VERIFY (valloc (size) == NULL); > + TEST_VERIFY (errno == ENOMEM); > + > + test_setup (); > + TEST_VERIFY (pvalloc (size) == NULL); > + TEST_VERIFY (errno == ENOMEM); > +} > + OK. > + > +#define FOURTEEN_ON_BITS ((1UL << 14) - 1) > +#define FIFTY_ON_BITS ((1UL << 50) - 1) > + > + > +static int > +do_test (void) > +{ > + > +#if __WORDSIZE >= 64 > + > + /* This test assumes that none of the supported targets have an address > + bus wider than 50 bits, and that therefore allocations for sizes wider > + than 50 bits will fail. Here, we ensure that the assumption continues > + to be true in the future when we might have address buses wider than 50 > + bits. */ > + > + struct rlimit alloc_size_limit > + = { > + .rlim_cur = FIFTY_ON_BITS, > + .rlim_max = FIFTY_ON_BITS > + }; > + > + setrlimit (RLIMIT_AS, &alloc_size_limit); > + > +#endif /* __WORDSIZE >= 64 */ > + > + DIAG_PUSH_NEEDS_COMMENT; > +#if __GNUC_PREREQ (7, 0) > + /* GCC 7 warns about too-large allocations; here we want to test > + that they fail. */ > + DIAG_IGNORE_NEEDS_COMMENT (7, "-Walloc-size-larger-than="); > +#endif > + > + /* Aligned memory allocation functions need to be tested up to alignment > + size equivalent to page size, which should be a power of 2. */ > + pagesize = sysconf (_SC_PAGESIZE); > + TEST_VERIFY_EXIT (powerof2 (pagesize)); > + > + /* Loop 1: Ensure that all allocations with SIZE close to SIZE_MAX, i.e. > + in the range (SIZE_MAX - 2^14, SIZE_MAX], fail. > + > + We can expect that this range of allocation sizes will always lead to > + an allocation failure on both 64 and 32 bit targets, because: > + > + 1. no currently supported 64-bit target has an address bus wider than > + 50 bits -- and (2^64 - 2^14) is much wider than that; > + > + 2. on 32-bit targets, even though 2^32 is only 4 GB and potentially > + addressable, glibc itself is more than 2^14 bytes in size, and > + therefore once glibc is loaded, less than (2^32 - 2^14) bytes remain > + available. */ > + > + for (size_t i = 0; i <= FOURTEEN_ON_BITS; i++) > + { > + test_large_allocations (SIZE_MAX - i); > + test_large_aligned_allocations (SIZE_MAX - i); > + } > + > +#if __WORDSIZE >= 64 > + /* On 64-bit targets, we need to test a much wider range of too-large > + sizes, so we test at intervals of (1 << 50) that allocation sizes > + ranging from SIZE_MAX down to (1 << 50) fail: > + The 14 MSBs are decremented starting from "all ON" going down to 1, > + the 50 LSBs are "all ON" and then "all OFF" during every iteration. */ > + for (size_t msbs = FOURTEEN_ON_BITS; msbs >= 1; msbs--) > + { > + size_t size = (msbs << 50) | FIFTY_ON_BITS; > + test_large_allocations (size); > + test_large_aligned_allocations (size); > + > + size = msbs << 50; > + test_large_allocations (size); > + test_large_aligned_allocations (size); > + } > +#endif /* __WORDSIZE >= 64 */ > + > + DIAG_POP_NEEDS_COMMENT; > + > + return 0; > +} > + > + > +#define TIMEOUT 5 > +#include > OK. -- Cheers, Carlos.