Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Critical] drm/vmwgfx: use-after-free race in vmw_dumb_create() on ttm_base_object --
commit 36be588adb31a9030632708fd24d6fb50dd07914 Author: Jayapal Ashwin Nair <[email protected]> drm/vmwgfx: Fix use-after-free race in vmw_dumb_create() This commit moves drm_gem_object_put(&vbo->tbo.base) after the access to vbo->dumb_surface and usurf->prime.base.refcount_release in vmw_dumb_create() to prevent a use-after-free when racing with a concurrent DRM_IOCTL_GEM_CLOSE. > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > index bd0563741e89d..2dfbd914bdef2 100644 > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c > @@ -2332,7 +2332,6 @@ int vmw_dumb_create(struct drm_file *file_priv, > vbo = res->guest_memory_bo; > vbo->is_dumb = true; > vbo->dumb_surface = vmw_res_to_srf(res); > - drm_gem_object_put(&vbo->tbo.base); > /* > * Unset the user surface dtor since this in not actually exposed > * to userspace. The suface is owned via the dumb_buffer's GEM handle > @@ -2340,6 +2339,7 @@ int vmw_dumb_create(struct drm_file *file_priv, > struct vmw_user_surface *usurf = container_of(vbo->dumb_surface, > struct vmw_user_surface, srf); > usurf->prime.base.refcount_release = NULL; > + drm_gem_object_put(&vbo->tbo.base); > err: > if (res) > vmw_resource_unreference(&res); [Severity: Critical] Does this still leave a use-after-free race on usurf->prime.base in the subsequent ttm_ref_object_base_unref() call at the end of vmw_dumb_create()? drivers/gpu/drm/vmwgfx/vmwgfx_surface.c:vmw_dumb_create() { ... usurf->prime.base.refcount_release = NULL; drm_gem_object_put(&vbo->tbo.base); err: if (res) vmw_resource_unreference(&res); ttm_ref_object_base_unref(tfile, arg.rep.handle); ... } Under the same concurrent DRM_IOCTL_GEM_CLOSE race on arg.rep.buffer_handle, after both drm_gem_object_put(&vbo->tbo.base) and drm_gem_close_ioctl() drop their GEM references on vbo, vmw_gem_object_free() -> ttm_bo_fini() -> vmw_bo_free() drops the vbo->dumb_surface reference on &usurf->srf.res: drivers/gpu/drm/vmwgfx/vmwgfx_bo.c:vmw_bo_free() { ... if (vbo->dumb_surface) { WARN_ON(!vbo->is_dumb); vmw_surface_unreference(&vbo->dumb_surface); } ... } Once vmw_resource_unreference(&res) and vmw_bo_free() have both dropped their references on res->kref, vmw_resource_release() -> vmw_user_surface_free() queues the entire struct vmw_user_surface (including usurf->prime.base) for freeing via kfree_rcu(): drivers/gpu/drm/vmwgfx/vmwgfx_surface.c:vmw_user_surface_free() { ... kfree(srf->offsets); kfree(srf->metadata.sizes); kfree(srf->snooper.image); ttm_prime_object_kfree(user_srf, prime); } At that point, usurf->prime.base is still registered in tfile->ref_hash and tdev->idr. When vmw_dumb_create() then calls ttm_ref_object_base_unref(tfile, arg.rep.handle) -> ttm_ref_object_release() -> ttm_base_object_unref() -> ttm_release_base(), it accesses and mutates usurf->prime.base (including base->refcount, base->handle, base->tfile, and base->refcount_release) outside RCU protection after kfree_rcu() has already been invoked on usurf: drivers/gpu/drm/vmwgfx/ttm_object.c:ttm_ref_object_release() { ... hash_del_rcu(&ref->hash.head); list_del(&ref->head); spin_unlock(&tfile->lock); ttm_base_object_unref(&ref->obj); kfree_rcu(ref, rcu_head); ... } drivers/gpu/drm/vmwgfx/ttm_object.c:ttm_release_base() { ... spin_lock(&tdev->object_lock); idr_remove(&tdev->idr, base->handle); spin_unlock(&tdev->object_lock); ... ttm_object_file_unref(&base->tfile); if (base->refcount_release) base->refcount_release(&base); } Should ttm_ref_object_base_unref(tfile, arg.rep.handle) be called before dropping the references via drm_gem_object_put(&vbo->tbo.base) and vmw_resource_unreference(&res) so that usurf cannot be freed until after its ttm_base_object is unreferenced? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/48e6780670395107cc9853bf63fa7ac1d034cdd7.1793249917.git.jayapalashwin.n...@amd.com?part=1
