drm/xe/vm: move xa_alloc to prevent UAF

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

Fixes 2149ded63079 ("drm/xe: Fix use after free when client stats are captured")

Signed-off-by Matthew Auld <[email protected]>

Cc Matthew Brost <[email protected]>

---
 drivers/gpu/drm/xe/xe_exec_queue.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/gpu/drm/xe/xe_exec_queue.c +0 -0

index 7f28b7fc68d5..a1d4b9b0726e 100644

drivers/gpu/drm/xe/xe_exec_queue.c +2 -1

+
+	/* user id alloc must always be last in ioctl to prevent UAF */
 	mutex_lock(&xef->exec_queue.lock);
 	err = xa_alloc(&xef->exec_queue.xa, &id, q, xa_limit_32b, GFP_KERNEL);
 	mutex_unlock(&xef->exec_queue.lock);

@@ -642,7 +645,6 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
 		goto kill_exec_queue;
 
 	args->exec_queue_id = id;
-	q->xef = xe_file_get(xef);
 
 	return 0;
 

Signature · 1 line

2.46.1

Quoted 20 lines · Matthew Auld

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

Fixes 2149ded63079 ("drm/xe: Fix use after free when client stats are captured")

Signed-off-by Matthew Auld <[email protected]>

Cc Matthew Brost <[email protected]>

---
  drivers/gpu/drm/xe/xe_exec_queue.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

drivers/gpu/drm/xe/xe_exec_queue.c +0 -0

index 7f28b7fc68d5..a1d4b9b0726e 100644
@@ -635,6 +635,9 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
  		}
  	}
 

drivers/gpu/drm/xe/xe_exec_queue.c int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data, · line 638

Quoted 1 line

drivers/gpu/drm/xe/xe_exec_queue.c +1 -0

+	q->xef = xe_file_get(xef);

Same comment as last patch, this looks coreect shall I merge my series and then you rebase?

Matt

Quoted 16 lines

drivers/gpu/drm/xe/xe_exec_queue.c +2 -1

+
+	/* user id alloc must always be last in ioctl to prevent UAF */
  	mutex_lock(&xef->exec_queue.lock);
  	err = xa_alloc(&xef->exec_queue.xa, &id, q, xa_limit_32b, GFP_KERNEL);
  	mutex_unlock(&xef->exec_queue.lock);

@@ -642,7 +645,6 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
  		goto kill_exec_queue;
 
  	args->exec_queue_id = id;
-	q->xef = xe_file_get(xef);
 
  	return 0;
 

Signature · 1 line

2.46.1

Quoted 13 lines · Matthew Brost

Quoted 12 lines · Matthew Auld

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

Fixes 2149ded63079 ("drm/xe: Fix use after free when client stats are captured")

Signed-off-by Matthew Auld <[email protected]>

Cc Matthew Brost <[email protected]>

---
  drivers/gpu/drm/xe/xe_exec_queue.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

Quoted 11 lines

drivers/gpu/drm/xe/xe_exec_queue.c int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data, · line 637

Quoted 8 lines

drivers/gpu/drm/xe/xe_exec_queue.c +1 -0

index 7f28b7fc68d5..a1d4b9b0726e 100644
@@ -635,6 +635,9 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
  		}
  	}
+	q->xef = xe_file_get(xef);

Same comment as last patch, this looks coreect shall I merge my series and then you rebase?

Yeah, sounds fine.

Quoted 16 lines

Matt

Quoted 13 lines

drivers/gpu/drm/xe/xe_exec_queue.c +2 -1

+
+	/* user id alloc must always be last in ioctl to prevent UAF */
  	mutex_lock(&xef->exec_queue.lock);
  	err = xa_alloc(&xef->exec_queue.xa, &id, q, xa_limit_32b, GFP_KERNEL);
  	mutex_unlock(&xef->exec_queue.lock);

@@ -642,7 +645,6 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
  		goto kill_exec_queue;
  	args->exec_queue_id = id;
-	q->xef = xe_file_get(xef);
  	return 0;

Signature · 1 line

2.46.1

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 <[email protected]>

Cc Matthew Brost <[email protected]>

Cc <[email protected]> # v6.8+

---
 drivers/gpu/drm/xe/xe_vm.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

drivers/gpu/drm/xe/xe_vm.c +10 -12

index a3d7cb7cfd22..f7182ef3d8e6 100644
@@ -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);
 

Signature · 1 line

2.46.1