[PATCH v2 0/7] Additional integrity checks for the malloc

[PATCH v2 0/7] Additional integrity checks for the malloc

[Istvan Kurucsai]

This is an actualized version of a patch set I submitted previously [8].

The patch set tries to improve on the current integrity checks in malloc. The
goal was to eliminate known exploitation techniques with the simplest possible
changes. 

The tests passed but I did no profiling. The performance impact of the mmap
related parts shouldn't be noticeable, the others I'm not sure about. I already
did copyright assignment.

A quick overview of the individual patches:

(1/7) An attempt at hardening the `use_top` part of malloc against corruption
and pivoting of the top chunk, known as the House of Force [1]. The possibility
of extending the top chunk from an mmapped arena into another remains. Note
that this is almost identical to a recently submitted patch [9].

(2/7) The binning code in malloc is rather attacker-friendly [2][3]. Change
this by enforcing as many invariants as possible on chunks from the unsorted
bin.

(3/7) `malloc_consolidate` contains no integrity checks beside the ones in
`unlink`. This can be abused by an attacker in a couple of ways [4]. The patch
limits the possibilities significantly.

(4/7) Fix an unsigned underflow and subsequent wild memcpy that can be
triggered by a corrupted chunk size in `__libc_realloc` [5].

(5/7) By corrupting the `IS_MMAPPED` bit of a free chunk, an attacker can force
calloc to return an uninitialized chunk [6]. The patch adds checks to the
`IS_MMAPPED` path in calloc, even though the protection is not complete.

(6/7), (7/7): Additional checks around the unmapping and remapping of chunks,
which are abusable in different ways [7]. Also feels somewhat incomplete but
still an improvement.


[1]: https://github.com/shellphish/how2heap/blob/master/house_of_force.c
[2]: https://www.contextis.com/documents/120/Glibc_Adventures-The_Forgotten_Chunks.pdf
[3]: https://github.com/shellphish/how2heap/blob/master/unsorted_bin_attack.c
[4]: http://tukan.farm/2016/09/04/fastbin-fever/
[5]: http://tukan.farm/2016/11/03/once-upon-a-realloc/
[6]: http://tukan.farm/2016/10/14/scraps-of-notes/
[7]: http://tukan.farm/2016/07/27/munmap-madness/
[8]: https://sourceware.org/ml/libc-alpha/2017-05/msg00899.html
[9]: https://sourceware.org/ml/libc-alpha/2017-10/msg01202.html


Istvan Kurucsai (7):
  malloc: Add check for top size corruption.
  malloc: Additional checks for unsorted bin integrity I.
  malloc: Ensure that the consolidated fast chunk has a sane size.
  malloc: Ensure lower bound on chunk size in __libc_realloc.
  malloc: Verify the integrity of mmapped chunks in calloc.
  malloc: Add more integrity checks to mremap_chunk.
  malloc: Check the alignment of mmapped chunks before unmapping.

 malloc/malloc.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH v2 3/7] malloc: Ensure that the consolidated fast chunk has a sane size.

[Istvan Kurucsai]

    * malloc/malloc.c (malloc_consolidate): Add size check.
---
 malloc/malloc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index d3fac7e..51d703c 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -4406,6 +4406,7 @@ static void malloc_consolidate(mstate av)
   mfastbinptr*    fb;                 /* current fastbin being consolidated */
   mfastbinptr*    maxfb;              /* last fastbin (for loop control) */
   mchunkptr       p;                  /* current chunk being consolidated */
+  unsigned int    idx;                /* fastbin index of current chunk */
   mchunkptr       nextp;              /* next chunk to consolidate */
   mchunkptr       unsorted_bin;       /* bin header */
   mchunkptr       first_unsorted;     /* chunk to link to */
