Re: [PATCH v4 2/3] drm/ch1115: add support for Chipwealth CH1115 OLED controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Heap buffer overflow due to undersized `hwbuf` allocation.
- [High] Calling sleeping functions in atomic context.
- [High] Missing required Kconfig dependencies leading to build failures or NULL pointer dereferences.
- [High] Visual corruption due to horizontally unaligned damage rectangles.
--

commit fe052a7a7d4de23b8d160888c9f118bbd31d156e
Author: Nicolás Antinori <nico.antinori.7@gmail.com>

    drm/ch1115: add support for Chipwealth CH1115 OLED controller
    
    Chipwealth CH1115 is a controller for monochrome dot matrix OLED panels.
    
    Add DRM support for this chip using i2c bus.

> diff --git a/drivers/gpu/drm/chipwealth/Kconfig b/drivers/gpu/drm/chipwealth/Kconfig
> new file mode 100644
> index 0000000000000..1cc5adc1e28e0
> --- /dev/null
> +++ b/drivers/gpu/drm/chipwealth/Kconfig
> @@ -0,0 +1,16 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config DRM_CH1115_I2C
> +	tristate "DRM support for Ch1115 OLED panels (I2C BUS)"
> +	depends on DRM && I2C

[Severity: High]
Is there a regression here due to missing Kconfig dependencies?

The driver code calls of_get_display_timing(), which requires
CONFIG_OF_VIDEOMODE. If not enabled, it returns -ENOSYS and probe fails
unconditionally.

Additionally, if CONFIG_BACKLIGHT_CLASS_DEVICE is compiled as a module (=m)
and this driver is built-in (=y), linking will fail due to unresolved symbols.

Should this include "depends on OF" (or OF_VIDEOMODE) and
"select BACKLIGHT_CLASS_DEVICE"?

> +	select DRM_CLIENT_SELECTION
> +	select DRM_GEM_SHMEM_HELPER
> +	select DRM_KMS_HELPER
> +	select REGMAP_I2C
> +	select VIDEOMODE_HELPERS

[ ... ]

> diff --git a/drivers/gpu/drm/chipwealth/ch1115-i2c.c b/drivers/gpu/drm/chipwealth/ch1115-i2c.c
> new file mode 100644
> index 0000000000000..ddcf25d19b5e0
> --- /dev/null
> +++ b/drivers/gpu/drm/chipwealth/ch1115-i2c.c
[ ... ]
> +static void ch1115_prepare_buffer(struct ch1115_device *ch1115,
> +				  const struct iosys_map *vmap,
> +				  struct drm_framebuffer *fb,
> +				  struct drm_rect *rect,
> +				  struct drm_format_conv_state *fmtcnv_state)
> +{
> +	unsigned int dst_pitch;
> +	struct iosys_map dst;
> +
> +	dst_pitch = DIV_ROUND_UP(ch1115->width, 8);
> +	rect->y1 = round_down(rect->y1, CH1115_PAGE_HEIGHT);
> +	rect->y2 = min_t(unsigned int, round_up(rect->y2, CH1115_PAGE_HEIGHT), ch1115->height);
> +
> +	iosys_map_set_vaddr(&dst, ch1115->hwbuf + rect->y1 * dst_pitch + rect->x1 / 8);
> +	drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect, fmtcnv_state);

[Severity: High]
Does this code introduce a regression causing visual corruption by passing
horizontally unaligned rectangles to the conversion helper?

This rounds the damage rectangle vertically, but neglects to round it
horizontally (rect->x1, rect->x2). If rect->x1 is not a multiple of 8,
drm_fb_xrgb8888_to_mono() starts reading from the unaligned rect->x1 and
packs those pixels starting at the 0th bit of the given destination address.

This will overwrite unrelated pixels and scramble the image in that byte.
When ch1115_transform_xy() later reads these bytes assuming correct alignment,
the display will output corrupted artifacts. Should rect->x1 and rect->x2 be
rounded to multiples of 8?

Additionally, drm_fb_xrgb8888_to_mono() writes full rows based on the rounded
up pitch, which can lead to an out-of-bounds write if the buffer is undersized.

