malloc/hooks.c: Correct check for overflow in memalign_check.

Message ID 52555E49.4050506@linaro.org
State Accepted
Commit 321e26847188300173a5dc0ca42c2ff7b9bf7a78
Headers show

Commit Message

Will Newton Oct. 9, 2013, 1:46 p.m.
A large value of bytes passed to memalign_check can cause an integer
overflow in _int_memalign and heap corruption. This issue can be
exposed by running tst-memalign with MALLOC_CHECK_=3.

ChangeLog:

2013-10-09  Will Newton  <will.newton@linaro.org>

	* malloc/hooks.c (memalign_check): Ensure the value of bytes
	passed to _int_memalign does not overflow.
---
 malloc/hooks.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Alexander Monakov Oct. 9, 2013, 2:15 p.m. | #1
On Wed, 9 Oct 2013, Will Newton wrote:
> +  /* Check for overflow.  */
> +  if (bytes > SIZE_MAX - alignment - MINSIZE)

At this point no upper bound is established on the value of 'alignment', so
the test may pass when 'alignment' is so large that right-hand side
overflows.

(also, when 'alignment' is larger than SIZE_MAX/2+1, _int_memalign enters an
infinite loop)

Alexander
Will Newton Oct. 9, 2013, 4:19 p.m. | #2
On 9 October 2013 15:15, Alexander Monakov <amonakov@ispras.ru> wrote:
> On Wed, 9 Oct 2013, Will Newton wrote:
>> +  /* Check for overflow.  */
>> +  if (bytes > SIZE_MAX - alignment - MINSIZE)
>
> At this point no upper bound is established on the value of 'alignment', so
> the test may pass when 'alignment' is so large that right-hand side
> overflows.

Thanks for noticing this. If it's ok I'd like to deal with that issue
as a separate patch though.
Carlos O'Donell Oct. 9, 2013, 6:36 p.m. | #3
On 10/09/2013 09:46 AM, Will Newton wrote:
> 
> A large value of bytes passed to memalign_check can cause an integer
> overflow in _int_memalign and heap corruption. This issue can be
> exposed by running tst-memalign with MALLOC_CHECK_=3.
> 
> ChangeLog:
> 
> 2013-10-09  Will Newton  <will.newton@linaro.org>
> 
> 	* malloc/hooks.c (memalign_check): Ensure the value of bytes
> 	passed to _int_memalign does not overflow.
> ---
>  malloc/hooks.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/malloc/hooks.c b/malloc/hooks.c
> index 8c25846..3f663bb 100644
> --- a/malloc/hooks.c
> +++ b/malloc/hooks.c
> @@ -361,10 +361,13 @@ memalign_check(size_t alignment, size_t bytes, const void *caller)
>    if (alignment <= MALLOC_ALIGNMENT) return malloc_check(bytes, NULL);
>    if (alignment <  MINSIZE) alignment = MINSIZE;
> 
> -  if (bytes+1 == 0) {
> -    __set_errno (ENOMEM);
> -    return NULL;
> -  }
> +  /* Check for overflow.  */
> +  if (bytes > SIZE_MAX - alignment - MINSIZE)
> +    {
> +      __set_errno (ENOMEM);
> +      return 0;
> +    }
> +
>    (void)mutex_lock(&main_arena.mutex);
>    mem = (top_check() >= 0) ? _int_memalign(&main_arena, alignment, bytes+1) :
>      NULL;
> 

This is better than `bytes+1' so it should go in immediately to fix
the test regression under _MALLOC_CHECK=3.

We still need to deal with `alignment' having no upper bound.

Cheers,
Carlos.
Will Newton Oct. 10, 2013, 1:56 p.m. | #4
On 9 October 2013 19:36, Carlos O'Donell <carlos@redhat.com> wrote:

Hi Carlos,

