[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gcc-patches
Subject:    Re: RFA: libiberty: cope with integer overflow in _objalloc_alloc
From:       Florian Weimer <fweimer () redhat ! com>
Date:       2012-08-31 11:02:30
Message-ID: 504099C6.2060501 () redhat ! com
[Download RAW message or body]

On 08/31/2012 12:33 PM, Nick Clifton wrote:
> Hi DJ, Hi Ian,
>
>    The _objalloc_alloc() function is currently vulnerable to an integer
>    overflow if it is passed a negative length.  For example if called
>    with len = -3 and assuming that OBJALLOC_ALIGN is 4 then:
>
>      line 122:  len = (len + OBJALLOC_ALIGN - 1) &~ (OBJALLOC_ALIGN - 1);
>
>    So len = (-3 + 3) & ~ 3 = 0, and then the function returns a pointer
>    that will be reused the next time _objalloc_alloc is called.
>
>    Or suppose that len = -4, then:
>
>      line 122:  len = (len + OBJALLOC_ALIGN - 1) &~ (OBJALLOC_ALIGN - 1);
>
>    Which gives len = (-4 + 3) & ~3 = -4 and then:
>
>      line 136:  ret = (char *) malloc (CHUNK_HEADER_SIZE + len);
>
>    So now the function returns a pointer to a memory block that is not
>    even big enough to contain the chunk header.
>
>    The proposed patch should take care of both of these scenarios. OK
>    to apply ?

If I'm not mistaken, this doesn't cover the -3 case properly:

PTR
_objalloc_alloc (struct objalloc *o, unsigned long len)
{
   len = (len + OBJALLOC_ALIGN - 1) &~ (OBJALLOC_ALIGN - 1);

   /* We avoid confusion from zero sized objects by always allocating
      at least OBJALLOC_ALIGN bytes.  */
   if (len == 0)
     len = OBJALLOC_ALIGN;

This still results in a pointer which is too small.  And this code is 
never called because the wraparound already happens in the 
objalloc_alloc macro in the header file.

Here's a different patch which should not suffer from this problem:
<http://gcc.gnu.org/ml/gcc-patches/2012-08/msg01986.html>

-- 
Florian Weimer / Red Hat Product Security Team
[prev in list] [next in list] [prev in thread] [next in thread]