[PATCH] drm/i915: fix use-after-free in i915_drop_caches_set

[PATCH] drm/i915: fix use-after-free in i915_drop_caches_set

[Michel Thierry]

With the new vma/ppgtt lifetime rules, the ppgtt (vm) could be removed
after i915_vma_unbind.

Use list_for_each_entry_safe() to prevent this use-after-free.

Found with gem_persistent_relocs and gem_evict_everything igt tests.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Michel Thierry <michel.thierry@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index dd736c0..4b05cd8 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3820,8 +3820,8 @@ i915_drop_caches_set(void *data, u64 val)
 	struct drm_device *dev = data;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj, *next;
-	struct i915_address_space *vm;
-	struct i915_vma *vma, *x;
+	struct i915_address_space *vm, *x;
+	struct i915_vma *vma, *y;
 	int ret;
 
 	DRM_DEBUG("Dropping caches: 0x%08llx\n", val);
@@ -3842,8 +3842,9 @@ i915_drop_caches_set(void *data, u64 val)
 		i915_gem_retire_requests(dev);
 
 	if (val & DROP_BOUND) {
-		list_for_each_entry(vm, &dev_priv->vm_list, global_link) {
-			list_for_each_entry_safe(vma, x, &vm->inactive_list,
+		list_for_each_entry_safe(vm, x, &dev_priv->vm_list,
+				global_link) {
+			list_for_each_entry_safe(vma, y, &vm->inactive_list,
 						 mm_list) {
 				if (vma->pin_count)
 					continue;
-- 
2.0.3

Re: [PATCH] drm/i915: fix use-after-free in i915_drop_caches_set

[Daniel Vetter]

On Fri, Sep 05, 2014 at 05:53:23PM +0100, Michel Thierry wrote:
> With the new vma/ppgtt lifetime rules, the ppgtt (vm) could be removed
> after i915_vma_unbind.
> 
> Use list_for_each_entry_safe() to prevent this use-after-free.
> 
> Found with gem_persistent_relocs and gem_evict_everything igt tests.
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>

I've merged Chris' version of this already, which additional also grabs a
temporary refcount to make sure the bo doesn't disappear untimely. Which
can happen if unbind ends up dropping the last ref when dropping the
active reference.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index dd736c0..4b05cd8 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3820,8 +3820,8 @@ i915_drop_caches_set(void *data, u64 val)
>  	struct drm_device *dev = data;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_object *obj, *next;
> -	struct i915_address_space *vm;
> -	struct i915_vma *vma, *x;
> +	struct i915_address_space *vm, *x;
> +	struct i915_vma *vma, *y;
>  	int ret;
>  
>  	DRM_DEBUG("Dropping caches: 0x%08llx\n", val);
> @@ -3842,8 +3842,9 @@ i915_drop_caches_set(void *data, u64 val)
>  		i915_gem_retire_requests(dev);
>  
>  	if (val & DROP_BOUND) {
> -		list_for_each_entry(vm, &dev_priv->vm_list, global_link) {
> -			list_for_each_entry_safe(vma, x, &vm->inactive_list,
> +		list_for_each_entry_safe(vm, x, &dev_priv->vm_list,
> +				global_link) {
> +			list_for_each_entry_safe(vma, y, &vm->inactive_list,
>  						 mm_list) {
>  				if (vma->pin_count)
>  					continue;
> -- 
> 2.0.3
> 

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