[v2] Fix integer overflow in malloc when tcache is enabled [BZ #22375]

  When the per-thread cache is enabled, __libc_malloc uses request2size (which
does not perform an overflow check) to calculate the chunk size from the
requested allocation size. This leads to an integer overflow causing malloc
to incorrectly return the last successfully allocated block when called with
a very large size argument (close to SIZE_MAX).

This patch uses checked_request2size instead, removing the overflow, and
adds a new test to guard against regressions.

ChangeLog:

2017-11-06  Arjun Shankar  <arjun@redhat.com>

	[BZ #22375]
	* malloc/malloc.c (__libc_malloc): Use checked_request2size instead
	of request2size.
	* malloc/tst-malloc-too-large.c: New test.
	* malloc/Makefile: Add tst-malloc-too-large.
---
Florian has flagged the bug as 'security+' in bugzilla and will eventually
get a CVE ID for it.

v1 discussion: https://sourceware.org/ml/libc-alpha/2017-11/msg00192.html

v2 includes a top level comment in the test source and includes a missing
call to 'free' in it.

 malloc/Makefile               |   1 +
 malloc/malloc.c               |   3 +-
 malloc/tst-malloc-too-large.c | 150 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 153 insertions(+), 1 deletion(-)
 create mode 100644 malloc/tst-malloc-too-large.c
  

On 11/08/2017 02:28 PM, Arjun Shankar wrote:

> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index f94d51c..2ebd97f 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -3022,7 +3022,8 @@ __libc_malloc (size_t bytes)
>       return (*hook)(bytes, RETURN_ADDRESS (0));
>   #if USE_TCACHE
>     /* int_free also calls request2size, be careful to not pad twice.  */
> -  size_t tbytes = request2size (bytes);
> +  size_t tbytes;
> +  checked_request2size (bytes, tbytes);
>     size_t tc_idx = csize2tidx (tbytes);

I think you should commit this fix separately from the test now, for 
reasons explained below.

> +/* Bug 22375 [1] 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.  This test guards against such regressions 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 supported by glibc.  The test verifies that such impossibly large
> +   allocations correctly fail.
> +
> +   [1] https://sourceware.org/bugzilla/show_bug.cgi?id=22375 */

Please remove the URL, it does not seem necessary.  We generally use 
local bug numbers without qualification.

> +#include <support/check.h>
> +
> +
> +#if __WORDSIZE < 64

I don't think the first part of the test depends on the word size at 
all, so I think we should run this part for 32-bit too.

> +#include <stdlib.h>
> +#include <errno.h>
> +#include <libc-diag.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.  */
> +static void
> +test_setup (void)
> +{
> +  void * ptr = malloc (16);
> +  TEST_VERIFY_EXIT (ptr != NULL);
> +  free (ptr);
> +  errno = 0;
> +}

This needs some sort of compiler barrier, maybe

   void *volatile ptr = malloc (16);
   TEST_VERIFY_EXIT (ptr != NULL);
   free (ptr);
   errno = 0;

Otherwise, GCC might optimize away the malloc/free pair.  I'm surprised 
it doesn't do this here.

> +/* This function tests each of:
> +   - malloc (SIZE)
> +   - calloc (1, SIZE)
> +   - calloc (SIZE, 1)
> +   - realloc (PTR_FOR_REALLOC, SIZE)
> +   and precedes each of these tests with a small malloc before it.  */
> +static void
> +test_large_allocations (size_t size)

This misses the memory alignment allocation functions.  Once you add 
those, you will hit bug 22343, which is an existing bug predating the 
tcache.  Hence my request to commit this test separately.

(This does not have to go into the first version of the test.)

> +static int
> +do_test (void)
> +{
> +  size_t lsbs, msbs, size;

We prefer inline declarations these days.

> +  /* The allocation SIZE used during each iteration is obtained by combining
> +     LSBS and MSBS. In order to catch all interesting cases but avoid
> +     looping through every too-large size, i.e. [2^50, 2^64), we will have
> +     two loops with different iteration patterns for LSBs and MSBs.  */
> +
> +  /* Loop 1: Ensure that all allocations with SIZE close to SIZE_MAX fail:
> +     The 14 MSBs are "all ON" during every iteration,
> +     the 50 LSBs are decremented starting from "all ON", for 1<<14
> +     iterations.  */
> +  for (lsbs = FIFTY_ON_BITS; lsbs > FIFTY_ON_BITS - (1UL << 14); lsbs--)
> +    {
> +      size = (FOURTEEN_ON_BITS << 50) | lsbs;
> +      test_large_allocations (size);
> +    }

Isn't this just a fancy way of counting down from SIZE_MAX?  If you 
write that loop in a more direct fashion, it would work independently of 
SIZE_MAX, and as such could run for 32 bit architectures too.

> +
> +  /* Loop 2: 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 (msbs = FOURTEEN_ON_BITS; msbs >= 1; msbs--)
> +    {
> +      size = (msbs << 50) | FIFTY_ON_BITS;
> +      test_large_allocations (size);
> +
> +      size = msbs << 50;
> +      test_large_allocations (size);
> +    }

This part looks 64-bit only, at least with those constants.

To be future-proof, I think you'd have to limit the address space 1 EiB 
before running the loop (setting an RLIMIT_AS limit, ignoring all errors).

Thanks,
Florian