[PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer

[PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer

[Chris Wilson]

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

Re: [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer

[Tommi Rantala]

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
>

Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer

[Ben Widawsky]

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

Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer

[Ben Widawsky]

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

Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer

[Chris Wilson]

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

Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer

[Ben Widawsky]

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

Re: [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer

[Chris Wilson]

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

Re: [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer

[Ben Widawsky]

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

Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer

[Chris Wilson]

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

Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer

[Daniel Vetter]

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

Re: [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer

[Chris Wilson]

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

Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer

[Dave Airlie]

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.

Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer

[Chris Wilson]

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

Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer

[Tommi Rantala]

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

Re: [Intel-gfx] [PATCH] drm/i915: Sanity check incoming ioctl data for a NULL pointer

[Dave Jones]

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