Re: [Intel-gfx] [PATCH] drm/i915: Avoid potential vm use-after-free

[Tvrtko Ursulin]

On 27/01/2023 13:10, Matthew Auld wrote:
> On Mon, 23 Jan 2023 at 16:57, Tvrtko Ursulin
> <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
> > + some more people based on e1a7ab4fca0c
> >
> > On 19/01/2023 17:32, Rob Clark wrote:
> > > From: Rob Clark <robdclark@xxxxxxxxxxxx>
> > >
> > > Adding the vm to the vm_xa table makes it visible to userspace, which
> > > could try to race with us to close the vm.  So we need to take our extra
> > > reference before putting it in the table.
> > >
> > > Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
> > > ---
> > > Note, you could list commit e1a7ab4fca0c ("drm/i915: Remove the vm open
> > > count") as the "fixed" commit, but really the issue seems to go back
> > > much further (with the fix needing some backporting in the process).
> >
> > It would probably be rather essential to identify the correct Fixes: tag.
> >
> > Since Thomas, Matt and Niranjana you were directly involved in the patch
> > which changed significantly how this works, perhaps there is something
> > still somewhat easily retrievable from your memory lanes to help with this?
>
> Sorry for the delay. Fix looks good to me,
> Reviewed-by: Matthew Auld <matthew.auld@xxxxxxxxx>
>
> Looking at the git history, the fixes tag I think needs to be:
>
> Fixes: 9ec8795e7d91 ("drm/i915: Drop __rcu from gem_context->vm")
> Cc: <stable@xxxxxxxxxxxxxxx> # v5.16+

As discussed offline this looks correct to me too. Thanks for looking 
into it!

Since the CI was green I have now merged the patch. Thanks for the fix Rob!

Regards,

Tvrtko

P.S. Backport to kernels which do not contain e1a7ab4fca0c ("drm/i915: 
Remove the vm open count"), so 5.16 to 5.18, will require a slightly 
different patch as Matt has also mentioned offline.

> > Regards,
> >
> > Tvrtko
> >
> > >    drivers/gpu/drm/i915/gem/i915_gem_context.c | 14 +++++++++++---
> > >    1 file changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > index 6250de9b9196..e4b78ab4773b 100644
> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> > > @@ -1861,11 +1861,19 @@ static int get_ppgtt(struct drm_i915_file_private *file_priv,
> > >        vm = ctx->vm;
> > >        GEM_BUG_ON(!vm);
> > >
> > > +     /*
> > > +      * Get a reference for the allocated handle.  Once the handle is
> > > +      * visible in the vm_xa table, userspace could try to close it
> > > +      * from under our feet, so we need to hold the extra reference
> > > +      * first.
> > > +      */
> > > +     i915_vm_get(vm);
> > > +
> > >        err = xa_alloc(&file_priv->vm_xa, &id, vm, xa_limit_32b, GFP_KERNEL);
> > > -     if (err)
> > > +     if (err) {
> > > +             i915_vm_put(vm);
> > >                return err;
> > > -
> > > -     i915_vm_get(vm);
> > > +     }
> > >
> > >        GEM_BUG_ON(id == 0); /* reserved for invalid/unassigned ppgtt */
> > >        args->value = id;