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

["Maaz Mombasawala <maaz.mombasawala@broadcom.com>" <maaz.mombasawala@broadcom.com>]

On Tue Oct 27, 2026 at 2:17 PM PDT, Zack Rusin wrote:
> Cursor atomic checks select an update path and look up surfaces through
> vps->uo. That object is copied from the previous plane state and is only
> replaced with the new framebuffer's object in prepare_fb(). Switching
> between snooped surfaces and buffer objects therefore applies the old
> framebuffer's cursor limits to the new image.
>
> KMS clients can also submit a guest-backed surface without a backing BO.
> The check accepts it, but the MOB update later copies from a NULL source.
> Zero-sized MOB images pass the check as well, then fail preparation when
> mapping zero pages, so TEST_ONLY accepts a commit that cannot succeed.
>
> Resolve the user object directly from the selected framebuffer in both
> validation and preparation. Apply the matching cursor limits, reject
> zero dimensions and require a backing BO for GB-only and MOB images.
> Recheck the backing BO in preparation before mapping it, while keeping
> snooped surfaces independent of a BO mapping.
>
> Fixes: 965544150d1c ("drm/vmwgfx: Refactor cursor handling")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Zack Rusin <zack.rusin@broadcom.com>
> ---
>  drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c | 61 ++++++++++++++------
>  1 file changed, 43 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c b/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
> index f0661e5effeb..9e8ea9e07f40 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_cursor_plane.c
> @@ -89,10 +89,26 @@ vmw_cursor_plane_update_legacy(struct vmw_private *vmw,
>  	}
>  }
>  
> -static enum vmw_cursor_update_type
> -vmw_cursor_update_type(struct vmw_private *vmw, struct vmw_plane_state *vps)
> +static struct vmw_user_object
> +vmw_cursor_plane_user_object(struct drm_framebuffer *fb)
>  {
> -	struct vmw_surface *surface = vmw_user_object_surface(&vps->uo);
> +	struct vmw_user_object uo = { 0 };
> +
> +	if (!fb)
> +		return uo;
> +
> +	if (vmw_framebuffer_to_vfb(fb)->bo)
> +		uo.buffer = vmw_framebuffer_to_vfbd(fb)->buffer;
> +	else
> +		uo = vmw_framebuffer_to_vfbs(fb)->uo;
> +
> +	return uo;
> +}
> +
> +static enum vmw_cursor_update_type
> +vmw_cursor_update_type(struct vmw_private *vmw, struct vmw_user_object *uo)
> +{
> +	struct vmw_surface *surface = vmw_user_object_surface(uo);
>  
>  	if (surface && surface->snooper.image)
>  		return VMW_CURSOR_UPDATE_LEGACY;
> @@ -642,17 +658,10 @@ int vmw_cursor_plane_prepare_fb(struct drm_plane *plane,
>  	if (!vmw_user_object_is_null(&vps->uo))
>  		vmw_user_object_unref(&vps->uo);
>  
> -	if (fb) {
> -		if (vmw_framebuffer_to_vfb(fb)->bo) {
> -			vps->uo.buffer = vmw_framebuffer_to_vfbd(fb)->buffer;
> -			vps->uo.surface = NULL;
> -		} else {
> -			memcpy(&vps->uo, &vmw_framebuffer_to_vfbs(fb)->uo, sizeof(vps->uo));
> -		}
> -		vmw_user_object_ref(&vps->uo);
> -	}
> +	vps->uo = vmw_cursor_plane_user_object(fb);
> +	vmw_user_object_ref(&vps->uo);

If fb == NULL then uo = 0 and that would cause a null pointer dereference in vmw_user_object_ref
since it's not doing a null check over there.

>  
> -	vps->cursor.update_type = vmw_cursor_update_type(vmw, vps);
> +	vps->cursor.update_type = vmw_cursor_update_type(vmw, &vps->uo);
>  	switch (vps->cursor.update_type) {
>  	case VMW_CURSOR_UPDATE_LEGACY:
>  		surface = vmw_user_object_surface(&vps->uo);
> @@ -662,6 +671,9 @@ int vmw_cursor_plane_prepare_fb(struct drm_plane *plane,
>  	case VMW_CURSOR_UPDATE_GB_ONLY:
>  	case VMW_CURSOR_UPDATE_MOB: {
>  		bo = vmw_user_object_buffer(&vps->uo);
> +		if (fb && !bo)
> +			return -EINVAL;
> +
>  		if (bo) {
>  			struct ttm_operation_ctx ctx = { false, false };
>  			size_t size = (size_t)new_state->crtc_w *
> @@ -744,7 +756,7 @@ int vmw_cursor_plane_atomic_check(struct drm_plane *plane,
>  	int ret = 0;
>  	struct drm_crtc_state *crtc_state = NULL;
>  	struct vmw_surface *surface = NULL;
> -	struct vmw_plane_state *vps = vmw_plane_state_to_vps(new_state);
> +	struct vmw_user_object uo;
>  	enum vmw_cursor_update_type update_type;
>  	struct drm_framebuffer *fb = new_state->fb;
>  
> @@ -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");
> @@ -789,8 +815,7 @@ int vmw_cursor_plane_atomic_check(struct drm_plane *plane,
>  					 SVGA_CB_MAX_COMMAND_SIZE -
>  					 sizeof(struct vmw_svga_fifo_cmd_define_cursor));
>  
> -		if (!new_state->crtc_w || !new_state->crtc_h ||
> -		    new_state->crtc_w > SVGA_MAX_CURSOR_CMD_DIMENSION ||

Why remove these checks?

> +		if (new_state->crtc_w > SVGA_MAX_CURSOR_CMD_DIMENSION ||
>  		    new_state->crtc_h > SVGA_MAX_CURSOR_CMD_DIMENSION ||
>  		    image_size > max_size) {
>  			drm_dbg_kms(&vmw->drm,


-- 
Maaz Mombasawala <maaz.mombasawala@broadcom.com>