[PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()

[PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()

[Xi Wang]

A large args->buffer_count from userspace may overflow the allocation
size, leading to out-of-bounds access.

Use kmalloc_array() to avoid that.

Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index f51a696..19962bd 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1409,8 +1409,8 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	exec2_list = kmalloc(sizeof(*exec2_list)*args->buffer_count,
-			     GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
+	exec2_list = kmalloc_array(args->buffer_count, sizeof(*exec2_list),
+				   GFP_KERNEL | __GFP_NOWARN | __GFP_NORETRY);
 	if (exec2_list == NULL)
 		exec2_list = drm_malloc_ab(sizeof(*exec2_list),
 					   args->buffer_count);
-- 
1.7.5.4

[PATCH 2/2] drm/i915: fix integer overflow in i915_gem_do_execbuffer()

[Xi Wang]

A large args->num_cliprects from userspace may overflow the allocation
size, leading to out-of-bounds access.

| i915_gem_do_execbuffer()
| i915_gem_execbuffer()

Use kmalloc_array() to avoid that.

Signed-off-by: Xi Wang <xi.wang@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 19962bd..607be3d 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1133,8 +1133,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			return -EINVAL;
 		}
 
-		cliprects = kmalloc(args->num_cliprects * sizeof(*cliprects),
-				    GFP_KERNEL);
+		cliprects = kmalloc_array(args->num_cliprects, sizeof(*cliprects),
+					  GFP_KERNEL);
 		if (cliprects == NULL) {
 			ret = -ENOMEM;
 			goto pre_mutex_err;
-- 
1.7.5.4

Re: [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()

[Chris Wilson]

On Fri,  6 Apr 2012 08:58:18 -0400, Xi Wang <xi.wang@gmail.com> wrote:
> A large args->buffer_count from userspace may overflow the allocation
> size, leading to out-of-bounds access.
> 
> Use kmalloc_array() to avoid that.

I can safely say that exec list larger than 4GiB is going to be an
illegal operation and would rather the ioctl failed outright with
EINVAL.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()

[Xi Wang]

On Apr 6, 2012, at 9:36 AM, Chris Wilson wrote:

> On Fri,  6 Apr 2012 08:58:18 -0400, Xi Wang <xi.wang@gmail.com> wrote:
>> A large args->buffer_count from userspace may overflow the allocation
>> size, leading to out-of-bounds access.
>> 
>> Use kmalloc_array() to avoid that.
> 
> I can safely say that exec list larger than 4GiB is going to be an
> illegal operation and would rather the ioctl failed outright with
> EINVAL.

On 32-bit platform?

- xi

Re: [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()

[Chris Wilson]

On Fri, 6 Apr 2012 09:46:46 -0400, Xi Wang <xi.wang@gmail.com> wrote:
> On Apr 6, 2012, at 9:36 AM, Chris Wilson wrote:
> 
> > On Fri,  6 Apr 2012 08:58:18 -0400, Xi Wang <xi.wang@gmail.com> wrote:
> >> A large args->buffer_count from userspace may overflow the allocation
> >> size, leading to out-of-bounds access.
> >> 
> >> Use kmalloc_array() to avoid that.
> > 
> > I can safely say that exec list larger than 4GiB is going to be an
> > illegal operation and would rather the ioctl failed outright with
> > EINVAL.
> 
> On 32-bit platform?

On any platform. The largest it can legally be is a few tens of megabytes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()

[Xi Wang]

On Apr 6, 2012, at 9:54 AM, Chris Wilson wrote:

> On Fri, 6 Apr 2012 09:46:46 -0400, Xi Wang <xi.wang@gmail.com> wrote:
>> On Apr 6, 2012, at 9:36 AM, Chris Wilson wrote:
>> 
>>> On Fri,  6 Apr 2012 08:58:18 -0400, Xi Wang <xi.wang@gmail.com> wrote:
>>>> A large args->buffer_count from userspace may overflow the allocation
>>>> size, leading to out-of-bounds access.
>>>> 
>>>> Use kmalloc_array() to avoid that.
>>> 
>>> I can safely say that exec list larger than 4GiB is going to be an
>>> illegal operation and would rather the ioctl failed outright with
>>> EINVAL.
>> 
>> On 32-bit platform?
> 
> On any platform. The largest it can legally be is a few tens of megabytes.

IDGI.  First we come to i915_gem_execbuffer2() from ioctl:

  exec2_list = kmalloc(sizeof(*exec2_list)*args->buffer_count, ...);

args->buffer_count is passed from userspace so it can be any value.
Let it overflow the 32-bit multiplication and turn the call to:

  exec2_list = kmalloc(0, ...);

Then the subsequent call to i915_gem_do_execbuffer(..., exec2_list)
may read exec2_list, which is out of bounds.

- xi

Re: [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()

[Chris Wilson]

On Fri, 6 Apr 2012 10:01:36 -0400, Xi Wang <xi.wang@gmail.com> wrote:
> On Apr 6, 2012, at 9:54 AM, Chris Wilson wrote:
> 
> > On Fri, 6 Apr 2012 09:46:46 -0400, Xi Wang <xi.wang@gmail.com> wrote:
> >> On Apr 6, 2012, at 9:36 AM, Chris Wilson wrote:
> >> 
> >>> On Fri,  6 Apr 2012 08:58:18 -0400, Xi Wang <xi.wang@gmail.com> wrote:
> >>>> A large args->buffer_count from userspace may overflow the allocation
> >>>> size, leading to out-of-bounds access.
> >>>> 
> >>>> Use kmalloc_array() to avoid that.
> >>> 
> >>> I can safely say that exec list larger than 4GiB is going to be an
> >>> illegal operation and would rather the ioctl failed outright with
> >>> EINVAL.
> >> 
> >> On 32-bit platform?
> > 
> > On any platform. The largest it can legally be is a few tens of megabytes.
> 
> IDGI.  First we come to i915_gem_execbuffer2() from ioctl:
> 
>   exec2_list = kmalloc(sizeof(*exec2_list)*args->buffer_count, ...);
> 
> args->buffer_count is passed from userspace so it can be any value.

That I agreed with, I just disagree with how you chose to handle it.
Rather than continue on and attempt to vmalloc a large array we should
just fail the ioctl with EINVAL.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()

[Xi Wang]

On Apr 6, 2012, at 10:44 AM, Chris Wilson wrote:

> That I agreed with, I just disagree with how you chose to handle it.
> Rather than continue on and attempt to vmalloc a large array we should
> just fail the ioctl with EINVAL.

Why an attempt to vmalloc?  The overflow check in drm_malloc_ab()
will simply return NULL and fail the ioctl with -ENOMEM.

- xi

Re: [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()

[Chris Wilson]

On Fri, 6 Apr 2012 14:17:41 -0400, Xi Wang <xi.wang@gmail.com> wrote:
> On Apr 6, 2012, at 10:44 AM, Chris Wilson wrote:
> 
> > That I agreed with, I just disagree with how you chose to handle it.
> > Rather than continue on and attempt to vmalloc a large array we should
> > just fail the ioctl with EINVAL.
> 
> Why an attempt to vmalloc?  The overflow check in drm_malloc_ab()
> will simply return NULL and fail the ioctl with -ENOMEM.

It's an invalid value for the ioctl and should be treated as such, not
making ENOMEM more ambiguous.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()

[Xi Wang]

On Apr 6, 2012, at 3:40 PM, Chris Wilson wrote:
> On Fri, 6 Apr 2012 14:17:41 -0400, Xi Wang <xi.wang@gmail.com> wrote:
>> 
>> Why an attempt to vmalloc?  The overflow check in drm_malloc_ab()
>> will simply return NULL and fail the ioctl with -ENOMEM.
> 
> It's an invalid value for the ioctl and should be treated as such, not
> making ENOMEM more ambiguous.

We could copy and paste the overflow check so as to return -EINVAL.
I just doubt how much that would help --- you can find existing usages
in other functions, for example, in i915_gem_execbuffer():

  /* Copy in the exec list from userland */
  exec_list = drm_malloc_ab(sizeof(*exec_list), args->buffer_count);
  exec2_list = drm_malloc_ab(sizeof(*exec2_list), args->buffer_count);
  if (exec_list == NULL || exec2_list == NULL) {
          DRM_DEBUG("Failed to allocate exec list for %d buffers\n",
                    args->buffer_count);
          drm_free_large(exec_list);
          drm_free_large(exec2_list);
          return -ENOMEM;
  }

Should we fix all these as well by repeating the checks and returning
-EINVAL?  I am worried about the code bloat / readability price you
would pay for getting a different error code.

BTW, I've also seen code using E2BIG.  Any documented guideline?

- xi