Re: [PATCH 23/70] drm/sitronix: st7571: Switch to drm_atomic_helper_crtc_create_state

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

Pre-existing issues:
- [High] Unconditional 8-line page iterations in `st7571_transform_xy` cause an out-of-bounds read when `nlines` is not a multiple of 8.
- [High] Mismatch between framebuffer pitch during conversion and reading causes display corruption and invalid reads from `hwbuf`.
--

--- Patch [23]: [PATCH 23/70] drm/sitronix: st7571: Switch to drm_atomic_helper_crtc_create_state ---
commit cf528fe94124a1e49e93edb3680754d3976e2864
Author: Maxime Ripard <[email protected]>

drm/sitronix: st7571: Switch to drm_atomic_helper_crtc_create_state
    
The st7571 crtc implementation uses the deprecated
drm_atomic_helper_crtc_reset() as its reset hook. Switch to
drm_atomic_helper_crtc_create_state() instead.

> diff --git a/drivers/gpu/drm/sitronix/st7571.c b/drivers/gpu/drm/sitronix/st7571.c
> index 20954c33eca93..fc3ba5e8600f6 100644
> --- a/drivers/gpu/drm/sitronix/st7571.c
> +++ b/drivers/gpu/drm/sitronix/st7571.c
> @@ -424,7 +424,7 @@ static const struct drm_crtc_helper_funcs st7571_crtc_helper_funcs = {
>  };
>  
>  static const struct drm_crtc_funcs st7571_crtc_funcs = {
> -	.reset = drm_atomic_helper_crtc_reset,
> +	.atomic_create_state = drm_atomic_helper_crtc_create_state,
>  	.destroy = drm_crtc_cleanup,
>  	.set_config = drm_atomic_helper_set_config,
>  	.page_flip = drm_atomic_helper_page_flip,

[Severity: High]
This isn't a bug introduced by this patch, but does this code cause an
out-of-bounds read when nlines is not a multiple of 8?

In drivers/gpu/drm/sitronix/st7571.c:st7571_probe():
	st7571->hwbuf = devm_kzalloc(st7571->dev,
				     (st7571->nlines * st7571->ncols * st7571->bpp) / 8,
				     GFP_KERNEL);

If nlines is not a multiple of 8, hwbuf is allocated strictly based on the
unaligned nlines value.

During a display update:

drivers/gpu/drm/sitronix/st7571.c:st7571_fb_update_rect_monochrome():
	for (int y = rect->y1; y < rect->y2; y += ST7571_PAGE_HEIGHT) {

drivers/gpu/drm/sitronix/st7571.c:st7571_transform_xy():
	for (int i = 0; i < 8; i++) {
		int row_idx = y + i;
		u8 byte = p[row_idx * row_len + x];

This iterates 8 times vertically, which could read beyond the allocated buffer
if nlines is smaller than the assumed 8-line page height.


[Severity: High]
This is a pre-existing issue, but does a mismatch between the framebuffer
pitch during conversion and reading cause invalid reads from hwbuf?

In drivers/gpu/drm/sitronix/st7571.c:st7571_prepare_buffer_monochrome():
	dst_pitch = DIV_ROUND_UP(drm_rect_width(rect), 8);
	iosys_map_set_vaddr(&dst, st7571->hwbuf);
	drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect, fmtcnv_state);

The converted partial damage is tightly packed at offset 0 of hwbuf with a
dynamic pitch.

However, when reading the data:

drivers/gpu/drm/sitronix/st7571.c:st7571_fb_update_rect_monochrome():
	row[x] = st7571_transform_xy(st7571->hwbuf, x, y, 1);

drivers/gpu/drm/sitronix/st7571.c:st7571_transform_xy():
	u8 row_len = 16 * bpp;
	...
	int row_idx = y + i;
	u8 byte = p[row_idx * row_len + x];

st7571_transform_xy() accesses hwbuf using absolute coordinates and assumes a
full-screen layout with a fixed pitch, which seems to completely miss the
packed damage data.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=23