Skip to contentSkip to conversation

drm/i915: fix integer overflow in i915_gem_execbuffer2()

PATCH Superseded Xi Wang 2 participants 10 messages Apr 6, 2012

Subsystems (2)
DRM drivers Intel i915

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Xi Wang

author
#0 lore raw

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

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 <[email protected]>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/i915_gem_execbuffer.c +2 -2
index f51a696..19962bd 100644
@@ -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);
Signature · 1 line
1.7.5.4
author
#1 lore raw

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

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 <[email protected]>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/i915_gem_execbuffer.c +2 -2
index 19962bd..607be3d 100644
@@ -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;
Signature · 1 line
1.7.5.4
#2 lore raw
In reply to Xi Wang
Quoted 4 lines · 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.

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

Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
author
#3 lore raw
Quoted 9 lines · Chris Wilson
Quoted 4 lines · 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.

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

#4 lore raw
Quoted 6 lines · Xi Wang
Quoted 4 lines · Chris Wilson
Quoted 3 lines · Xi Wang

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

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

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

Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
author
#5 lore raw
Quoted 9 lines · Chris Wilson
Quoted 8 lines · Xi Wang
Quoted 6 lines · Chris Wilson
Quoted 4 lines · 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.

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

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

#6 lore raw
Quoted 17 lines · Xi Wang
Quoted 15 lines · Chris Wilson
Quoted 13 lines · Xi Wang
Quoted 9 lines · Chris Wilson
Quoted 4 lines · 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.

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?

Quoted 7 lines
Quoted 1 line

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

Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
author
#7 lore raw
Quoted 3 lines · Chris Wilson

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

#8 lore raw
Quoted 8 lines · Xi Wang
Quoted 3 lines · Chris Wilson

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

Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
author
#9 lore raw
Quoted 7 lines · Chris Wilson
Quoted 3 lines · Xi Wang

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