Re: [PATCH] drm/qxl: fix cursor OOB read, dirty-rect bounds, and monitors double-fetch

[sashiko-bot@kernel.org]

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 <benquike@gmail.com>
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/20260919215230.3469792-1-benquike@gmail.com?part=1