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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 27 Jan 2023 13:10:58 +0000
From:   Matthew Auld <matthew.william.auld@...il.com>
To:     Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>
Cc:     Rob Clark <robdclark@...il.com>, dri-devel@...ts.freedesktop.org,
        Rob Clark <robdclark@...omium.org>,
        Thomas Hellström 
        <thomas.hellstrom@...ux.intel.com>,
        intel-gfx@...ts.freedesktop.org,
        open list <linux-kernel@...r.kernel.org>,
        Chris Wilson <chris@...is-wilson.co.uk>,
        Matthew Auld <matthew.auld@...el.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        David Airlie <airlied@...il.com>,
        katrinzhou <katrinzhou@...cent.com>
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Avoid potential vm use-after-free

On Mon, 23 Jan 2023 at 16:57, Tvrtko Ursulin
<tvrtko.ursulin@...ux.intel.com> wrote:
>
>
> + some more people based on e1a7ab4fca0c
>
> On 19/01/2023 17:32, Rob Clark wrote:
> > From: Rob Clark <robdclark@...omium.org>
> >
> > 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@...omium.org>
> > ---
> > 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@...el.com>

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@...r.kernel.org> # v5.16+

>
> 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;