Re: [PATCH 5/6] drm/vmwgfx: validate the framebuffer selected for cursor updates

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/vmwgfx: cursor atomic_check accepts VMW_CURSOR_UPDATE_NONE framebuffers
--

commit 9cca2550f2f0603a47b5047da765ad6aeb14d8cc
Author: Zack Rusin <zack.rusin@broadcom.com>

drm/vmwgfx: validate the framebuffer selected for cursor updates

This commit resolves the user object directly from the selected framebuffer
during cursor plane validation and preparation rather than relying on the
duplicated plane state's previous user object. It also adds checks to reject
zero cursor dimensions and require a backing buffer object for GB-only and
MOB cursor updates.

> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c b/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
> index f0661e5effeb1..9e8ea9e07f40f 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c

[ ... ]

> @@ -763,7 +775,21 @@ int vmw_cursor_plane_atomic_check(struct drm_plane *plane,
>  	if (!fb)
>  		return 0;
>  
> -	update_type = vmw_cursor_update_type(vmw, vps);
> +	if (!new_state->crtc_w || !new_state->crtc_h) {
> +		drm_dbg_kms(&vmw->drm,
> +			    "Cursor dimensions (%u, %u) must be nonzero\n",
> +			    new_state->crtc_w, new_state->crtc_h);
> +		return -EINVAL;
> +	}
> +
> +	/* The duplicated plane state's user object still belongs to the old FB. */
> +	uo = vmw_cursor_plane_user_object(fb);
> +	update_type = vmw_cursor_update_type(vmw, &uo);
> +	if ((update_type == VMW_CURSOR_UPDATE_GB_ONLY ||
> +	     update_type == VMW_CURSOR_UPDATE_MOB) &&
> +	    !vmw_user_object_buffer(&uo))
> +		return -EINVAL;
> +
>  	if (update_type == VMW_CURSOR_UPDATE_LEGACY) {
>  		if (new_state->crtc_w != VMW_CURSOR_SNOOP_WIDTH ||
>  		    new_state->crtc_h != VMW_CURSOR_SNOOP_HEIGHT) {
> @@ -772,7 +798,7 @@ int vmw_cursor_plane_atomic_check(struct drm_plane *plane,
>  				 new_state->crtc_w, new_state->crtc_h);
>  			return -EINVAL;
>  		}
> -		surface = vmw_user_object_surface(&vps->uo);
> +		surface = vmw_user_object_surface(&uo);
>  		if (!surface || !surface->snooper.image) {
>  			drm_warn(&vmw->drm,
>  				 "surface not suitable for cursor\n");

[Severity: Medium]
Can this check for !surface || !surface->snooper.image ever be reached, and
what happens when update_type is VMW_CURSOR_UPDATE_NONE?

Looking at vmw_cursor_update_type(), it only returns VMW_CURSOR_UPDATE_LEGACY
when surface && surface->snooper.image is already true. If vmw->has_mob is
false and the new framebuffer does not have a snooper image (for example, a
BO-backed dumb framebuffer or an atomic client surface where
surface->snooper.image is NULL), vmw_cursor_update_type() returns
VMW_CURSOR_UPDATE_NONE:

drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c:vmw_cursor_update_type() {
	struct vmw_surface *surface = vmw_user_object_surface(uo);

	if (surface && surface->snooper.image)
		return VMW_CURSOR_UPDATE_LEGACY;

	if (vmw->has_mob) {
		if ((vmw->capabilities2 & SVGA_CAP2_CURSOR_MOB) != 0)
			return VMW_CURSOR_UPDATE_MOB;
		else
			return VMW_CURSOR_UPDATE_GB_ONLY;
	}
	drm_warn_once(&vmw->drm, "Unknown Cursor Type!\n");
	return VMW_CURSOR_UPDATE_NONE;
}

When vmw_cursor_update_type() returns VMW_CURSOR_UPDATE_NONE,
vmw_cursor_plane_atomic_check() skips the VMW_CURSOR_UPDATE_LEGACY,
VMW_CURSOR_UPDATE_GB_ONLY, and VMW_CURSOR_UPDATE_MOB branches and returns 0
at the end of the function.

Should vmw_cursor_plane_atomic_check() reject VMW_CURSOR_UPDATE_NONE with
-EINVAL so that non-snooped framebuffers on non-MOB devices are not accepted?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027211729.646988-1-zack.rusin@broadcom.com?part=5