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


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