Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/qxl: missing offset in clipping rectangles breaks rendering - [Low] drm/qxl: dead code in dumb_shadow_offset bounds check --
commit dbbac4275ce84b9a554a655eca70f851083fc445 Author: Hui Peng <[email protected]> drm/qxl: fix cursor OOB read, dirty-rect bounds, and monitors double-fetch This commit fixes out-of-bounds accesses and missing bounds checks in the QXL display and dirty-framebuffer drawing paths. > diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c > --- a/drivers/gpu/drm/qxl/qxl_draw.c > +++ b/drivers/gpu/drm/qxl/qxl_draw.c > @@ -154,27 +154,40 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev, > struct qxl_drm_image *dimage; > int ret; > > - ret = alloc_drawable(qdev, &release); > - if (ret) > - return; > - > - clips->x1 += dumb_shadow_offset; > - clips->x2 += dumb_shadow_offset; > - > - left = clips->x1; > - right = clips->x2; > + left = clips->x1 + dumb_shadow_offset; > + right = clips->x2 + dumb_shadow_offset; [Severity: High] Will this change break rendering on secondary monitors by omitting the offset from the hardware clipping rectangles? The original code modified clips->x1 and clips->x2 in place, which meant dumb_shadow_offset was automatically included when setting up the rects array later in the function. Now that clips is not mutated, rects gets the original un-offset coordinates: drivers/gpu/drm/qxl/qxl_draw.c:qxl_draw_dirty_fb() { ... clips_ptr = clips; for (i = 0; i < num_clips; i++, clips_ptr += inc) { rects[i].left = clips_ptr->x1; rects[i].right = clips_ptr->x2; ... } The hardware expects clip rectangles to match the shadow surface coordinates of the drawable. Because skipping this offset clips out the valid rendering area entirely, should dumb_shadow_offset be explicitly added to rects[i].left and rects[i].right? > top = clips->y1; > bottom = clips->y2; > > /* skip the first clip rect */ > for (i = 1, clips_ptr = clips + inc; > i < num_clips; i++, clips_ptr += inc) { > - left = min_t(int, left, (int)clips_ptr->x1); > - right = max_t(int, right, (int)clips_ptr->x2); > + left = min_t(int, left, (int)clips_ptr->x1 + > dumb_shadow_offset); > + right = max_t(int, right, (int)clips_ptr->x2 + > dumb_shadow_offset); > top = min_t(int, top, (int)clips_ptr->y1); > bottom = max_t(int, bottom, (int)clips_ptr->y2); > } > > + if (dumb_shadow_offset < 0 || dumb_shadow_offset > INT_MAX - fb->width) > + return; [Severity: Low] Is this bounds check operating as intended? Because dumb_shadow_offset is a uint32_t, the < 0 check is unconditionally false. Additionally, fb->width is unsigned, so does INT_MAX - fb->width result in an implicit unsigned calculation? While this may functionally protect against overflow due to core DRM limits on fb->width, it leaves the validation logic brittle. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
