Messages in this thread Patch in this message | | | Date | Fri, 27 Jan 2023 13:50:49 +0000 | Subject | Re: [Intel-gfx] [PATCH] drm/i915: Avoid potential vm use-after-free | From | Tvrtko Ursulin <> |
| |
On 27/01/2023 13:10, Matthew Auld wrote: > On Mon, 23 Jan 2023 at 16:57, Tvrtko Ursulin > <tvrtko.ursulin@linux.intel.com> wrote: >> >> >> + some more people based on e1a7ab4fca0c >> >> On 19/01/2023 17:32, Rob Clark wrote: >>> From: Rob Clark <robdclark@chromium.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@chromium.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@intel.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@vger.kernel.org> # 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;
| |