[PATCH/22885] Detect altered malloc chunks

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH/22885] Detect altered malloc chunks

Mike Thomas
Fixes bug 22885

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.


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..46197acb85
--- /dev/null
+++ b/malloc/tst-malloc-smallbins-chunksize.c
@@ -0,0 +1,68 @@
+/* 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);
+
+    char *ptr = (char *) malloc (100);
+
+    /* This prevents coalescing.  */
+    (void) malloc (100);
+
+    free (ptr);
+
+    /* This triggers processing of the unsorted bin.  */
+    (void) malloc (10000);
+
+    ptr -= sizeof (size_t);
+    size_t val = (size_t) *ptr;
+    size_t *stptr = (size_t *) ptr;
+    val += 16;
+    *stptr = val;
+
+    (void) malloc (100);
+
+    /* Execution should not reach here, if FAIL_RET executes look at
+       malloc/malloc.c smallbins assertion of 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>
diff --git a/malloc/tst-malloc-smallbins2tcache-chunksize.c b/malloc/tst-malloc-smallbins2tcache-chunksize.c
new file mode 100644
index 0000000000..6fee43fc7e
--- /dev/null
+++ b/malloc/tst-malloc-smallbins2tcache-chunksize.c
@@ -0,0 +1,104 @@
+/* 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);
+
+    /* tcache size is set to 4, so allocate 4 that will go into tcache.  */
+    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);
+
+    /* And 4 that will go into smallbins.  */
+    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);
+
+    /* These will go into tcache.  */
+    free (ptr5);
+    free (ptr6);
+    free (ptr7);
+    free (ptr8);
+
+    /* These will go into smallbins.  */
+    free (ptr1);
+    free (ptr2);
+    free (ptr3);
+    free (ptr4);
+
+    /* This triggers processing of the unsorted bin.  */
+    (void) malloc (10000);
+
+    /* Alter of the middle smallbins which will be moved to tcache.  */
+    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 we will assert before we
+       get all the chunks.  */
+    (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 smallbins assertion of 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>
diff --git a/malloc/tst-malloc-tcache-chunksize.c b/malloc/tst-malloc-tcache-chunksize.c
new file mode 100644
index 0000000000..e4ac668e2b
--- /dev/null
+++ b/malloc/tst-malloc-tcache-chunksize.c
@@ -0,0 +1,55 @@
+/* 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)
+{
+    char *ptr = (char *) malloc (100);
+    free (ptr);
+    ptr -= sizeof (size_t);
+    size_t val = (size_t) *ptr;
+    size_t *stptr = (size_t *) ptr;
+    val += 8;
+    *stptr = val;
+    (void) malloc (100);
+    /* Execution should not reach here, if FAIL_RET executes look at
+       malloc/malloc.c tcache_get assertion of 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>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH/22885] Detect altered malloc chunks

Carlos O'Donell-6
On 11/7/18 3:21 AM, Mike Thomas wrote:
> Fixes bug 22885

Mike,

Thank you very much for the patch to fix this issue (and for filling
a bug to track this)!

For a patch of this size we will need copyright assignment to the FSF
so we can accept the patch and continue to accept patches from you in
the future.

Would you be willing to start a "Future Assignment" process with
the FSF? This is the most flexible assignment and allows us to review
this and all future patches easily and commit them immediately when
ready.

The contribution checklist has this to say about Copyright Assignment:
https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment

The Future Assignment form is here:
http://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/request-assign.future

I can help you with your submission if you need it. Please feel free
to reach out to me off list. As a project steward I'm happy to answer
any of your questions.

Cheers,
Carlos.

> 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.
>
>
> 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..46197acb85
> --- /dev/null
> +++ b/malloc/tst-malloc-smallbins-chunksize.c
> @@ -0,0 +1,68 @@
> +/* 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);
> +
> +    char *ptr = (char *) malloc (100);
> +
> +    /* This prevents coalescing.  */
> +    (void) malloc (100);
> +
> +    free (ptr);
> +
> +    /* This triggers processing of the unsorted bin.  */
> +    (void) malloc (10000);
> +
> +    ptr -= sizeof (size_t);
> +    size_t val = (size_t) *ptr;
> +    size_t *stptr = (size_t *) ptr;
> +    val += 16;
> +    *stptr = val;
> +
> +    (void) malloc (100);
> +
> +    /* Execution should not reach here, if FAIL_RET executes look at
> +       malloc/malloc.c smallbins assertion of 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>
> diff --git a/malloc/tst-malloc-smallbins2tcache-chunksize.c b/malloc/tst-malloc-smallbins2tcache-chunksize.c
> new file mode 100644
> index 0000000000..6fee43fc7e
> --- /dev/null
> +++ b/malloc/tst-malloc-smallbins2tcache-chunksize.c
> @@ -0,0 +1,104 @@
> +/* 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);
> +
> +    /* tcache size is set to 4, so allocate 4 that will go into tcache.  */
> +    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);
> +
> +    /* And 4 that will go into smallbins.  */
> +    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);
> +
> +    /* These will go into tcache.  */
> +    free (ptr5);
> +    free (ptr6);
> +    free (ptr7);
> +    free (ptr8);
> +
> +    /* These will go into smallbins.  */
> +    free (ptr1);
> +    free (ptr2);
> +    free (ptr3);
> +    free (ptr4);
> +
> +    /* This triggers processing of the unsorted bin.  */
> +    (void) malloc (10000);
> +
> +    /* Alter of the middle smallbins which will be moved to tcache.  */
> +    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 we will assert before we
> +       get all the chunks.  */
> +    (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 smallbins assertion of 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>
> diff --git a/malloc/tst-malloc-tcache-chunksize.c b/malloc/tst-malloc-tcache-chunksize.c
> new file mode 100644
> index 0000000000..e4ac668e2b
> --- /dev/null
> +++ b/malloc/tst-malloc-tcache-chunksize.c
> @@ -0,0 +1,55 @@
> +/* 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)
> +{
> +    char *ptr = (char *) malloc (100);
> +    free (ptr);
> +    ptr -= sizeof (size_t);
> +    size_t val = (size_t) *ptr;
> +    size_t *stptr = (size_t *) ptr;
> +    val += 8;
> +    *stptr = val;
> +    (void) malloc (100);
> +    /* Execution should not reach here, if FAIL_RET executes look at
> +       malloc/malloc.c tcache_get assertion of 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>
>


--
Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH/22885] Detect altered malloc chunks

Carlos O'Donell-6
On 11/8/18 1:33 PM, Carlos O'Donell wrote:

> On 11/7/18 3:21 AM, Mike Thomas wrote:
>> Fixes bug 22885
>
> Mike,
>
> Thank you very much for the patch to fix this issue (and for filling
> a bug to track this)!
>
> For a patch of this size we will need copyright assignment to the FSF
> so we can accept the patch and continue to accept patches from you in
> the future.
>
> Would you be willing to start a "Future Assignment" process with
> the FSF? This is the most flexible assignment and allows us to review
> this and all future patches easily and commit them immediately when
> ready.
>
> The contribution checklist has this to say about Copyright Assignment:
> https://sourceware.org/glibc/wiki/Contribution%20checklist#FSF_copyright_Assignment
>
> The Future Assignment form is here:
> http://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/request-assign.future
>
> I can help you with your submission if you need it. Please feel free
> to reach out to me off list. As a project steward I'm happy to answer
> any of your questions.
My apologies you *do* have a copyright assignment.

In the future please make sure you try to match poster name and email
name with what you have registered with the FSF.

To avoid any problems, please post from '[hidden email]' in the
future :-)

--
Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH/22885] Detect altered malloc chunks

Carlos O'Donell-6
In reply to this post by Carlos O'Donell-6
On 11/8/18 1:33 PM, Carlos O'Donell wrote:
> On 11/7/18 3:21 AM, Mike Thomas wrote:
>> Fixes bug 22885

Mike,

Please post v2 with additional comments and detailed commit message
and I'll review again. I think you're almost done.

DJ,

Are you available to run this patch through a performance test with
the simulator and workloads? All the changes are fairly light weight
and touching data we likely already have hot in the cache, and they
are mostly mask/test operations. I don't expect them to have a visible
impact.
 

>> 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.
>>
>>
>> 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 \

OK. Great to see 3 new tests!

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

OK. Use tunables to disable tcache and alter tcache_count.

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

OK. This is the key check here to make sure the size was not
altered while it was in it's own index.

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

OK. This is the same check but for small bins.

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

OK. Again the same check for tcache too.

>> +
>>        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..46197acb85
>> --- /dev/null
>> +++ b/malloc/tst-malloc-smallbins-chunksize.c
>> @@ -0,0 +1,68 @@
>> +/* Test and verify that tcache checks for altered chunk size.

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

OK.

>> +
>> +    char *ptr = (char *) malloc (100);
>> +
>> +    /* This prevents coalescing.  */
>> +    (void) malloc (100);

OK. Yes.

>> +
>> +    free (ptr);
>> +
>> +    /* This triggers processing of the unsorted bin.  */

This comment needs a reference to exact implementation place
in malloc.c that triggers this because we're going to have to
fix this up later if the implementation changes. Therefore the
more detail you have here the better.

Second. It should say why 10KB is enough.

>> +    (void) malloc (10000);

>> +

>> +    ptr -= sizeof (size_t);
>> +    size_t val = (size_t) *ptr;
>> +    size_t *stptr = (size_t *) ptr;
>> +    val += 16;
>> +    *stptr = val;

You need to describe what this is doing in a comment.

>> +
>> +    (void) malloc (100);

Why 100?

>> +
>> +    /* Execution should not reach here, if FAIL_RET executes look at
>> +       malloc/malloc.c smallbins assertion of idx == chunksize.  */

Please reference a function also.

>> +    FAIL_RET ("no assertion occurred in malloc due to altered chunk size");
>> +    return (0);
>> +}
>> +
>> +#pragma GCC optimize ("O3")

