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

[Juha-Pekka Heikkila]

On 23.01.2016 01:22, Nicolai Hähnle wrote:
> 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.
>>>>

Hi, just to comment here. I did make set of patches which fixed all the 
breaking places for Mesa. Tapani then pointed the problem I can only 
test my patches on Intel HW, more so I have only SNB and IVB on my desk.

Making real fix here would be source of all kind of really nasty bugs on 
different HW for some time to come thus I dropped the ball on the fixes. 
In the end for Mesa on Intel HW there was not so much to fix but to find 
all the correct places to fix require running lot of Valgrind.

I seem to have missed the patch where I was cc'd, sorry about that.

/Juha-Pekka

>>>> 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.
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev