Re: [PATCH v5 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] core: uninitialized variable `bl` used in IS_ERR check
- [High] drm: out-of-bounds heap read in ch1115_transform_xy()
- [High] drm: sleeping in atomic context during plane and encoder callbacks
- [Medium] sysfs: unprotected state modification on registered I2C device
--

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

drm/ch1115: add support for Chipwealth CH1115 OLED controller
    
This commit adds DRM support for the Chipwealth CH1115 monochrome dot matrix
OLED controller connected via an I2C bus.

> diff --git a/drivers/gpu/drm/tiny/ch1115-i2c.c b/drivers/gpu/drm/tiny/ch1115-i2c.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/tiny/ch1115-i2c.c
[ ... ]
> +static inline u8 ch1115_transform_xy(const char *p, int x, int y, u8 bytes_per_row)
> +{
> +	int xrest = x % 8;
> +	u8 result = 0;
> +
> +	/*
> +	 * Transforms an (x, y) pixel coordinate into a vertical 8-bit
> +	 * column from the framebuffer. It calculates the corresponding byte in the
> +	 * framebuffer, extracts the bit at the given x position across 8 consecutive
> +	 * rows, and packs those bits into a single byte.
> +	 *
> +	 * Return an 8-bit value representing a vertical column of pixels.
> +	 */
> +
> +	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 result in an out-of-bounds memory read?

In ch1115_probe(), the hardware buffer ch1115->hwbuf is allocated with a size of
bytes_per_row * ch1115->height. 

Here, the loop unconditionally iterates 8 times to pack 8 vertical pixels. If
ch1115->height is not a multiple of 8, the final loop iterations will calculate
a row_idx that exceeds the allocated height, causing a heap out-of-bounds read.

> +		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)
> +{
> +	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 execute sleepable functions in atomic context?

Plane atomic_update callbacks run in atomic context during non-blocking commits,
so sleeping is forbidden. Here, drm_gem_fb_begin_cpu_access() is called, which
waits for fences and can sleep.

Additionally, this path calls ch1115_fb_update_rect() -> ch1115_draw_screen() ->
regmap_raw_write(), which executes a sleepable i2c_transfer(). This will cause
kernel warnings and potential deadlocks.

> +
> +	if (!drm_dev_enter(dev, &idx))
> +		goto out_drm_gem_fb_end_cpu_access;
[ ... ]
> +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 callback also sleep in atomic context?

Encoder atomic_enable callbacks also execute in atomic context. Calling
backlight_enable() attempts to acquire a mutex, and ch1115_oled_init() performs
sleepable I2C transfers. These operations are unsafe here.

> +}
[ ... ]
> +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);
> +
> +	dev_set_name(dev, "ch1115-i2c");

[Severity: Medium]
Is it safe to alter the kobject name of an actively registered device here?

The driver modifies the kobject name of the already-registered I2C client device
(dev = &client->dev) without using device_rename().

Modifying the name while the device is exposed in sysfs frees the existing name
string, creating a race condition that can lead to sysfs corruption and
use-after-free bugs if concurrently read.

> +
> +	i2c_set_clientdata(client, ch1115);
[ ... ]
> +	ch1115->hwbuf = devm_kzalloc(dev,
> +				     DIV_ROUND_UP(ch1115->width, 8) * ch1115->height,
> +				     GFP_KERNEL);
> +
> +	if (!ch1115->hwbuf)
> +		return -ENOMEM;
> +
> +	ch1115->row = devm_kzalloc(dev, ch1115->width, GFP_KERNEL);
> +	if (!ch1115->row)
> +		return -ENOMEM;
> +
> +	// Backlight
> +	const struct backlight_properties bl_props = {
> +		.type = BACKLIGHT_RAW,
> +		.brightness = ch1115->contrast,
> +		.max_brightness = CH1115_MAX_CONTRAST,
> +	};
> +
> +	ch1115->bl_dev = devm_backlight_device_register(dev, dev_name(dev), dev, ch1115,
> +							&ch1115_bl_ops, &bl_props);
> +	if (IS_ERR(bl))

[Severity: High]
Is the uninitialized variable bl being evaluated here instead of ch1115->bl_dev?

The result of devm_backlight_device_register() is stored in ch1115->bl_dev, but
the error check uses the uninitialized stack variable bl. This will evaluate
stack garbage, either causing spurious probe failures by returning garbage, or
bypassing the error check entirely if registration fails, guaranteeing a panic
when the ERR_PTR is dereferenced later.

> +		return dev_err_probe(dev, PTR_ERR(bl),
> +				     "Unable to register backlight device\n");
> +
> +	// DRM

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