Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] The driver performs I2C transfers, which are sleeping functions, inside DRM atomic context callbacks.
- [High] The `hwbuf` allocation size calculation fails to account for row pitch alignment, leading to heap buffer overflows.
- [High] Missing format conversion state reservation in `atomic_check` causes a `GFP_KERNEL` allocation in the atomic update path.
- [High] Missing zero validation for display dimensions can lead to ZERO_SIZE_PTR dereference.
- [High] Missing `drm_dev_unplug()` causes `drm_dev_enter()` to fail to protect against Use-After-Free of devm-managed resources.
- [High] Missing `drm_dev_enter()`/`drm_dev_exit()` protection in encoder callbacks.
- [High] Improper handling of plane source coordinates causes Out-Of-Bounds memory accesses.
- [Medium] Hardware shutdown commands are sent before unregistering the DRM device, exposing the powered-off device to userspace commits.
- [Medium] Damage clip X coordinates are not rounded to byte boundaries, causing the monochrome conversion helper to corrupt the destination buffer.
- [Medium] `CH1115_CMD_OFFSET` is programmed with a value of 64, which exceeds the valid hardware limit.
--
--- Patch [2]: [PATCH v2 2/3] drm/ch1115: add support for Chipwealth CH1115 OLED controller ---
commit b8a12cbafccb463172c071e5d13f894de430373b
Author: Nicolás Antinori <[email protected]>
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
> --- /dev/null
> +++ b/drivers/gpu/drm/chipwealth/ch1115-i2c.c
[ ... ]
> +static int ch1115_oled_init(struct ch1115_device *ch1115)
> +{
[ ... ]
> + CH1115_CMD_OFFSET,
> + ch1115->flip_vertical
> + ? ch1115->y_start_offset
> + : 64 - ch1115->y_start_offset,
[Severity: Medium]
Is this offset calculation correct for when flip_vertical is false? If
y_start_offset is 0, this evaluates to 64. Display offset values for this
controller type are 6-bit and must be strictly between 0 and 63. Sending 64
might be out of bounds and could corrupt the internal display state machine.
[ ... ]
> +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]
Could this calculation result in an out-of-bounds heap memory access? The
damage rectangles returned by drm_atomic_helper_damage_iter_next are in
plane source coordinates. If a user sets a non-zero plane source offset or a
framebuffer larger than the panel, rect->x1 and rect->y1 can exceed the
panel dimensions, causing the absolute offsets to point outside the hwbuf
allocation. These coordinates should be translated to the plane's destination
coordinates first.
[Severity: Medium]
Are we guaranteed that rect->x1 and rect->x2 are aligned to byte boundaries?
The monochrome conversion helper drm_fb_xrgb8888_to_mono assumes the start
pixel maps to bit 0. If rect->x1 is not a multiple of 8, converted pixels
will be shifted improperly in the destination byte, corrupting the image and
overwriting adjacent pre-existing pixels outside the clip boundary.
> + drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect, fmtcnv_state);
> +}
[ ... ]
> +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]
Can this index exceed the bounds of the row array? Similar to the hwbuf offset
above, rect->x1 and rect->x2 can exceed the panel width if the plane source
coordinates are larger than the display, writing out of bounds into the heap.
[ ... ]
> +static int ch1115_primary_plane_helper_atomic_check(struct drm_plane *plane,
> + struct drm_atomic_commit *state)
> +{
[ ... ]
> + return drm_atomic_helper_check_plane_state(new_plane_state, new_crtc_state,
> + DRM_PLANE_NO_SCALING,
> + DRM_PLANE_NO_SCALING,
> + false, false);
> +}
[Severity: High]
Should we call drm_format_conv_state_reserve here? Because the driver uses
drm_fb_xrgb8888_to_mono during the atomic update phase, failing to reserve
the format conversion state here (where sleeping is allowed) forces a
GFP_KERNEL allocation inside the non-failable atomic update path.
[ ... ]
> +static void ch1115_primary_plane_helper_atomic_update(struct drm_plane *plane,
> + struct drm_atomic_commit *state)
> +{
[ ... ]
> + 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);
[Severity: High]
Does this sequence allocate memory in atomic context? Since
drm_format_conv_state_reserve is not called in atomic_check, the
drm_fb_xrgb8888_to_mono helper (invoked by ch1115_prepare_buffer) will attempt
a GFP_KERNEL allocation. This causes a scheduling while atomic panic.
> +
> + ch1115_fb_update_rect(fb, &damage);
[Severity: High]
Can this trigger a sleep while in atomic context? The ch1115_fb_update_rect
function issues I2C commands. The DRM subsystem strictly forbids sleeping in
plane atomic update callbacks, but i2c_transfer acquires a mutex and will
trigger a scheduling while atomic kernel panic.
> + }
> +
> + drm_dev_exit(idx);
[ ... ]
> +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);
[Severity: High]
Is it safe to issue I2C commands directly in the encoder enable callback? This
path executes in atomic context where sleeping is prohibited, but
ch1115_send_command_list triggers an i2c_transfer which can sleep.
[Severity: High]
Does this hardware access need drm_dev_enter and drm_dev_exit protection?
Unlike the plane update functions, the encoder enable callback accesses the
regmap directly without verifying if the device is still plugged in. If an
atomic commit races with device unplug, this attempts a use-after-free on
the devm-managed regmap.
> +}
> +
> +static void ch1115_encoder_atomic_disable(struct drm_encoder *encoder,
> + struct drm_atomic_commit *state)
> +{
> + struct drm_device *drm = encoder->dev;
> + struct ch1115_device *ch1115 = drm_to_ch1115(drm);
> +
> + u8 command = CH1115_CMD_OFF;
> +
> + ch1115_send_command_list(ch1115, &command, 1);
[Severity: High]
Does this sleeping function also violate the atomic context constraints? Just
like the enable callback, atomic_disable executes in atomic context and cannot
use i2c_transfer.
[Severity: High]
Is the regmap access protected here? Similar to the enable callback, this
requires drm_dev_enter and drm_dev_exit to avoid a use-after-free on device
unplug.
> +}
[ ... ]
> +static int ch1115_parse_dt(struct ch1115_device *ch1115)
> +{
[ ... ]
> + if (dt.hactive.typ > CH1115_MAX_WIDTH) {
> + dev_err(dev, "width=%d must be less than %d\n", dt.hactive.typ, CH1115_MAX_WIDTH);
> + return -EINVAL;
> + }
> + if (dt.vactive.typ > CH1115_MAX_HEIGHT) {
> + dev_err(dev, "height=%d must be less than %d\n", dt.vactive.typ, CH1115_MAX_HEIGHT);
> + return -EINVAL;
> + }
[Severity: High]
What happens if the device tree provides 0 for either of these dimensions?
The parsing validates the upper bounds but missing a check against zero
would cause the hwbuf and row allocations to request 0 bytes. This returns
the ZERO_SIZE_PTR (0x10), leading to a kernel panic upon dereference.
[ ... ]
> +static int ch1115_probe(struct i2c_client *client)
> +{
[ ... ]
> + ch1115->hwbuf = devm_kzalloc(&client->dev,
> + (ch1115->width * ch1115->height) / 8,
> + GFP_KERNEL);
[Severity: High]
Can this allocation be smaller than the required buffer size? The size is
calculated without padding, but ch1115_prepare_buffer converts formats
using a pitch of DIV_ROUND_UP(width, 8). If width is not a multiple of 8,
drm_fb_xrgb8888_to_mono will write past the end of the allocation.
[ ... ]
> +static void ch1115_remove(struct i2c_client *client)
> +{
[ ... ]
> + ret = ch1115_send_command_list(ch1115, cmd_list, ARRAY_SIZE(cmd_list));
> + if (ret < 0)
> + dev_err(&client->dev, "There was an error executing the shutdown commands");
> +
> + drm_dev_unregister(&ch1115->dev);
> + drm_atomic_helper_shutdown(&ch1115->dev);
> +}
[Severity: Medium]
Should drm_dev_unregister be called before turning off the hardware? By
shutting down the panel while it is still registered, there is a window
where userspace can perform atomic commits on a powered-off display, which
could leave the hardware in an inconsistent state.
[Severity: High]
Will using drm_dev_unregister here instead of drm_dev_unplug cause problems
for devm allocations? Calling drm_dev_unregister leaves the dev->unplugged
flag unset. This allows drm_dev_enter to succeed even after the device is
unbound. Delayed atomic commits could then bypass drm_dev_enter and access
the freed devm-managed memory.
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.