OK.

>> +
>> +#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..6fee43fc7e
>> --- /dev/null
>> +++ b/malloc/tst-malloc-smallbins2tcache-chunksize.c
>> @@ -0,0 +1,104 @@
>> +/* Test and verify that tcache checks for altered chunk size.

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 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")

OK.

>> +
>> +static int
>> +do_test (void)
>> +{
>> +    /* Disable fastbins */
>> +    mallopt (M_MXFAST, 0);

OK.

>> +
>> +    /* tcache size is set to 4, so allocate 4 that will go into tcache.  */

They don't go "into" the tcache, the tcache fills, and 4 come *from*
the tcache.

>> +    char *ptr1 = (char *) malloc (100);

Please document why "100" bytes.

It's sufficient to say "We allocate 100 bytes because it fits in the tcache."

The intent of the allocation size needs to be documented.

>> +    (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);
>> +
>> +    /* And 4 that will go into smallbins.  */

Please add a comment about the assumptions of smallbin size, this way we
can fix this test up later if we violate those assumptions.

Likewise we get 4 chunks from the top, they aren't in smallbins yet?

>> +    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);
>> +
>> +    /* These will go into tcache.  */

OK, yes.

>> +    free (ptr5);
>> +    free (ptr6);
>> +    free (ptr7);
>> +    free (ptr8);
>> +
>> +    /* These will go into smallbins.  */

