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/22885] Detect altered malloc chunks


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.


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