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@xxxxxxxxx> > Cc: Matthew Brost <matthew.brost@xxxxxxxxx> > Cc: <stable@xxxxxxxxxxxxxxx> # 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 >
