[PATCH] malloc: Check for large bin list corruption when inserting unsorted chunk

[PATCH] malloc: Check for large bin list corruption when inserting unsorted chunk

[Adam Maris]

Fixes bug 24216. This patch adds security checks for bk and bk_nextsize pointers
of chunks in large bin when inserting chunk from unsorted bin. It was possible
to write the pointer to victim (newly inserted chunk) to arbitrary memory
locations if bk or bk_nextsize pointers of the next large bin chunk
got corrupted.

Tested with no regressions.

* malloc/malloc.c (_int_malloc): Add security checks for large bin
chunks when inserting unsorted chunk.

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 6e766d11bc..801ba1f499 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3876,10 +3876,14 @@ _int_malloc (mstate av, size_t bytes)
                         {
                           victim->fd_nextsize = fwd;
                           victim->bk_nextsize = fwd->bk_nextsize;
+                          if (__glibc_unlikely
(fwd->bk_nextsize->fd_nextsize != fwd))
+                            malloc_printerr ("malloc(): largebin
double linked list corrupted (nextsize)");
                           fwd->bk_nextsize = victim;
                           victim->bk_nextsize->fd_nextsize = victim;
                         }
                       bck = fwd->bk;
+                      if (bck->fd != fwd)
+                        malloc_printerr ("malloc(): largebin double
linked list corrupted (bk)");
                     }
                 }
               else

Re: [PATCH] malloc: Check for large bin list corruption when inserting unsorted chunk

[Adam Maris]

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

On Tue, Feb 12, 2019 at 5:13 PM Adam Maris <amaris@redhat.com> wrote:
>
> Fixes bug 24216. This patch adds security checks for bk and bk_nextsize pointers
> of chunks in large bin when inserting chunk from unsorted bin. It was possible
> to write the pointer to victim (newly inserted chunk) to arbitrary memory
> locations if bk or bk_nextsize pointers of the next large bin chunk
> got corrupted.
>

Sending again with patch as attachment for better readability.

Best Regards,

Adam Mariš

[-- Attachment #2: frontlink.patch --]
[-- Type: text/x-patch, Size: 928 bytes --]

diff --git a/malloc/malloc.c b/malloc/malloc.c
index 6e766d11bc..801ba1f499 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3876,10 +3876,14 @@ _int_malloc (mstate av, size_t bytes)
                         {
                           victim->fd_nextsize = fwd;
                           victim->bk_nextsize = fwd->bk_nextsize;
+                          if (__glibc_unlikely (fwd->bk_nextsize->fd_nextsize != fwd))
+                            malloc_printerr ("malloc(): largebin double linked list corrupted (nextsize)");
                           fwd->bk_nextsize = victim;
                           victim->bk_nextsize->fd_nextsize = victim;
                         }
                       bck = fwd->bk;
+                      if (bck->fd != fwd)
+                        malloc_printerr ("malloc(): largebin double linked list corrupted (bk)");
                     }
                 }
               else

Re: [PATCH] malloc: Check for large bin list corruption when inserting unsorted chunk

[Adam Maris]

On Tue, Feb 12, 2019 at 5:34 PM Adam Maris <amaris@redhat.com> wrote:
>
> On Tue, Feb 12, 2019 at 5:13 PM Adam Maris <amaris@redhat.com> wrote:
> >
> > Fixes bug 24216. This patch adds security checks for bk and bk_nextsize pointers
> > of chunks in large bin when inserting chunk from unsorted bin. It was possible
> > to write the pointer to victim (newly inserted chunk) to arbitrary memory
> > locations if bk or bk_nextsize pointers of the next large bin chunk
> > got corrupted.
> >
>
> Sending again with patch as attachment for better readability.
>

Thoughts?

Re: [PATCH] malloc: Check for large bin list corruption when inserting unsorted chunk

[DJ Delorie]

Adam Maris <amaris@redhat.com> writes:
>                          {
>                            victim->fd_nextsize = fwd;
>                            victim->bk_nextsize = fwd->bk_nextsize;
> +                          if (__glibc_unlikely (fwd->bk_nextsize->fd_nextsize != fwd))
> +                            malloc_printerr ("malloc(): largebin double linked list corrupted (nextsize)");

At this point, the size of the chunk matches neither the previous nor
next chunks; we're creating a new "unique size" sub-head.  So, both fd
and fd_nextsize should point to the next (fwd) chunk.  We've hooked in
our new chunk but haven't yet fixed up the existing chunks, so this test
verifies that the links between the next chunk (fwd, which is a
start-of-size chunk) and the previous start-of-size chunk are still
cyclic.  Ok.

>                            fwd->bk_nextsize = victim;
>                            victim->bk_nextsize->fd_nextsize = victim;
>                          }
>                        bck = fwd->bk;
> +                      if (bck->fd != fwd)
> +                        malloc_printerr ("malloc(): largebin double linked list corrupted (bk)");

