[Mesa-dev] [PATCH 12/15] egl: add eglCreateImage

[Eirik Byrkjeflot Anonsen]

Marek Olšák <maraeo at gmail.com> writes:

> I don't understand. Using size_t should prevent the integer overflow.
> Is there anything else wrong other than no fail path for malloc? I
> also don't understand how calloc can help here.
>
> Marek

"size * sizeof(int_attribs[0])" may overflow and thus wrap to a small
number. Using calloc, you'd have "calloc(size, sizeof(int_attribs[0]))",
moving the overflow inside calloc(). So if calloc() does its job
properly, it will protect against it.

eirik


> On Wed, May 27, 2015 at 9:07 PM, Chad Versace <chad.versace at intel.com> wrote:
>> On Fri 15 May 2015, Emil Velikov wrote:
>>> On 12/05/15 22:54, Marek Olšák wrote:
>>> > From: Marek Olšák <marek.olsak at amd.com>
>>> >
>>> > ---
>>> >  src/egl/main/eglapi.c | 38 ++++++++++++++++++++++++++++++++++++++
>>> >  1 file changed, 38 insertions(+)
>>> >
>>> > diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
>>> > index 6457798..34a113b 100644
>>> > --- a/src/egl/main/eglapi.c
>>> > +++ b/src/egl/main/eglapi.c
>>> > @@ -251,6 +251,30 @@ _eglUnlockDisplay(_EGLDisplay *dpy)
>>> >  }
>>> >
>>> >
>>> > +static EGLint *
>>> > +_eglConvertAttribsToInt(const EGLAttrib *attr_list)
>>> > +{
>>> > +   EGLint *int_attribs = NULL;
>>> > +
>>> > +   /* Convert attributes from EGLAttrib[] to EGLint[] */
>>> > +   if (attr_list) {
>>> > +      int i, size = 0;
>>> > +
>>> > +      while (attr_list[size] != EGL_NONE)
>>> > +         size += 2;
>>> > +
>>> > +      if (size) {
>>> > +         size += 1; /* add space for EGL_NONE */
>>> > +         int_attribs = malloc(size * sizeof(int_attribs[0]));
>>> > +
>>> > +         for (i = 0; i < size; i++)
>>> > +            int_attribs[i] = attr_list[i];
>>
>>> In the unlikely event that malloc fails, it'll be nice to not crash.
>>
>> NAK.
>>
>> There is a stack overflow vulnerability here, even when malloc succeeds.
>> An attacker can pass a very large but valid `EGLint *attrib_list` into
>> an EGL entry point, forcing the size calculation given to malloc to
>> overflow to a small positive integer.  Then _eglConvertAttribsToInt will
>> blithely copy a portion (perhaps most) of the attacker's attrib list onto
>> the stack!
>>
>> To prevent the stack overflow, _eglConvertAttribsToInt should use
>> calloc() and abort if allocation fails.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev