Re: [PATCH] drm/qxl: size packed dumb heads from the plane source

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/qxl: out-of-bounds write in primary surface due to uncompensated src_y
- [High] drm/qxl: out-of-bounds read in SPICE client due to double-applied X offset
- [Medium] drm/qxl: active CRTCs are blanked when shadow surface is reallocated on plane updates
--

commit 6cdb15db884bed833722eee3adac75f129fa0806
Author: Dillon Amburgey <dillona@gmail.com>

    drm/qxl: size packed dumb heads from the plane source
    
    QXL packs per-CRTC dumb buffers into a single primary surface.
    qxl_update_dumb_head() recorded each dumb BO allocation (bo->surf)
    instead of the plane source rectangle. Scanning 1280x800 from a
    2048x1024 dumb framebuffer beside a 1024x768 head therefore created
    a 3072x1024 primary and placed head 1 at +2048, rather than 2304x800
    with head 1 at +1280.

> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> index 0719fc6a52d53..e57aeeb97f84e 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -670,13 +670,17 @@ static void qxl_primary_atomic_update(struct drm_plane *plane,
>  	struct qxl_device *qdev = to_qxl(plane->dev);
>  	struct qxl_bo *bo = gem_to_qxl_bo(new_state->fb->obj[0]);
>  	struct qxl_bo *primary;
> -	struct drm_clip_rect norect = {
> -	    .x1 = 0,
> -	    .y1 = 0,
> -	    .x2 = new_state->fb->width,
> -	    .y2 = new_state->fb->height
> -	};
> +	struct drm_clip_rect norect;
>  	uint32_t dumb_shadow_offset = 0;
> +	u32 src_x = new_state->src_x >> 16;
> +	u32 src_y = new_state->src_y >> 16;
> +	u32 src_w = new_state->src_w >> 16;
> +	u32 src_h = new_state->src_h >> 16;
> +
> +	norect.x1 = src_x;
> +	norect.y1 = src_y;
> +	norect.x2 = src_x + src_w;
> +	norect.y2 = src_y + src_h;

[Severity: High]
Can this result in an out-of-bounds write in the primary surface?

The destination Y coordinates are set directly to the source Y offset (src_y)
without being compensated or normalized. In qxl_calc_dumb_shadow(), the
primary surface height is allocated as just the maximum src_h across all
heads.

If a plane is configured with a non-zero src_y, src_y + src_h will exceed the
allocated surface height, causing qxl_draw_dirty_fb() to instruct the host to
draw outside the bounds of the primary surface.

>  
>  	primary = bo->shadow ? bo->shadow : bo;
>  
> @@ -689,7 +693,7 @@ static void qxl_primary_atomic_update(struct drm_plane *plane,
>  
>  	if (bo->is_dumb)
>  		dumb_shadow_offset =
> -			qdev->dumb_heads[new_state->crtc->index].x;
> +			qdev->dumb_heads[new_state->crtc->index].x - src_x;

[Severity: High]
Does this cause an out-of-bounds read in the SPICE client?

By subtracting src_x here, the image is packed at exactly dumb_heads[i].x
in the shadow surface. However, qxl_crtc_update_monitors_config() was not
updated and still adds crtc->x to the head position:

	if (crtc->state->active) {
		...
		head.x = crtc->x;
		if (qdev->primary_bo == qdev->dumb_shadow_bo)
			head.x += qdev->dumb_heads[i].x;

Since the viewport was placed exactly at dumb_heads[i].x in the primary
surface, instructing SPICE to read from dumb_heads[i].x + crtc->x means it
will read past the valid image data for the secondary CRTC.

[ ... ]

> @@ -869,7 +868,9 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
>  
>  	if (plane->type == DRM_PLANE_TYPE_PRIMARY &&
>  	    user_bo->is_dumb) {
> -		qxl_prepare_shadow(qdev, user_bo, new_state->crtc->index);
> +		qxl_prepare_shadow(qdev, user_bo, new_state->crtc->index,
> +				   new_state->src_w >> 16,
> +				   new_state->src_h >> 16);
>  	}

[Severity: Medium]
Will this blank active CRTCs when the shadow surface is reallocated?

By tying the shared shadow surface layout to per-plane mutable crop state
(src_w and src_h), qxl_prepare_shadow() will dynamically reallocate the
shared primary surface if a plane's dimensions change:

static void qxl_prepare_shadow(...)
{
	...
	if (!qdev->dumb_shadow_bo ||
	    qdev->dumb_shadow_bo->surf.width  != surf.width ||
	    qdev->dumb_shadow_bo->surf.height != surf.height) {
		...
		qxl_bo_create(qdev, surf.height * surf.stride, ...);

When this reallocation happens during a single-plane atomic update, other
active CRTCs are not participating in the commit and won't redraw their
contents into the new shared surface, potentially leaving them blank.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924025222.6077-1-dillona@gmail.com?part=1