[PATCH] drm/i915/ppgtt: Prevent NULL deref in reset ioctl

[PATCH] drm/i915/ppgtt: Prevent NULL deref in reset ioctl

[Ben Widawsky]

If we look up an invalid context ID, the idr will return NULL. The ptr
is unconditionally dereferenced afterwards causing a problem.

Note that if the context does not exist, we still return success. This
appears to be the behavior desired by gem_reset_stats --subtest ban

Introduced in v3 of
commit 41bde5535a7d48876095926bb55b1aed5ccd6b2c
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Fri Dec 6 14:11:21 2013 -0800

    drm/i915: Get context early in execbuf

Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_uncore.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index e52fcce..aa4c55a 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -850,7 +850,7 @@ int i915_get_reset_stats_ioctl(struct drm_device *dev,
 		return ret;
 
 	ctx = i915_gem_context_get(file->driver_priv, args->ctx_id);
-	if (IS_ERR(ctx)) {
+	if (IS_ERR_OR_NULL(ctx)) {
 		mutex_unlock(&dev->struct_mutex);
 		return PTR_ERR(ctx);
 	}
-- 
1.8.5.1

Re: [PATCH] drm/i915/ppgtt: Prevent NULL deref in reset ioctl

[Daniel Vetter]

On Fri, Dec 20, 2013 at 12:22 AM, Ben Widawsky
<benjamin.widawsky@intel.com> wrote:
>         ctx = i915_gem_context_get(file->driver_priv, args->ctx_id);
> -       if (IS_ERR(ctx)) {
> +       if (IS_ERR_OR_NULL(ctx)) {

We now have half the callers check for IS_ERR and the others not.
Afaics i915_gem_context_get can only return NULL or a real context
though. Also from a quite read the expected error for a lookup failure
is ENOENT (and there seems to be a testcase for this).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915/ppgtt: Prevent NULL deref in reset ioctl

[Ben Widawsky]

On Fri, Dec 20, 2013 at 07:05:10AM +0100, Daniel Vetter wrote:
> On Fri, Dec 20, 2013 at 12:22 AM, Ben Widawsky
> <benjamin.widawsky@intel.com> wrote:
> >         ctx = i915_gem_context_get(file->driver_priv, args->ctx_id);
> > -       if (IS_ERR(ctx)) {
> > +       if (IS_ERR_OR_NULL(ctx)) {
> 
> We now have half the callers check for IS_ERR and the others not.
> Afaics i915_gem_context_get can only return NULL or a real context
> though. Also from a quite read the expected error for a lookup failure
> is ENOENT (and there seems to be a testcase for this).
> -Daniel


To your first point:
I think checking null is always the right thing currently, but for
future proofing, IS_ERR_OR_NULL is really the right thing. After his
patch, I believe only i915_gem_context_destroy_ioctl is still incorrect.


To the second:
This is only based on my memory, so feel free to change whatever you
need. When I retuned -ENXIO, the test failed.

It should be:
return ctx ? PTR_ERR(ctx) : -ENOENT;

I had
return ctx ? PTR_ERR(ctx) : -ENXIO;
which made the subtest fail. However, as we've noted, this itself was
not correct. Try the return above.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915/ppgtt: Prevent NULL deref in reset ioctl

[Daniel Vetter]

On Thu, Dec 19, 2013 at 10:55:56PM -0800, Ben Widawsky wrote:
> On Fri, Dec 20, 2013 at 07:05:10AM +0100, Daniel Vetter wrote:
> > On Fri, Dec 20, 2013 at 12:22 AM, Ben Widawsky
> > <benjamin.widawsky@intel.com> wrote:
> > >         ctx = i915_gem_context_get(file->driver_priv, args->ctx_id);
> > > -       if (IS_ERR(ctx)) {
> > > +       if (IS_ERR_OR_NULL(ctx)) {
> > 
> > We now have half the callers check for IS_ERR and the others not.
> > Afaics i915_gem_context_get can only return NULL or a real context
> > though. Also from a quite read the expected error for a lookup failure
> > is ENOENT (and there seems to be a testcase for this).
> > -Daniel
> 
> 
> To your first point:
> I think checking null is always the right thing currently, but for
> future proofing, IS_ERR_OR_NULL is really the right thing. After his
> patch, I believe only i915_gem_context_destroy_ioctl is still incorrect.

Using IS_ERR_OR_NULL on a return value which can never contain an
encoded errno value is imo confusing, more so when it's inconsitently
applied.

> To the second:
> This is only based on my memory, so feel free to change whatever you
> need. When I retuned -ENXIO, the test failed.
> 
> It should be:
> return ctx ? PTR_ERR(ctx) : -ENOENT;

On a cursory read that's been the semantics before your patch. And there
seems to be a testcase in gem_reset_stat for this, have you run all
subtests?

> I had
> return ctx ? PTR_ERR(ctx) : -ENXIO;
> which made the subtest fail. However, as we've noted, this itself was
> not correct. Try the return above.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915/ppgtt: Prevent NULL deref in reset ioctl

[Jani Nikula]

On Fri, 20 Dec 2013, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Dec 19, 2013 at 10:55:56PM -0800, Ben Widawsky wrote:
>> On Fri, Dec 20, 2013 at 07:05:10AM +0100, Daniel Vetter wrote:
>> > On Fri, Dec 20, 2013 at 12:22 AM, Ben Widawsky
>> > <benjamin.widawsky@intel.com> wrote:
>> > >         ctx = i915_gem_context_get(file->driver_priv, args->ctx_id);
>> > > -       if (IS_ERR(ctx)) {
>> > > +       if (IS_ERR_OR_NULL(ctx)) {
>> > 
>> > We now have half the callers check for IS_ERR and the others not.
>> > Afaics i915_gem_context_get can only return NULL or a real context
>> > though. Also from a quite read the expected error for a lookup failure
>> > is ENOENT (and there seems to be a testcase for this).
>> > -Daniel
>> 
>> 
>> To your first point:
>> I think checking null is always the right thing currently, but for
>> future proofing, IS_ERR_OR_NULL is really the right thing. After his
>> patch, I believe only i915_gem_context_destroy_ioctl is still incorrect.
>
> Using IS_ERR_OR_NULL on a return value which can never contain an
> encoded errno value is imo confusing, more so when it's inconsitently
> applied.

Moreover, the error path then goes on to return 0 for success when the
pointer is NULL.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm/i915/ppgtt: Prevent NULL deref in reset ioctl

[Ben Widawsky]

On Fri, Dec 20, 2013 at 04:37:34PM +0200, Jani Nikula wrote:
> On Fri, 20 Dec 2013, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Dec 19, 2013 at 10:55:56PM -0800, Ben Widawsky wrote:
> >> On Fri, Dec 20, 2013 at 07:05:10AM +0100, Daniel Vetter wrote:
> >> > On Fri, Dec 20, 2013 at 12:22 AM, Ben Widawsky
> >> > <benjamin.widawsky@intel.com> wrote:
> >> > >         ctx = i915_gem_context_get(file->driver_priv, args->ctx_id);
> >> > > -       if (IS_ERR(ctx)) {
> >> > > +       if (IS_ERR_OR_NULL(ctx)) {
> >> > 
> >> > We now have half the callers check for IS_ERR and the others not.
> >> > Afaics i915_gem_context_get can only return NULL or a real context
> >> > though. Also from a quite read the expected error for a lookup failure
> >> > is ENOENT (and there seems to be a testcase for this).
> >> > -Daniel
> >> 
> >> 
> >> To your first point:
> >> I think checking null is always the right thing currently, but for
> >> future proofing, IS_ERR_OR_NULL is really the right thing. After his
> >> patch, I believe only i915_gem_context_destroy_ioctl is still incorrect.
> >
> > Using IS_ERR_OR_NULL on a return value which can never contain an
> > encoded errno value is imo confusing, more so when it's inconsitently
> > applied.
> 
> Moreover, the error path then goes on to return 0 for success when the
> pointer is NULL.
> 

That was mentioned in the commit message.

> BR,
> Jani.
> 
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915/ppgtt: Prevent NULL deref in reset ioctl

[Ben Widawsky]

On Fri, Dec 20, 2013 at 03:11:09PM +0100, Daniel Vetter wrote:
> On Thu, Dec 19, 2013 at 10:55:56PM -0800, Ben Widawsky wrote:
> > On Fri, Dec 20, 2013 at 07:05:10AM +0100, Daniel Vetter wrote:
> > > On Fri, Dec 20, 2013 at 12:22 AM, Ben Widawsky
> > > <benjamin.widawsky@intel.com> wrote:
> > > >         ctx = i915_gem_context_get(file->driver_priv, args->ctx_id);
> > > > -       if (IS_ERR(ctx)) {
> > > > +       if (IS_ERR_OR_NULL(ctx)) {
> > > 
> > > We now have half the callers check for IS_ERR and the others not.
> > > Afaics i915_gem_context_get can only return NULL or a real context
> > > though. Also from a quite read the expected error for a lookup failure
> > > is ENOENT (and there seems to be a testcase for this).
> > > -Daniel
> > 
> > 
> > To your first point:
> > I think checking null is always the right thing currently, but for
> > future proofing, IS_ERR_OR_NULL is really the right thing. After his
> > patch, I believe only i915_gem_context_destroy_ioctl is still incorrect.
> 
> Using IS_ERR_OR_NULL on a return value which can never contain an
> encoded errno value is imo confusing, more so when it's inconsitently
> applied.
> 
> > To the second:
> > This is only based on my memory, so feel free to change whatever you
> > need. When I retuned -ENXIO, the test failed.
> > 
> > It should be:
> > return ctx ? PTR_ERR(ctx) : -ENOENT;
> 
> On a cursory read that's been the semantics before your patch. And there
> seems to be a testcase in gem_reset_stat for this, have you run all
> subtests?

As I mentioned on IRC, but I think you missed it - a later subtest hits
a really bad BUG (pre-ppgtt). It was all pass up to that subtest.

> 
> > I had
> > return ctx ? PTR_ERR(ctx) : -ENXIO;
> > which made the subtest fail. However, as we've noted, this itself was
> > not correct. Try the return above.
> 
> Thanks, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH] drm/i915/ppgtt: Never return a NULL context

[Ben Widawsky]

It makes all the code which calls into this function way too confusing.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=72903

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index ebe0f67..4794c18 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -526,10 +526,16 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
 struct i915_hw_context *
 i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
 {
+	struct i915_hw_context *ctx;
+
 	if (!HAS_HW_CONTEXTS(file_priv->dev_priv->dev))
 		return file_priv->private_default_ctx;
 
-	return (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
+	ctx = (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
+	if (!ctx)
+		return ERR_PTR(-ENOENT);
+
+	return ctx;
 }
 
 static inline int
-- 
1.8.5.2

[PATCH] [v2] drm/i915/ppgtt: Never return a NULL context

[Ben Widawsky]

It makes all the code which calls into this function way too confusing.

v2: Fix destroy IOCTL as well

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=72903
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index a90a0a1..f3ac9a2 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -524,10 +524,16 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
 struct i915_hw_context *
 i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
 {
+	struct i915_hw_context *ctx;
+
 	if (!HAS_HW_CONTEXTS(file_priv->dev_priv->dev))
 		return file_priv->private_default_ctx;
 
-	return (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
+	ctx = (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
+	if (!ctx)
+		return ERR_PTR(-ENOENT);
+
+	return ctx;
 }
 
 static inline int
@@ -774,9 +780,9 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 		return ret;
 
 	ctx = i915_gem_context_get(file_priv, args->ctx_id);
-	if (!ctx) {
+	if (IS_ERR(ctx)) {
 		mutex_unlock(&dev->struct_mutex);
-		return -ENOENT;
+		return PTR_ERR(ctx);
 	}
 
 	idr_remove(&ctx->file_priv->context_idr, ctx->id);
-- 
1.8.5.2

Re: [PATCH] [v2] drm/i915/ppgtt: Never return a NULL context

[Mika Kuoppala]

Ben Widawsky <benjamin.widawsky@intel.com> writes:

> It makes all the code which calls into this function way too confusing.
>
> v2: Fix destroy IOCTL as well
>

callsite inside i915_gem_validate_context() and after it is called
in i915_gem_do_execbuffer() are still confusing as they seem to expect
NULL. Latter even tries to dig out error value from NULL.

-- Mika

> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=72903
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index a90a0a1..f3ac9a2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -524,10 +524,16 @@ void i915_gem_context_close(struct drm_device *dev, struct drm_file *file)
>  struct i915_hw_context *
>  i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
>  {
> +	struct i915_hw_context *ctx;
> +
>  	if (!HAS_HW_CONTEXTS(file_priv->dev_priv->dev))
>  		return file_priv->private_default_ctx;
>  
> -	return (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
> +	ctx = (struct i915_hw_context *)idr_find(&file_priv->context_idr, id);
> +	if (!ctx)
> +		return ERR_PTR(-ENOENT);
> +
> +	return ctx;
>  }
>  
>  static inline int
> @@ -774,9 +780,9 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
>  		return ret;
>  
>  	ctx = i915_gem_context_get(file_priv, args->ctx_id);
> -	if (!ctx) {
> +	if (IS_ERR(ctx)) {
>  		mutex_unlock(&dev->struct_mutex);
> -		return -ENOENT;
> +		return PTR_ERR(ctx);
>  	}
>  
>  	idr_remove(&ctx->file_priv->context_idr, ctx->id);
> -- 
> 1.8.5.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx