[PATCH] drm/i915: Prevent use-after-free in invalidate_range_start callback

[PATCH] drm/i915: Prevent use-after-free in invalidate_range_start callback

[Michał Winiarski]

It's possible for invalidate_range_start mmu notifier callback to race
against userptr object release. If the gem object was released prior to
obtaining the spinlock in invalidate_range_start we're hitting null
pointer dereference.

Testcase: igt/gem_userptr_blits/stress-mm-invalidate-close
Testcase: igt/gem_userptr_blits/stress-mm-invalidate-close-overlap
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_userptr.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index d182058..64b8802 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -113,7 +113,10 @@ restart:
 			continue;
 
 		obj = mo->obj;
-		drm_gem_object_reference(&obj->base);
+
+		if (!kref_get_unless_zero(&obj->base.refcount))
+			continue;
+
 		spin_unlock(&mn->lock);
 
 		cancel_userptr(obj);
@@ -149,7 +152,13 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
 			it = interval_tree_iter_first(&mn->objects, start, end);
 		if (it != NULL) {
 			obj = container_of(it, struct i915_mmu_object, it)->obj;
-			drm_gem_object_reference(&obj->base);
+
+			if (!kref_get_unless_zero(&obj->base.refcount)) {
+				spin_unlock(&mn->lock);
+				serial = 0;
+				continue;
+			}
+
 			serial = mn->serial;
 		}
 		spin_unlock(&mn->lock);
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Prevent use-after-free in invalidate_range_start callback

[Chris Wilson]

On Tue, Feb 03, 2015 at 03:48:17PM +0100, Michał Winiarski wrote:
> It's possible for invalidate_range_start mmu notifier callback to race
> against userptr object release. If the gem object was released prior to
> obtaining the spinlock in invalidate_range_start we're hitting null
> pointer dereference.
> 
> Testcase: igt/gem_userptr_blits/stress-mm-invalidate-close*
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Though I would personally remove the extra newline and add an extra comment instead:

> ---
>  drivers/gpu/drm/i915/i915_gem_userptr.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index d182058..64b8802 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -113,7 +113,10 @@ restart:
>  			continue;
>  
>  		obj = mo->obj;
> -		drm_gem_object_reference(&obj->base);
> +		if (!kref_get_unless_zero(&obj->base.refcount))
> +			continue;
> +
>  		spin_unlock(&mn->lock);
>  
>  		cancel_userptr(obj);
> @@ -149,7 +152,13 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>  			it = interval_tree_iter_first(&mn->objects, start, end);
>  		if (it != NULL) {
>  			obj = container_of(it, struct i915_mmu_object, it)->obj;
> -			drm_gem_object_reference(&obj->base);
			/* The mmu_object is released late when
			 * destroying the GEM object so it is entirely
			 * possible to gain a reference on an object
			 * in the process of being freed since our
			 * serialisation is via the spinlock and not the
			 * struct_mutex - and consequently use it
			 * after it is freed and then double free it.
			 */
> +			if (!kref_get_unless_zero(&obj->base.refcount)) {
> +				spin_unlock(&mn->lock);
> +				serial = 0;
> +				continue;
> +			}
> +
>  			serial = mn->serial;
>  		}
>  		spin_unlock(&mn->lock);
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Prevent use-after-free in invalidate_range_start callback

[Daniel Vetter]

On Tue, Feb 03, 2015 at 03:08:17PM +0000, Chris Wilson wrote:
> On Tue, Feb 03, 2015 at 03:48:17PM +0100, Michał Winiarski wrote:
> > It's possible for invalidate_range_start mmu notifier callback to race
> > against userptr object release. If the gem object was released prior to
> > obtaining the spinlock in invalidate_range_start we're hitting null
> > pointer dereference.
> > 
> > Testcase: igt/gem_userptr_blits/stress-mm-invalidate-close*
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Since it blows up in the real world already also

Cc: stable@vger.kernel.org

and for Jani. And Jani can add the comment while applying, I like it -
explaining that kind of weak ref stuff is always good.
-Daniel

> 
> Though I would personally remove the extra newline and add an extra comment instead:
> 
> > ---
> >  drivers/gpu/drm/i915/i915_gem_userptr.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> > index d182058..64b8802 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> > @@ -113,7 +113,10 @@ restart:
> >  			continue;
> >  
> >  		obj = mo->obj;
> > -		drm_gem_object_reference(&obj->base);
> > +		if (!kref_get_unless_zero(&obj->base.refcount))
> > +			continue;
> > +
> >  		spin_unlock(&mn->lock);
> >  
> >  		cancel_userptr(obj);
> > @@ -149,7 +152,13 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
> >  			it = interval_tree_iter_first(&mn->objects, start, end);
> >  		if (it != NULL) {
> >  			obj = container_of(it, struct i915_mmu_object, it)->obj;
> > -			drm_gem_object_reference(&obj->base);
> 			/* The mmu_object is released late when
> 			 * destroying the GEM object so it is entirely
> 			 * possible to gain a reference on an object
> 			 * in the process of being freed since our
> 			 * serialisation is via the spinlock and not the
> 			 * struct_mutex - and consequently use it
> 			 * after it is freed and then double free it.
> 			 */
> > +			if (!kref_get_unless_zero(&obj->base.refcount)) {
> > +				spin_unlock(&mn->lock);
> > +				serial = 0;
> > +				continue;
> > +			}
> > +
> >  			serial = mn->serial;
> >  		}
> >  		spin_unlock(&mn->lock);
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Prevent use-after-free in invalidate_range_start callback

[Jani Nikula]

On Tue, 03 Feb 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Feb 03, 2015 at 03:08:17PM +0000, Chris Wilson wrote:
>> On Tue, Feb 03, 2015 at 03:48:17PM +0100, Michał Winiarski wrote:
>> > It's possible for invalidate_range_start mmu notifier callback to race
>> > against userptr object release. If the gem object was released prior to
>> > obtaining the spinlock in invalidate_range_start we're hitting null
>> > pointer dereference.
>> > 
>> > Testcase: igt/gem_userptr_blits/stress-mm-invalidate-close*
>> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> Since it blows up in the real world already also
>
> Cc: stable@vger.kernel.org
>
> and for Jani. And Jani can add the comment while applying, I like it -
> explaining that kind of weak ref stuff is always good.

Pushed to drm-intel-next-fixes (and therefore headed for 3.20) with
Chris' comment added.

BR,
Jani.


> -Daniel
>
>> 
>> Though I would personally remove the extra newline and add an extra comment instead:
>> 
>> > ---
>> >  drivers/gpu/drm/i915/i915_gem_userptr.c | 13 +++++++++++--
>> >  1 file changed, 11 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
>> > index d182058..64b8802 100644
>> > --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
>> > +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
>> > @@ -113,7 +113,10 @@ restart:
>> >  			continue;
>> >  
>> >  		obj = mo->obj;
>> > -		drm_gem_object_reference(&obj->base);
>> > +		if (!kref_get_unless_zero(&obj->base.refcount))
>> > +			continue;
>> > +
>> >  		spin_unlock(&mn->lock);
>> >  
>> >  		cancel_userptr(obj);
>> > @@ -149,7 +152,13 @@ static void i915_gem_userptr_mn_invalidate_range_start(struct mmu_notifier *_mn,
>> >  			it = interval_tree_iter_first(&mn->objects, start, end);
>> >  		if (it != NULL) {
>> >  			obj = container_of(it, struct i915_mmu_object, it)->obj;
>> > -			drm_gem_object_reference(&obj->base);
>> 			/* The mmu_object is released late when
>> 			 * destroying the GEM object so it is entirely
>> 			 * possible to gain a reference on an object
>> 			 * in the process of being freed since our
>> 			 * serialisation is via the spinlock and not the
>> 			 * struct_mutex - and consequently use it
>> 			 * after it is freed and then double free it.
>> 			 */
>> > +			if (!kref_get_unless_zero(&obj->base.refcount)) {
>> > +				spin_unlock(&mn->lock);
>> > +				serial = 0;
>> > +				continue;
>> > +			}
>> > +
>> >  			serial = mn->serial;
>> >  		}
>> >  		spin_unlock(&mn->lock);
>> -Chris
>> 
>> -- 
>> Chris Wilson, Intel Open Source Technology Centre
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx