[PATCHv2/22885] Detect altered malloc chunks

Message ID 5be9e4b1.f8ONiDEbTYBFjqwT%michael@thomii.com
State New
Headers show
Series
  • [PATCHv2/22885] Detect altered malloc chunks
Related show

Commit Message

Mike Thomas Nov. 12, 2018, 8:38 p.m.
Fixes bug 22885 v2

Addressed all comments given in v1 of submission.
Now using email address that matches email address in copyright assignment.

Includes test cases.  Tested with no regressions.

	* malloc/Makefile: Run the new tests.
	* malloc/malloc.c (tcache_get): Add consistency check.
	(_int_malloc): Likewise.
	* malloc/tst-malloc-smallbins-chunksize.c: New.
	* malloc/tst-malloc-smallbins2tcache-chunksize.c: New.
	* malloc/tst-malloc-tcache-chunksize.c: New.

Patch

diff --git a/malloc/Makefile b/malloc/Makefile
index 7d54bad866..1bb3543a7a 100644
--- a/malloc/Makefile
+++ b/malloc/Makefile
@@ -38,6 +38,9 @@  tests := mallocbug tst-malloc tst-valloc tst-calloc tst-obstack \
 	 tst-malloc_info \
 	 tst-malloc-too-large \
 	 tst-malloc-stats-cancellation \
+	 tst-malloc-tcache-chunksize \
+	 tst-malloc-smallbins-chunksize \
+	 tst-malloc-smallbins2tcache-chunksize \
 
 tests-static := \
 	 tst-interpose-static-nothread \
@@ -194,6 +197,8 @@  tst-malloc-usable-ENV = MALLOC_CHECK_=3
 tst-malloc-usable-static-ENV = $(tst-malloc-usable-ENV)
 tst-malloc-usable-tunables-ENV = GLIBC_TUNABLES=glibc.malloc.check=3
 tst-malloc-usable-static-tunables-ENV = $(tst-malloc-usable-tunables-ENV)
+tst-malloc-smallbins-chunksize-ENV = GLIBC_TUNABLES=glibc.malloc.tcache_count=0
+tst-malloc-smallbins2tcache-chunksize-ENV = GLIBC_TUNABLES=glibc.malloc.tcache_count=4
 
 ifeq ($(experimental-malloc),yes)
 CPPFLAGS-malloc.c += -DUSE_TCACHE=1
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 67cdfd0ad2..ac59f6f34b 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -2924,6 +2924,17 @@  tcache_get (size_t tc_idx)
   tcache_entry *e = tcache->entries[tc_idx];
   assert (tc_idx < TCACHE_MAX_BINS);
   assert (tcache->entries[tc_idx] > 0);
+
+  /* Check for potential buffer overrun that altered
+     the chunk size by ensuring it's slot is correct
+     for it's size.  */
+
+  mchunkptr chunk = mem2chunk (e);
+  INTERNAL_SIZE_T chunksize = chunksize (chunk);
+  size_t chunkindex = csize2tidx (chunksize);
+
+  assert (tc_idx == chunkindex);
+
   tcache->entries[tc_idx] = e->next;
   --(tcache->counts[tc_idx]);
   return (void *) e;
