Re: [PATCH v2 3/3] drm/vmwgfx: Don't leak a GEM handle when referencing a surface by fd

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Unsynchronized rbtree and xarray traversal in vmw_bo_surface() can lead to crashes or UAF.
- [High] Kernel stack memory leak in `vmw_gb_surface_reference_internal` error path.
--

commit 6ca6707ae183fa071e8ebb217050fe9e1947a0b2
Author: Michal TOMA <michaltoma@sicoop.com>

drm/vmwgfx: Don't leak a GEM handle when referencing a surface by fd

This commit prevents a GEM handle leak by looking up a surface buffer
through its dma-buf directly instead of importing the fd into the DRM file.
It rejects buffers not exported by the device and simplifies object
reference cleanup.

Cc: stable@vger.kernel.org # v6.11+

> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> index bd0563741e89d..27f68fd9c22bd 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c

[ ... ]

> @@ -931,33 +934,37 @@ u32 vmw_lookup_surface_handle_for_buffer(struct vmw_private *vmw,
>  
>  static int vmw_buffer_prime_to_surface_base(struct vmw_private *dev_priv,
>  					    struct drm_file *file_priv,
> -					    u32 fd, u32 *handle,
> +					    u32 fd,
>  					    struct ttm_base_object **base_p)
>  {

[ ... ]

> -	user_srf = vmw_lookup_user_surface_for_buffer(dev_priv, bo, *handle);
> -	if (WARN_ON(!user_srf)) {
> -		drm_warn(&dev_priv->drm,
> -			 "User surface fd %d (handle %d) is null.\n", fd, *handle);
> +	user_srf = vmw_lookup_user_surface_for_buffer(dev_priv,
> +						      to_vmw_bo(dma_buf->priv),
> +						      fd);

[Severity: Critical]
This is a pre-existing issue, but does this lockless lookup risk a
use-after-free or kernel crash?

When vmw_lookup_user_surface_for_buffer() is called here, it calls
vmw_bo_surface() which iterates over the detached_resources xarray and
res_tree rbtree without holding the dma_resv lock or an RCU read lock:

drivers/gpu/drm/vmwgfx/vmwgfx_bo.c:vmw_bo_surface() {
        ...
        xa_for_each(&vbo->detached_resources, index, res) {
                if (res->func->res_type == vmw_res_surface)
                        goto out;
        }
        ...
        for (rb_itr = rb_first(&vbo->res_tree); rb_itr;
             rb_itr = rb_next(rb_itr)) {
        ...
}

If another thread concurrently modifies these structures (for example, by
adding or removing resources), this lockless traversal could access freed
nodes. Should this lookup take the buffer object's dma_resv lock?

> +	if (!user_srf) {
> +		drm_dbg_driver(&dev_priv->drm,
> +			       "No user surface for buffer fd %d.\n", fd);
>  		ret = -EINVAL;
>  		goto out;
>  	}

[ ... ]

[Severity: High]
As an additional observation in this file, this is a pre-existing issue,
but does vmw_gb_surface_reference_internal() leak kernel stack memory to
userspace in its error path?

If srf->res.guest_memory_bo is NULL, the error path skips setting a
negative error code and jumps directly to out_bad_resource:

drivers/gpu/drm/vmwgfx/vmwgfx_surface.c:vmw_gb_surface_reference_internal() {
        ...
        if (!srf->res.guest_memory_bo) {
                DRM_ERROR("Shared GB surface is missing a backup buffer.\n");
                goto out_bad_resource;
        }
        ...
out_bad_resource:
        ...
        return ret;
}

Because ret is still 0 at this point, the caller
vmw_gb_surface_reference_ioctl() assumes success and copies the
uninitialized stack variable rep_ext into the user-visible rep pointer:

drivers/gpu/drm/vmwgfx/vmwgfx_surface.c:vmw_gb_surface_reference_ioctl() {
        struct drm_vmw_gb_surface_ref_ext_rep rep_ext;
        int ret;

        ret = vmw_gb_surface_reference_internal(dev, req, &rep_ext, file_priv);

        if (unlikely(ret != 0))
                return ret;

        rep->creq = rep_ext.creq.base;
        rep->crep = rep_ext.crep;
}

Could this be fixed by explicitly setting ret = -EINVAL before jumping to
out_bad_resource in vmw_gb_surface_reference_internal()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910165221.7558-1-michaltoma@sicoop.com?part=3