[Mesa-dev] [PATCH] util/ralloc: Remove double zero'ing of rzalloc buffers

[Nicolai Hähnle]

On 22.01.2016 16:02, Kenneth Graunke wrote:
> On Friday, January 22, 2016 12:09:18 PM PST Nicolai Hähnle wrote:
>> On 22.01.2016 02:53, Jordan Justen wrote:
>>> Juha-Pekka found this back in May 2015:
>>> <1430915727-28677-1-git-send-email-juhapekka.heikkila at gmail.com>
>>>
>>>   From the discussion, obviously it would be preferable to make
>>> ralloc_size no longer return zeroed memory, but Juha-Pekka found that
>>> it would break Mesa.
>>>
>>> For now, let's point out the flaw, and stop doing the double zeroing
>>> of rzalloc buffers.
>>>
>>> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
>>> Cc: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
>>> Cc: Kenneth Graunke <kenneth at whitecape.org>
>>> ---
>>>
>>>    For a release build, I saw the code size shrink by 64 bytes.
>>>
>>>    src/util/ralloc.c | 15 +++++++++++++--
>>>    1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/util/ralloc.c b/src/util/ralloc.c
>>> index 6d4032b..24c1eee 100644
>>> --- a/src/util/ralloc.c
>>> +++ b/src/util/ralloc.c
>>> @@ -49,6 +49,14 @@ _CRTIMP int _vscprintf(const char *format, va_list
> argptr);
>>>    #endif
>>>    #endif
>>>
>>> +/* ralloc_size has always used calloc to allocate memory. This has
> allowed
>>> + * code using ralloc_size to depend on always receiving a cleared buffer.
>>> + *
>>> + * FIXME: Clean up the code base to allow this to be set to false, and
> then
>>> + * remove it altogether.
>>> + */
>>> +static const bool always_allocate_zeroed_memory = true;
>>> +
>>>    #define CANARY 0x5A1106
>>>
>>>    struct ralloc_header
>>> @@ -110,7 +118,10 @@ ralloc_context(const void *ctx)
>>>    void *
>>>    ralloc_size(const void *ctx, size_t size)
>>>    {
>>> -   void *block = calloc(1, size + sizeof(ralloc_header));
>>> +   void *block =
>>> +      always_allocate_zeroed_memory ?
>>> +      calloc(1, size + sizeof(ralloc_header)) :
>>> +      malloc(size + sizeof(ralloc_header));
>>
>> There's an integer overflow here which would be good to fix.
>
> Please explain?  ralloc_header is 40-44 bytes - the only way this will
> overflow is if you asked for an absurd amount of memory (already near
> the max value of size_t).  And, if you did, I'm not sure what we're
> supposed to do about it...

A common method of triggering buffer overflows leading to security 
exploits is that the attacker sets an absurdly large buffer size 
somewhere - so large that additional calculations that increase the 
buffer size wrap around and result in a very small successful 
allocation. The code will then write memory based on the original, 
absurdly large buffer size. This means writing beyond the end of the 
allocated buffer.

The people who look for security exploits for a living are really good 
at finding ways in which such a situation can then be used to hijack the 
control flow of the program somehow (for example, targeted overwriting 
of the internal metadata of the heap... sounds crazy, but it's used) to 
do more or less whatever they want.

I think a decent way to protect against it is something like:

    /* Overflow of unsigned integer types is well defined */
    if (size + sizeof(ralloc_header) < size)
       return NULL;

or perhaps

    if (size > SIZE_MAX - sizeof(ralloc_header))
       return NULL;

Newer versions of gcc have nicer builtins for arithmetic with overflow 
check, but I don't know if we want to depend on those being available.

The hope is that the calling code properly handles allocation failure. 
Even if it doesn't, the result is much more likely to just be a segfault 
before anything dangerous can happen.

One may say that all of this depends on an attacker gaining access to 
Mesa, but WebGL is a thing, so...

Cheers,
Nicolai

>
>> Since it was there already in the older version, the patch is
>>
>> Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>
>>
>> as is.
>>
>>>       ralloc_header *info;
>>>       ralloc_header *parent;
>>>
>>> @@ -132,7 +143,7 @@ void *
>>>    rzalloc_size(const void *ctx, size_t size)
>>>    {
>>>       void *ptr = ralloc_size(ctx, size);
>>> -   if (likely(ptr != NULL))
>>> +   if (!always_allocate_zeroed_memory && likely(ptr != NULL))
>>>          memset(ptr, 0, size);
>>>       return ptr;
>>>    }
>>>
>>
>
> Dropping the memset seems reasonable.  I would prefer it if we simply
> moved the contents of ralloc_size into rzalloc_size, and made
> ralloc_size call rzalloc_size with a comment.
>