| Message ID | 87y5kvpcz0.fsf@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
Index: libiberty/objalloc.c =================================================================== RCS file: /cvs/src/src/libiberty/objalloc.c,v retrieving revision 1.8 diff -u -3 -p -r1.8 objalloc.c --- libiberty/objalloc.c 22 Jul 2005 03:26:05 -0000 1.8 +++ libiberty/objalloc.c 31 Aug 2012 10:28:33 -0000 @@ -114,12 +114,12 @@ objalloc_create (void) 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 1 byte. */ + at least OBJALLOC_ALIGN bytes. */ if (len == 0) - len = 1; - - len = (len + OBJALLOC_ALIGN - 1) &~ (OBJALLOC_ALIGN - 1); + len = OBJALLOC_ALIGN; if (len <= o->current_space) { @@ -133,6 +133,10 @@ _objalloc_alloc (struct objalloc *o, uns char *ret; struct objalloc_chunk *chunk; + /* Protect against integer overflow. */ + if (len + CHUNK_HEADER_SIZE < len) + return NULL; + ret = (char *) malloc (CHUNK_HEADER_SIZE + len); if (ret == NULL) return NULL;
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 ? Cheers Nick libiberty/ChangeLog 2012-08-31 Jan Lieskovsky <jlieskov@redhat.com> Nick Clifton <nickc@redhat.com> * objalloc.c (_objalloc_alloc): Align length before testing for zero length. Refuse to allocate when length is so big that integer overflow would occur during internal calculations.