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

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

[Dillon Amburgey]

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.

Use src_w/src_h when building the packed shadow, and copy only that
source rectangle into it.

Fixes: 90adda2ce898 ("drm/qxl: cover all crtcs in shadow bo.")
Assisted-by: LLM sparse
Signed-off-by: Dillon Amburgey <dillona@gmail.com>
---
Tested on torvalds/linux 62f4c998b297. A DRM client (not Xorg, not
SPICE) programmed one QXL device with max_outputs=2. CRTC 0 scans a
1280x800 rectangle from a 2048x1024 dumb framebuffer (the allocation
is larger than the scanout). CRTC 1 scans a separate 1024x768 dumb
framebuffer. Unpatched, qxl_update_dumb_head() sizes packed heads
from bo->surf, so QEMU's qxl_create_guest_primary is 3072x1024
(2048+1024 by max height) and monitors_config places head 1 at +2048.
With this patch, the primary is 2304x800 (1280+1024) and head 1 is at
+1280.

Content outside CRTC 0's 1280x800 source rectangle did not appear in
the packed primary, and both heads' source rectangles did.

checkpatch.pl --strict: 0 errors, 0 warnings, 0 checks. W=1 and Sparse
on drivers/gpu/drm/qxl/qxl_display.c added no warnings.

 drivers/gpu/drm/qxl/qxl_display.c | 39 ++++++++++++++++---------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index 0719fc6a52d5..e57aeeb97f84 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;
 
 	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;
 
 	qxl_draw_dirty_fb(qdev, new_state->fb, bo, 0, 0, &norect, 1, 1,
 			  dumb_shadow_offset);
@@ -764,18 +768,14 @@ static void qxl_cursor_atomic_disable(struct drm_plane *plane,
 	qcrtc->cursor_bo = NULL;
 }
 
