[PATCH] drm/vmwgfx: Fix use-after-free race in vmw_dumb_create()

[PATCH] drm/vmwgfx: Fix use-after-free race in vmw_dumb_create()

[Jayapal Ashwin Nair]

vmw_dumb_create(), called from DRM_IOCTL_MODE_CREATE_DUMB, drops its
extra GEM reference on the vbo before it reads vbo->dumb_surface and
clears the user surface's refcount_release. After that put, the only
reference left is held by the GEM handle, which is already visible to
userspace. A concurrent DRM_IOCTL_GEM_CLOSE on that handle can drop it
and free the vbo (and the dumb surface) while vmw_dumb_create() is
still using them:

  CPU0 (DRM_IOCTL_MODE_CREATE_DUMB)      CPU1 (DRM_IOCTL_GEM_CLOSE)
  vmw_dumb_create()
   vmw_gb_surface_define_ext_ioctl()
     /* vbo refcount == 2 */
   drm_gem_object_put(&vbo->tbo.base)
     /* vbo refcount == 1 */
                                         drm_gem_close_ioctl()
                                          drm_gem_handle_delete()
                                           drm_gem_object_release_handle()
                                            drm_gem_object_handle_put_unlocked()
                                             drm_gem_object_put()
                                              /* vbo refcount == 0 */
                                              vmw_gem_object_free()
                                               ttm_bo_fini()
                                                vmw_bo_free()
                                                 vmw_surface_unreference()
                                                 kfree(vbo)
   usurf = container_of(vbo->dumb_surface, ...)  /* UAF read */
   usurf->prime.base.refcount_release = NULL     /* UAF write */

Move drm_gem_object_put() after the last access to the vbo.

Fixes: f42c09e614f1 ("drm/vmwgfx: Fix dumb buffer leak")
Signed-off-by: Jayapal Ashwin Nair <jayapalashwin.nair@amd.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index bd0563741e89..2dfbd914bdef 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);
-- 
2.53.0

Re: [PATCH] drm/vmwgfx: Fix use-after-free race in vmw_dumb_create()

[sashiko-bot]

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 <jayapalashwin.nair@amd.com>

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.nair@amd.com?part=1