* [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()
@ 2012-04-06 12:58 Xi Wang
2012-04-06 12:58 ` [PATCH 2/2] drm/i915: fix integer overflow in i915_gem_do_execbuffer() Xi Wang
2012-04-06 13:36 ` [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2() Chris Wilson
0 siblings, 2 replies; 10+ messages in thread
From: Xi Wang @ 2012-04-06 12:58 UTC (permalink / raw)
To: Keith Packard, Daniel Vetter
Cc: David Airlie, dri-devel, linux-kernel, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] drm/i915: fix integer overflow in i915_gem_do_execbuffer()
2012-04-06 12:58 [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2() Xi Wang
@ 2012-04-06 12:58 ` Xi Wang
2012-04-06 13:36 ` [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2() Chris Wilson
1 sibling, 0 replies; 10+ messages in thread
From: Xi Wang @ 2012-04-06 12:58 UTC (permalink / raw)
To: Keith Packard, Daniel Vetter
Cc: David Airlie, dri-devel, linux-kernel, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()
2012-04-06 12:58 [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2() Xi Wang
2012-04-06 12:58 ` [PATCH 2/2] drm/i915: fix integer overflow in i915_gem_do_execbuffer() Xi Wang
@ 2012-04-06 13:36 ` Chris Wilson
2012-04-06 13:46 ` Xi Wang
1 sibling, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2012-04-06 13:36 UTC (permalink / raw)
To: Xi Wang, Keith Packard, Daniel Vetter; +Cc: linux-kernel, dri-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()
2012-04-06 13:36 ` [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2() Chris Wilson
@ 2012-04-06 13:46 ` Xi Wang
2012-04-06 13:54 ` Chris Wilson
0 siblings, 1 reply; 10+ messages in thread
From: Xi Wang @ 2012-04-06 13:46 UTC (permalink / raw)
To: Chris Wilson; +Cc: Keith Packard, Daniel Vetter, linux-kernel, dri-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()
2012-04-06 13:46 ` Xi Wang
@ 2012-04-06 13:54 ` Chris Wilson
2012-04-06 14:01 ` Xi Wang
0 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2012-04-06 13:54 UTC (permalink / raw)
To: Xi Wang; +Cc: Keith Packard, Daniel Vetter, linux-kernel, dri-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()
2012-04-06 13:54 ` Chris Wilson
@ 2012-04-06 14:01 ` Xi Wang
2012-04-06 14:44 ` Chris Wilson
0 siblings, 1 reply; 10+ messages in thread
From: Xi Wang @ 2012-04-06 14:01 UTC (permalink / raw)
To: Chris Wilson; +Cc: Keith Packard, Daniel Vetter, linux-kernel, dri-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()
2012-04-06 14:01 ` Xi Wang
@ 2012-04-06 14:44 ` Chris Wilson
2012-04-06 18:17 ` Xi Wang
0 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2012-04-06 14:44 UTC (permalink / raw)
To: Xi Wang; +Cc: Keith Packard, Daniel Vetter, linux-kernel, dri-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()
2012-04-06 14:44 ` Chris Wilson
@ 2012-04-06 18:17 ` Xi Wang
2012-04-06 19:40 ` Chris Wilson
0 siblings, 1 reply; 10+ messages in thread
From: Xi Wang @ 2012-04-06 18:17 UTC (permalink / raw)
To: Chris Wilson; +Cc: Keith Packard, Daniel Vetter, linux-kernel, dri-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()
2012-04-06 18:17 ` Xi Wang
@ 2012-04-06 19:40 ` Chris Wilson
2012-04-06 20:34 ` Xi Wang
0 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2012-04-06 19:40 UTC (permalink / raw)
To: Xi Wang; +Cc: Keith Packard, Daniel Vetter, linux-kernel, dri-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2()
2012-04-06 19:40 ` Chris Wilson
@ 2012-04-06 20:34 ` Xi Wang
0 siblings, 0 replies; 10+ messages in thread
From: Xi Wang @ 2012-04-06 20:34 UTC (permalink / raw)
To: Chris Wilson; +Cc: Keith Packard, Daniel Vetter, linux-kernel, dri-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-04-06 20:35 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-06 12:58 [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2() Xi Wang
2012-04-06 12:58 ` [PATCH 2/2] drm/i915: fix integer overflow in i915_gem_do_execbuffer() Xi Wang
2012-04-06 13:36 ` [PATCH 1/2] drm/i915: fix integer overflow in i915_gem_execbuffer2() Chris Wilson
2012-04-06 13:46 ` Xi Wang
2012-04-06 13:54 ` Chris Wilson
2012-04-06 14:01 ` Xi Wang
2012-04-06 14:44 ` Chris Wilson
2012-04-06 18:17 ` Xi Wang
2012-04-06 19:40 ` Chris Wilson
2012-04-06 20:34 ` Xi Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).