| Message ID | 20200123152602.1432282-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/gem: Prevent NULL pointer dereference on missing ctx->vm | expand |
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
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
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;
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(-)