Re: [PATCH] fix: drm/vmwgfx: vmw_simple_resource_create_ioctl: fix base object refcount leak on ttm_base_object_init failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Type Mismatch / NULL Pointer Dereference in ttm_base_object_unref.
- [High] Double Free / Use-After-Free of vmw_resource on error path.
- [High] IDR Double Remove and Corruption.
--

commit 0e71ac7463131ddc891fdadbc6bca7ae37b5bcac
Author: WenTao Liang <vulab@iscas.ac.cn>

fix: drm/vmwgfx: vmw_simple_resource_create_ioctl: fix base object refcount leak on ttm_base_object_init failure

This commit attempts to fix a refcount leak by adding a call to
ttm_base_object_unref() in the error path of vmw_simple_resource_create_ioctl()
when ttm_base_object_init() fails.

> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_simple_resource.c b/drivers/gpu/drm/vmwgfx/vmwgfx_simple_resource.c
> index 0d51b45422699..05f0f55441426 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_simple_resource.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_simple_resource.c
> @@ -176,6 +176,7 @@ vmw_simple_resource_create_ioctl(struct drm_device *dev, void *data,
>  
>  	if (ret) {
>  		vmw_resource_unreference(&tmp);
> +		ttm_base_object_unref(&usimple->base);

[Severity: Critical]
Does this pass the correct pointer type to ttm_base_object_unref()?

ttm_base_object_unref() expects a struct ttm_base_object **, but &usimple->base
is a struct ttm_base_object *.

If this compiles, ttm_base_object_unref() will interpret the first 8 bytes of
usimple->base as a pointer. Since usimple was allocated with kzalloc(), this
reads as NULL:

drivers/gpu/drm/vmwgfx/ttm_object.c:ttm_base_object_unref() {
	struct ttm_base_object *base = *p_base;

	*p_base = NULL;

	kref_put(&base->refcount, ttm_release_base);
}

Can this dereference NULL when passing &base->refcount to kref_put()?


[Severity: High]
Does ttm_base_object_init() already clean up its own state on failure,
making this unref call unsafe?

If ttm_base_object_init() fails (for example, in ttm_ref_object_add()), it
cleans up the IDR entry in its error path:

drivers/gpu/drm/vmwgfx/ttm_object.c:ttm_base_object_init() {
    ...
out_err1:
	spin_lock(&tdev->object_lock);
	idr_remove(&tdev->idr, base->handle);
	spin_unlock(&tdev->object_lock);
	return ret;
}

If ttm_base_object_unref() is called afterward, it will trigger
ttm_release_base() which also unconditionally calls idr_remove() for the same
handle:

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

If another thread successfully allocates an object and receives the same ID in
the window between these two calls, will this corrupt the IDR by deleting the
other thread's valid ID?

>  		goto out_err;
>  	}
>  
>  	func->set_arg_handle(data, usimple->base.handle);
>  out_err:
>  	vmw_resource_unreference(&res);

[Severity: High]
If the type mismatch above is fixed, will this cause a double free and
use-after-free of the vmw_resource?

When ttm_base_object_unref() drops the refcount to 0, it calls
ttm_release_base(), which invokes the provided release callback
vmw_simple_resource_base_release(). That callback drops a reference on the
embedded vmw_resource, dropping its refcount to 0 and freeing usimple.

However, the error path then proceeds here to the out_err label and
unconditionally calls vmw_resource_unreference(&res) on the already freed
memory.

>  out_ret:
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626150224.49792-1-vulab@iscas.ac.cn?part=1