-static void qxl_update_dumb_head(struct qxl_device *qdev,
-				 int index, struct qxl_bo *bo)
+static void qxl_update_dumb_head(struct qxl_device *qdev, int index,
+				 struct qxl_bo *bo, uint32_t width,
+				 uint32_t height)
 {
-	uint32_t width, height;
-
 	if (index >= qdev->monitors_config->max_allowed)
 		return;
 
-	if (bo && bo->is_dumb) {
-		width = bo->surf.width;
-		height = bo->surf.height;
-	} else {
+	if (!bo || !bo->is_dumb) {
 		width = 0;
 		height = 0;
 	}
@@ -820,12 +820,11 @@ static void qxl_calc_dumb_shadow(struct qxl_device *qdev,
 }
 
 static void qxl_prepare_shadow(struct qxl_device *qdev, struct qxl_bo *user_bo,
-			       int crtc_index)
+			       int crtc_index, uint32_t width, uint32_t height)
 {
 	struct qxl_surface surf;
 
-	qxl_update_dumb_head(qdev, crtc_index,
-			     user_bo);
+	qxl_update_dumb_head(qdev, crtc_index, user_bo, width, height);
 	qxl_calc_dumb_shadow(qdev, &surf);
 	if (!qdev->dumb_shadow_bo ||
 	    qdev->dumb_shadow_bo->surf.width  != surf.width ||
@@ -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);
 	}
 
 	if (plane->type == DRM_PLANE_TYPE_CURSOR &&

base-commit: 62f4c998b297cf233997a2b4cd6fc2d2df0319c9
-- 
2.43.0

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

[sashiko-bot]

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

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

[Dillon Amburgey]

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.

Use src_w/src_h when building the packed shadow, and copy only that
source rectangle into it. Place that rectangle at the packed origin:
crtc->x and crtc->y are the plane source position, so adding them to
the packed offset publishes the head past the end of the primary.
When the shadow is reallocated, copy the other active dumb heads from
their current framebuffers. DRM_IOCTL_MODE_DIRTYFB intersects each
clip with the plane source and uses that same origin, so a dirty
update of one head is not painted at (0, 0).

Fixes: 90adda2ce898 ("drm/qxl: cover all crtcs in shadow bo.")
Assisted-by: LLM sparse
Signed-off-by: Dillon Amburgey <dillona@gmail.com>
---
Changes in v2:
- Place each source rectangle at the packed origin. crtc->x and
  crtc->y are the plane source position, so adding them published
  head 1 at +2560 on a 2304-wide primary. A shadow primary now
  reports dumb_heads[i].x and .y. A shared framebuffer with head 1
  scanning at x=1280 now reports +1280.
- A non-zero source y is no longer the destination y. The copy reads
  at src_y and writes at dumb_heads[i].y. A 1280x800 source starting
  at y=200 is drawn at the top of the primary.
- Replacing the shadow recopies the other active dumb heads from
  their current framebuffers. The failure was stale pixels, not a
  cleared head: after recoloring head 0 and enabling head 1, head 0
  kept the old color until this recopy.
- DRM_IOCTL_MODE_DIRTYFB intersects the clip with the plane source
  and uses that same origin. A dirty update of head 1 no longer
  paints that buffer at (0, 0).

 drivers/gpu/drm/qxl/qxl_display.c | 132 ++++++++++++++++++++++++------
 drivers/gpu/drm/qxl/qxl_draw.c    |   6 +-
 drivers/gpu/drm/qxl/qxl_drv.h     |   3 +-
 3 files changed, 113 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index 0719fc6a52d5..d01504332bc8 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -348,8 +348,10 @@ static void qxl_crtc_update_monitors_config(struct drm_crtc *crtc,
 		head.y = crtc->y;
 		if (qdev->monitors_config->count < i + 1)
 			qdev->monitors_config->count = i + 1;
-		if (qdev->primary_bo == qdev->dumb_shadow_bo)
-			head.x += qdev->dumb_heads[i].x;
+		if (qdev->primary_bo == qdev->dumb_shadow_bo) {
+			head.x = qdev->dumb_heads[i].x;
+			head.y = qdev->dumb_heads[i].y;
+		}
 	} else if (i > 0) {
 		head.width = 0;
 		head.height = 0;
@@ -458,8 +460,47 @@ static int qxl_framebuffer_surface_dirty(struct drm_framebuffer *fb,
 		inc = 2; /* skip source rects */
 	}
 
-	qxl_draw_dirty_fb(qdev, fb, qobj, flags, color,
-			  clips, num_clips, inc, 0);
+	if (qobj->shadow) {
+		struct drm_crtc *crtc;
+		unsigned int n;
+
+		drm_for_each_crtc(crtc, &qdev->ddev) {
+			struct drm_plane_state *st;
+			u32 sx, sy, sw, sh;
+
+			st = crtc->primary->state;
+			if (st->fb != fb ||
+			    !qdev->dumb_heads[crtc->index].width)
+				continue;
+			sx = st->src_x >> 16;
+			sy = st->src_y >> 16;
+			sw = st->src_w >> 16;
+			sh = st->src_h >> 16;
+			for (n = 0; n < num_clips; n++) {
+				struct drm_clip_rect *in = clips + n * inc;
+				struct drm_clip_rect c;
+				u32 x1 = max_t(u32, in->x1, sx);
+				u32 y1 = max_t(u32, in->y1, sy);
+				u32 x2 = min_t(u32, in->x2, sx + sw);
+				u32 y2 = min_t(u32, in->y2, sy + sh);
+
+				if (x1 >= x2 || y1 >= y2)
+					continue;
+				c.x1 = x1;
+				c.y1 = y1;
+				c.x2 = x2;
+				c.y2 = y2;
+				qxl_draw_dirty_fb(qdev, fb, qobj, flags, color,
+						  &c, 1, 1,
+						  qdev->dumb_heads[crtc->index].x - sx,
+						  (int)qdev->dumb_heads[crtc->index].y -
+						  (int)sy);
+			}
+		}
+	} else {
+		qxl_draw_dirty_fb(qdev, fb, qobj, flags, color,
+				  clips, num_clips, inc, 0, 0);
+	}
 
 out_lock_end:
 	DRM_MODESET_LOCK_ALL_END(fb->dev, ctx, ret);
@@ -662,6 +703,37 @@ static void qxl_free_cursor(struct qxl_bo *cursor_bo)
 	qxl_bo_unref(&cursor_bo);
 }
 
+static void qxl_redraw_other_dumb_heads(struct qxl_device *qdev, int skip)
+{
+	struct drm_crtc *crtc;
+
+	drm_for_each_crtc(crtc, &qdev->ddev) {
+		struct drm_plane_state *st;
+		struct qxl_bo *other;
+		struct drm_clip_rect clip;
+		u32 sx, sy;
+
+		if (crtc->index == skip ||
+		    !qdev->dumb_heads[crtc->index].width)
+			continue;
+		st = crtc->primary->state;
+		if (!st->fb)
+			continue;
+		other = gem_to_qxl_bo(st->fb->obj[0]);
+		if (!other->is_dumb)
+			continue;
+		sx = st->src_x >> 16;
+		sy = st->src_y >> 16;
+		clip.x1 = sx;
+		clip.y1 = sy;
+		clip.x2 = sx + (st->src_w >> 16);
+		clip.y2 = sy + (st->src_h >> 16);
+		qxl_draw_dirty_fb(qdev, st->fb, other, 0, 0, &clip, 1, 1,
+				  qdev->dumb_heads[crtc->index].x - sx,
+				  (int)qdev->dumb_heads[crtc->index].y - (int)sy);
+	}
+}
+
 static void qxl_primary_atomic_update(struct drm_plane *plane,
 				      struct drm_atomic_commit *state)
 {
@@ -670,13 +742,18 @@ 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;
+	int y_off = 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;
 
 	primary = bo->shadow ? bo->shadow : bo;
 
@@ -687,12 +764,19 @@ static void qxl_primary_atomic_update(struct drm_plane *plane,
 		qxl_primary_apply_cursor(qdev, plane->state);
 	}
 
-	if (bo->is_dumb)
+	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;
+		y_off = (int)qdev->dumb_heads[new_state->crtc->index].y -
+			(int)src_y;
+	}
 
 	qxl_draw_dirty_fb(qdev, new_state->fb, bo, 0, 0, &norect, 1, 1,
-			  dumb_shadow_offset);
+			  dumb_shadow_offset, y_off);
+	if (qdev->dumb_shadow_needs_redraw) {
+		qdev->dumb_shadow_needs_redraw = false;
+		qxl_redraw_other_dumb_heads(qdev, new_state->crtc->index);
+	}
 }
 
 static void qxl_primary_atomic_disable(struct drm_plane *plane,
@@ -764,18 +848,14 @@ static void qxl_cursor_atomic_disable(struct drm_plane *plane,
 	qcrtc->cursor_bo = NULL;
 }
 
-static void qxl_update_dumb_head(struct qxl_device *qdev,
-				 int index, struct qxl_bo *bo)
+static void qxl_update_dumb_head(struct qxl_device *qdev, int index,
+				 struct qxl_bo *bo, uint32_t width,
+				 uint32_t height)
 {
-	uint32_t width, height;
-
 	if (index >= qdev->monitors_config->max_allowed)
 		return;
 
-	if (bo && bo->is_dumb) {
-		width = bo->surf.width;
-		height = bo->surf.height;
-	} else {
+	if (!bo || !bo->is_dumb) {
 		width = 0;
 		height = 0;
 	}
@@ -820,12 +900,11 @@ static void qxl_calc_dumb_shadow(struct qxl_device *qdev,
 }
 
 static void qxl_prepare_shadow(struct qxl_device *qdev, struct qxl_bo *user_bo,
-			       int crtc_index)
+			       int crtc_index, uint32_t width, uint32_t height)
 {
 	struct qxl_surface surf;
 
-	qxl_update_dumb_head(qdev, crtc_index,
-			     user_bo);
+	qxl_update_dumb_head(qdev, crtc_index, user_bo, width, height);
 	qxl_calc_dumb_shadow(qdev, &surf);
 	if (!qdev->dumb_shadow_bo ||
 	    qdev->dumb_shadow_bo->surf.width  != surf.width ||
@@ -839,6 +918,7 @@ static void qxl_prepare_shadow(struct qxl_device *qdev, struct qxl_bo *user_bo,
 		qxl_bo_create(qdev, surf.height * surf.stride,
 			      true, true, QXL_GEM_DOMAIN_SURFACE, 0,
 			      &surf, &qdev->dumb_shadow_bo);
+		qdev->dumb_shadow_needs_redraw = true;
 	}
 	if (user_bo->shadow != qdev->dumb_shadow_bo) {
 		if (user_bo->shadow) {
@@ -869,7 +949,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);
 	}
 
 	if (plane->type == DRM_PLANE_TYPE_CURSOR &&
diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c
index 3a3e127ce297..f99f4afa46d9 100644
--- a/drivers/gpu/drm/qxl/qxl_draw.c
+++ b/drivers/gpu/drm/qxl/qxl_draw.c
@@ -129,7 +129,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
 		       unsigned int flags, unsigned int color,
 		       struct drm_clip_rect *clips,
 		       unsigned int num_clips, int inc,
-		       uint32_t dumb_shadow_offset)
+		       u32 dumb_shadow_offset, int y_off)
 {
 	/*
 	 * TODO: if flags & DRM_MODE_FB_DIRTY_ANNOTATE_FILL then we should
@@ -160,6 +160,8 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
 
 	clips->x1 += dumb_shadow_offset;
 	clips->x2 += dumb_shadow_offset;
+	clips->y1 += y_off;
+	clips->y2 += y_off;
 
 	left = clips->x1;
 	right = clips->x2;
@@ -210,7 +212,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
 
 	ret = qxl_image_init(qdev, release, dimage, surface_base,
 			     left - dumb_shadow_offset,
-			     top, width, height, depth, stride);
+			     top - y_off, width, height, depth, stride);
 	qxl_bo_vunmap_locked(bo);
 	if (ret)
 		goto out_release_backoff;
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index cc02b5f10ad9..f7edb620d4d7 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -202,6 +202,7 @@ struct qxl_device {
 
 	struct qxl_bo *primary_bo;
 	struct qxl_bo *dumb_shadow_bo;
+	bool dumb_shadow_needs_redraw;
 	struct qxl_head *dumb_heads;
 
 	struct qxl_memslot main_slot;
@@ -394,7 +395,7 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
 		       unsigned int flags, unsigned int color,
 		       struct drm_clip_rect *clips,
 		       unsigned int num_clips, int inc,
-		       uint32_t dumb_shadow_offset);
+		       u32 dumb_shadow_offset, int y_off);
 
 void qxl_release_free(struct qxl_device *qdev,
 		      struct qxl_release *release);

base-commit: 62f4c998b297cf233997a2b4cd6fc2d2df0319c9
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/qxl: UAF in qxl_redraw_other_dumb_heads via lockless state access
- [Medium] drm/qxl: Data race on dumb_shadow_needs_redraw causes missed redraws
--

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

drm/qxl: size packed dumb heads from the plane source

This commit updates QXL to size and position dumb shadow buffers based on
plane source rectangles instead of dumb BO allocations. It introduces a new
flag to track redraw requirements and a function to recopy active heads during
shadow reallocation.

> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> index 0719fc6a52d53..d01504332bc83 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
[ ... ]
> @@ -662,6 +703,37 @@ static void qxl_free_cursor(struct qxl_bo *cursor_bo)
>  	qxl_bo_unref(&cursor_bo);
>  }
>  
> +static void qxl_redraw_other_dumb_heads(struct qxl_device *qdev, int skip)
> +{
> +	struct drm_crtc *crtc;
> +
> +	drm_for_each_crtc(crtc, &qdev->ddev) {
> +		struct drm_plane_state *st;
> +		struct qxl_bo *other;
> +		struct drm_clip_rect clip;
> +		u32 sx, sy;
> +
> +		if (crtc->index == skip ||
> +		    !qdev->dumb_heads[crtc->index].width)
> +			continue;
> +		st = crtc->primary->state;
> +		if (!st->fb)
> +			continue;
> +		other = gem_to_qxl_bo(st->fb->obj[0]);

[Severity: High]
Does this lockless state access risk a use-after-free?

When an atomic commit updates a dumb shadow buffer,
qxl_primary_atomic_update() calls this function to loop over all CRTCs. If a
concurrent atomic commit processes one of these other CRTCs, it could swap and
free the old plane state and its framebuffer while they are being accessed
here, because the iterating commit holds no modeset locks for these disjoint
CRTCs.

> +		if (!other->is_dumb)
> +			continue;
[ ... ]
> @@ -670,13 +742,18 @@ 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;
[ ... ]
>  	qxl_draw_dirty_fb(qdev, new_state->fb, bo, 0, 0, &norect, 1, 1,
> -			  dumb_shadow_offset);
> +			  dumb_shadow_offset, y_off);
> +	if (qdev->dumb_shadow_needs_redraw) {
> +		qdev->dumb_shadow_needs_redraw = false;
> +		qxl_redraw_other_dumb_heads(qdev, new_state->crtc->index);
> +	}

[Severity: Medium]
Can this shared flag cause missed screen redraws due to a data race?

The qdev->dumb_shadow_needs_redraw flag is shared globally across the device.
When two atomic commits run concurrently on different CRTCs, both may set
the flag to true during qxl_prepare_shadow(). The first commit to reach
this update phase will clear the flag. The second commit will then see
the flag as false and skip the required redraws for other heads.

>  }
>  
>  static void qxl_primary_atomic_disable(struct drm_plane *plane,
[ ... ]
> @@ -839,6 +918,7 @@ static void qxl_prepare_shadow(struct qxl_device *qdev, struct qxl_bo *user_bo,
>  		qxl_bo_create(qdev, surf.height * surf.stride,
>  			      true, true, QXL_GEM_DOMAIN_SURFACE, 0,
>  			      &surf, &qdev->dumb_shadow_bo);
> +		qdev->dumb_shadow_needs_redraw = true;

[Severity: Medium]
This is where the shared flag is set locklessly by potentially concurrent
plane preparation steps.

>  	}
>  	if (user_bo->shadow != qdev->dumb_shadow_bo) {
>  		if (user_bo->shadow) {

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