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

[sashiko-bot@kernel.org]

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