This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix integer overflows in internal memalign and malloc functions [BZ #22343]


On 01/04/2018 09:02 AM, 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.

High level:

You alter checked_request2size such that it performs a new overflow
check on the request->size conversion, and you add a new alignment-based
overflow check in _int_memalign to catch overflows with alignment greater
than MALLOC_ALIGNMENT.

Overall what you have done makes sense, but as Paul Eggert points out,
this code needs a real once-over to make it more readable and refactor
to minimize the number of times we compute overflow and alignments. This
however is outside the scope of minimally fixing this for 2.27.

This patch is OK with me if you chance the brace style as suggested below.
Please check this fix in as a fix for 2.27, since I'd like to see this fixed.

Design:

The test case here amazing, exactly what we needed.

The code in question is probably the minimal amount of code we want to change
to fix this. It makes sure that all checked_request2size calls make sure that
MALLOC_ALIGNMENT does not cause overflows, and that with that internal padding
taken into account that we do not get too close to SIZE_MAX (within 2*MINSIZE).

Implementation:

The test case looks perfect to me, the right blend of testing the corner cases
and covering some other regions of interest.

I would like to see the macros in malloc use ({...}) because it's easier to
read and simpler.

> ChangeLog:
> 
> 2018-01-04  Arjun Shankar  <arjun@redhat.com>
> 
> 	[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.
> ---
> Two notes:
> 
> 1. This bug has been flagged as security relevant and needs a CVE ID.
> 
> 2. Although this patch stands on its own, a previous version of the test
> was reviewed by Florian. I dropped it when committing the earlier (related)
> patch pending this new change. Here is that review:
> https://sourceware.org/ml/libc-alpha/2017-11/msg00325.html
> Since then, I have expanded the scope of the test a little.
> 
>  malloc/Makefile               |   1 +
>  malloc/malloc.c               |  32 ++++--
>  malloc/tst-malloc-too-large.c | 253 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 278 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. Add test.

>  
>  tests-static := \
>  	 tst-interpose-static-nothread \
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 48106f9bd4..2bcf64cdb6 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -1224,14 +1224,23 @@ 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)   \
> +  do                                    \
> +    {                                   \
> +      (sz) = request2size (req);        \

OK, this rounds req up to an aligned minimum chunk size first.

> +      if (((sz) < (req))                \

OK, look for overflow in request2size (new check).

> +          || REQUEST_OUT_OF_RANGE (sz)) \

OK, still look for request that is too big.

> +        {                               \
> +          __set_errno (ENOMEM);         \
> +          return 0;                     \
> +        }                               \
> +    }                                   \
> +    while (0)

Please rewrite this as:

#define foo(...) \
({			\
	...		\
})

The nested braces are less verbose IMHO.

>  
>  /*
>     --------------- Physical chunk operations ---------------
> @@ -4672,6 +4681,13 @@ _int_memalign (mstate av, size_t alignment, size_t bytes)
>     */
>  
>  
> +  /* Check for overflow.  */
> +  if (nb > SIZE_MAX - alignment - MINSIZE)

OK, look for alignment requested overflow in _int_memalign.

> +    {
> +      __set_errno (ENOMEM);
> +      return 0;
> +    }
> +
>    /* 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..705a07d850
> --- /dev/null
> +++ b/malloc/tst-malloc-too-large.c
> @@ -0,0 +1,253 @@
> +/* 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
> +   <http://www.gnu.org/licenses/>.  */
> +
> +/* 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 <stdlib.h>
> +#include <malloc.h>
> +#include <errno.h>
> +#include <stdint.h>
> +#include <sys/resource.h>
> +#include <libc-diag.h>
> +#include <support/check.h>
> +#include <unistd.h>
> +#include <sys/param.h>
> +
> +
> +/* 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.  */

OK.

> +static void
> +test_setup (void)
> +{
> +  void *volatile ptr = malloc (16);
> +  TEST_VERIFY_EXIT (ptr != NULL);
> +  free (ptr);
> +  errno = 0;
> +}

OK.

> +
> +
> +/* 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
> +     should fail, posix_memalign should never touch PTR.  We set it to
> +     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.  */
> +  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);
> +}
> +
> +
> +#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
> +      };
> +

OK.

> +  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=");

OK.

> +#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);
> +    }

OK.

> +
> +#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.  */

OK.

> +  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;

OK.

> +
> +  return 0;
> +}
> +
> +
> +#include <support/test-driver.c>
> 


-- 
Cheers,
Carlos.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]