Re: [PATCH] drm/vmwgfx: Return gem fd handle for scanout buffers

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Passing an `int *` to a function expecting a `u32 *` causes an incompatible pointer type build failure.

Pre-existing issues:
- [High] Concurrent lockless access to the RB-tree `vbo->res_tree` leads to tree corruption and an associated Use-After-Free window.
--

--- Patch [1]: [PATCH] drm/vmwgfx: Return gem fd handle for scanout buffers ---
commit b5937886d1e50afefe640e402b3c52ae509b9578
Author: Maaz Mombasawala <maaz.mombasawala@broadcom.com>

drm/vmwgfx: Return gem fd handle for scanout buffers

When prime_handle_to_fd is called on a gem buffer handle, we return an
fd for the surface handle if the gem buffer backs a surface.
Return the gem buffer's handle instead for cases when the surface is a
scanout buffer.

> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> index 38bea8abab84c..ff33ba1075a40 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h

[ ... ]

> @@ -1180,9 +1180,11 @@ int vmw_gb_surface_define(struct vmw_private *dev_priv,
>  struct vmw_surface *vmw_lookup_surface_for_buffer(struct vmw_private *vmw,
>  						  struct vmw_bo *bo,
>  						  u32 handle);
> -u32 vmw_lookup_surface_handle_for_buffer(struct vmw_private *vmw,
> -					 struct vmw_bo *bo,
> -					 u32 handle);
> +void vmw_lookup_surface_and_handle_for_buffer(struct vmw_private *vmw,
> +					      struct vmw_bo *bo,
> +					      u32 handle,
> +					      u32 *srf_handle,
> +					      struct vmw_surface **p_srf);
>  int vmw_dumb_create(struct drm_file *file_priv,

[ ... ]

> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c
> index 598b90ac7590b..7e6581ee09cc1 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_prime.c
> @@ -93,8 +93,9 @@ int vmw_prime_handle_to_fd(struct drm_device *dev,
>  	struct vmw_private *vmw = vmw_priv(dev);
>  	struct ttm_object_file *tfile = vmw_fpriv(file_priv)->tfile;
>  	struct vmw_bo *vbo;
> +	struct vmw_surface *surface = NULL;
> +	int surf_handle = 0;
>  	int ret;
> -	int surf_handle;
>  
>  	if (handle > VMWGFX_NUM_MOB) {
>  		ret = ttm_prime_handle_to_fd(tfile, handle, flags, prime_fd);
> @@ -106,16 +107,19 @@ int vmw_prime_handle_to_fd(struct drm_device *dev,
>  			ret = drm_gem_prime_handle_to_fd(dev, file_priv, handle,
>  							 flags, prime_fd);
>  		} else {
> -			surf_handle = vmw_lookup_surface_handle_for_buffer(vmw,
> -									   vbo,
> -									   handle);
> -			if (surf_handle > 0)
> +			vmw_lookup_surface_and_handle_for_buffer(vmw, vbo,
> +								 handle,
> +								 &surf_handle,
                                                                 ^^^^^^^^^^^^

[Severity: Low]
Does passing an int pointer to a parameter that expects a u32 pointer cause a
compiler warning or build failure?

The function signature for vmw_lookup_surface_and_handle_for_buffer() explicitly
requires a u32 pointer for the fourth parameter.

[Severity: High]
This isn't a bug introduced by this patch, but does calling this locklessly
lead to tree corruption or a use-after-free?

When vmw_prime_handle_to_fd() calls vmw_lookup_surface_and_handle_for_buffer(),
it eventually calls vmw_bo_surface() which iterates over the vbo->res_tree
RB-tree locklessly:

drivers/gpu/drm/vmwgfx/vmwgfx_bo.c:vmw_bo_surface() {
    ...
    for (rb_itr = rb_first(&vbo->res_tree); rb_itr;
         rb_itr = rb_next(rb_itr)) {
    ...
}

Concurrently, other ioctls can acquire the dma_resv lock and modify the
res_tree. Can this concurrent modification result in an infinite loop or a
kernel crash?

Also, in vmw_lookup_user_surface_for_buffer(), the RCU read lock is acquired
after the pointer surf has already been retrieved:

drivers/gpu/drm/vmwgfx/vmwgfx_surface.c:vmw_lookup_user_surface_for_buffer() {
    ...
    surf = vmw_bo_surface(bo);
    if (surf) {
        rcu_read_lock();
    ...
}

Could this leave a window where the object can be freed before the lock is
acquired?

> +								 &surface);
> +			if (surface && !surface->metadata.scanout)
>  				ret = ttm_prime_handle_to_fd(tfile, surf_handle,
>  							     flags, prime_fd);
>  			else
>  				ret = drm_gem_prime_handle_to_fd(dev, file_priv,
>  								 handle, flags,
>  								 prime_fd);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820165838.442723-1-maaz.mombasawala@broadcom.com?part=1