[v2,BZ,#15855] malloc: Check for integer overflow in pvalloc.

Message ID 521327C7.3020501@linaro.org
State Superseded
Headers show

Commit Message

Will Newton Aug. 20, 2013, 8:24 a.m.
A large bytes parameter to pvalloc could cause an integer overflow
and corrupt allocator internals. Check the overflow does not occur
before continuing with the allocation.

ChangeLog:

2013-08-12  Will Newton  <will.newton@linaro.org>

	[BZ #15855]
	* malloc/malloc.c (__libc_pvalloc): Check the value of bytes
	does not overflow.
---
 malloc/malloc.c | 4 ++++
 1 file changed, 4 insertions(+)

Changes in v2:
 - Add BZ number

Comments

Will Newton Aug. 29, 2013, 8:28 a.m. | #1
On 20 August 2013 09:24, Will Newton <will.newton@linaro.org> wrote:
>
> A large bytes parameter to pvalloc could cause an integer overflow
> and corrupt allocator internals. Check the overflow does not occur
> before continuing with the allocation.
>
> ChangeLog:
>
> 2013-08-12  Will Newton  <will.newton@linaro.org>
>
>         [BZ #15855]
>         * malloc/malloc.c (__libc_pvalloc): Check the value of bytes
>         does not overflow.
> ---
>  malloc/malloc.c | 4 ++++
>  1 file changed, 4 insertions(+)

Does anybody have any comments on this patch series?

Thanks,
Will Newton Sept. 9, 2013, 8:24 a.m. | #2
On 20 August 2013 09:24, Will Newton <will.newton@linaro.org> wrote:
>
> A large bytes parameter to pvalloc could cause an integer overflow
> and corrupt allocator internals. Check the overflow does not occur
> before continuing with the allocation.
>
> ChangeLog:
>
> 2013-08-12  Will Newton  <will.newton@linaro.org>
>
>         [BZ #15855]
>         * malloc/malloc.c (__libc_pvalloc): Check the value of bytes
>         does not overflow.
> ---
>  malloc/malloc.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> Changes in v2:
>  - Add BZ number

Ping?
Siddhesh Poyarekar Sept. 10, 2013, 2:40 a.m. | #3
On Tue, Aug 20, 2013 at 09:24:39AM +0100, Will Newton wrote:
> 
> A large bytes parameter to pvalloc could cause an integer overflow
> and corrupt allocator internals. Check the overflow does not occur
> before continuing with the allocation.
> 
> ChangeLog:
> 
> 2013-08-12  Will Newton  <will.newton@linaro.org>
> 
> 	[BZ #15855]
> 	* malloc/malloc.c (__libc_pvalloc): Check the value of bytes
> 	does not overflow.
> ---
>  malloc/malloc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> Changes in v2:
>  - Add BZ number
> 
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index be472b2..7468758 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -3082,6 +3082,10 @@ __libc_pvalloc(size_t bytes)
>    size_t page_mask = GLRO(dl_pagesize) - 1;
>    size_t rounded_bytes = (bytes + page_mask) & ~(page_mask);
> 
> +  /* Check for overflow.  */
> +  if (bytes + 2*pagesz + MINSIZE < bytes)
> +    return 0;
> +

It might be safer to use SIZE_MAX instead of relying on overflow
behaviour of the processor:

    if (SIZE_MAX - 2 * pagesz - MINSIZE < bytes)
      return 0;

>    void *(*hook) (size_t, size_t, const void *) =
>      force_reg (__memalign_hook);
>    if (__builtin_expect (hook != NULL, 0))
> -- 
> 1.8.1.4
> 

Siddhesh
Rich Felker Sept. 10, 2013, 4:46 a.m. | #4
On Tue, Sep 10, 2013 at 08:10:44AM +0530, Siddhesh Poyarekar wrote:
> > diff --git a/malloc/malloc.c b/malloc/malloc.c
> > index be472b2..7468758 100644
> > --- a/malloc/malloc.c
> > +++ b/malloc/malloc.c
> > @@ -3082,6 +3082,10 @@ __libc_pvalloc(size_t bytes)
> >    size_t page_mask = GLRO(dl_pagesize) - 1;
> >    size_t rounded_bytes = (bytes + page_mask) & ~(page_mask);
> > 
> > +  /* Check for overflow.  */
> > +  if (bytes + 2*pagesz + MINSIZE < bytes)
> > +    return 0;
> > +
> 
> It might be safer to use SIZE_MAX instead of relying on overflow
> behaviour of the processor:

This code does not "rely on overflow behavior of the processor".
Unsigned arithmetic cannot overflow. It's performed modulo one plus
the maximum value of the type.

In general, expressions of the form "a+b+c < a" are not safe to check
for overflow; since even if a+b<a, a+b+c>=a is possible. However,
since b+c (i.e. 2*pagesz+MINSIZE) cannot overflow in the above
expression, the check is safe.

>     if (SIZE_MAX - 2 * pagesz - MINSIZE < bytes)
>       return 0;

I do prefer this form, but you got it backwards in the process of
Yoda-fying it. It should be:

     if (bytes > SIZE_MAX - 2 * pagesz - MINSIZE)

and the "value being tested" should be on the left-hand side of a
comparison (i.e. non-Yoda-fied).

By the way, I think the code is failing to set errno, too.

Rich
Siddhesh Poyarekar Sept. 10, 2013, 5:15 a.m. | #5
On Tue, Sep 10, 2013 at 12:46:00AM -0400, Rich Felker wrote:
> This code does not "rely on overflow behavior of the processor".
> Unsigned arithmetic cannot overflow. It's performed modulo one plus
> the maximum value of the type.

ACK.  Thanks.

Siddhesh

Patch

diff --git a/malloc/malloc.c b/malloc/malloc.c
index be472b2..7468758 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -3082,6 +3082,10 @@  __libc_pvalloc(size_t bytes)
   size_t page_mask = GLRO(dl_pagesize) - 1;
   size_t rounded_bytes = (bytes + page_mask) & ~(page_mask);

+  /* Check for overflow.  */
+  if (bytes + 2*pagesz + MINSIZE < bytes)
+    return 0;
+
   void *(*hook) (size_t, size_t, const void *) =
     force_reg (__memalign_hook);
   if (__builtin_expect (hook != NULL, 0))