Re: [PATCH 35/70] drm/tiny: sharp: Switch to drm_atomic_helper_crtc_create_state

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

Pre-existing issues:
- [High] The background kthread or PWM signal is not stopped on probe error paths, leading to Use-After-Free when the device memory is freed.
- [High] The driver does not check the return value of kthread_run(), which causes a kernel panic in the remove path if thread creation fails.
- [High] Plane atomic update callback calls sleeping functions (mutex_lock and spi_write) in an atomic context.
--

--- Patch [35]: [PATCH 35/70] drm/tiny: sharp: Switch to drm_atomic_helper_crtc_create_state ---
commit d365eb0228322a04306eb8553f60a438b3220d14
Author: Maxime Ripard <[email protected]>

drm/tiny: sharp: Switch to drm_atomic_helper_crtc_create_state

The sharp crtc implementation uses the deprecated
drm_atomic_helper_crtc_reset() as its reset hook. Switch to
drm_atomic_helper_crtc_create_state() instead.

> diff --git a/drivers/gpu/drm/tiny/sharp-memory.c b/drivers/gpu/drm/tiny/sharp-memory.c
> index 7efd7b567f3b2..434e73bd25a58 100644
> --- a/drivers/gpu/drm/tiny/sharp-memory.c
> +++ b/drivers/gpu/drm/tiny/sharp-memory.c
> @@ -345,7 +345,7 @@ static const struct drm_crtc_helper_funcs sharp_memory_crtc_helper_funcs = {
>  };
>  
>  static const struct drm_crtc_funcs sharp_memory_crtc_funcs = {
> -	.reset = drm_atomic_helper_crtc_reset,
> +	.atomic_create_state = drm_atomic_helper_crtc_create_state,

[Severity: High]
This isn't a bug introduced by this patch, but does the plane atomic update
callback call sleeping functions in an atomic context?

The DRM subsystem executes plane atomic update callbacks in an atomic
context where sleeping is forbidden, but sharp_memory_plane_atomic_update()
invokes the display update flow:

drivers/gpu/drm/tiny/sharp-memory.c:sharp_memory_plane_atomic_update() {
    ...
    sharp_memory_fb_dirty(plane_state->fb, shadow_plane_state->data,
                          &rect, &shadow_plane_state->fmtcnv_state);
    ...
}

which then calls sleeping functions unconditionally:

drivers/gpu/drm/tiny/sharp-memory.c:sharp_memory_update_display() {
    ...
    mutex_lock(&smd->tx_mutex);

    /* Populate the transmit buffer with frame data */
    ...
    ret = sharp_memory_spi_write(smd->spi, tx_buffer, tx_buffer_size);
    ...
}

Could this lead to kernel splats and potential deadlocks on any atomic
plane update from userspace?


[Severity: High]
This is a pre-existing issue, but does the probe error path return directly
without stopping the kthread or PWM?

In sharp_memory_probe(), the background kthread is started, but if subsequent
initialization steps fail, the function returns directly:

drivers/gpu/drm/tiny/sharp-memory.c:sharp_memory_probe() {
    ...
    smd->sw_vcom_signal = kthread_run(sharp_memory_sw_vcom_signal_thread,
                                      smd, "sw_vcom_signal");
    ...
    if (ret)
        return dev_err_probe(dev, ret, "Failed to initialize display pipeline.\n");
    ...
}

Can this lead to a Use-After-Free memory corruption if the smd structure is
freed by devres while the running kthread continues executing and
dereferencing the freed memory?


[Severity: High]
This is also a pre-existing issue, but is the return value of kthread_run()
used without checking IS_ERR()?

If kthread creation fails in sharp_memory_probe(), smd->sw_vcom_signal would
hold an ERR_PTR:

drivers/gpu/drm/tiny/sharp-memory.c:sharp_memory_probe() {
    ...
    smd->sw_vcom_signal = kthread_run(sharp_memory_sw_vcom_signal_thread,
                                      smd, "sw_vcom_signal");
    ...
}

Would this cause a kernel panic due to an invalid pointer dereference when
the driver is removed and kthread_stop() is unconditionally called?

drivers/gpu/drm/tiny/sharp-memory.c:sharp_memory_remove() {
    ...
    kthread_stop(smd->sw_vcom_signal);
    ...
}

>  	.destroy = drm_crtc_cleanup,
>  	.set_config = drm_atomic_helper_set_config,
>  	.page_flip = drm_atomic_helper_page_flip,
>  	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>  };

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