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

Message ID 20171106163248.GA48861@aloka.lostca.se
State Superseded, archived
Headers

Commit Message

Arjun Shankar Nov. 6, 2017, 4:32 p.m. UTC
  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) instead of returning NULL and
setting errno to ENOMEM.

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.
---
Tested with no regressions on x86_64, ppc64, ppc64le, and x86 (but the new
test is unsupported on 32 bit targets).

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

Comments

Siddhesh Poyarekar Nov. 7, 2017, 6:27 a.m. UTC | #1
On Monday 06 November 2017 10:02 PM, Arjun Shankar wrote:
> 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) instead of returning NULL and
> setting errno to ENOMEM.

That sounds CVE-worthy.

Siddhesh
  

Patch

diff --git a/malloc/Makefile b/malloc/Makefile
index 7ae3d82..a01279d 100644
--- a/malloc/Makefile
+++ b/malloc/Makefile
@@ -35,6 +35,7 @@  tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
 	 tst-interpose-thread \
 	 tst-alloc_buffer \
 	 tst-malloc-tcache-leak \
+	 tst-malloc-too-large \
 
 tests-static := \
 	 tst-interpose-static-nothread \
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);
 
   MAYBE_INIT_TCACHE ();
diff --git a/malloc/tst-malloc-too-large.c b/malloc/tst-malloc-too-large.c
new file mode 100644
index 0000000..002a893
--- /dev/null
+++ b/malloc/tst-malloc-too-large.c
@@ -0,0 +1,139 @@ 
+/* Test and verify that too-large memory allocations fail with ENOMEM.
+   Copyright (C) 2017 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/>.  */
+
+
+#include <support/check.h>
+
+
+#if __WORDSIZE < 64
+
+
+static int
+do_test (void)
+{
+  FAIL_UNSUPPORTED ("Test does not support targets with word size < 64-bit");
+}
+
+
+#else /* __WORDSIZE >= 64 */
+
+
+#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.  The small malloc/free is done to catch regressions
+   such as the one reported here:
+   https://sourceware.org/bugzilla/show_bug.cgi?id=22375  */
+static void
+test_setup (void)
+{
+  void * ptr = malloc (16);
+  TEST_VERIFY_EXIT (ptr != NULL);
+  free (ptr);
+  errno = 0;
+}
+
+
+/* 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)
+{
+  void * ptr_to_realloc;
+  test_setup ();
+  TEST_VERIFY (malloc (size) == NULL);
+  TEST_VERIFY (errno == ENOMEM);
+
+  test_setup ();
+  TEST_VERIFY (calloc (size, 1) == NULL);
+  TEST_VERIFY (errno == ENOMEM);
+
+  test_setup ();
+  TEST_VERIFY (calloc (1, 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);
+}
+
+
+#define FOURTEEN_ON_BITS ((1UL << 14) - 1)
+#define FIFTY_ON_BITS ((1UL << 50) - 1)
+
+
+static int
+do_test (void)
+{
+  size_t lsbs, msbs, size;
+
+  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
+
+  /* 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);
+    }
+
+  /* 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);
+    }
+
+  DIAG_POP_NEEDS_COMMENT;
+
+  return 0;
+}
+
+
+#endif /* __WORDSIZE >= 64 */
+
+
+#include <support/test-driver.c>