Re: [PATCH] drm/vkms: Fix vertical read step for R1/R2/R4 formats

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Integer overflow in `packed_pixels_offset()` causes a massive out-of-bounds kernel memory read.
--

--- Patch [1]: [PATCH] drm/vkms: Fix vertical read step for R1/R2/R4 formats ---
commit 27fa0b8f7b4814c63740fbad03cf74d10431bd82
Author: Shixiong Ou <oushixiong@kylinos.cn>

drm/vkms: Fix vertical read step for R1/R2/R4 formats

This commit fixes the vertical read step for R1/R2/R4 formats by using the
block height instead of block width to calculate byte offsets. This prevents
out-of-bounds reads and incorrect sampling when rotating or reflecting planes.

> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index dfb8e13cba87a..1e3a6af9cea52 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -103,11 +103,11 @@ static int get_block_step_bytes(struct drm_framebuffer *fb, enum pixel_read_dire
>  	case READ_RIGHT_TO_LEFT:
>  		return -fb->format->char_per_block[plane_index];
>  	case READ_TOP_TO_BOTTOM:
> -		return (int)fb->pitches[plane_index] * drm_format_info_block_width(fb->format,
> -										   plane_index);
> -	case READ_BOTTOM_TO_TOP:
> -		return -(int)fb->pitches[plane_index] * drm_format_info_block_width(fb->format,
> +		return (int)fb->pitches[plane_index] * drm_format_info_block_height(fb->format,
>  										    plane_index);
> +	case READ_BOTTOM_TO_TOP:
> +		return -(int)fb->pitches[plane_index] * drm_format_info_block_height(fb->format,
> +										     plane_index);
>  	}
>  
>  	return 0;

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential integer
overflow in packed_pixels_offset() in the same file.

If a user configures a framebuffer with a specific height and pitch (e.g.,
height = 3 and pitch = 0x40000000), this satisfies the bounds check in
framebuffer_check(), but might cause an issue during VKMS software composition:

drivers/gpu/drm/vkms/vkms_formats.c:packed_pixels_offset() {
...
	int block_y = y / drm_format_info_block_height(format, plane_index);
	int block_pitch = fb->pitches[plane_index] * drm_format_info_block_height(format,
										  plane_index);
	*rem_x = x % drm_format_info_block_width(format, plane_index);
	*rem_y = y % drm_format_info_block_height(format, plane_index);
	*offset = fb->offsets[plane_index] +
		  block_y * block_pitch +
		  block_x * format->char_per_block[plane_index];
...
}

Since block_pitch and block_y are signed 32-bit integers, could the
multiplication of block_y * block_pitch overflow into a negative value?

This negative offset propagates to *offset, and later in packed_pixels_addr():

drivers/gpu/drm/vkms/vkms_formats.c:packed_pixels_addr() {
...
	packed_pixels_offset(frame_info, x, y, plane_index, &offset, rem_x, rem_y);
	*addr = (u8 *)frame_info->map[0].vaddr + offset;
}

Could adding this signed 32-bit integer offset to the 64-bit virtual address
pointer trigger sign-extension, effectively subtracting a large amount from
the base address and allowing an out-of-bounds kernel memory read?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820083123.544655-3-oushixiong1025@163.com?part=1