@@ -4437,6 +4438,10 @@ static void malloc_consolidate(mstate av)
     p = atomic_exchange_acq (fb, NULL);
     if (p != 0) {
       do {
+	idx = fastbin_index (chunksize (p));
+	if ((&fastbin (av, idx)) != fb)
+	  malloc_printerr ("malloc_consolidate(): invalid chunk size");
+
 	check_inuse_chunk(av, p);
 	nextp = p->fd;
 
-- 
2.7.4

Re: [PATCH v2 3/7] malloc: Ensure that the consolidated fast chunk has a sane size.

[Florian Weimer]

On 11/07/2017 04:27 PM, Istvan Kurucsai wrote:
>      * malloc/malloc.c (malloc_consolidate): Add size check.
> ---
>   malloc/malloc.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index d3fac7e..51d703c 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -4406,6 +4406,7 @@ static void malloc_consolidate(mstate av)
>     mfastbinptr*    fb;                 /* current fastbin being consolidated */
>     mfastbinptr*    maxfb;              /* last fastbin (for loop control) */
>     mchunkptr       p;                  /* current chunk being consolidated */
> +  unsigned int    idx;                /* fastbin index of current chunk */
>     mchunkptr       nextp;              /* next chunk to consolidate */
>     mchunkptr       unsorted_bin;       /* bin header */
>     mchunkptr       first_unsorted;     /* chunk to link to */
> @@ -4437,6 +4438,10 @@ static void malloc_consolidate(mstate av)
>       p = atomic_exchange_acq (fb, NULL);
>       if (p != 0) {
>         do {
> +	idx = fastbin_index (chunksize (p));
> +	if ((&fastbin (av, idx)) != fb)
> +	  malloc_printerr ("malloc_consolidate(): invalid chunk size");
> +

This looks good.  I'm going to minimize the scope for idx and push this.

Thanks,
Florian

[PATCH v2 1/7] malloc: Add check for top size corruption.

[Istvan Kurucsai]

Ensure that the size of top is below av->system_mem.

    * malloc/malloc.c (_int_malloc): Check top size.
---
 malloc/malloc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index f94d51c..4a30c42 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -4078,6 +4078,10 @@ _int_malloc (mstate av, size_t bytes)
 
       if ((unsigned long) (size) >= (unsigned long) (nb + MINSIZE))
         {
+          if (__glibc_unlikely ((unsigned long) (size) > 
+                                (unsigned long) (av->system_mem)))
+            malloc_printerr("malloc(): corrupted top chunk");
+
           remainder_size = size - nb;
           remainder = chunk_at_offset (victim, nb);
           av->top = remainder;
-- 
2.7.4

Re: [PATCH v2 1/7] malloc: Add check for top size corruption.

[Andreas Schwab]

On Nov 07 2017, Istvan Kurucsai <pistukem@gmail.com> wrote:

> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index f94d51c..4a30c42 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -4078,6 +4078,10 @@ _int_malloc (mstate av, size_t bytes)
>  
>        if ((unsigned long) (size) >= (unsigned long) (nb + MINSIZE))
>          {
> +          if (__glibc_unlikely ((unsigned long) (size) > 
> +                                (unsigned long) (av->system_mem)))

Line break before operator, not after.  Redundant parens.

> +            malloc_printerr("malloc(): corrupted top chunk");

Space before paren.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH v2 1/7] malloc: Add check for top size corruption.

[Florian Weimer]

On 11/07/2017 04:27 PM, Istvan Kurucsai wrote:
> Ensure that the size of top is below av->system_mem.
> 
>      * malloc/malloc.c (_int_malloc): Check top size.
> ---
>   malloc/malloc.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index f94d51c..4a30c42 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -4078,6 +4078,10 @@ _int_malloc (mstate av, size_t bytes)
>   
>         if ((unsigned long) (size) >= (unsigned long) (nb + MINSIZE))
>           {
> +          if (__glibc_unlikely ((unsigned long) (size) >
> +                                (unsigned long) (av->system_mem)))
> +            malloc_printerr("malloc(): corrupted top chunk");
> +

Andreas already pointed out style issues.

I'm somewhat surprised that we have accurate accounting in av->system_mem.

Furthermore, for non-main arenas, I think the check should be against 
the size of a single heap, or maybe the minimum of av->system_mem and 
that size.

Thanks,
Florian

Re: [PATCH v2 1/7] malloc: Add check for top size corruption.

[Istvan Kurucsai]

> Andreas already pointed out style issues.
>
> I'm somewhat surprised that we have accurate accounting in av->system_mem.
>
> Furthermore, for non-main arenas, I think the check should be against the
> size of a single heap, or maybe the minimum of av->system_mem and that size.

I thought about this and believe that we can ensure something more
strict: that the end of the top chunk is the same as the end of the
arena (contiguous main_arena case) or the heap (mmapped arena case),
see below. Tests passed but I'm a bit uncertain if these invariants
are always held.


Ensure that the end of the top chunk is the same as
 the end of the arena/heap.

    * malloc/malloc.c (_int_malloc): Check top size.
---
 malloc/malloc.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index f5aafd2..fd0f001 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -2251,6 +2251,33 @@ do_check_malloc_state (mstate av)
 }
 #endif

+static bool
+valid_top_chunk (mstate av, mchunkptr top)
+{
+  size_t size = chunksize(top);
+
+  assert (av);
+  assert (av->top != initial_top (av));
+
+  if (av == &main_arena)
+    {
+      if ((contiguous (&main_arena)
+          && __glibc_unlikely ((uintptr_t) top + size
+                               != (uintptr_t) mp_.sbrk_base + av->system_mem))
+          || (!contiguous (&main_arena)
+              && __glibc_unlikely (size > av->system_mem)))
+        return false;
+    }
+  else
+    {
+      heap_info *heap = heap_for_ptr (top);
+      uintptr_t heap_end = (uintptr_t) heap + heap->size;
+      if (__glibc_unlikely ((uintptr_t) top + size != heap_end))
+        return false;
+    }
+
+  return true;
+}

 /* ----------------- Support for debugging hooks -------------------- */
 #include "hooks.c"
@@ -4088,6 +4115,8 @@ _int_malloc (mstate av, size_t bytes)

       if ((unsigned long) (size) >= (unsigned long) (nb + MINSIZE))
         {
+          if (__glibc_unlikely (!valid_top_chunk (av, victim)))
+            malloc_printerr ("malloc(): corrupted top chunk");
           remainder_size = size - nb;
           remainder = chunk_at_offset (victim, nb);
           av->top = remainder;
-- 
2.7.4

Re: [PATCH v2 1/7] malloc: Add check for top size corruption.

[Florian Weimer]

On 01/16/2018 01:05 PM, Istvan Kurucsai wrote:
>> Andreas already pointed out style issues.
>>
>> I'm somewhat surprised that we have accurate accounting in av->system_mem.
>>
>> Furthermore, for non-main arenas, I think the check should be against the
>> size of a single heap, or maybe the minimum of av->system_mem and that size.
> 
> I thought about this and believe that we can ensure something more
> strict: that the end of the top chunk is the same as the end of the
> arena (contiguous main_arena case) or the heap (mmapped arena case),
> see below. Tests passed but I'm a bit uncertain if these invariants
> are always held.
> 
> 
> Ensure that the end of the top chunk is the same as
>   the end of the arena/heap.
> 
>      * malloc/malloc.c (_int_malloc): Check top size.
> ---
>   malloc/malloc.c | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
> 
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index f5aafd2..fd0f001 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -2251,6 +2251,33 @@ do_check_malloc_state (mstate av)
>   }
>   #endif
> 
> +static bool
> +valid_top_chunk (mstate av, mchunkptr top)
> +{
> +  size_t size = chunksize(top);
> +
> +  assert (av);

I think we can drop that assert, it is implied by the pointer 
dereference in the subsequent assert.

> +  assert (av->top != initial_top (av));
> +
> +  if (av == &main_arena)
> +    {
> +      if ((contiguous (&main_arena)
> +          && __glibc_unlikely ((uintptr_t) top + size
> +                               != (uintptr_t) mp_.sbrk_base + av->system_mem))
> +          || (!contiguous (&main_arena)
> +              && __glibc_unlikely (size > av->system_mem)))
> +        return false;
> +    }
> +  else
> +    {
> +      heap_info *heap = heap_for_ptr (top);
> +      uintptr_t heap_end = (uintptr_t) heap + heap->size;
> +      if (__glibc_unlikely ((uintptr_t) top + size != heap_end))
> +        return false;
> +    }
> +
> +  return true;
> +}

I wonder if it is possible to write this in a slightly clearer way.

Maybe add a nested if (contiguous (&main_arena)) for the first branch, 
and directly return the value of the inner-most conditional?

Thanks,
Florian

Re: [PATCH v2 1/7] malloc: Add check for top size corruption.

[Florian Weimer]

On 02/20/2018 02:45 PM, Florian Weimer wrote:
> On 01/16/2018 01:05 PM, Istvan Kurucsai wrote:
>>> Andreas already pointed out style issues.
>>>
>>> I'm somewhat surprised that we have accurate accounting in 
>>> av->system_mem.
>>>
>>> Furthermore, for non-main arenas, I think the check should be against 
>>> the
>>> size of a single heap, or maybe the minimum of av->system_mem and 
>>> that size.
>>
>> I thought about this and believe that we can ensure something more
>> strict: that the end of the top chunk is the same as the end of the
>> arena (contiguous main_arena case) or the heap (mmapped arena case),
>> see below. Tests passed but I'm a bit uncertain if these invariants
>> are always held.
>>
>>
>> Ensure that the end of the top chunk is the same as
>>   the end of the arena/heap.
>>
>>      * malloc/malloc.c (_int_malloc): Check top size.
>> ---
>>   malloc/malloc.c | 29 +++++++++++++++++++++++++++++
>>   1 file changed, 29 insertions(+)
>>
>> diff --git a/malloc/malloc.c b/malloc/malloc.c
>> index f5aafd2..fd0f001 100644
>> --- a/malloc/malloc.c
>> +++ b/malloc/malloc.c
>> @@ -2251,6 +2251,33 @@ do_check_malloc_state (mstate av)
>>   }
>>   #endif
>>
>> +static bool
>> +valid_top_chunk (mstate av, mchunkptr top)
>> +{
>> +  size_t size = chunksize(top);
>> +
>> +  assert (av);
> 
> I think we can drop that assert, it is implied by the pointer 
> dereference in the subsequent assert.
> 
>> +  assert (av->top != initial_top (av));
>> +
>> +  if (av == &main_arena)
>> +    {
>> +      if ((contiguous (&main_arena)
>> +          && __glibc_unlikely ((uintptr_t) top + size
>> +                               != (uintptr_t) mp_.sbrk_base + 
>> av->system_mem))
>> +          || (!contiguous (&main_arena)
>> +              && __glibc_unlikely (size > av->system_mem)))
>> +        return false;
>> +    }
>> +  else
>> +    {
>> +      heap_info *heap = heap_for_ptr (top);
>> +      uintptr_t heap_end = (uintptr_t) heap + heap->size;
>> +      if (__glibc_unlikely ((uintptr_t) top + size != heap_end))
>> +        return false;
>> +    }
>> +
>> +  return true;
>> +}
> 
> I wonder if it is possible to write this in a slightly clearer way.
> 
> Maybe add a nested if (contiguous (&main_arena)) for the first branch, 
> and directly return the value of the inner-most conditional?

Any further comments here?

Thanks,
Florian

[PATCH v2 7/7] malloc: Check the alignment of mmapped chunks before unmapping.

[Istvan Kurucsai]

    * malloc/malloc.c (munmap_chunk): Verify chunk alignment.
---
 malloc/malloc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 1a2ba04..0df4f14 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -2819,6 +2819,7 @@ systrim (size_t pad, mstate av)
 static void
 munmap_chunk (mchunkptr p)
 {
+  size_t pagesize = GLRO (dl_pagesize);
   INTERNAL_SIZE_T size = chunksize (p);
 
   assert (chunk_is_mmapped (p));
@@ -2828,6 +2829,7 @@ munmap_chunk (mchunkptr p)
   if (DUMPED_MAIN_ARENA_CHUNK (p))
     return;
 
+  uintptr_t mem = (uintptr_t) chunk2mem(p);
   uintptr_t block = (uintptr_t) p - prev_size (p);
   size_t total_size = prev_size (p) + size;
   /* Unfortunately we have to do the compilers job by hand here.  Normally
@@ -2835,7 +2837,8 @@ munmap_chunk (mchunkptr p)
      page size.  But gcc does not recognize the optimization possibility
      (in the moment at least) so we combine the two values into one before
      the bit test.  */
-  if (__builtin_expect (((block | total_size) & (GLRO (dl_pagesize) - 1)) != 0, 0))
+  if (__glibc_unlikely ((block | total_size) & (pagesize - 1)) != 0
+      || __glibc_unlikely (!powerof2 (mem & (pagesize - 1))))
     malloc_printerr ("munmap_chunk(): invalid pointer");
 
   atomic_decrement (&mp_.n_mmaps);
-- 
2.7.4

Re: [PATCH v2 7/7] malloc: Check the alignment of mmapped chunks before unmapping.

[DJ Delorie]

I +1'd this patch series last year when it was first posted (sorry about
the lack of consensus-building) but just to revive it I'll +1 it again
independently.  Could we get a second review too?  Florian?

Reviewed-Again-By: DJ Delorie <dj@redhat.com>

Istvan Kurucsai <pistukem@gmail.com> writes:
> +  size_t pagesize = GLRO (dl_pagesize);

pagesize is used multiple times, so save the overhead.  OK.

> +  uintptr_t mem = (uintptr_t) chunk2mem(p);

This is the pointer the application sees.  OK

> -  if (__builtin_expect (((block | total_size) & (GLRO (dl_pagesize) - 1)) != 0, 0))
> +  if (__glibc_unlikely ((block | total_size) & (pagesize - 1)) != 0
> +      || __glibc_unlikely (!powerof2 (mem & (pagesize - 1))))

This adds a test for "is the pointer the application saw, some
unexpected offset into the page?".  OK.

Re: [PATCH v2 7/7] malloc: Check the alignment of mmapped chunks before unmapping.

[Florian Weimer]

* DJ Delorie:

> I +1'd this patch series last year when it was first posted (sorry about
> the lack of consensus-building) but just to revive it I'll +1 it again
> independently.  Could we get a second review too?  Florian?

It's similar to the other case which check chunk alignment in mapped
chunks, so this looks okay to me.

Thanks,
Florian

Re: [PATCH v2 7/7] malloc: Check the alignment of mmapped chunks before unmapping.

[DJ Delorie]

Florian Weimer <fweimer@redhat.com> writes:
> * DJ Delorie:
>
>> I +1'd this patch series last year when it was first posted (sorry about
>> the lack of consensus-building) but just to revive it I'll +1 it again
>> independently.  Could we get a second review too?  Florian?
>
> It's similar to the other case which check chunk alignment in mapped
> chunks, so this looks okay to me.

Belated, but committed.

[PATCH v2 5/7] malloc: Verify the integrity of mmapped chunks in calloc.

[Istvan Kurucsai]

    * malloc/malloc.c (__libc_calloc): Check mmapped chunks.
---
 malloc/malloc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 8e48952..5eb661e 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3447,6 +3447,15 @@ __libc_calloc (size_t n, size_t elem_size)
   /* Two optional cases in which clearing not necessary */
   if (chunk_is_mmapped (p))
     {
+      size_t pagesize = GLRO (dl_pagesize);
+      INTERNAL_SIZE_T offset = prev_size (p);
+      INTERNAL_SIZE_T size = chunksize (p);
+      uintptr_t block = (uintptr_t) p - offset;
+      size_t total_size = offset + size;
+      if (__glibc_unlikely ((block | total_size) & (pagesize - 1)) != 0
+          || __glibc_unlikely (!powerof2 ((uintptr_t) mem & (pagesize - 1))))
+        malloc_printerr ("calloc(): invalid mmapped chunk");
+
       if (__builtin_expect (perturb_byte, 0))
         return memset (mem, 0, sz);
 
-- 
2.7.4

Re: [PATCH v2 5/7] malloc: Verify the integrity of mmapped chunks in calloc.

[Florian Weimer]

On 11/07/2017 04:27 PM, Istvan Kurucsai wrote:
>      * malloc/malloc.c (__libc_calloc): Check mmapped chunks.
> ---
>   malloc/malloc.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 8e48952..5eb661e 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -3447,6 +3447,15 @@ __libc_calloc (size_t n, size_t elem_size)
>     /* Two optional cases in which clearing not necessary */
>     if (chunk_is_mmapped (p))
>       {
> +      size_t pagesize = GLRO (dl_pagesize);
> +      INTERNAL_SIZE_T offset = prev_size (p);
> +      INTERNAL_SIZE_T size = chunksize (p);
> +      uintptr_t block = (uintptr_t) p - offset;
> +      size_t total_size = offset + size;
> +      if (__glibc_unlikely ((block | total_size) & (pagesize - 1)) != 0
> +          || __glibc_unlikely (!powerof2 ((uintptr_t) mem & (pagesize - 1))))
> +        malloc_printerr ("calloc(): invalid mmapped chunk");
> +
>         if (__builtin_expect (perturb_byte, 0))
>           return memset (mem, 0, sz);

Sorry, I don't understand the powerof2 check.  Could you elaborate?

Thanks,
Florian

Re: [PATCH v2 5/7] malloc: Verify the integrity of mmapped chunks in calloc.

[Florian Weimer]

* Florian Weimer:

> On 11/07/2017 04:27 PM, Istvan Kurucsai wrote:
>>      * malloc/malloc.c (__libc_calloc): Check mmapped chunks.
>> ---
>>   malloc/malloc.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/malloc/malloc.c b/malloc/malloc.c
>> index 8e48952..5eb661e 100644
>> --- a/malloc/malloc.c
>> +++ b/malloc/malloc.c
>> @@ -3447,6 +3447,15 @@ __libc_calloc (size_t n, size_t elem_size)
>>     /* Two optional cases in which clearing not necessary */
>>     if (chunk_is_mmapped (p))
>>       {
>> +      size_t pagesize = GLRO (dl_pagesize);
>> +      INTERNAL_SIZE_T offset = prev_size (p);
>> +      INTERNAL_SIZE_T size = chunksize (p);
>> +      uintptr_t block = (uintptr_t) p - offset;
>> +      size_t total_size = offset + size;
>> +      if (__glibc_unlikely ((block | total_size) & (pagesize - 1)) != 0
>> +          || __glibc_unlikely (!powerof2 ((uintptr_t) mem & (pagesize - 1))))
>> +        malloc_printerr ("calloc(): invalid mmapped chunk");
>> +
>>         if (__builtin_expect (perturb_byte, 0))
>>           return memset (mem, 0, sz);
>
> Sorry, I don't understand the powerof2 check.  Could you elaborate?

I believe this is similar to the mremap_chunk case DJ reviewed.

Thanks,
Florian

[PATCH v2 6/7] malloc: Add more integrity checks to mremap_chunk.

[Istvan Kurucsai]

Similarly to the ones in munmap_chunk, ensure that the mapped region begins at
a page boundary, that the size is page-aligned and that the offset of the
chunk into its page is a power of 2.

    * malloc/malloc.c (mremap_chunk): Additional checks.
---
 malloc/malloc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 5eb661e..1a2ba04 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -2858,16 +2858,22 @@ mremap_chunk (mchunkptr p, size_t new_size)
   char *cp;
 
   assert (chunk_is_mmapped (p));
-  assert (((size + offset) & (GLRO (dl_pagesize) - 1)) == 0);
+
+  uintptr_t block = (uintptr_t) p - offset;
+  uintptr_t mem = (uintptr_t) chunk2mem(p);
+  size_t total_size = offset + size;
+  if (__glibc_unlikely ((block | total_size) & (pagesize - 1)) != 0
+      || __glibc_unlikely (!powerof2 (mem & (pagesize - 1))))
+    malloc_printerr("mremap_chunk(): invalid pointer");
 
   /* Note the extra SIZE_SZ overhead as in mmap_chunk(). */
   new_size = ALIGN_UP (new_size + offset + SIZE_SZ, pagesize);
 
   /* No need to remap if the number of pages does not change.  */
-  if (size + offset == new_size)
+  if (total_size == new_size)
     return p;
 
-  cp = (char *) __mremap ((char *) p - offset, size + offset, new_size,
+  cp = (char *) __mremap ((char *) block, total_size, new_size,
                           MREMAP_MAYMOVE);
 
   if (cp == MAP_FAILED)
-- 
2.7.4

Re: [PATCH v2 6/7] malloc: Add more integrity checks to mremap_chunk.

[DJ Delorie]

I +1'd this patch series last year when it was first posted (sorry about
the lack of consensus-building) but just to revive it I'll +1 it again
independently.  Could we get a second review too?  Florian?

Reviewed-Again-By: DJ Delorie <dj@redhat.com>

Istvan Kurucsai <pistukem@gmail.com> writes:
> -  assert (((size + offset) & (GLRO (dl_pagesize) - 1)) == 0);

pagesize is set earlier in this function.

> +  uintptr_t block = (uintptr_t) p - offset;
> +  uintptr_t mem = (uintptr_t) chunk2mem(p);

block is the page that the chunk header is in; mem is the pointer the
application sees.

> +  size_t total_size = offset + size;

offset is "page start to chunk header", size is chunk size.  This should
span to the end of a page.  So...

> +  if (__glibc_unlikely ((block | total_size) & (pagesize - 1)) != 0

If block or total_size are misaligned,

> +      || __glibc_unlikely (!powerof2 (mem & (pagesize - 1))))

Or if the offset of the memory within the page is an unexpected size
(for us, 2*SIZE_SZ, is expected), report the error.  OK.

> -  if (size + offset == new_size)
> +  if (total_size == new_size)

Saving an operation.  OK.

>  
> -  cp = (char *) __mremap ((char *) p - offset, size + offset, new_size,
> +  cp = (char *) __mremap ((char *) block, total_size, new_size,

Likewise.  OK.

Re: [PATCH v2 6/7] malloc: Add more integrity checks to mremap_chunk.

[Florian Weimer]

* DJ Delorie:

> I +1'd this patch series last year when it was first posted (sorry about
> the lack of consensus-building) but just to revive it I'll +1 it again
> independently.  Could we get a second review too?  Florian?

Patch looks fine to me.

Florian

Re: [PATCH v2 6/7] malloc: Add more integrity checks to mremap_chunk.

[DJ Delorie]

Florian Weimer <fweimer@redhat.com> writes:
> * DJ Delorie:
>
>> I +1'd this patch series last year when it was first posted (sorry about
>> the lack of consensus-building) but just to revive it I'll +1 it again
>> independently.  Could we get a second review too?  Florian?
>
> Patch looks fine to me.

Belated, but committed.

[PATCH v2 4/7] malloc: Ensure lower bound on chunk size in __libc_realloc.

[Istvan Kurucsai]

Under some circumstances, a chunk size of SIZE_SZ could lead to an underflow
when calculating the length argument of memcpy.

   * malloc/malloc.c (__libc_realloc): Check chunk size.
---
 malloc/malloc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 51d703c..8e48952 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3154,8 +3154,9 @@ __libc_realloc (void *oldmem, size_t bytes)
      accident or by "design" from some intruder.  We need to bypass
      this check for dumped fake mmap chunks from the old main arena
      because the new malloc may provide additional alignment.  */
-  if ((__builtin_expect ((uintptr_t) oldp > (uintptr_t) -oldsize, 0)
-       || __builtin_expect (misaligned_chunk (oldp), 0))
+  if ((__glibc_unlikely ((uintptr_t) oldp > (uintptr_t) -oldsize)
+       || __glibc_unlikely (misaligned_chunk (oldp))
+       || __glibc_unlikely (oldsize <= 2 * SIZE_SZ))
       && !DUMPED_MAIN_ARENA_CHUNK (oldp))
       malloc_printerr ("realloc(): invalid pointer");
 
-- 
2.7.4

Re: [PATCH v2 4/7] malloc: Ensure lower bound on chunk size in __libc_realloc.

[Florian Weimer]

On 11/07/2017 04:27 PM, Istvan Kurucsai wrote:
> Under some circumstances, a chunk size of SIZE_SZ could lead to an underflow
> when calculating the length argument of memcpy.
> 
>     * malloc/malloc.c (__libc_realloc): Check chunk size.
> ---
>   malloc/malloc.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 51d703c..8e48952 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -3154,8 +3154,9 @@ __libc_realloc (void *oldmem, size_t bytes)
>        accident or by "design" from some intruder.  We need to bypass
>        this check for dumped fake mmap chunks from the old main arena
>        because the new malloc may provide additional alignment.  */
> -  if ((__builtin_expect ((uintptr_t) oldp > (uintptr_t) -oldsize, 0)
> -       || __builtin_expect (misaligned_chunk (oldp), 0))
> +  if ((__glibc_unlikely ((uintptr_t) oldp > (uintptr_t) -oldsize)
> +       || __glibc_unlikely (misaligned_chunk (oldp))
> +       || __glibc_unlikely (oldsize <= 2 * SIZE_SZ))
>         && !DUMPED_MAIN_ARENA_CHUNK (oldp))
>         malloc_printerr ("realloc(): invalid pointer");

DJ, can you benchmark this change?  If it turns out this is visible in 
profiles, we may have to move this check closer to the actual memcpy.

Thanks,
Florian

Re: [PATCH v2 4/7] malloc: Ensure lower bound on chunk size in __libc_realloc.

[DJ Delorie]

Florian Weimer <fweimer@redhat.com> writes:
> DJ, can you benchmark this change?  If it turns out this is visible in 
> profiles, we may have to move this check closer to the actual memcpy.

I couldn't detect any measurable change from this patch.

Re: [PATCH v2 4/7] malloc: Ensure lower bound on chunk size in __libc_realloc.

[Carlos O'Donell]

On 08/20/2018 05:20 PM, DJ Delorie wrote:
> 
> Florian Weimer <fweimer@redhat.com> writes:
>> DJ, can you benchmark this change?  If it turns out this is visible in 
>> profiles, we may have to move this check closer to the actual memcpy.
> 
> I couldn't detect any measurable change from this patch.
 
How did you benchmark it? perf?

What were the results?

-- 
Cheers,
Carlos.

Re: [PATCH v2 4/7] malloc: Ensure lower bound on chunk size in __libc_realloc.

[DJ Delorie]

"Carlos O'Donell" <carlos@redhat.com> writes:
>> I couldn't detect any measurable change from this patch.
>  
> How did you benchmark it? perf?

I time multiple runs of trace_run with all my non-stupidly-big
workloads, and average the results.  The raw results are:


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

PRISTINE:
Total call time: 7,365,545,225 cycles

PATCHED: 
Total call time: 7,443,954,619 cycles

----------  dj ----------

PRISTINE:
Total call time: 831,383,129 cycles

PATCHED: 
Total call time: 848,574,833 cycles

----------  dj2 ----------

PRISTINE:
Total call time: 7,391,093,601 cycles

PATCHED: 
Total call time: 7,389,861,196 cycles

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

PRISTINE:
Total call time: 3,175,103,662 cycles

PATCHED: 
Total call time: 3,190,124,319 cycles

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

PRISTINE:
Total call time: 1,037,282,464 cycles

PATCHED: 
Total call time: 1,037,154,283 cycles

----------  proprietary-2 ----------

PRISTINE:
Total call time: 1,999,062,083 cycles

PATCHED: 
Total call time: 1,998,548,797 cycles

----------  qemu-virtio ----------

PRISTINE:
Total call time: 806,573,463 cycles

PATCHED: 
Total call time: 795,207,373 cycles

----------  qemu-win7 ----------

PRISTINE:
Total call time: 649,777,212 cycles

PATCHED: 
Total call time: 659,636,829 cycles

Re: [PATCH v2 4/7] malloc: Ensure lower bound on chunk size in __libc_realloc.

[Carlos O'Donell]

On 08/20/2018 08:17 PM, DJ Delorie wrote:
> "Carlos O'Donell" <carlos@redhat.com> writes:
>>> I couldn't detect any measurable change from this patch.
>>  
>> How did you benchmark it? perf?
> 
> I time multiple runs of trace_run with all my non-stupidly-big
> workloads, and average the results.  The raw results are:

OK, so the mean seems to be ~0.08% slower. So no measurable change
as you stated. Thanks for the numbers. If someone has an application
that shows a measurable difference we can ask for a trace of it.

-- 
Cheers,
Carlos.

[PATCH v2 2/7] malloc: Additional checks for unsorted bin integrity I.

[Istvan Kurucsai]

Ensure the following properties of chunks encountered during binning:
- victim chunk has reasonable size
- next chunk has reasonable size
- next->prev_size == victim->size
- valid double linked list
- PREV_INUSE of next chunk is unset

    * malloc/malloc.c (_int_malloc): Additional binning code checks.
---
 malloc/malloc.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 4a30c42..d3fac7e 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3513,6 +3513,7 @@ _int_malloc (mstate av, size_t bytes)
   INTERNAL_SIZE_T size;             /* its size */
   int victim_index;                 /* its bin index */
 
+  mchunkptr next;                   /* next contiguous chunk */
   mchunkptr remainder;              /* remainder from a split */
   unsigned long remainder_size;     /* its size */
 
@@ -3718,11 +3719,22 @@ _int_malloc (mstate av, size_t bytes)
       while ((victim = unsorted_chunks (av)->bk) != unsorted_chunks (av))
         {
           bck = victim->bk;
-          if (__builtin_expect (chunksize_nomask (victim) <= 2 * SIZE_SZ, 0)
-              || __builtin_expect (chunksize_nomask (victim)
-				   > av->system_mem, 0))
-            malloc_printerr ("malloc(): memory corruption");
           size = chunksize (victim);
+          next = chunk_at_offset (victim, size);
+
+          if (__glibc_unlikely (chunksize_nomask (victim) <= 2 * SIZE_SZ)
+              || __glibc_unlikely (chunksize_nomask (victim) > av->system_mem))
+            malloc_printerr("malloc(): invalid size (unsorted)");
+          if (__glibc_unlikely (chunksize_nomask (next) < 2 * SIZE_SZ)
+              || __glibc_unlikely (chunksize_nomask (next) > av->system_mem))
+            malloc_printerr("malloc(): invalid next size (unsorted)");
+          if (__glibc_unlikely ((prev_size (next) & ~(SIZE_BITS)) != size))
+            malloc_printerr("malloc(): mismatching next->prev_size (unsorted)");
+          if (__glibc_unlikely (bck->fd != victim)
+              || __glibc_unlikely (victim->fd != unsorted_chunks (av)))
+            malloc_printerr("malloc(): unsorted double linked list corrupted");
+          if (__glibc_unlikely (prev_inuse(next)))
+            malloc_printerr("malloc(): invalid next->prev_inuse (unsorted)");
 
           /*
              If a small request, try to use last remainder if it is the
-- 
2.7.4

Re: [PATCH v2 2/7] malloc: Additional checks for unsorted bin integrity I.

[Florian Weimer]

On 11/07/2017 04:27 PM, Istvan Kurucsai wrote:
> +          next = chunk_at_offset (victim, size);

For new code, we prefer declarations with initializers.

> +          if (__glibc_unlikely (chunksize_nomask (victim) <= 2 * SIZE_SZ)
> +              || __glibc_unlikely (chunksize_nomask (victim) > av->system_mem))
> +            malloc_printerr("malloc(): invalid size (unsorted)");
> +          if (__glibc_unlikely (chunksize_nomask (next) < 2 * SIZE_SZ)
> +              || __glibc_unlikely (chunksize_nomask (next) > av->system_mem))
> +            malloc_printerr("malloc(): invalid next size (unsorted)");
> +          if (__glibc_unlikely ((prev_size (next) & ~(SIZE_BITS)) != size))
> +            malloc_printerr("malloc(): mismatching next->prev_size (unsorted)");

I think this check is redundant because prev_size (next) and chunksize 
(victim) are loaded from the same memory location.

> +          if (__glibc_unlikely (bck->fd != victim)
> +              || __glibc_unlikely (victim->fd != unsorted_chunks (av)))
> +            malloc_printerr("malloc(): unsorted double linked list corrupted");
> +          if (__glibc_unlikely (prev_inuse(next)))
> +            malloc_printerr("malloc(): invalid next->prev_inuse (unsorted)");

There's a missing space after malloc_printerr.

Why do you keep using chunksize_nomask?  We never investigated why the 
original code uses it.  It may have been an accident.

Again, for non-main arenas, the checks against av->system_mem could be 
made tighter (against the heap size).  Maybe you could put the condition 
into a separate inline function?

Thanks,
Florian

Re: [PATCH v2 2/7] malloc: Additional checks for unsorted bin integrity I.

[Istvan Kurucsai]

[-- Attachment #1: Type: text/plain, Size: 2713 bytes --]

On Thu, Jan 11, 2018 at 3:50 PM, Florian Weimer <fweimer@redhat.com> wrote:
> On 11/07/2017 04:27 PM, Istvan Kurucsai wrote:
>>
>> +          next = chunk_at_offset (victim, size);
>
>
> For new code, we prefer declarations with initializers.

Noted.

>> +          if (__glibc_unlikely (chunksize_nomask (victim) <= 2 * SIZE_SZ)
>> +              || __glibc_unlikely (chunksize_nomask (victim) >
>> av->system_mem))
>> +            malloc_printerr("malloc(): invalid size (unsorted)");
>> +          if (__glibc_unlikely (chunksize_nomask (next) < 2 * SIZE_SZ)
>> +              || __glibc_unlikely (chunksize_nomask (next) >
>> av->system_mem))
>> +            malloc_printerr("malloc(): invalid next size (unsorted)");
>> +          if (__glibc_unlikely ((prev_size (next) & ~(SIZE_BITS)) !=
>> size))
>> +            malloc_printerr("malloc(): mismatching next->prev_size
>> (unsorted)");
>
>
> I think this check is redundant because prev_size (next) and chunksize
> (victim) are loaded from the same memory location.

I'm fairly certain that it compares mchunk_size of victim against
mchunk_prev_size of the next chunk, i.e. the size of victim in its
header and footer.

>> +          if (__glibc_unlikely (bck->fd != victim)
>> +              || __glibc_unlikely (victim->fd != unsorted_chunks (av)))
>> +            malloc_printerr("malloc(): unsorted double linked list
>> corrupted");
>> +          if (__glibc_unlikely (prev_inuse(next)))
>> +            malloc_printerr("malloc(): invalid next->prev_inuse
>> (unsorted)");
>
>
> There's a missing space after malloc_printerr.

Noted.

> Why do you keep using chunksize_nomask?  We never investigated why the
> original code uses it.  It may have been an accident.

You are right, I don't think it makes a difference in these checks. So
the size local can be reused for the checks against victim. For next,
leaving it as such avoids the masking operation.

> Again, for non-main arenas, the checks against av->system_mem could be made
> tighter (against the heap size).  Maybe you could put the condition into a
> separate inline function?

We could also do a chunk boundary check similar to what I proposed in
the thread for the first patch in the series to be even more strict.
I'll gladly try to implement either but believe that refining these
checks would bring less benefits than in the case of the top chunk.
Intra-arena or intra-heap overlaps would still be doable here with
unsorted chunks and I don't see any way to counter that besides more
generic measures like randomizing allocations and your metadata
encoding patches.

I've attached a revised version with the above comments incorporated
but without the refined checks.

Thanks,
Istvan

[-- Attachment #2: 0001-malloc-Additional-checks-for-unsorted-bin-integrity-.patch --]
[-- Type: text/x-patch, Size: 2271 bytes --]

From a12d5d40fd7aed5fa10fc444dcb819947b72b315 Mon Sep 17 00:00:00 2001
From: Istvan Kurucsai <pistukem@gmail.com>
Date: Tue, 16 Jan 2018 14:48:16 +0100
Subject: [PATCH v2 1/1] malloc: Additional checks for unsorted bin integrity
 I.

Ensure the following properties of chunks encountered during binning:
- victim chunk has reasonable size
- next chunk has reasonable size
- next->prev_size == victim->size
- valid double linked list
- PREV_INUSE of next chunk is unset

    * malloc/malloc.c (_int_malloc): Additional binning code checks.
---
 malloc/malloc.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/malloc/malloc.c b/malloc/malloc.c
index f5aafd2..73f7aba 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3728,11 +3728,22 @@ _int_malloc (mstate av, size_t bytes)
       while ((victim = unsorted_chunks (av)->bk) != unsorted_chunks (av))
         {
           bck = victim->bk;
-          if (__builtin_expect (chunksize_nomask (victim) <= 2 * SIZE_SZ, 0)
-              || __builtin_expect (chunksize_nomask (victim)
-				   > av->system_mem, 0))
-            malloc_printerr ("malloc(): memory corruption");
           size = chunksize (victim);
+          mchunkptr next = chunk_at_offset (victim, size);
+
+          if (__glibc_unlikely (size <= 2 * SIZE_SZ)
+              || __glibc_unlikely (size > av->system_mem))
+            malloc_printerr ("malloc(): invalid size (unsorted)");
+          if (__glibc_unlikely (chunksize_nomask (next) < 2 * SIZE_SZ)
+              || __glibc_unlikely (chunksize_nomask (next) > av->system_mem))
+            malloc_printerr ("malloc(): invalid next size (unsorted)");
+          if (__glibc_unlikely ((prev_size (next) & ~(SIZE_BITS)) != size))
+            malloc_printerr ("malloc(): mismatching next->prev_size (unsorted)");
+          if (__glibc_unlikely (bck->fd != victim)
+              || __glibc_unlikely (victim->fd != unsorted_chunks (av)))
+            malloc_printerr ("malloc(): unsorted double linked list corrupted");
+          if (__glibc_unlikely (prev_inuse(next)))
+            malloc_printerr ("malloc(): invalid next->prev_inuse (unsorted)");
 
           /*
              If a small request, try to use last remainder if it is the
-- 
2.7.4

Re: [PATCH v2 2/7] malloc: Additional checks for unsorted bin integrity I.

[Florian Weimer]

On 01/16/2018 02:54 PM, Istvan Kurucsai wrote:
> +          if (__glibc_unlikely ((prev_size (next) & ~(SIZE_BITS)) != size))
> +            malloc_printerr ("malloc(): mismatching next->prev_size (unsorted)");

Is the masking required?  I think prev_size is stored without the bits.

> +          if (__glibc_unlikely (bck->fd != victim)
> +              || __glibc_unlikely (victim->fd != unsorted_chunks (av)))
> +            malloc_printerr ("malloc(): unsorted double linked list corrupted");
> +          if (__glibc_unlikely (prev_inuse(next)))
> +            malloc_printerr ("malloc(): invalid next->prev_inuse (unsorted)");

Space missing after prev_inuse.

Otherwise, this looks okay.

Thanks,
Florian

Re: [PATCH v2 2/7] malloc: Additional checks for unsorted bin integrity I.

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]

On 08/17/2018 04:07 PM, Florian Weimer wrote:
> On 01/16/2018 02:54 PM, Istvan Kurucsai wrote:
>> +          if (__glibc_unlikely ((prev_size (next) & ~(SIZE_BITS)) != 
>> size))
>> +            malloc_printerr ("malloc(): mismatching next->prev_size 
>> (unsorted)");
> 
> Is the masking required?  I think prev_size is stored without the bits.
> 
>> +          if (__glibc_unlikely (bck->fd != victim)
>> +              || __glibc_unlikely (victim->fd != unsorted_chunks (av)))
>> +            malloc_printerr ("malloc(): unsorted double linked list 
>> corrupted");
>> +          if (__glibc_unlikely (prev_inuse(next)))
>> +            malloc_printerr ("malloc(): invalid next->prev_inuse 
>> (unsorted)");
> 
> Space missing after prev_inuse.
> 
> Otherwise, this looks okay.

I accidentally pushed this without a ChangeLog entry.  Fixed with the 
attached patch.  Sorry about that.

Florian

[-- Attachment #2: 0001-malloc-Add-ChangeLog-for-accidentally-committed-chan.patch --]
[-- Type: text/x-patch, Size: 1638 bytes --]

From 35cfefd96062145eeb8aee6bd72d07e0909a6b2e Mon Sep 17 00:00:00 2001
Message-Id: <35cfefd96062145eeb8aee6bd72d07e0909a6b2e.1534769912.git.fweimer@redhat.com>
From: Florian Weimer <fweimer@redhat.com>
Date: Mon, 20 Aug 2018 14:57:13 +0200
Subject: [PATCH] malloc: Add ChangeLog for accidentally committed change
To: libc-alpha@sourceware.org

Commit b90ddd08f6dd688e651df9ee89ca3a69ff88cd0c ("malloc: Additional
checks for unsorted bin integrity I.") was committed without a
whitespace fix, so it is adjusted here as well.
---
 ChangeLog       | 4 ++++
 malloc/malloc.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index deb099483f..56ab51d1b8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -20,6 +20,10 @@
 
 	* sysdeps/s390/fpu/libm-test-ulps: Regenerate.
 
+2018-08-17  Istvan Kurucsai  <pistukem@gmail.com>
+
+	* malloc/malloc.c (_int_malloc): Additional binning code checks.
+
 2018-08-16  Florian Weimer  <fweimer@redhat.com>
 
 	* configure.ac: Add --with-nonshared-cflags option.
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 47795601c8..67cdfd0ad2 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3730,7 +3730,7 @@ _int_malloc (mstate av, size_t bytes)
           if (__glibc_unlikely (bck->fd != victim)
               || __glibc_unlikely (victim->fd != unsorted_chunks (av)))
             malloc_printerr ("malloc(): unsorted double linked list corrupted");
-          if (__glibc_unlikely (prev_inuse(next)))
+          if (__glibc_unlikely (prev_inuse (next)))
             malloc_printerr ("malloc(): invalid next->prev_inuse (unsorted)");
 
           /*
-- 
2.14.4

Re: [PATCH v2 0/7] Additional integrity checks for the malloc

[DJ Delorie]

I've reviewed the patches and they all LGTM but I'd want someone else to
check them also, preferably someone with more security experience.

Performance-wise, I benchmarked your patches against an unpatched glibc,
and saw no discernable performance change - half the benchmarks were
within 0.3% faster, the other half within 0.3% slower, which is still
"in the noise".  Most were much closer to "same".

Some of the patches got me wondering if we could, for example, store a
global "largest prev-chunk for an mmapped area" or store more data in
the mmap's fake prev chunk to further validate things.  Something for a
future patch perhaps.

Thanks!