> +}
> +
> +static inline u8 ch1115_transform_xy(const char *p, int x, int y, u8 bytes_per_row)
> +{
> +	int xrest = x % 8;
> +	u8 result = 0;
[ ... ]
> +	x = x / 8;
> +	y = (y / 8) * 8;
> +
> +	for (int i = 0; i < 8; i++) {
> +		int row_idx = y + i;
> +		u8 byte = p[row_idx * bytes_per_row + x];

[Severity: High]
Can this loop introduce a regression by performing out-of-bounds reads when
iterating through the vertical page blocks?

If width or height are not multiples of 8, the allocated hwbuf size may be
too small because it stores data with a pitch of DIV_ROUND_UP(width, 8) and
up to round_up(height, 8) rows. This could cause this loop to read past the
end of the buffer.

> +		u8 bit = (byte >> xrest) & 1;
> +
> +		result |= (bit << i);
> +	}
> +
> +	return result;
> +}
[ ... ]
> +static void ch1115_primary_plane_helper_atomic_update(struct drm_plane *plane,
> +						      struct drm_atomic_commit *state)
> +{
[ ... ]
> +	if (!fb)
> +		return;
> +
> +	if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE))
> +		return;
> +
> +	if (!drm_dev_enter(dev, &idx))
> +		goto out_drm_gem_fb_end_cpu_access;
> +
> +	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
> +	drm_atomic_for_each_plane_damage(&iter, &damage) {
> +		ch1115_prepare_buffer(ch1115,
> +				      &shadow_plane_state->data[0],
> +				      fb, &damage,
> +				      &shadow_plane_state->fmtcnv_state);
> +
> +		ch1115_fb_update_rect(fb, &damage);

[Severity: High]
Does this code introduce a regression by calling sleeping functions in an
atomic context?

Plane atomic update callbacks execute in atomic context during non-blocking
commits and must not sleep.

Here, drm_gem_fb_begin_cpu_access() is called, which can sleep. Additionally,
ch1115_fb_update_rect() triggers I2C transfers via regmap, which is also a
sleeping function.

Executing I2C transfers, mutex locks, and CPU syncs in atomic context can
cause scheduling while atomic panics and potential deadlocks.

> +	}
> +
> +	drm_dev_exit(idx);
> +
> +out_drm_gem_fb_end_cpu_access:
> +	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
> +}
[ ... ]
> +static void ch1115_encoder_atomic_enable(struct drm_encoder *encoder,
> +					 struct drm_atomic_commit *state)
> +{
> +	struct drm_device *drm = encoder->dev;
> +	struct ch1115_device *ch1115 = drm_to_ch1115(drm);
> +
> +	ch1115_oled_init(ch1115);
> +
> +	u8 command = CH1115_CMD_ON;
> +
> +	ch1115_send_command_list(ch1115, &command, 1);
> +
> +	backlight_enable(ch1115->bl_dev);

[Severity: High]
Does this cause a regression by calling a sleeping backlight API inside an
atomic encoder callback?

Encoder atomic enable and disable callbacks run in atomic context during
non-blocking commits. 

backlight_enable() acquires a mutex internally, and both this and the I2C
transfers above will sleep, which violates the atomic context requirements.

> +}
[ ... ]
> +static int ch1115_probe(struct i2c_client *client)
> +{
[ ... ]
> +	ch1115->pages = DIV_ROUND_UP(ch1115->height, CH1115_PAGE_HEIGHT);
> +	ch1115->bytes_per_row = DIV_ROUND_UP(ch1115->width, 8);
> +	ch1115->x_start_offset = ch1115->flip_horizontal
> +		? CH1115_MAX_WIDTH - ch1115->width
> +		: 0;
> +	ch1115->mode = ch1115_mode(ch1115);
> +
> +	ch1115->hwbuf = devm_kzalloc(dev, (ch1115->width * ch1115->height) / 8,
> +				     GFP_KERNEL);

[Severity: High]
Does this buffer allocation introduce a regression by failing to account for
row pitch padding and page alignment?

The hwbuf is allocated as (width * height) / 8 bytes. If width or height
are not multiples of 8, this size is too small. 

The buffer stores data with a pitch of DIV_ROUND_UP(width, 8) and up to
round_up(height, 8) rows. For example, if width=12 and height=12, the
allocated size is 18 bytes. 

Later, drm_fb_xrgb8888_to_mono() writes 12 rows with a pitch of 2
(24 bytes), causing an out-of-bounds write of 6 bytes. This memory
corruption can lead to system instability.

> +	if (!ch1115->hwbuf)
> +		return -ENOMEM;
> +
> +	ch1115->row = devm_kzalloc(dev, ch1115->width, GFP_KERNEL);
> +	if (!ch1115->row)
> +		return -ENOMEM;
> +
> +	// Backlight
> +	bl = devm_backlight_device_register(dev, dev_name(dev), dev, ch1115,
> +					    &ch1115_bl_ops, NULL);
> +	if (IS_ERR(bl))
> +		return dev_err_probe(dev, PTR_ERR(bl),
> +				     "Unable to register backlight device\n");
> +
> +	bl->props.brightness = ch1115->contrast;

[Severity: High]
Can this introduce a regression where bl is NULL, causing a NULL pointer
dereference?

If CONFIG_BACKLIGHT_CLASS_DEVICE is disabled, the fallback stub for
devm_backlight_device_register() returns NULL. 

Since IS_ERR(bl) does not check for NULL, the next line will dereference
a NULL pointer.

> +	bl->props.max_brightness = CH1115_MAX_CONTRAST;
> +	ch1115->bl_dev = bl;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788711082.git.nico.antinori.7@gmail.com?part=2