Re: [PATCH] drm/ssd130x: Set the address window in ssd132x_clear_screen()

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

Pre-existing issues:
- [High] 1-byte out-of-bounds read in ssd132x_update_rect() when the device width is configured to an odd number.
- [High] Pre-existing logic error in atomic_disable callbacks causes ssd132x_clear_screen() to be completely unreachable dead code, meaning the patch applies a fix to a code path that never executes.
--

--- Patch [1]: [PATCH] drm/ssd130x: Set the address window in ssd132x_clear_screen() ---
commit ce857c983f38da1567b74e3e8854ddf27505140a
Author: Fabio Piparo <[email protected]>

    drm/ssd130x: Set the address window in ssd132x_clear_screen()
    
    ssd132x_clear_screen() sends its zeroes without setting the column and
    row ranges first, so they land in whatever window the last
    ssd132x_update_rect() left programmed. A partial final update then
    leaves the rest of the screen uncleared, with fbcon the last damage is
    typically the cursor cell, so blanking keeps the console content on
    the panel.
    
    Set the full-screen window first, as ssd130x_clear_screen() already
    does.

> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 0b0fc6fe3df23..256d370e24034 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -900,9 +900,20 @@ static void ssd132x_clear_screen(struct ssd130x_device *ssd130x, u8 *data_array)
>  {

[Severity: High]
This is a pre-existing issue, but is ssd132x_clear_screen() ever actually
called during plane disable?

Looking at ssd132x_primary_plane_atomic_disable() where this is used, the
function returns early if the new plane state's crtc is NULL:

drivers/gpu/drm/solomon/ssd130x.c:ssd132x_primary_plane_atomic_disable() {
    ...
	if (!plane_state->crtc)
		return;
    ...
	ssd132x_clear_screen(ssd130x, ssd130x_crtc_state->data_array);
}

Since drm_atomic_get_new_plane_state() returns a state with a NULL crtc
pointer during a disable transition, doesn't this early return always
trigger, preventing the clear_screen fix from executing?

>  	unsigned int columns = DIV_ROUND_UP(ssd130x->width, SSD132X_SEGMENT_WIDTH);
>  	unsigned int height = ssd130x->height;
> +	int ret;
>  
>  	memset(data_array, 0, columns * height);
>  
> +	/* Set column start and end */
> +	ret = ssd130x_write_cmd(ssd130x, 3, SSD132X_SET_COL_RANGE, 0, columns - 1);
> +	if (ret < 0)
> +		return;
> +
> +	/* Set row start and end */
> +	ret = ssd130x_write_cmd(ssd130x, 3, SSD132X_SET_ROW_RANGE, 0, height - 1);
> +	if (ret < 0)
> +		return;
> +
>  	/* Write out update in one go since horizontal addressing mode is used */
>  	ssd130x_write_data(ssd130x, data_array, columns * height);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but can ssd132x_update_rect() read
out of bounds if the device tree configures an odd solomon,width (e.g., 127)?

In ssd132x_primary_plane_atomic_check(), the shadow buffer is allocated as
exactly width * height bytes for DRM_FORMAT_R8:

drivers/gpu/drm/solomon/ssd130x.c:ssd132x_primary_plane_atomic_check() {
    ...
	ssd130x_state->buffer = kcalloc(pitch, ssd130x->height, GFP_KERNEL);
    ...
}

In ssd132x_update_rect(), the loop increments by segment_width (2):

drivers/gpu/drm/solomon/ssd130x.c:ssd132x_update_rect() {
    ...
	for (i = 0; i < height; i++) {
		for (j = 0; j < width; j += segment_width) {
			u8 n1 = buf[i * width + j];
			u8 n2 = buf[i * width + j + 1];
    ...
}

If width is 127, on the last iteration of the last row (i = height - 1,
j = 126), doesn't buf[i * width + j + 1] read buf[height * 127], which is
1 byte past the end of the allocated buffer?

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