[PATCH 1/3] drm/xe/vm: prevent UAF with asid based lookup

[Lucas De Marchi]

On Fri, Apr 12, 2024 at 12:31:45PM +0100, Matthew Auld wrote:
>The asid is only erased from the xarray when the vm refcount reaches
>zero, however this leads to potential UAF since the xe_vm_get() only

I'm not sure I understand the call chain an where xe_vm_get() is coming
into play here.


>works on a vm with refcount != 0. Since the asid is allocated in the vm
>create ioctl, rather erase it when closing the vm, prior to dropping the
>potential last ref. This should also work when user closes driver fd
>without explicit vm destroy.

what seems weird is that you are moving it earlier in the call stack
rather than later, outside of the worker, to prevent the UAF.

what exactly was the UAF on?

Lucas De Marchi

>
>Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
>Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1594
>Signed-off-by: Matthew Auld <matthew.auld at intel.com>
>Cc: Matthew Brost <matthew.brost at intel.com>
>Cc: <stable at vger.kernel.org> # v6.8+
>---
> drivers/gpu/drm/xe/xe_vm.c | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>index a196dbe65252..c5c26b3d1b76 100644
>--- a/drivers/gpu/drm/xe/xe_vm.c
>+++ b/drivers/gpu/drm/xe/xe_vm.c
>@@ -1581,6 +1581,16 @@ void xe_vm_close_and_put(struct xe_vm *vm)
> 		xe->usm.num_vm_in_fault_mode--;
> 	else if (!(vm->flags & XE_VM_FLAG_MIGRATION))
> 		xe->usm.num_vm_in_non_fault_mode--;
>+
>+	if (vm->usm.asid) {
>+		void *lookup;
>+
>+		xe_assert(xe, xe->info.has_asid);
>+		xe_assert(xe, !(vm->flags & XE_VM_FLAG_MIGRATION));
>+
>+		lookup = xa_erase(&xe->usm.asid_to_vm, vm->usm.asid);
>+		xe_assert(xe, lookup == vm);
>+	}
> 	mutex_unlock(&xe->usm.lock);
>
> 	for_each_tile(tile, xe, id)
>@@ -1596,24 +1606,15 @@ static void vm_destroy_work_func(struct work_struct *w)
> 	struct xe_device *xe = vm->xe;
> 	struct xe_tile *tile;
> 	u8 id;
>-	void *lookup;
>
> 	/* xe_vm_close_and_put was not called? */
> 	xe_assert(xe, !vm->size);
>
> 	mutex_destroy(&vm->snap_mutex);
>
>-	if (!(vm->flags & XE_VM_FLAG_MIGRATION)) {
>+	if (!(vm->flags & XE_VM_FLAG_MIGRATION))
> 		xe_device_mem_access_put(xe);
>
>-		if (xe->info.has_asid && vm->usm.asid) {
>-			mutex_lock(&xe->usm.lock);
>-			lookup = xa_erase(&xe->usm.asid_to_vm, vm->usm.asid);
>-			xe_assert(xe, lookup == vm);
>-			mutex_unlock(&xe->usm.lock);
>-		}
>-	}
>-
> 	for_each_tile(tile, xe, id)
> 		XE_WARN_ON(vm->pt_root[id]);
>
>-- 
>2.44.0
>