> On 10/09/2013 09:46 AM, Will Newton wrote:
>>
>> A large value of bytes passed to memalign_check can cause an integer
>> overflow in _int_memalign and heap corruption. This issue can be
>> exposed by running tst-memalign with MALLOC_CHECK_=3.
>>
>> ChangeLog:
>>
>> 2013-10-09  Will Newton  <will.newton@linaro.org>
>>
>>       * malloc/hooks.c (memalign_check): Ensure the value of bytes
>>       passed to _int_memalign does not overflow.
>> ---
>>  malloc/hooks.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/malloc/hooks.c b/malloc/hooks.c
>> index 8c25846..3f663bb 100644
>> --- a/malloc/hooks.c
>> +++ b/malloc/hooks.c
>> @@ -361,10 +361,13 @@ memalign_check(size_t alignment, size_t bytes, const void *caller)
>>    if (alignment <= MALLOC_ALIGNMENT) return malloc_check(bytes, NULL);
>>    if (alignment <  MINSIZE) alignment = MINSIZE;
>>
>> -  if (bytes+1 == 0) {
>> -    __set_errno (ENOMEM);
>> -    return NULL;
>> -  }
>> +  /* Check for overflow.  */
>> +  if (bytes > SIZE_MAX - alignment - MINSIZE)
>> +    {
>> +      __set_errno (ENOMEM);
>> +      return 0;
>> +    }
>> +
>>    (void)mutex_lock(&main_arena.mutex);
>>    mem = (top_check() >= 0) ? _int_memalign(&main_arena, alignment, bytes+1) :
>>      NULL;
>>
>
> This is better than `bytes+1' so it should go in immediately to fix
> the test regression under _MALLOC_CHECK=3.

I took that as an OK and applied the patch.
Carlos O'Donell Oct. 15, 2013, 7:04 p.m. | #5
On 10/10/2013 09:56 AM, Will Newton wrote:
> On 9 October 2013 19:36, Carlos O'Donell <carlos@redhat.com> wrote:
> 
> Hi Carlos,
> 
>> On 10/09/2013 09:46 AM, Will Newton wrote:
>>>
>>> A large value of bytes passed to memalign_check can cause an integer
>>> overflow in _int_memalign and heap corruption. This issue can be
>>> exposed by running tst-memalign with MALLOC_CHECK_=3.
>>>
>>> ChangeLog:
>>>
>>> 2013-10-09  Will Newton  <will.newton@linaro.org>
>>>
>>>       * malloc/hooks.c (memalign_check): Ensure the value of bytes
>>>       passed to _int_memalign does not overflow.
>>> ---
>>>  malloc/hooks.c | 11 +++++++----
>>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/malloc/hooks.c b/malloc/hooks.c
>>> index 8c25846..3f663bb 100644
>>> --- a/malloc/hooks.c
>>> +++ b/malloc/hooks.c
>>> @@ -361,10 +361,13 @@ memalign_check(size_t alignment, size_t bytes, const void *caller)
>>>    if (alignment <= MALLOC_ALIGNMENT) return malloc_check(bytes, NULL);
>>>    if (alignment <  MINSIZE) alignment = MINSIZE;
>>>
>>> -  if (bytes+1 == 0) {
>>> -    __set_errno (ENOMEM);
>>> -    return NULL;
>>> -  }
>>> +  /* Check for overflow.  */
>>> +  if (bytes > SIZE_MAX - alignment - MINSIZE)
>>> +    {
>>> +      __set_errno (ENOMEM);
>>> +      return 0;
>>> +    }
>>> +
>>>    (void)mutex_lock(&main_arena.mutex);
>>>    mem = (top_check() >= 0) ? _int_memalign(&main_arena, alignment, bytes+1) :
>>>      NULL;
>>>
>>
>> This is better than `bytes+1' so it should go in immediately to fix
>> the test regression under _MALLOC_CHECK=3.
> 
> I took that as an OK and applied the patch.

It was. Sorry for not being sufficiently explicit.

Cheers,
Carlos.

Patch

diff --git a/malloc/hooks.c b/malloc/hooks.c
index 8c25846..3f663bb 100644
--- a/malloc/hooks.c
+++ b/malloc/hooks.c
@@ -361,10 +361,13 @@  memalign_check(size_t alignment, size_t bytes, const void *caller)
   if (alignment <= MALLOC_ALIGNMENT) return malloc_check(bytes, NULL);
   if (alignment <  MINSIZE) alignment = MINSIZE;

-  if (bytes+1 == 0) {
-    __set_errno (ENOMEM);
-    return NULL;
-  }
+  /* Check for overflow.  */
+  if (bytes > SIZE_MAX - alignment - MINSIZE)
+    {
+      __set_errno (ENOMEM);
+      return 0;
+    }
+
   (void)mutex_lock(&main_arena.mutex);
   mem = (top_check() >= 0) ? _int_memalign(&main_arena, alignment, bytes+1) :
     NULL;