At this point the newly inserted chunk may be a start-of-size chunk, or
a second-in-size chunk; either way, we've not yet linked it into the
fd-bk chain, so we can verify that the fwd->bk->fd is still cyclic.  Ok.

So, looks correct to me.
Reviewed-by: DJ Delorie <dj@redhat.com>

Re: [PATCH] malloc: Check for large bin list corruption when inserting unsorted chunk

[Florian Weimer]

* DJ Delorie:

> Adam Maris <amaris@redhat.com> writes:
>>                          {
>>                            victim->fd_nextsize = fwd;
>>                            victim->bk_nextsize = fwd->bk_nextsize;
>> +                          if (__glibc_unlikely (fwd->bk_nextsize->fd_nextsize != fwd))
>> +                            malloc_printerr ("malloc(): largebin double linked list corrupted (nextsize)");
>
> At this point, the size of the chunk matches neither the previous nor
> next chunks; we're creating a new "unique size" sub-head.  So, both fd
> and fd_nextsize should point to the next (fwd) chunk.  We've hooked in
> our new chunk but haven't yet fixed up the existing chunks, so this test
> verifies that the links between the next chunk (fwd, which is a
> start-of-size chunk) and the previous start-of-size chunk are still
> cyclic.  Ok.
>
>>                            fwd->bk_nextsize = victim;
>>                            victim->bk_nextsize->fd_nextsize = victim;
>>                          }
>>                        bck = fwd->bk;
>> +                      if (bck->fd != fwd)
>> +                        malloc_printerr ("malloc(): largebin double linked list corrupted (bk)");
>
> At this point the newly inserted chunk may be a start-of-size chunk, or
> a second-in-size chunk; either way, we've not yet linked it into the
> fd-bk chain, so we can verify that the fwd->bk->fd is still cyclic.  Ok.
>
> So, looks correct to me.
> Reviewed-by: DJ Delorie <dj@redhat.com>

I agree with this analysis.

Adam's submission is no longer covered by the Red Hat copyright
assignment, though, but I believe we can still accept it because it was
submitted under the assignment, and it is a small change anyway.

Thanks,
Florian

Re: [PATCH] malloc: Check for large bin list corruption when inserting unsorted chunk

[DJ Delorie]

Florian Weimer <fweimer@redhat.com> writes:
> * DJ Delorie:
>
>> Adam Maris <amaris@redhat.com> writes:
>>>                          {
>>>                            victim->fd_nextsize = fwd;
>>>                            victim->bk_nextsize = fwd->bk_nextsize;
>>> +                          if (__glibc_unlikely (fwd->bk_nextsize->fd_nextsize != fwd))
>>> +                            malloc_printerr ("malloc(): largebin double linked list corrupted (nextsize)");
>>
>> At this point, the size of the chunk matches neither the previous nor
>> next chunks; we're creating a new "unique size" sub-head.  So, both fd
>> and fd_nextsize should point to the next (fwd) chunk.  We've hooked in
>> our new chunk but haven't yet fixed up the existing chunks, so this test
>> verifies that the links between the next chunk (fwd, which is a
>> start-of-size chunk) and the previous start-of-size chunk are still
>> cyclic.  Ok.
>>
>>>                            fwd->bk_nextsize = victim;
>>>                            victim->bk_nextsize->fd_nextsize = victim;
>>>                          }
>>>                        bck = fwd->bk;
>>> +                      if (bck->fd != fwd)
>>> +                        malloc_printerr ("malloc(): largebin double linked list corrupted (bk)");
>>
>> At this point the newly inserted chunk may be a start-of-size chunk, or
>> a second-in-size chunk; either way, we've not yet linked it into the
>> fd-bk chain, so we can verify that the fwd->bk->fd is still cyclic.  Ok.
>>
>> So, looks correct to me.
>> Reviewed-by: DJ Delorie <dj@redhat.com>
>
> I agree with this analysis.
>
> Adam's submission is no longer covered by the Red Hat copyright
> assignment, though, but I believe we can still accept it because it was
> submitted under the assignment, and it is a small change anyway.

Agreed.  Pushed.  Finally ;-)

Re: [PATCH] malloc: Check for large bin list corruption when inserting unsorted chunk

[Andreas Schwab]

This lacks a ChangeLog entry.

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] malloc: Check for large bin list corruption when inserting unsorted chunk

[DJ Delorie]

Andreas Schwab <schwab@suse.de> writes:
> This lacks a ChangeLog entry.

Fixed.