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); >