@@ -3627,6 +3638,15 @@  _int_malloc (mstate av, size_t bytes)
 
       if ((victim = last (bin)) != bin)
         {
+          /* Check for potential buffer overrun that altered
+             the chunk size by ensuring it's slot is correct
+             for it's size.  */
+
+          INTERNAL_SIZE_T chunksize = chunksize (victim);
+          size_t chunkindex = smallbin_index (chunksize);
+
+          assert (idx == chunkindex);
+
           bck = victim->bk;
 	  if (__glibc_unlikely (bck->fd != victim))
 	    malloc_printerr ("malloc(): smallbin double linked list corrupted");
@@ -3651,6 +3671,13 @@  _int_malloc (mstate av, size_t bytes)
 		{
 		  if (tc_victim != 0)
 		    {
+                      /* Check for overrun here too.  */
+
+                      INTERNAL_SIZE_T tc_chunksize = chunksize (tc_victim);
+                      size_t tc_chunkindex = smallbin_index (tc_chunksize);
+
+                      assert (idx == tc_chunkindex);
+
 		      bck = tc_victim->bk;
 		      set_inuse_bit_at_offset (tc_victim, nb);
 		      if (av != &main_arena)
diff --git a/malloc/tst-malloc-smallbins-chunksize.c b/malloc/tst-malloc-smallbins-chunksize.c
new file mode 100644
index 0000000000..79507641da
--- /dev/null
+++ b/malloc/tst-malloc-smallbins-chunksize.c
@@ -0,0 +1,79 @@ 
+/* Test and verify that tcache checks for altered chunk size.
+   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 22885 reported smallbins wasn't checking for altered chunksize.
+   After freeing memory the user could alter the size field of the
+   chunk and smallbins wouldn't check for this.  */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <malloc.h>
+#include <string.h>
+#include <signal.h>
+#include <support/check.h>
+
+/* The following pragma is to prevent the optimize from completely
+   optimizing out the test code within do_test.  */
+
+#pragma GCC optimize ("O0")
+
+static int
+do_test (void)
+{
+    /* Disable fastbins */
+    mallopt (M_MXFAST, 0);
+
+    /* The size of 100 was arbitrarily chosen, but needs to be small
+       enough to use smallbins and large enough to not use a fastbin.  */
+    char *ptr = (char *) malloc (100);
+
+    /* This prevents coalescing.  */
+    (void) malloc (100);
+ 
+    /* This will free a chunk in the middle of the heap so that we
+       don't merge the allocation back into the top chunk.  This
+       chunk will be placed in the unsorted bin.  */
+    free (ptr);
+
+    /* This triggers processing of the unsorted bin (around line 3740
+       in _int_malloc, look for "for (;; )") which moves the chunk
+       we just freed to a smallbin.  10000 is an arbitrarily chosen
+       size that is at least larger than a smallbin size.  */
+    (void) malloc (10000);
+
+    /* We are corrupting the size field of the chunk in the smallbin,
+       by increasing it's size by 16 bytes.  */
+    ptr -= sizeof (size_t);
+    size_t val = (size_t) *ptr;
+    size_t *stptr = (size_t *) ptr;
+    val += 16;
+    *stptr = val;
+
+    /* This attempts to reuse the chunk we just corrupted.  */
+    (void) malloc (100);
+
+    /* Execution should not reach here, if FAIL_RET executes look at
+       malloc/malloc.c _int_malloc, "assert (idx == chunkindex)".  */
+    FAIL_RET ("no assertion occurred in malloc due to altered chunk size");
+    return (0);
+}
+
+#pragma GCC optimize ("O3")
+
+#define EXPECTED_SIGNAL SIGABRT
+#include <support/test-driver.c>
diff --git a/malloc/tst-malloc-smallbins2tcache-chunksize.c b/malloc/tst-malloc-smallbins2tcache-chunksize.c
new file mode 100644
index 0000000000..d29530be4b
--- /dev/null
+++ b/malloc/tst-malloc-smallbins2tcache-chunksize.c
@@ -0,0 +1,112 @@ 
+/* Test and verify that tcache checks for altered chunk size.
+   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 22885 reported smallbins wasn't checking for altered chunksize.
+   After freeing memory the user could alter the size field of the
+   chunk and smallbins transfer to tcache wouldn't check for this.  */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <malloc.h>
+#include <string.h>
+#include <signal.h>
+#include <support/check.h>
+
+/* The following pragma is to prevent the optimize from completely
+   optimizing out the test code within do_test.  */
+
+#pragma GCC optimize ("O0")
+
+static int
+do_test (void)
+{
+    /* Disable fastbins */
+    mallopt (M_MXFAST, 0);
+
+    /* Allocate a bunch of chunks of size 100, this size was arbitrarily
+       chosen to be small enough to use smallbin and large enough to not
+       use a fastbin.  */
+    /* The allocations below that we do not retain pointers to are used
+       to prevent coalescing.  */
+    char *ptr1 = (char *) malloc (100);
+    (void) malloc (100);
+    char *ptr2 = (char *) malloc (100);
+    (void) malloc (100);
+    char *ptr3 = (char *) malloc (100);
+    (void) malloc (100);
+    char *ptr4 = (char *) malloc (100);
+    (void) malloc (100);
+
+    char *ptr5 = (char *) malloc (100);
+    (void) malloc (100);
+    char *ptr6 = (char *) malloc (100);
+    (void) malloc (100);
+    char *ptr7 = (char *) malloc (100);
+    (void) malloc (100);
+    char *ptr8 = (char *) malloc (100);
+    (void) malloc (100);
+
+    /* tcache size is set to 4, so these 4 will be moved into tcache.  */
+    free (ptr5);
+    free (ptr6);
+    free (ptr7);
+    free (ptr8);
+
+    /* These 4 will go into smallbins eventually.  */
+    free (ptr1);
+    free (ptr2);
+    free (ptr3);
+    free (ptr4);
+
+    /* This triggers processing of the unsorted bin (around line 3740
+       in _int_malloc, look for "for (;; )") which moves the chunks
+       we just freed to a smallbin.  10000 is an arbitrarily chosen
+       size that is at least larger than a smallbin size.  */
+    (void) malloc (10000);
+
+    /* We are corrupting the size field of the chunk in the smallbin,
+       by increasing it's size by 16 bytes.  */
+    char *ptr = ptr2;
+    ptr -= sizeof (size_t);
+    size_t val = (size_t) *ptr;
+    size_t *stptr = (size_t *) ptr;
+    val += 16;
+    *stptr = val;
+
+    /* Malloc all of the 8 chunks again but one of the allocations
+       will assert before we get all the chunks, because of the corruption
+       done just above.  */
+    (void) malloc (100);
+    (void) malloc (100);
+    (void) malloc (100);
+    (void) malloc (100);
+    (void) malloc (100);
+    (void) malloc (100);
+    (void) malloc (100);
+    (void) malloc (100);
+
+    /* Execution should not reach here, if FAIL_RET executes look at
+       malloc/malloc.c _int_malloc, "assert (idx == tc_chunkindex)".  */
+    FAIL_RET ("no assertion occurred in malloc due to altered chunk size");
+    return (0);
+}
+
+#pragma GCC optimize ("O3")
+
+#define EXPECTED_SIGNAL SIGABRT
+#include <support/test-driver.c>
diff --git a/malloc/tst-malloc-tcache-chunksize.c b/malloc/tst-malloc-tcache-chunksize.c
new file mode 100644
index 0000000000..da63e8cde4
--- /dev/null
+++ b/malloc/tst-malloc-tcache-chunksize.c
@@ -0,0 +1,63 @@ 
+/* Test and verify that tcache checks for altered chunk size.
+   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 22885 reported tcache wasn't checking for altered chunksize.
+   After freeing memory the user could alter the size field of the
+   chunk and tcache wouldn't check for this.  */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <malloc.h>
+#include <string.h>
+#include <signal.h>
+#include <support/check.h>
+
+/* The following pragma is to prevent the compiler from completely
+   optimizing out the test code within do_test.  */
+
+#pragma GCC optimize ("O0")
+
+static int
+do_test (void)
+{
+    /* The size of 100 was arbitrarily chosen, but needs to be small
+       enough to use tcache.  */
+    char *ptr = (char *) malloc (100);
+    free (ptr);
+
+    /* We are corrupting the size field of the chunk in the tcache,
+       by increasing it's size by 8 bytes.  */
+    ptr -= sizeof (size_t);
+    size_t val = (size_t) *ptr;
+    size_t *stptr = (size_t *) ptr;
+    val += 8;
+    *stptr = val;
+
+    /* This attempts to reuse the chunk we just corrupted.  */
+    (void) malloc (100);
+
+    /* Execution should not reach here, if FAIL_RET executes look at
+       malloc/malloc.c tcache_get, "assert (tc_idx == chunksize)".  */
+    FAIL_RET ("no assertion occurred in malloc due to altered chunk size");
+    return (0);
+}
+
+#pragma GCC optimize ("O3")
+
+#define EXPECTED_SIGNAL SIGABRT
+#include <support/test-driver.c>