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 <[email protected]> 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/[email protected]?part=1
