Patchwork PR other/54411: libiberty: objalloc_alloc integer overflows (CVE-2012-3509)

login
register
mail settings
Submitter Florian Weimer
Date Sept. 17, 2012, 9:49 a.m.
Message ID <5056F210.7060404@redhat.com>
Download mbox | patch
Permalink /patch/184357/
State New
Headers show

Comments

Florian Weimer - Sept. 17, 2012, 9:49 a.m.
On 09/05/2012 07:31 AM, Ian Lance Taylor wrote:
> On Wed, Aug 29, 2012 at 10:32 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>>
>> This patches fixes an integer overflow in libiberty, which leads to
>> crashes in binutils.  The long version of the objalloc_alloc macro
>> would have needed another conditional, so I removed that and replaced
>> it with a call to the actual implementation.
>
> I guess I don't see why removing the macro is desirable.  In many uses
> of objalloc_alloc the conditional can be optimized out anyhow.  It's
> true that it can't always be, but, so what?  The macro is probably
> still a win.

Fair enough.  I've added a wraparound check to the macro.  Okay for trunk?
Ian Taylor - Sept. 17, 2012, 3:59 p.m.
On Mon, Sep 17, 2012 at 2:49 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 09/05/2012 07:31 AM, Ian Lance Taylor wrote:
>>
>> On Wed, Aug 29, 2012 at 10:32 AM, Florian Weimer <fw@deneb.enyo.de> wrote:
>>>
>>>
>>> This patches fixes an integer overflow in libiberty, which leads to
>>> crashes in binutils.  The long version of the objalloc_alloc macro
>>> would have needed another conditional, so I removed that and replaced
>>> it with a call to the actual implementation.
>>
>>
>> I guess I don't see why removing the macro is desirable.  In many uses
>> of objalloc_alloc the conditional can be optimized out anyhow.  It's
>> true that it can't always be, but, so what?  The macro is probably
>> still a win.
>
>
> Fair enough.  I've added a wraparound check to the macro.  Okay for trunk?

>  {
> +  unsigned long len = original_len;
>    /* We avoid confusion from zero sized objects by always allocating
>       at least 1 byte.  */

Please add a blank line after the variable declaration.

> -     (__len <= __o->current_space					\
> +     (__len && __len <= __o->current_space				\

Please write __len != 0 or len > 0.

This is OK with those changes.

Thanks.

Ian
Florian Weimer - Sept. 18, 2012, 8:35 a.m.
On 09/17/2012 05:59 PM, Ian Lance Taylor wrote:

>> Fair enough.  I've added a wraparound check to the macro.  Okay for trunk?
>
>>   {
>> +  unsigned long len = original_len;
>>     /* We avoid confusion from zero sized objects by always allocating
>>        at least 1 byte.  */
>
> Please add a blank line after the variable declaration.
>
>> -     (__len <= __o->current_space					\
>> +     (__len && __len <= __o->current_space				\
>
> Please write __len != 0 or len > 0.
>
> This is OK with those changes.

Thanks, committed with these changes.

Patch

2012-09-17  Florian Weimer  <fweimer@redhat.com>

        PR other/54411
        * objalloc.h (objalloc_alloc): Do not use fast path on wraparound.

2012-09-17  Florian Weimer  <fweimer@redhat.com>

        PR other/54411
        * objalloc.c (_objalloc_alloc): Add overflow check covering
        alignment and CHUNK_HEADER_SIZE addition.

Index: libiberty/objalloc.c
===================================================================
--- libiberty/objalloc.c	(revision 191378)
+++ libiberty/objalloc.c	(working copy)
@@ -1,5 +1,5 @@ 
 /* objalloc.c -- routines to allocate memory for objects
-   Copyright 1997 Free Software Foundation, Inc.
+   Copyright 1997-2012 Free Software Foundation, Inc.
    Written by Ian Lance Taylor, Cygnus Solutions.
 
 This program is free software; you can redistribute it and/or modify it
@@ -112,8 +112,9 @@ 
 /* Allocate space from an objalloc structure.  */
 
 PTR
-_objalloc_alloc (struct objalloc *o, unsigned long len)
+_objalloc_alloc (struct objalloc *o, unsigned long original_len)
 {
+  unsigned long len = original_len;
   /* We avoid confusion from zero sized objects by always allocating
      at least 1 byte.  */
   if (len == 0)
@@ -121,6 +122,11 @@ 
 
   len = (len + OBJALLOC_ALIGN - 1) &~ (OBJALLOC_ALIGN - 1);
 
+  /* Check for overflow in the alignment operation above and the
+     malloc argument below. */
+  if (len + CHUNK_HEADER_SIZE < original_len)
+    return NULL;
+
   if (len <= o->current_space)
     {
       o->current_ptr += len;
Index: include/objalloc.h
===================================================================
--- include/objalloc.h	(revision 191378)
+++ include/objalloc.h	(working copy)
@@ -1,5 +1,5 @@ 
 /* objalloc.h -- routines to allocate memory for objects
-   Copyright 1997, 2001 Free Software Foundation, Inc.
+   Copyright 1997-2012 Free Software Foundation, Inc.
    Written by Ian Lance Taylor, Cygnus Solutions.
 
 This program is free software; you can redistribute it and/or modify it
@@ -91,7 +91,7 @@ 
      if (__len == 0)							\
        __len = 1;							\
      __len = (__len + OBJALLOC_ALIGN - 1) &~ (OBJALLOC_ALIGN - 1);	\
-     (__len <= __o->current_space					\
+     (__len && __len <= __o->current_space				\
       ? (__o->current_ptr += __len,					\
 	 __o->current_space -= __len,					\
 	 (void *) (__o->current_ptr - __len))				\