* [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
@ 2013-03-14 12:59 ` Chris Wilson
0 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2013-03-14 12:59 UTC (permalink / raw)
To: Tommi Rantala
Cc: David Airlie, Daniel Vetter, intel-gfx, linux-kernel, dri-devel,
Dave Jones
In order to prevent a potential NULL deference with hostile userspace,
we need to check whether the ioctl was passed an invalid args pointer.
Reported-by: Tommi Rantala <tt.rantala@gmail.com>
Link: http://lkml.kernel.org/r/CA+ydwtpuBvbwxbt-tdgPUvj1EU7itmCHo_2B3w13HkD5+jWKow@mail.gmail.com
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 365e41a..9f5602e 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1103,7 +1103,11 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
struct drm_i915_gem_exec_object2 *exec2_list = NULL;
int ret, i;
- if (args->buffer_count < 1) {
+ if (args == NULL)
+ return -EINVAL;
+
+ if (args->buffer_count < 1 ||
+ args->buffer_count > INT_MAX / sizeof(*exec2_list)) {
DRM_DEBUG("execbuf with %d buffers\n", args->buffer_count);
return -EINVAL;
}
@@ -1182,8 +1186,11 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
struct drm_i915_gem_exec_object2 *exec2_list = NULL;
int ret;
+ if (args == NULL)
+ return -EINVAL;
+
if (args->buffer_count < 1 ||
- args->buffer_count > UINT_MAX / sizeof(*exec2_list)) {
+ args->buffer_count > INT_MAX / sizeof(*exec2_list)) {
DRM_DEBUG("execbuf2 with %d buffers\n", args->buffer_count);
return -EINVAL;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
2013-03-14 12:59 ` Chris Wilson
(?)
@ 2013-03-14 13:39 ` Tommi Rantala
2013-04-12 9:39 ` [PATCH] drm: Perform ioctl command validation on the stored kernel values Chris Wilson
-1 siblings, 1 reply; 23+ messages in thread
From: Tommi Rantala @ 2013-03-14 13:39 UTC (permalink / raw)
To: Chris Wilson
Cc: Daniel Vetter, David Airlie, intel-gfx, dri-devel, LKML, Dave Jones
2013/3/14 Chris Wilson <chris@chris-wilson.co.uk>:
> In order to prevent a potential NULL deference with hostile userspace,
> we need to check whether the ioctl was passed an invalid args pointer.
>
> Reported-by: Tommi Rantala <tt.rantala@gmail.com>
> Link: http://lkml.kernel.org/r/CA+ydwtpuBvbwxbt-tdgPUvj1EU7itmCHo_2B3w13HkD5+jWKow@mail.gmail.com
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Thanks, looks good. I still see a flood of oopses from the other
ioctls, so it's a bit difficult to test this patch properly.
> ---
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 365e41a..9f5602e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1103,7 +1103,11 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> struct drm_i915_gem_exec_object2 *exec2_list = NULL;
> int ret, i;
>
> - if (args->buffer_count < 1) {
> + if (args == NULL)
> + return -EINVAL;
> +
> + if (args->buffer_count < 1 ||
> + args->buffer_count > INT_MAX / sizeof(*exec2_list)) {
> DRM_DEBUG("execbuf with %d buffers\n", args->buffer_count);
> return -EINVAL;
> }
> @@ -1182,8 +1186,11 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
> struct drm_i915_gem_exec_object2 *exec2_list = NULL;
> int ret;
>
> + if (args == NULL)
> + return -EINVAL;
> +
> if (args->buffer_count < 1 ||
> - args->buffer_count > UINT_MAX / sizeof(*exec2_list)) {
> + args->buffer_count > INT_MAX / sizeof(*exec2_list)) {
> DRM_DEBUG("execbuf2 with %d buffers\n", args->buffer_count);
> return -EINVAL;
> }
> --
> 1.7.10.4
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] drm: Perform ioctl command validation on the stored kernel values
2013-03-14 13:39 ` Tommi Rantala
@ 2013-04-12 9:39 ` Chris Wilson
2013-04-16 3:18 ` Dave Airlie
0 siblings, 1 reply; 23+ messages in thread
From: Chris Wilson @ 2013-04-12 9:39 UTC (permalink / raw)
To: dri-devel; +Cc: intel-gfx
Userspace is free to pass in any command bits it feels like through the
ioctl cmd, and for example trinity likes to fuzz those bits to create
conflicting commands. So instead of relying upon userspace to pass along
the correct IN/OUT flags for the ioctl, use the flags as expected by the
kernel.
This does have a side-effect that NULL pointers can not be substituted
by userspace in place of a struct. This feature was not being used by
any driver, but instead exposed all of the command handlers to a user
triggerable OOPS.
Reported-by: Tommi Rantala <tt.rantala@gmail.com>
Link: http://lkml.kernel.org/r/CA+ydwtpuBvbwxbt-tdgPUvj1EU7itmCHo_2B3w13HkD5+jWKow@mail.gmail.com
Signed-off-by: Tommi Rantala <tt.rantala@gmail.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/drm_drv.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 25f91cd..0ac1991 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -408,6 +408,7 @@ long drm_ioctl(struct file *filp,
usize = asize = _IOC_SIZE(cmd);
if (drv_size > asize)
asize = drv_size;
+ cmd = ioctl->cmd_drv;
}
else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) {
ioctl = &drm_ioctls[nr];
--
1.7.10.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
2013-03-14 12:59 ` Chris Wilson
(?)
(?)
@ 2013-03-15 4:43 ` Ben Widawsky
-1 siblings, 0 replies; 23+ messages in thread
From: Ben Widawsky @ 2013-03-15 4:43 UTC (permalink / raw)
To: Chris Wilson
Cc: Tommi Rantala, David Airlie, Daniel Vetter, intel-gfx,
linux-kernel, dri-devel, Dave Jones
On Thu, Mar 14, 2013 at 12:59:57PM +0000, Chris Wilson wrote:
> In order to prevent a potential NULL deference with hostile userspace,
> we need to check whether the ioctl was passed an invalid args pointer.
>
> Reported-by: Tommi Rantala <tt.rantala@gmail.com>
> Link: http://lkml.kernel.org/r/CA+ydwtpuBvbwxbt-tdgPUvj1EU7itmCHo_2B3w13HkD5+jWKow@mail.gmail.com
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 365e41a..9f5602e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1103,7 +1103,11 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> struct drm_i915_gem_exec_object2 *exec2_list = NULL;
> int ret, i;
>
> - if (args->buffer_count < 1) {
> + if (args == NULL)
> + return -EINVAL;
> +
> + if (args->buffer_count < 1 ||
> + args->buffer_count > INT_MAX / sizeof(*exec2_list)) {
> DRM_DEBUG("execbuf with %d buffers\n", args->buffer_count);
> return -EINVAL;
> }
> @@ -1182,8 +1186,11 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
> struct drm_i915_gem_exec_object2 *exec2_list = NULL;
> int ret;
>
> + if (args == NULL)
> + return -EINVAL;
> +
> if (args->buffer_count < 1 ||
> - args->buffer_count > UINT_MAX / sizeof(*exec2_list)) {
> + args->buffer_count > INT_MAX / sizeof(*exec2_list)) {
> DRM_DEBUG("execbuf2 with %d buffers\n", args->buffer_count);
> return -EINVAL;
> }
Why did you change UINT_MAX to INT_MAX? TBH, I'm confused what we're
trying to achieve, and why we need anything other than:
if (!args->buffer_count)
I'm also not seeing how the NULL checks are needed since at least it
seems to be for execbuffer (IOW) we could never have NULL args.
if (cmd & (IOC_IN | IOC_OUT)) {
if (asize <= sizeof(stack_kdata)) {
kdata = stack_kdata;
} else {
kdata = kmalloc(asize, GFP_KERNEL);
if (!kdata) {
retcode = -ENOMEM;
goto err_i1;
}
}
if (asize > usize)
memset(kdata + usize, 0, asize - usize);
}
Sorry if these are stupid questions.
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
2013-03-14 12:59 ` Chris Wilson
` (2 preceding siblings ...)
(?)
@ 2013-03-15 4:50 ` Ben Widawsky
2013-03-15 8:24 ` Chris Wilson
-1 siblings, 1 reply; 23+ messages in thread
From: Ben Widawsky @ 2013-03-15 4:50 UTC (permalink / raw)
To: Chris Wilson
Cc: Tommi Rantala, David Airlie, Daniel Vetter, intel-gfx,
linux-kernel, dri-devel, Dave Jones
On Thu, Mar 14, 2013 at 12:59:57PM +0000, Chris Wilson wrote:
> In order to prevent a potential NULL deference with hostile userspace,
> we need to check whether the ioctl was passed an invalid args pointer.
>
> Reported-by: Tommi Rantala <tt.rantala@gmail.com>
> Link: http://lkml.kernel.org/r/CA+ydwtpuBvbwxbt-tdgPUvj1EU7itmCHo_2B3w13HkD5+jWKow@mail.gmail.com
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 365e41a..9f5602e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1103,7 +1103,11 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> struct drm_i915_gem_exec_object2 *exec2_list = NULL;
> int ret, i;
>
> - if (args->buffer_count < 1) {
> + if (args == NULL)
> + return -EINVAL;
> +
> + if (args->buffer_count < 1 ||
> + args->buffer_count > INT_MAX / sizeof(*exec2_list)) {
> DRM_DEBUG("execbuf with %d buffers\n", args->buffer_count);
> return -EINVAL;
> }
> @@ -1182,8 +1186,11 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
> struct drm_i915_gem_exec_object2 *exec2_list = NULL;
> int ret;
>
> + if (args == NULL)
> + return -EINVAL;
> +
> if (args->buffer_count < 1 ||
> - args->buffer_count > UINT_MAX / sizeof(*exec2_list)) {
> + args->buffer_count > INT_MAX / sizeof(*exec2_list)) {
> DRM_DEBUG("execbuf2 with %d buffers\n", args->buffer_count);
> return -EINVAL;
> }
Why did you change UINT_MAX to INT_MAX? TBH, I'm confused what we're
trying to achieve, and why we need anything other than:
if (!args->buffer_count)
I'm also not seeing how the NULL checks are needed since at least it
seems to be for execbuffer (IOW) we could never have NULL args.
if (cmd & (IOC_IN | IOC_OUT)) {
if (asize <= sizeof(stack_kdata)) {
kdata = stack_kdata;
} else {
kdata = kmalloc(asize, GFP_KERNEL);
if (!kdata) {
retcode = -ENOMEM;
goto err_i1;
}
}
if (asize > usize)
memset(kdata + usize, 0, asize - usize);
}
Sorry if these are stupid questions.
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
2013-03-15 4:50 ` Ben Widawsky
@ 2013-03-15 8:24 ` Chris Wilson
0 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2013-03-15 8:24 UTC (permalink / raw)
To: Ben Widawsky
Cc: Tommi Rantala, David Airlie, Daniel Vetter, intel-gfx,
linux-kernel, dri-devel, Dave Jones
On Thu, Mar 14, 2013 at 09:50:04PM -0700, Ben Widawsky wrote:
> On Thu, Mar 14, 2013 at 12:59:57PM +0000, Chris Wilson wrote:
> > In order to prevent a potential NULL deference with hostile userspace,
> > we need to check whether the ioctl was passed an invalid args pointer.
> >
> > Reported-by: Tommi Rantala <tt.rantala@gmail.com>
> > Link: http://lkml.kernel.org/r/CA+ydwtpuBvbwxbt-tdgPUvj1EU7itmCHo_2B3w13HkD5+jWKow@mail.gmail.com
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 365e41a..9f5602e 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -1103,7 +1103,11 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> > struct drm_i915_gem_exec_object2 *exec2_list = NULL;
> > int ret, i;
> >
> > - if (args->buffer_count < 1) {
> > + if (args == NULL)
> > + return -EINVAL;
> > +
> > + if (args->buffer_count < 1 ||
> > + args->buffer_count > INT_MAX / sizeof(*exec2_list)) {
> > DRM_DEBUG("execbuf with %d buffers\n", args->buffer_count);
> > return -EINVAL;
> > }
> > @@ -1182,8 +1186,11 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
> > struct drm_i915_gem_exec_object2 *exec2_list = NULL;
> > int ret;
> >
> > + if (args == NULL)
> > + return -EINVAL;
> > +
> > if (args->buffer_count < 1 ||
> > - args->buffer_count > UINT_MAX / sizeof(*exec2_list)) {
> > + args->buffer_count > INT_MAX / sizeof(*exec2_list)) {
> > DRM_DEBUG("execbuf2 with %d buffers\n", args->buffer_count);
> > return -EINVAL;
> > }
>
> Why did you change UINT_MAX to INT_MAX?
Because we check later against INT_MAX, and I didn't like the confusion.
If we are going to pick an arbitrary limit, lets at least be consistent.
> TBH, I'm confused what we're
> trying to achieve, and why we need anything other than:
> if (!args->buffer_count)
Because we then promptly do a u32 multiply and we need to be sure that
userspace can't trigger an overflow there and cause us to read
unallocated memory later.
>
> I'm also not seeing how the NULL checks are needed since at least it
> seems to be for execbuffer (IOW) we could never have NULL args.
That's what I thought too. Looking at the stack trace, the empirical
evidence is that we need the check.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
@ 2013-03-15 8:24 ` Chris Wilson
0 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2013-03-15 8:24 UTC (permalink / raw)
To: Ben Widawsky
Cc: Daniel Vetter, intel-gfx, linux-kernel, dri-devel, Dave Jones
On Thu, Mar 14, 2013 at 09:50:04PM -0700, Ben Widawsky wrote:
> On Thu, Mar 14, 2013 at 12:59:57PM +0000, Chris Wilson wrote:
> > In order to prevent a potential NULL deference with hostile userspace,
> > we need to check whether the ioctl was passed an invalid args pointer.
> >
> > Reported-by: Tommi Rantala <tt.rantala@gmail.com>
> > Link: http://lkml.kernel.org/r/CA+ydwtpuBvbwxbt-tdgPUvj1EU7itmCHo_2B3w13HkD5+jWKow@mail.gmail.com
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 365e41a..9f5602e 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -1103,7 +1103,11 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> > struct drm_i915_gem_exec_object2 *exec2_list = NULL;
> > int ret, i;
> >
> > - if (args->buffer_count < 1) {
> > + if (args == NULL)
> > + return -EINVAL;
> > +
> > + if (args->buffer_count < 1 ||
> > + args->buffer_count > INT_MAX / sizeof(*exec2_list)) {
> > DRM_DEBUG("execbuf with %d buffers\n", args->buffer_count);
> > return -EINVAL;
> > }
> > @@ -1182,8 +1186,11 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
> > struct drm_i915_gem_exec_object2 *exec2_list = NULL;
> > int ret;
> >
> > + if (args == NULL)
> > + return -EINVAL;
> > +
> > if (args->buffer_count < 1 ||
> > - args->buffer_count > UINT_MAX / sizeof(*exec2_list)) {
> > + args->buffer_count > INT_MAX / sizeof(*exec2_list)) {
> > DRM_DEBUG("execbuf2 with %d buffers\n", args->buffer_count);
> > return -EINVAL;
> > }
>
> Why did you change UINT_MAX to INT_MAX?
Because we check later against INT_MAX, and I didn't like the confusion.
If we are going to pick an arbitrary limit, lets at least be consistent.
> TBH, I'm confused what we're
> trying to achieve, and why we need anything other than:
> if (!args->buffer_count)
Because we then promptly do a u32 multiply and we need to be sure that
userspace can't trigger an overflow there and cause us to read
unallocated memory later.
>
> I'm also not seeing how the NULL checks are needed since at least it
> seems to be for execbuffer (IOW) we could never have NULL args.
That's what I thought too. Looking at the stack trace, the empirical
evidence is that we need the check.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
2013-03-15 8:24 ` Chris Wilson
(?)
@ 2013-03-15 16:36 ` Ben Widawsky
2013-03-15 22:06 ` Chris Wilson
-1 siblings, 1 reply; 23+ messages in thread
From: Ben Widawsky @ 2013-03-15 16:36 UTC (permalink / raw)
To: Chris Wilson, Tommi Rantala, David Airlie, Daniel Vetter,
intel-gfx, linux-kernel, dri-devel, Dave Jones
On Fri, Mar 15, 2013 at 08:24:03AM +0000, Chris Wilson wrote:
> On Thu, Mar 14, 2013 at 09:50:04PM -0700, Ben Widawsky wrote:
> > On Thu, Mar 14, 2013 at 12:59:57PM +0000, Chris Wilson wrote:
> > > In order to prevent a potential NULL deference with hostile userspace,
> > > we need to check whether the ioctl was passed an invalid args pointer.
> > >
> > > Reported-by: Tommi Rantala <tt.rantala@gmail.com>
> > > Link: http://lkml.kernel.org/r/CA+ydwtpuBvbwxbt-tdgPUvj1EU7itmCHo_2B3w13HkD5+jWKow@mail.gmail.com
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +++++++++--
> > > 1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > index 365e41a..9f5602e 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > @@ -1103,7 +1103,11 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> > > struct drm_i915_gem_exec_object2 *exec2_list = NULL;
> > > int ret, i;
> > >
> > > - if (args->buffer_count < 1) {
> > > + if (args == NULL)
> > > + return -EINVAL;
> > > +
> > > + if (args->buffer_count < 1 ||
> > > + args->buffer_count > INT_MAX / sizeof(*exec2_list)) {
> > > DRM_DEBUG("execbuf with %d buffers\n", args->buffer_count);
> > > return -EINVAL;
> > > }
> > > @@ -1182,8 +1186,11 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
> > > struct drm_i915_gem_exec_object2 *exec2_list = NULL;
> > > int ret;
> > >
> > > + if (args == NULL)
> > > + return -EINVAL;
> > > +
> > > if (args->buffer_count < 1 ||
> > > - args->buffer_count > UINT_MAX / sizeof(*exec2_list)) {
> > > + args->buffer_count > INT_MAX / sizeof(*exec2_list)) {
> > > DRM_DEBUG("execbuf2 with %d buffers\n", args->buffer_count);
> > > return -EINVAL;
> > > }
> >
> > Why did you change UINT_MAX to INT_MAX?
>
> Because we check later against INT_MAX, and I didn't like the confusion.
> If we are going to pick an arbitrary limit, lets at least be consistent.
>
> > TBH, I'm confused what we're
> > trying to achieve, and why we need anything other than:
> > if (!args->buffer_count)
>
> Because we then promptly do a u32 multiply and we need to be sure that
> userspace can't trigger an overflow there and cause us to read
> unallocated memory later.
> >
> > I'm also not seeing how the NULL checks are needed since at least it
> > seems to be for execbuffer (IOW) we could never have NULL args.
>
> That's what I thought too. Looking at the stack trace, the empirical
> evidence is that we need the check.
> -Chris
I think we need to investigate the issue more then, or put a BUG_ON() in
the drm code and run it through trinity. We have other places where arg
can't/shouldn't be NULL and we don't check.
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
2013-03-15 16:36 ` Ben Widawsky
@ 2013-03-15 22:06 ` Chris Wilson
0 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2013-03-15 22:06 UTC (permalink / raw)
To: Ben Widawsky
Cc: Tommi Rantala, David Airlie, Daniel Vetter, intel-gfx,
linux-kernel, dri-devel, Dave Jones
On Fri, Mar 15, 2013 at 09:36:07AM -0700, Ben Widawsky wrote:
> On Fri, Mar 15, 2013 at 08:24:03AM +0000, Chris Wilson wrote:
> > That's what I thought too. Looking at the stack trace, the empirical
> > evidence is that we need the check.
> > -Chris
>
> I think we need to investigate the issue more then, or put a BUG_ON() in
> the drm code and run it through trinity. We have other places where arg
> can't/shouldn't be NULL and we don't check.
Actually we are both wrong. drm_ioctl() does not validate that the
user struct matches the expected size, just ensures that if that user
cmd specifies that the arg is to be used that it only up to the known
size is copied.
A hostile userspace can bludgen a NULL pointer through drm_ioctl() into
the driver->ioctl->func().
If we used driver->ioctl->cmd instead of the user supplied cmd, we would
have a whole other can of worms to deal with (as we suddenly get a
struct of zeroes). However, those worms should already be treated as
invalid. Choose your poison.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
@ 2013-03-15 22:06 ` Chris Wilson
0 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2013-03-15 22:06 UTC (permalink / raw)
To: Ben Widawsky
Cc: Tommi Rantala, David Airlie, Daniel Vetter, intel-gfx,
linux-kernel, dri-devel, Dave Jones
On Fri, Mar 15, 2013 at 09:36:07AM -0700, Ben Widawsky wrote:
> On Fri, Mar 15, 2013 at 08:24:03AM +0000, Chris Wilson wrote:
> > That's what I thought too. Looking at the stack trace, the empirical
> > evidence is that we need the check.
> > -Chris
>
> I think we need to investigate the issue more then, or put a BUG_ON() in
> the drm code and run it through trinity. We have other places where arg
> can't/shouldn't be NULL and we don't check.
Actually we are both wrong. drm_ioctl() does not validate that the
user struct matches the expected size, just ensures that if that user
cmd specifies that the arg is to be used that it only up to the known
size is copied.
A hostile userspace can bludgen a NULL pointer through drm_ioctl() into
the driver->ioctl->func().
If we used driver->ioctl->cmd instead of the user supplied cmd, we would
have a whole other can of worms to deal with (as we suddenly get a
struct of zeroes). However, those worms should already be treated as
invalid. Choose your poison.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
2013-03-15 22:06 ` Chris Wilson
@ 2013-03-15 23:49 ` Ben Widawsky
-1 siblings, 0 replies; 23+ messages in thread
From: Ben Widawsky @ 2013-03-15 23:49 UTC (permalink / raw)
To: Chris Wilson, Tommi Rantala, David Airlie, Daniel Vetter,
intel-gfx, linux-kernel, dri-devel, Dave Jones
On Fri, Mar 15, 2013 at 10:06:19PM +0000, Chris Wilson wrote:
> On Fri, Mar 15, 2013 at 09:36:07AM -0700, Ben Widawsky wrote:
> > On Fri, Mar 15, 2013 at 08:24:03AM +0000, Chris Wilson wrote:
> > > That's what I thought too. Looking at the stack trace, the empirical
> > > evidence is that we need the check.
> > > -Chris
> >
> > I think we need to investigate the issue more then, or put a BUG_ON() in
> > the drm code and run it through trinity. We have other places where arg
> > can't/shouldn't be NULL and we don't check.
>
> Actually we are both wrong. drm_ioctl() does not validate that the
> user struct matches the expected size, just ensures that if that user
> cmd specifies that the arg is to be used that it only up to the known
> size is copied.
>
> A hostile userspace can bludgen a NULL pointer through drm_ioctl() into
> the driver->ioctl->func().
> > > + if (args == NULL)
> > > + return -EINVAL;
> > > +
I must be failing to see the obvious, but I'm still not getting how args
can ever be NULL. kdata which is passed to us as "data" and cast to
"args' is either always some stack variable, or some kmalloc'd memory. I
see how the arguments themselves can be crap which is really only when
user size != drv_size.
So tell me, which case can result in a NULL arg?
1. user size == drv_size // better not be this one
2. user size < driver size
3. user size > driver size
It seems to me we still must [simply] be missing something in our
parameter validation.
>
> If we used driver->ioctl->cmd instead of the user supplied cmd, we would
> have a whole other can of worms to deal with (as we suddenly get a
> struct of zeroes). However, those worms should already be treated as
> invalid. Choose your poison.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
@ 2013-03-15 23:49 ` Ben Widawsky
0 siblings, 0 replies; 23+ messages in thread
From: Ben Widawsky @ 2013-03-15 23:49 UTC (permalink / raw)
To: Chris Wilson, Tommi Rantala, David Airlie, Daniel Vetter,
intel-gfx, linux-kernel, dri-devel, Dave Jones
On Fri, Mar 15, 2013 at 10:06:19PM +0000, Chris Wilson wrote:
> On Fri, Mar 15, 2013 at 09:36:07AM -0700, Ben Widawsky wrote:
> > On Fri, Mar 15, 2013 at 08:24:03AM +0000, Chris Wilson wrote:
> > > That's what I thought too. Looking at the stack trace, the empirical
> > > evidence is that we need the check.
> > > -Chris
> >
> > I think we need to investigate the issue more then, or put a BUG_ON() in
> > the drm code and run it through trinity. We have other places where arg
> > can't/shouldn't be NULL and we don't check.
>
> Actually we are both wrong. drm_ioctl() does not validate that the
> user struct matches the expected size, just ensures that if that user
> cmd specifies that the arg is to be used that it only up to the known
> size is copied.
>
> A hostile userspace can bludgen a NULL pointer through drm_ioctl() into
> the driver->ioctl->func().
> > > + if (args == NULL)
> > > + return -EINVAL;
> > > +
I must be failing to see the obvious, but I'm still not getting how args
can ever be NULL. kdata which is passed to us as "data" and cast to
"args' is either always some stack variable, or some kmalloc'd memory. I
see how the arguments themselves can be crap which is really only when
user size != drv_size.
So tell me, which case can result in a NULL arg?
1. user size == drv_size // better not be this one
2. user size < driver size
3. user size > driver size
It seems to me we still must [simply] be missing something in our
parameter validation.
>
> If we used driver->ioctl->cmd instead of the user supplied cmd, we would
> have a whole other can of worms to deal with (as we suddenly get a
> struct of zeroes). However, those worms should already be treated as
> invalid. Choose your poison.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
2013-03-15 23:49 ` Ben Widawsky
(?)
@ 2013-03-16 10:19 ` Chris Wilson
2013-03-17 19:50 ` Daniel Vetter
-1 siblings, 1 reply; 23+ messages in thread
From: Chris Wilson @ 2013-03-16 10:19 UTC (permalink / raw)
To: Ben Widawsky
Cc: Tommi Rantala, David Airlie, Daniel Vetter, intel-gfx,
linux-kernel, dri-devel, Dave Jones
On Fri, Mar 15, 2013 at 04:49:42PM -0700, Ben Widawsky wrote:
> On Fri, Mar 15, 2013 at 10:06:19PM +0000, Chris Wilson wrote:
> > On Fri, Mar 15, 2013 at 09:36:07AM -0700, Ben Widawsky wrote:
> > > On Fri, Mar 15, 2013 at 08:24:03AM +0000, Chris Wilson wrote:
> > > > That's what I thought too. Looking at the stack trace, the empirical
> > > > evidence is that we need the check.
> > > > -Chris
> > >
> > > I think we need to investigate the issue more then, or put a BUG_ON() in
> > > the drm code and run it through trinity. We have other places where arg
> > > can't/shouldn't be NULL and we don't check.
> >
> > Actually we are both wrong. drm_ioctl() does not validate that the
> > user struct matches the expected size, just ensures that if that user
> > cmd specifies that the arg is to be used that it only up to the known
> > size is copied.
> >
> > A hostile userspace can bludgen a NULL pointer through drm_ioctl() into
> > the driver->ioctl->func().
>
> > > > + if (args == NULL)
> > > > + return -EINVAL;
> > > > +
>
> I must be failing to see the obvious, but I'm still not getting how args
> can ever be NULL. kdata which is passed to us as "data" and cast to
> "args' is either always some stack variable, or some kmalloc'd memory. I
> see how the arguments themselves can be crap which is really only when
> user size != drv_size.
>
> So tell me, which case can result in a NULL arg?
> 1. user size == drv_size // better not be this one
> 2. user size < driver size
> 3. user size > driver size
>
> It seems to me we still must [simply] be missing something in our
> parameter validation.
If *userspace* doesn't request either IOC_IN | IOC_OUT in their ioctl
command (which are seperate from the ioctl number), then kdata is set to
NULL.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
2013-03-16 10:19 ` [Intel-gfx] " Chris Wilson
@ 2013-03-17 19:50 ` Daniel Vetter
2013-03-17 21:40 ` Chris Wilson
2013-03-17 21:58 ` Dave Jones
0 siblings, 2 replies; 23+ messages in thread
From: Daniel Vetter @ 2013-03-17 19:50 UTC (permalink / raw)
To: Chris Wilson, Ben Widawsky, Tommi Rantala, David Airlie,
Daniel Vetter, intel-gfx, linux-kernel, dri-devel, Dave Jones
On Sat, Mar 16, 2013 at 11:19 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Mar 15, 2013 at 04:49:42PM -0700, Ben Widawsky wrote:
>> On Fri, Mar 15, 2013 at 10:06:19PM +0000, Chris Wilson wrote:
>> > On Fri, Mar 15, 2013 at 09:36:07AM -0700, Ben Widawsky wrote:
>> > > On Fri, Mar 15, 2013 at 08:24:03AM +0000, Chris Wilson wrote:
>> > > > That's what I thought too. Looking at the stack trace, the empirical
>> > > > evidence is that we need the check.
>> > > > -Chris
>> > >
>> > > I think we need to investigate the issue more then, or put a BUG_ON() in
>> > > the drm code and run it through trinity. We have other places where arg
>> > > can't/shouldn't be NULL and we don't check.
>> >
>> > Actually we are both wrong. drm_ioctl() does not validate that the
>> > user struct matches the expected size, just ensures that if that user
>> > cmd specifies that the arg is to be used that it only up to the known
>> > size is copied.
>> >
>> > A hostile userspace can bludgen a NULL pointer through drm_ioctl() into
>> > the driver->ioctl->func().
>>
>> > > > + if (args == NULL)
>> > > > + return -EINVAL;
>> > > > +
>>
>> I must be failing to see the obvious, but I'm still not getting how args
>> can ever be NULL. kdata which is passed to us as "data" and cast to
>> "args' is either always some stack variable, or some kmalloc'd memory. I
>> see how the arguments themselves can be crap which is really only when
>> user size != drv_size.
>>
>> So tell me, which case can result in a NULL arg?
>> 1. user size == drv_size // better not be this one
>> 2. user size < driver size
>> 3. user size > driver size
>>
>> It seems to me we still must [simply] be missing something in our
>> parameter validation.
>
> If *userspace* doesn't request either IOC_IN | IOC_OUT in their ioctl
> command (which are seperate from the ioctl number), then kdata is set to
> NULL.
Doesn't that mean that we need these checks everywhere? Or at least a
fixup in drm core proper?
And I think we need to add trinity to our test setup eventually ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
2013-03-17 19:50 ` Daniel Vetter
@ 2013-03-17 21:40 ` Chris Wilson
2013-03-17 21:58 ` Dave Jones
1 sibling, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2013-03-17 21:40 UTC (permalink / raw)
To: Daniel Vetter
Cc: Ben Widawsky, Tommi Rantala, David Airlie, intel-gfx,
linux-kernel, dri-devel, Dave Jones
On Sun, Mar 17, 2013 at 08:50:03PM +0100, Daniel Vetter wrote:
> On Sat, Mar 16, 2013 at 11:19 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > If *userspace* doesn't request either IOC_IN | IOC_OUT in their ioctl
> > command (which are seperate from the ioctl number), then kdata is set to
> > NULL.
>
> Doesn't that mean that we need these checks everywhere? Or at least a
> fixup in drm core proper?
That's my conclusion. We either add a flag to ask drm_ioctl to prevent
passing NULL pointers (as the existing behaviour may be useful
somewhere, and I have not checked all callees) or saturate our callbacks
with NULL checks.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
@ 2013-03-17 21:40 ` Chris Wilson
0 siblings, 0 replies; 23+ messages in thread
From: Chris Wilson @ 2013-03-17 21:40 UTC (permalink / raw)
To: Daniel Vetter
Cc: Ben Widawsky, David Airlie, intel-gfx, Tommi Rantala, dri-devel,
linux-kernel, Dave Jones
On Sun, Mar 17, 2013 at 08:50:03PM +0100, Daniel Vetter wrote:
> On Sat, Mar 16, 2013 at 11:19 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > If *userspace* doesn't request either IOC_IN | IOC_OUT in their ioctl
> > command (which are seperate from the ioctl number), then kdata is set to
> > NULL.
>
> Doesn't that mean that we need these checks everywhere? Or at least a
> fixup in drm core proper?
That's my conclusion. We either add a flag to ask drm_ioctl to prevent
passing NULL pointers (as the existing behaviour may be useful
somewhere, and I have not checked all callees) or saturate our callbacks
with NULL checks.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
2013-03-17 21:40 ` Chris Wilson
(?)
@ 2013-03-17 21:42 ` Dave Airlie
2013-03-17 21:51 ` Chris Wilson
-1 siblings, 1 reply; 23+ messages in thread
From: Dave Airlie @ 2013-03-17 21:42 UTC (permalink / raw)
To: Chris Wilson, Daniel Vetter, Ben Widawsky, Tommi Rantala,
David Airlie, intel-gfx, linux-kernel, dri-devel, Dave Jones
On Mon, Mar 18, 2013 at 7:40 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Sun, Mar 17, 2013 at 08:50:03PM +0100, Daniel Vetter wrote:
>> On Sat, Mar 16, 2013 at 11:19 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > If *userspace* doesn't request either IOC_IN | IOC_OUT in their ioctl
>> > command (which are seperate from the ioctl number), then kdata is set to
>> > NULL.
>>
>> Doesn't that mean that we need these checks everywhere? Or at least a
>> fixup in drm core proper?
>
> That's my conclusion. We either add a flag to ask drm_ioctl to prevent
> passing NULL pointers (as the existing behaviour may be useful
> somewhere, and I have not checked all callees) or saturate our callbacks
> with NULL checks.
Do we have the kernel's expected IOC_IN/IOC_OUT flags at that point as well?
we could check them and block NULL in that case.
Dave.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
2013-03-17 21:42 ` [Intel-gfx] " Dave Airlie
@ 2013-03-17 21:51 ` Chris Wilson
2013-04-11 18:59 ` Tommi Rantala
0 siblings, 1 reply; 23+ messages in thread
From: Chris Wilson @ 2013-03-17 21:51 UTC (permalink / raw)
To: Dave Airlie
Cc: Daniel Vetter, Ben Widawsky, Tommi Rantala, David Airlie,
intel-gfx, linux-kernel, dri-devel, Dave Jones
On Mon, Mar 18, 2013 at 07:42:58AM +1000, Dave Airlie wrote:
> On Mon, Mar 18, 2013 at 7:40 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Sun, Mar 17, 2013 at 08:50:03PM +0100, Daniel Vetter wrote:
> >> On Sat, Mar 16, 2013 at 11:19 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >> > If *userspace* doesn't request either IOC_IN | IOC_OUT in their ioctl
> >> > command (which are seperate from the ioctl number), then kdata is set to
> >> > NULL.
> >>
> >> Doesn't that mean that we need these checks everywhere? Or at least a
> >> fixup in drm core proper?
> >
> > That's my conclusion. We either add a flag to ask drm_ioctl to prevent
> > passing NULL pointers (as the existing behaviour may be useful
> > somewhere, and I have not checked all callees) or saturate our callbacks
> > with NULL checks.
>
> Do we have the kernel's expected IOC_IN/IOC_OUT flags at that point as well?
>
> we could check them and block NULL in that case.
Yes. For the core ioctls, we use drm_ioctls[nr].cmd rather than the
value passed in by userspace for the IOC_IN|IN_OUT bits. So:
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 25f91cd..79b8bd1 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -408,6 +408,7 @@ long drm_ioctl(struct file *filp,
usize = asize = _IOC_SIZE(cmd);
if (drv_size > asize)
asize = drv_size;
+ cmd = ioctl->cmd;
}
else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) {
ioctl = &drm_ioctls[nr];
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
2013-03-17 21:51 ` Chris Wilson
@ 2013-04-11 18:59 ` Tommi Rantala
0 siblings, 0 replies; 23+ messages in thread
From: Tommi Rantala @ 2013-04-11 18:59 UTC (permalink / raw)
To: Chris Wilson, Dave Airlie, Daniel Vetter, Ben Widawsky,
Tommi Rantala, David Airlie, intel-gfx, LKML, dri-devel,
Dave Jones
2013/3/17 Chris Wilson <chris@chris-wilson.co.uk>:
> On Mon, Mar 18, 2013 at 07:42:58AM +1000, Dave Airlie wrote:
>> On Mon, Mar 18, 2013 at 7:40 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > On Sun, Mar 17, 2013 at 08:50:03PM +0100, Daniel Vetter wrote:
>> >> On Sat, Mar 16, 2013 at 11:19 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> >> > If *userspace* doesn't request either IOC_IN | IOC_OUT in their ioctl
>> >> > command (which are seperate from the ioctl number), then kdata is set to
>> >> > NULL.
>> >>
>> >> Doesn't that mean that we need these checks everywhere? Or at least a
>> >> fixup in drm core proper?
>> >
>> > That's my conclusion. We either add a flag to ask drm_ioctl to prevent
>> > passing NULL pointers (as the existing behaviour may be useful
>> > somewhere, and I have not checked all callees) or saturate our callbacks
>> > with NULL checks.
>>
>> Do we have the kernel's expected IOC_IN/IOC_OUT flags at that point as well?
>>
>> we could check them and block NULL in that case.
>
> Yes. For the core ioctls, we use drm_ioctls[nr].cmd rather than the
> value passed in by userspace for the IOC_IN|IN_OUT bits. So:
Thanks, trinity can indeed set the in/out bits randomly, in a way that
does not match the driver ioctl definition.
Your patch almost fixes this. For the driver ioctls we will want to
grab the cmd from cmd_drv. So the patch should be:
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 25f91cd..5210f33 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -408,6 +408,7 @@ long drm_ioctl(struct file *filp,
usize = asize = _IOC_SIZE(cmd);
if (drv_size > asize)
asize = drv_size;
+ cmd = ioctl->cmd_drv;
}
else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) {
ioctl = &drm_ioctls[nr];
Can you please submit this officially?
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 25f91cd..79b8bd1 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -408,6 +408,7 @@ long drm_ioctl(struct file *filp,
> usize = asize = _IOC_SIZE(cmd);
> if (drv_size > asize)
> asize = drv_size;
> + cmd = ioctl->cmd;
> }
> else if ((nr >= DRM_COMMAND_END) || (nr < DRM_COMMAND_BASE)) {
> ioctl = &drm_ioctls[nr];
>
>
> --
> Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer
2013-03-17 19:50 ` Daniel Vetter
2013-03-17 21:40 ` Chris Wilson
@ 2013-03-17 21:58 ` Dave Jones
1 sibling, 0 replies; 23+ messages in thread
From: Dave Jones @ 2013-03-17 21:58 UTC (permalink / raw)
To: Daniel Vetter
Cc: Chris Wilson, Ben Widawsky, Tommi Rantala, David Airlie,
intel-gfx, linux-kernel, dri-devel
On Sun, Mar 17, 2013 at 08:50:03PM +0100, Daniel Vetter wrote:
> Doesn't that mean that we need these checks everywhere? Or at least a
> fixup in drm core proper?
>
> And I think we need to add trinity to our test setup eventually ;-)
Note that trinity's ioctl fuzzing is still very new (added in just
the last few weeks), and for drm isn't very advanced at all yet.
I was pretty surprised when Tommi's changes started turning up bugs
so quickly, but I guess a lot of the ioctl paths have just never been
audited for these kinds of bugs.
As you can see at
https://github.com/kernelslacker/trinity/blob/master/ioctls/drm.c
It's literally just enumerating the known ioctl's, and using the
generic fuzzing routines (so it just guesses what the argument is,
and hence passes crap like NULL, or a page of garbage).
Eventually I'd like to have routines for each of the individual ioctl
cases to pass something that looks slightly more realistic to what
it's expecting to see.
(Compare to say, the SCSI SG_IO routines here:
https://github.com/kernelslacker/trinity/blob/master/ioctls/scsi.c
[still kinda dumb, but gives an idea of the direction])
Lots of work ahead.
Dave
^ permalink raw reply [flat|nested] 23+ messages in thread