drm/i915/gem: Prevent NULL pointer dereference on missing ctx->vm
diff mbox series

Message ID 20200123152602.1432282-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • drm/i915/gem: Prevent NULL pointer dereference on missing ctx->vm
Related show

Commit Message

Chris Wilson Jan. 23, 2020, 3:26 p.m. UTC
If the ctx->vm is freed before we can acquire a local reference to it,
we proceed to call i915_vm_put(NULL), which is invalid.

Reported-by: Colin Ian King <colin.king@canonical.com>
Fixes: 5dbd2b7be61e ("drm/i915/gem: Convert vm idr to xarray")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Colin Ian King <colin.king@canonical.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Colin King Jan. 23, 2020, 3:27 p.m. UTC | #1
On 23/01/2020 15:26, Chris Wilson wrote:
> If the ctx->vm is freed before we can acquire a local reference to it,
> we proceed to call i915_vm_put(NULL), which is invalid.
> 
> Reported-by: Colin Ian King <colin.king@canonical.com>
> Fixes: 5dbd2b7be61e ("drm/i915/gem: Convert vm idr to xarray")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_context.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 5d4157e1ccf7..8873db5be123 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -1002,13 +1002,14 @@ static int get_ppgtt(struct drm_i915_file_private *file_priv,
>  	if (!rcu_access_pointer(ctx->vm))
>  		return -ENODEV;
>  
> -	err = -ENODEV;
>  	rcu_read_lock();
>  	vm = context_get_vm_rcu(ctx);
> -	if (vm)
> -		err = xa_alloc(&file_priv->vm_xa, &id, vm,
> -			       xa_limit_32b, GFP_KERNEL);
>  	rcu_read_unlock();
> +	if (!vm)
> +		return -ENODEV;
> +
> +	err = xa_alloc(&file_priv->vm_xa, &id, vm,
> +		       xa_limit_32b, GFP_KERNEL);
>  	if (err)
>  		goto err_put;
>  
> 
Ah, yes, that's definitely an improvement on my fix. Thanks Chris.

Colin
Joonas Lahtinen Jan. 24, 2020, 10:17 a.m. UTC | #2
Quoting Chris Wilson (2020-01-23 17:26:02)
> If the ctx->vm is freed before we can acquire a local reference to it,
> we proceed to call i915_vm_put(NULL), which is invalid.
> 
> Reported-by: Colin Ian King <colin.king@canonical.com>
> Fixes: 5dbd2b7be61e ("drm/i915/gem: Convert vm idr to xarray")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_context.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 5d4157e1ccf7..8873db5be123 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -1002,13 +1002,14 @@ static int get_ppgtt(struct drm_i915_file_private *file_priv,
>         if (!rcu_access_pointer(ctx->vm))
>                 return -ENODEV;
>  
> -       err = -ENODEV;
>         rcu_read_lock();
>         vm = context_get_vm_rcu(ctx);
> -       if (vm)
> -               err = xa_alloc(&file_priv->vm_xa, &id, vm,
> -                              xa_limit_32b, GFP_KERNEL);
>         rcu_read_unlock();
> +       if (!vm)
> +               return -ENODEV;
> +
> +       err = xa_alloc(&file_priv->vm_xa, &id, vm,
> +                      xa_limit_32b, GFP_KERNEL);

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 5d4157e1ccf7..8873db5be123 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1002,13 +1002,14 @@  static int get_ppgtt(struct drm_i915_file_private *file_priv,
 	if (!rcu_access_pointer(ctx->vm))
 		return -ENODEV;
 
-	err = -ENODEV;
 	rcu_read_lock();
 	vm = context_get_vm_rcu(ctx);
-	if (vm)
-		err = xa_alloc(&file_priv->vm_xa, &id, vm,
-			       xa_limit_32b, GFP_KERNEL);
 	rcu_read_unlock();
+	if (!vm)
+		return -ENODEV;
+
+	err = xa_alloc(&file_priv->vm_xa, &id, vm,
+		       xa_limit_32b, GFP_KERNEL);
 	if (err)
 		goto err_put;