OK, yes.

>> +    free (ptr1);
>> +    free (ptr2);
>> +    free (ptr3);
>> +    free (ptr4);
>> +
>> +    /* This triggers processing of the unsorted bin.  */

Comment about why 10KB is enough.

>> +    (void) malloc (10000);
>> +
>> +    /* Alter of the middle smallbins which will be moved to tcache.  */

Please comment exactly how you alter it and why.

>> +    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 we will assert before we
>> +       get all the chunks.  */

So this below triggers smallbin transfer to cache? Please add this as a
comment.

>> +    (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 smallbins assertion of 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>
>> diff --git a/malloc/tst-malloc-tcache-chunksize.c b/malloc/tst-malloc-tcache-chunksize.c
>> new file mode 100644
>> index 0000000000..e4ac668e2b
>> --- /dev/null
>> +++ b/malloc/tst-malloc-tcache-chunksize.c
>> @@ -0,0 +1,55 @@
>> +/* Test and verify that tcache checks for altered chunk size.

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

OK.

>> +
>> +#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)
>> +{

This needs a comment explaining why 100. Arbitrary? Specific?

>> +    char *ptr = (char *) malloc (100);
>> +    free (ptr);

This needs a comment explaining the alteration being made.

>> +    ptr -= sizeof (size_t);
>> +    size_t val = (size_t) *ptr;
>> +    size_t *stptr = (size_t *) ptr;
>> +    val += 8;
>> +    *stptr = val;
>> +    (void) malloc (100);

Likewise, why 100?

>> +    /* Execution should not reach here, if FAIL_RET executes look at
>> +       malloc/malloc.c tcache_get assertion of tc_idx == chunksize.  */
>> +    FAIL_RET ("no assertion occurred in malloc due to altered chunk size");
>> +    return (0);
>> +}
>> +
>> +#pragma GCC optimize ("O3")

OK.

>> +
>> +#define EXPECTED_SIGNAL SIGABRT
>> +#include <support/test-driver.c>
>>
>
>


--
Cheers,
Carlos.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH/22885] Detect altered malloc chunks

DJ Delorie-2

"Carlos O'Donell" <[hidden email]> writes:
> DJ,
>
> Are you available to run this patch through a performance test with
> the simulator and workloads? All the changes are fairly light weight
> and touching data we likely already have hot in the cache, and they
> are mostly mask/test operations. I don't expect them to have a visible
> impact.

Will do.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH/22885] Detect altered malloc chunks

DJ Delorie-2
In reply to this post by Carlos O'Donell-6

"Carlos O'Donell" <[hidden email]> writes:
> Are you available to run this patch through a performance test with
> the simulator and workloads? All the changes are fairly light weight
> and touching data we likely already have hot in the cache, and they
> are mostly mask/test operations. I don't expect them to have a visible
> impact.

I combined Mike's patch with Florian's two (no strict aliasing,
accessors - the fastbin one was already committed), Adam's, and mine,
and benchmarked them all together.  I ran each workload through the
simulator 16 times and averaged the results, for each of four workloads,
against both pristine and patched sources.  Results below.  Deviations
are within 1% but no specific trend.

dj@envy pts/27 ~/tools
$ go oocalc okular-1 fluentd 389ds

----------  oocalc ----------

PRISTINE:
Total call time: 1,073,761,546 cycles

PATCHED:
Total call time: 1,079,895,621 cycles

----------  okular-1 ----------

PRISTINE:
Total call time: 3,271,045,477 cycles

PATCHED:
Total call time: 3,258,433,903 cycles

----------  fluentd ----------

PRISTINE:
Total call time: 6,317,728,016 cycles

PATCHED:
Total call time: 6,266,545,907 cycles

----------  389ds ----------

PRISTINE:
Total call time: 7,613,754,098 cycles

PATCHED:
Total call time: 7,564,173,273 cycles