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

Submitted by Marek Olšák on May 12, 2015, 10:54 p.m.

Details

Message ID 1431471290-7299-13-git-send-email-maraeo@gmail.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Marek Olšák May 12, 2015, 10:54 p.m.
From: Marek Olšák <marek.olsak@amd.com>

---
 src/egl/main/eglapi.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

Patch hide | download patch | download mbox

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];
+      }
+   }
+   return int_attribs;
+}
+
+
 /**
  * This is typically the first EGL function that an application calls.
  * It associates a private _EGLDisplay object to the native display.
@@ -1162,6 +1186,7 @@  eglGetProcAddress(const char *procname)
       { "eglDestroySync", (_EGLProc) eglDestroySync },
       { "eglClientWaitSync", (_EGLProc) eglClientWaitSync },
       { "eglGetSyncAttrib", (_EGLProc) eglGetSyncAttrib },
+      { "eglCreateImage", (_EGLProc) eglCreateImage },
       { "eglDestroyImage", (_EGLProc) eglDestroyImage },
       { "eglWaitSync", (_EGLProc) eglWaitSync },
 #endif /* _EGL_GET_CORE_ADDRESSES */
@@ -1370,6 +1395,19 @@  eglCreateImageKHR(EGLDisplay dpy, EGLContext ctx, EGLenum target,
 }
 
 
+EGLImage EGLAPIENTRY
+eglCreateImage(EGLDisplay dpy, EGLContext ctx, EGLenum target,
+               EGLClientBuffer buffer, const EGLAttrib *attr_list)
+{
+   EGLImage image;
+   EGLint *int_attribs = _eglConvertAttribsToInt(attr_list);
+
+   image = eglCreateImageKHR(dpy, ctx, target, buffer, int_attribs);
+   free(int_attribs);
+   return image;
+}
+
+
 EGLBoolean EGLAPIENTRY
 eglDestroyImage(EGLDisplay dpy, EGLImage image)
 {

Comments

On 12/05/15 22:54, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak@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.

-Emil
On Fri 15 May 2015, Emil Velikov wrote:
> On 12/05/15 22:54, Marek Olšák wrote:
> > From: Marek Olšák <marek.olsak@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.
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


On Wed, May 27, 2015 at 9:07 PM, Chad Versace <chad.versace@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@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.
Marek Olšák <maraeo@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@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@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Thu 28 May 2015, Eirik Byrkjeflot Anonsen wrote:
> Marek Olšák <maraeo@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.

Right.

It's very unlikely that an attacker could coerce the size calculation to
overflow, but better safe than sorry.

calloc() [and ralloc() too] will refuse to allocate memory if the size
calculation overflows. ralloc() checks for overflow with some simple
arithmetic. I expect that calloc() checks for overflow using a faster
method: multiply first, then inspect the overflow flag in a status
register. Recent GCC provides builtin functions for that [1].

[1] https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html#Integer-Overflow-Builtins