drm/vkms: fix vertical stride miscalculation in get_block_step_bytes()
PATCH 1/1 drm/vkms: fix vertical stride miscalculation in get_block_step_bytes()
get_block_step_bytes() computes the byte distance between two vertically adjacent pixel blocks for the READ_TOP_TO_BOTTOM and READ_BOTTOM_TO_TOP directions as:
pitch * drm_format_info_block_width(fb->format, plane_index)
This is wrong: the vertical distance between two rows of blocks is the block *pitch*, i.e. pitch multiplied by the block *height*, not the block width. This is exactly how packed_pixels_offset() computes it a few lines above in the same file:
int block_pitch = fb->pitches[plane_index] * drm_format_info_block_height(format, plane_index);
For 1x1-block formats (block_width == block_height == 1) the two expressions happen to coincide, which is why the bug went unnoticed for regular formats. Packed sub-byte formats break the assumption:DRM_FORMAT_R1 has block_width == 8 and block_height == 1, so the vertical step computed by get_block_step_bytes() is 8x too large.
This over-large step is consumed verbatim by Rx_read_line(), which advances src_pixels by step once per output row without ever re-validating the result against the framebuffer object bounds (those bounds were only checked assuming a correct, 1x, step):
src_pixels += step;
so reading a plane in the vertical direction (READ_TOP_TO_BOTTOM / READ_BOTTOM_TO_TOP, reached via a plane rotation of 90 or 270 degrees) walks off the end of the source buffer object after very few rows and over-reads adjacent kernel memory (an out-of-bounds read, not a write). With DRM_FORMAT_R1 and a writeback connector attached, this is reachable by a local process that holds DRM master on a loaded vkms device (composition happens synchronously in vkms_composer_worker()).
Fix get_block_step_bytes() to use drm_format_info_block_height() for the vertical directions, which matches packed_pixels_offset() and restores the invariant that the vertical step is exactly one row of blocks.
Runtime-verified on a v6.19 KASAN (KASAN_VMALLOC) stand: before this fix, a writeback commit on a rotated DRM_FORMAT_R1 plane reliably tripped a KASAN-attributed out-of-bounds-read page fault in Rx_read_line() during vkms_composer_worker(); with this fix applied, the identical reproducer completes cleanly with no fault and no KASAN report.
--- drivers/gpu/drm/vkms/vkms_formats.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/gpu/drm/vkms/vkms_formats.c +4 -4
@@ -103,11 +103,11 @@ static int get_block_step_bytes(struct drm_framebuffer *fb, enum pixel_read_direcase 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 ;
Signature · 1 line
2.50.1 (Apple Git-155)
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] An integer overflow in packed_pixels_offset() causes an out-of-bounds read of kernel memory, which can be leaked to userspace via the writeback connector. --
--- Patch [1]: [PATCH] drm/vkms: fix vertical stride miscalculation in get_block_step_bytes() --- commit a61fac2777668983a99bbe0fb747107d5091181a
Author: Ibrahim Hashimov <[email protected]>
drm/vkms: fix vertical stride miscalculation in get_block_step_bytes()
This commit fixes a vertical stride miscalculation in get_block_step_bytes() by replacing block width with block height for vertical directions. This corrects an out-of-bounds read and restores the invariant that the vertical step is exactly one row of blocks.
Quoted 5 lines
drm/vkms: fix vertical stride miscalculation in
get_block_step_bytes()
get_block_step_bytes()computes the byte distance between two vertically adjacent pixel blocks for theREAD_TOP_TO_BOTTOMandREAD_BOTTOM_TO_TOPdirections as:
[ ... ]
Quoted 5 lines
This is exactly how
packed_pixels_offset()computes it a few lines above in the same file:int block_pitch = fb->pitches[plane_index] * drm_format_info_block_height(format, plane_index);
[Severity: High]
This is a pre-existing issue, but does the calculation in packed_pixels_offset() suffer from a signed integer overflow?
In drivers/gpu/drm/vkms/vkms_formats.c:packed_pixels_offset(), both block_y and block_pitch are evaluated as signed 32-bit integers. While the DRM core restricts height * pitch <= UINT_MAX, it does not prevent the product from exceeding INT_MAX.
If a valid framebuffer is created with height = 8192 and pitch = 524287, the y coordinate can go up to 8191. The resulting offset calculation:
*offset = fb->offsets[plane_index] +
block_y * block_pitch +
block_x * format->char_per_block[plane_index];would cause block_y * block_pitch to exceed 2,147,483,647 and overflow into a negative value.
Could this resulting negative offset cause the blending logic to read from out-of-bounds kernel memory, potentially leaking it to userspace via the writeback connector?
Quoted 2 lines
For 1x1-block formats (
block_width==block_height== 1) the two expressions happen to coincide, which is why the bug went unnoticed
[ ... ]
Quoted 4 lines
drivers/gpu/drm/vkms/vkms_formats.c +0
-0
[ ... ]
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1