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);
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;
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
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
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
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
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
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
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
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
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Xi Wang