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

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

[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

Reply | Threaded
Open this post in threaded view
|

[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

Reply | Threaded
Open this post in threaded view
|

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

Istvan Kurucsai
In reply to this post by 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

Reply | Threaded
Open this post in threaded view
|

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

Istvan Kurucsai
In reply to this post by 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

Reply | Threaded
Open this post in threaded view
|

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

Istvan Kurucsai
In reply to this post by 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

Reply | Threaded
Open this post in threaded view
|

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

Istvan Kurucsai
In reply to this post by 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

Reply | Threaded
Open this post in threaded view
|

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

Istvan Kurucsai
In reply to this post by 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

Reply | Threaded
Open this post in threaded view
|

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

Istvan Kurucsai
In reply to this post by 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

Reply | Threaded
Open this post in threaded view
|

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

Andreas Schwab
In reply to this post by Istvan Kurucsai
On Nov 07 2017, Istvan Kurucsai <[hidden email]> 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, [hidden email]
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

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

DJ Delorie-2
In reply to this post by Istvan Kurucsai

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!
Reply | Threaded
Open this post in threaded view
|

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

Florian Weimer-5
In reply to this post by Istvan Kurucsai
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
Reply | Threaded
Open this post in threaded view
|

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

Florian Weimer-5
In reply to this post by Istvan Kurucsai
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
Reply | Threaded
Open this post in threaded view
|

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

Florian Weimer-5
In reply to this post by Istvan Kurucsai
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
Reply | Threaded
Open this post in threaded view
|

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

Istvan Kurucsai
In reply to this post by Florian Weimer-5
> 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
Reply | Threaded
Open this post in threaded view
|

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

Istvan Kurucsai
In reply to this post by Florian Weimer-5
On Thu, Jan 11, 2018 at 3:50 PM, Florian Weimer <[hidden email]> 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

0001-malloc-Additional-checks-for-unsorted-bin-integrity-.patch (3K) Download Attachment