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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Sleeping functions are called from DRM atomic context callbacks
- [High] Heap buffer overflow due to under-allocated unpadded shadow buffer
- [High] Heap buffer overflow in `row` and `hwbuf` due to Framebuffer vs. Panel coordinate mismatch
- [Medium] Display corruption due to unaligned damage rect X coordinates
- [Medium] Hardware misconfiguration due to out-of-range display offset calculation
--

commit cf38fdd0d34bcb3a6b00f5ba5e5408b34feba7a9
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/ch1115-i2c.c b/drivers/gpu/drm/chipwealth/ch1115-i2c.c
> new file mode 100644
> index 0000000000000..2ceb5e948e82f
> --- /dev/null
> +++ b/drivers/gpu/drm/chipwealth/ch1115-i2c.c
[ ... ]
> +static int ch1115_oled_init(struct ch1115_device *ch1115)
> +{
> +	u8 commands[] = {
> +		CH1115_CMD_OFF,
> +
> +		CH1115_CMD_CLOCK_DIVIDE_RATIO,
> +		CH1115_VAL_CLOCK_DIVIDE_RATIO(CH1115_VAL_CLOCK_OSC_FREQ,
> +					      CH1115_VAL_CLOCK_DIV_RATIO),
> +
> +		CH1115_CMD_MUX_RATIO,
> +		ch1115->height - 1,
> +
> +		CH1115_CMD_OFFSET,
> +		ch1115->flip_vertical
> +			? ch1115->y_start_offset
> +			: CH1115_MAX_HEIGHT - ch1115->y_start_offset,
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Is this offset calculation correct when y_start_offset is 0?

In ch1115_oled_init(), CH1115_MAX_HEIGHT - ch1115->y_start_offset evaluates
to 64 when y_start_offset is 0. However, the CH1115 offset register only
accepts values up to 63 (0x3F). Could sending an out-of-bounds value of 64
cause the hardware to misbehave or ignore the command?

[ ... ]
> +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);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Can this pointer arithmetic point out of bounds if the framebuffer viewport
is panned?

The damage iterator yields coordinates in absolute framebuffer (FB)
coordinates. If userspace configures a framebuffer larger than the panel
and pans the source viewport (e.g., src.x1 = 64), rect->x1 and rect->y1
will exceed the bounds of the panel-sized hwbuf. Does the driver need to
translate FB coordinates to panel-relative coordinates by subtracting
src.x1 and src.y1?

[Severity: Medium]
Could an unaligned rect->x1 cause screen corruption?

The damage rectangle Y coordinates are rounded to 8-pixel boundaries above,
but X coordinates are not. Passing an unaligned rect->x1 to
drm_fb_xrgb8888_to_mono() will shift the damaged pixels horizontally within
the target byte, clobbering adjacent unaffected pixels in the process.

> +	drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect, fmtcnv_state);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Will this cause a heap buffer overflow for unaligned display dimensions?

The shadow framebuffer hwbuf is allocated in ch1115_probe() as exactly
(width * height) / 8 bytes. If width or height isn't a multiple of 8
(e.g., 127x64), integer division truncates the allocation size. However,
dst_pitch is padded to DIV_ROUND_UP(width, 8). Calling
drm_fb_xrgb8888_to_mono() will write full padded rows, overflowing the
unpadded heap allocation.

[Severity: High]
Is it safe to trigger a GFP_KERNEL allocation here?

This is called from ch1115_primary_plane_helper_atomic_update(). According
to DRM subsystem guidelines, plane atomic_update callbacks run in atomic
context during non-blocking commits, where sleeping is forbidden. However,
drm_fb_xrgb8888_to_mono() can trigger memory allocations with GFP_KERNEL
inside the format conversion helper.

> +}
[ ... ]
> +static int ch1115_fb_update_rect(struct drm_framebuffer *fb, struct drm_rect *rect)
> +{
> +	struct ch1115_device *ch1115 = drm_to_ch1115(fb->dev);
> +	char *row = ch1115->row;
> +	int ret;
> +
> +	rect->y1 = round_down(rect->y1, CH1115_PAGE_HEIGHT);
> +	rect->y2 = min_t(unsigned int, round_up(rect->y2, CH1115_PAGE_HEIGHT), ch1115->height);
> +
> +	for (int y = rect->y1; y < rect->y2; y += CH1115_PAGE_HEIGHT) {
> +		for (int x = rect->x1; x < rect->x2; x++)
> +			row[x] = ch1115_transform_xy(ch1115->hwbuf, x, y, ch1115->bytes_per_row);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this array access write out of bounds when the CRTC is panned?

Since row is allocated to exactly ch1115->width bytes, an absolute
framebuffer coordinate like rect->x1 exceeding the panel width will cause
row[x] to corrupt kernel heap memory. Should the loop index be translated
to panel-relative coordinates?

> +
> +		ret = ch1115_set_position(ch1115, ch1115->x_start_offset + rect->x1, y);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = ch1115_draw_screen(ch1115, row + rect->x1, rect->x2 - rect->x1);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static void ch1115_primary_plane_helper_atomic_update(struct drm_plane *plane,
> +						      struct drm_atomic_commit *state)
> +{
> +	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> +	struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
> +	struct drm_framebuffer *fb = plane_state->fb;
> +	struct drm_atomic_helper_damage_iter iter;
> +	struct drm_device *dev = plane->dev;
> +	struct drm_rect damage;
> +	struct ch1115_device *ch1115 = drm_to_ch1115(plane->dev);
> +	int idx;
> +
> +	if (!fb)
> +		return;
> +
> +	if (drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE))
> +		return;
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this callback sleep in atomic context?

drm_gem_fb_begin_cpu_access() can block to wait on DMA fences. Since
ch1115_primary_plane_helper_atomic_update() runs in atomic context during
non-blocking commits, this can cause might_sleep() warnings and system
deadlocks.

> +
> +	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]
Will this initiate blocking I2C transfers from atomic context?

ch1115_fb_update_rect() calls ch1115_set_position() and ch1115_draw_screen(),
which perform regmap writes over I2C. I2C transfers lock the bus mutex and
can sleep, which is forbidden in atomic update paths.

> +	}
[ ... ]
> +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]
Is it safe to acquire the backlight mutex here?

ch1115_encoder_atomic_enable() runs in atomic context. Calling
backlight_enable() acquires the backlight update_lock mutex, which
will sleep and violate the atomic context constraints.

> +}
[ ... ]
> +static int ch1115_probe(struct i2c_client *client)
> +{
> +	struct ch1115_device *ch1115;
> +	struct device *dev;
> +	struct backlight_device *bl;
> +	int ret;
> +
> +	dev = &client->dev;
> +	ch1115 = devm_drm_dev_alloc(&client->dev, &ch1115_driver,
> +				    struct ch1115_device, dev);
> +	if (IS_ERR(ch1115))
> +		return PTR_ERR(ch1115);
[ ... ]
> +	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]
Could this allocation be undersized for unaligned display dimensions?

When width or height is not a multiple of 8, integer division truncates
the size, leaving hwbuf too small to hold the padded rows written by
ch1115_prepare_buffer().

> +	if (!ch1115->hwbuf)
> +		return -ENOMEM;
> +
> +	ch1115->row = devm_kzalloc(dev, ch1115->width, GFP_KERNEL);
> +	if (!ch1115->row)
> +		return -ENOMEM;

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