On 9/25/2024 9:14 AM, 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.
>
> v2:
> - Rebase
>
> Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> Signed-off-by: Matthew Auld <matthew.auld@xxxxxxxxx>
> Cc: Matthew Brost <matthew.brost@xxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # v6.8+
Interesting find, LGTM:
Reviewed-by: Nirmoy Das <nirmoy.das@xxxxxxxxx>
> ---
> drivers/gpu/drm/xe/xe_vm.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 31fe31db3fdc..ce9dca4d4e87 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1765,10 +1765,6 @@ int xe_vm_create_ioctl(struct drm_device *dev, void *data,
> if (IS_ERR(vm))
> return PTR_ERR(vm);
>
> - err = xa_alloc(&xef->vm.xa, &id, vm, xa_limit_32b, GFP_KERNEL);
> - 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,
> @@ -1776,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 */
> @@ -1794,10 +1789,15 @@ 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
>
> + /* user id alloc must always be last in ioctl to prevent UAF */
> + err = xa_alloc(&xef->vm.xa, &id, vm, xa_limit_32b, GFP_KERNEL);
> + if (err)
> + goto err_close_and_put;
> +
> + args->vm_id = id;
> +
> return 0;
>
> -err_free_id:
> - xa_erase(&xef->vm.xa, id);
> err_close_and_put:
> xe_vm_close_and_put(vm);
>
