| Message ID | 20220512094045.792373-1-thomas.hellstrom@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix vm use-after-free in vma destruction | expand |
Hi Thomas, On Thu, May 12, 2022 at 11:40:45AM +0200, Thomas Hellström wrote: > In vma destruction, the following race may occur: > > Thread 1: Thread 2: > i915_vma_destroy(); > > ... > list_del_init(vma->vm_link); > ... > mutex_unlock(vma->vm->mutex); > __i915_vm_release(); > release_references(); > > And in release_reference() we dereference vma->vm to get to the > vm gt pointer, leadin go a use-after free. leading to [...] > -static void release_references(struct i915_vma *vma, bool vm_ddestroy) > +static void release_references(struct i915_vma *vma, struct intel_gt *gt, > + bool vm_ddestroy) > { > struct drm_i915_gem_object *obj = vma->obj; > - struct intel_gt *gt = vma->vm->gt; > > GEM_BUG_ON(i915_vma_is_active(vma)); but then we have if (vm_ddestroy) i915_vm_resv_put(vma->vm); were we reference to a freed vm, right? Do we need to check it here, as well? Andi
Hi, Andi On 5/19/22 23:46, Andi Shyti wrote: > Hi Thomas, > > On Thu, May 12, 2022 at 11:40:45AM +0200, Thomas Hellström wrote: >> In vma destruction, the following race may occur: >> >> Thread 1: Thread 2: >> i915_vma_destroy(); >> >> ... >> list_del_init(vma->vm_link); >> ... >> mutex_unlock(vma->vm->mutex); >> __i915_vm_release(); >> release_references(); >> >> And in release_reference() we dereference vma->vm to get to the >> vm gt pointer, leadin go a use-after free. > leading to Thanks, will fix. > > [...] > >> -static void release_references(struct i915_vma *vma, bool vm_ddestroy) >> +static void release_references(struct i915_vma *vma, struct intel_gt *gt, >> + bool vm_ddestroy) >> { >> struct drm_i915_gem_object *obj = vma->obj; >> - struct intel_gt *gt = vma->vm->gt; >> >> GEM_BUG_ON(i915_vma_is_active(vma)); > but then we have > > if (vm_ddestroy) > i915_vm_resv_put(vma->vm); > > were we reference to a freed vm, right? Do we need to check it > here, as well? No, it's not needed, since if vm_ddestroy is true, we keep the vm alive using the vm resv_ref until i915_vm_resv_put(). This is for the rare occasions where, during vm destruction, we fail to grab an object reference and therefore vma destruction is left for the object destructor. In those cases the vma needs to keep the vm in memory for it to be able to grab the vm mutex. /Thomas > > Andi
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index 4f6db539571a..5d6850a91a69 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -1636,10 +1636,10 @@ static void force_unbind(struct i915_vma *vma) GEM_BUG_ON(drm_mm_node_allocated(&vma->node)); } -static void release_references(struct i915_vma *vma, bool vm_ddestroy) +static void release_references(struct i915_vma *vma, struct intel_gt *gt, + bool vm_ddestroy) { struct drm_i915_gem_object *obj = vma->obj; - struct intel_gt *gt = vma->vm->gt; GEM_BUG_ON(i915_vma_is_active(vma)); @@ -1694,11 +1694,12 @@ void i915_vma_destroy_locked(struct i915_vma *vma) force_unbind(vma); list_del_init(&vma->vm_link); - release_references(vma, false); + release_references(vma, vma->vm->gt, false); } void i915_vma_destroy(struct i915_vma *vma) { + struct intel_gt *gt; bool vm_ddestroy; mutex_lock(&vma->vm->mutex); @@ -1706,8 +1707,11 @@ void i915_vma_destroy(struct i915_vma *vma) list_del_init(&vma->vm_link); vm_ddestroy = vma->vm_ddestroy; vma->vm_ddestroy = false; + + /* vma->vm may be freed when releasing vma->vm->mutex. */ + gt = vma->vm->gt; mutex_unlock(&vma->vm->mutex); - release_references(vma, vm_ddestroy); + release_references(vma, gt, vm_ddestroy); } void i915_vma_parked(struct intel_gt *gt)
In vma destruction, the following race may occur: Thread 1: Thread 2: i915_vma_destroy(); ... list_del_init(vma->vm_link); ... mutex_unlock(vma->vm->mutex); __i915_vm_release(); release_references(); And in release_reference() we dereference vma->vm to get to the vm gt pointer, leadin go a use-after free. However, __i915_vm_release() grabs the vm->mutex so the vm won't be destroyed before vma->vm->mutex is released, so extract the gt pointer under the vm->mutex to avoid the vma->vm dereference in release_references(). Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5944 Fixes: e1a7ab4fca ("drm/i915: Remove the vm open count") Cc: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com> Cc: Matthew Auld <matthew.auld@intel.com> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> --- drivers/gpu/drm/i915/i915_vma.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)