[PATCH 1/2] drm/xe/vm: move xa_alloc to prevent UAF

[PATCH 1/2] drm/xe/vm: move xa_alloc to prevent UAF

[Matthew Auld]

Evil user can guess the next id of the vm before the ioctl completes and
then call vm destroy ioctl to trigger UAF since create ioctl is still
referencing the same vm. Move the xa_alloc all the way to the end to
prevent this.

Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: <stable@vger.kernel.org> # v6.8+
---
 drivers/gpu/drm/xe/xe_vm.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index a3d7cb7cfd22..f7182ef3d8e6 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -1765,12 +1765,6 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
 	if (IS_ERR(vm))
 		return PTR_ERR(vm);
 
-	mutex_lock(&xef->vm.lock);
-	err = xa_alloc(&xef->vm.xa, &id, vm, xa_limit_32b, GFP_KERNEL);
-	mutex_unlock(&xef->vm.lock);
-	if (err)
-		goto err_close_and_put;
-
 	if (xe->info.has_asid) {
 		down_write(&xe->usm.lock);
 		err = xa_alloc_cyclic(&xe->usm.asid_to_vm, &asid, vm,
@@ -1778,12 +1772,11 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
 				      &xe->usm.next_asid, GFP_KERNEL);
 		up_write(&xe->usm.lock);
 		if (err < 0)
-			goto err_free_id;
+			goto err_close_and_put;
 
 		vm->usm.asid = asid;
 	}
 
-	args->vm_id = id;
 	vm->xef = xe_file_get(xef);
 
 	/* Record BO memory for VM pagetable created against client */
@@ -1796,12 +1789,17 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
 	args->reserved[0] = xe_bo_main_addr(vm->pt_root[0]->bo, XE_PAGE_SIZE);
 #endif
 
-	return 0;
-
-err_free_id:
+	/* user id alloc must always be last in ioctl to prevent UAF */
 	mutex_lock(&xef->vm.lock);
-	xa_erase(&xef->vm.xa, id);
+	err = xa_alloc(&xef->vm.xa, &id, vm, xa_limit_32b, GFP_KERNEL);
 	mutex_unlock(&xef->vm.lock);
+	if (err)
+		goto err_close_and_put;
+
+	args->vm_id = id;
+
+	return 0;
+
 err_close_and_put:
 	xe_vm_close_and_put(vm);
 
-- 
2.46.1

Re: [PATCH 1/2] drm/xe/vm: move xa_alloc to prevent UAF

[Matthew Brost]

On Mon, Sep 23, 2024 at 01:57:34PM +0100, Matthew Auld wrote:
> Evil user can guess the next id of the vm before the ioctl completes and
> then call vm destroy ioctl to trigger UAF since create ioctl is still
> referencing the same vm. Move the xa_alloc all the way to the end to
> prevent this.
> 

I think this is correct. Shall I merge this series [1] first and then
you can rebase?

Matt

[1] https://patchwork.freedesktop.org/patch/615303/?series=138938&rev=1

> Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: <stable@vger.kernel.org> # v6.8+
> ---
>  drivers/gpu/drm/xe/xe_vm.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index a3d7cb7cfd22..f7182ef3d8e6 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1765,12 +1765,6 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
>  	if (IS_ERR(vm))
>  		return PTR_ERR(vm);
>  
> -	mutex_lock(&xef->vm.lock);
> -	err = xa_alloc(&xef->vm.xa, &id, vm, xa_limit_32b, GFP_KERNEL);
> -	mutex_unlock(&xef->vm.lock);
> -	if (err)
> -		goto err_close_and_put;
> -
>  	if (xe->info.has_asid) {
>  		down_write(&xe->usm.lock);
>  		err = xa_alloc_cyclic(&xe->usm.asid_to_vm, &asid, vm,
> @@ -1778,12 +1772,11 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
>  				      &xe->usm.next_asid, GFP_KERNEL);
>  		up_write(&xe->usm.lock);
>  		if (err < 0)
> -			goto err_free_id;
> +			goto err_close_and_put;
>  
>  		vm->usm.asid = asid;
>  	}
>  
> -	args->vm_id = id;
>  	vm->xef = xe_file_get(xef);
>  
>  	/* Record BO memory for VM pagetable created against client */
> @@ -1796,12 +1789,17 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
>  	args->reserved[0] = xe_bo_main_addr(vm->pt_root[0]->bo, XE_PAGE_SIZE);
>  #endif
>  
> -	return 0;
> -
> -err_free_id:
> +	/* user id alloc must always be last in ioctl to prevent UAF */
>  	mutex_lock(&xef->vm.lock);
> -	xa_erase(&xef->vm.xa, id);
> +	err = xa_alloc(&xef->vm.xa, &id, vm, xa_limit_32b, GFP_KERNEL);
>  	mutex_unlock(&xef->vm.lock);
> +	if (err)
> +		goto err_close_and_put;
> +
> +	args->vm_id = id;
> +
> +	return 0;
> +
>  err_close_and_put:
>  	xe_vm_close_and_put(vm);
>  
> -- 
> 2.46.1
>