Re: [PATCH] drm/bridge: ti-sn65dsi83: Cancel reset_work on remove to avoid use-after-free

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

Pre-existing issues:
- [High] Concurrent DRM commit can re-queue monitor_work after it has been cancelled, causing a use-after-free in polling mode.
- [High] SRCU read lock leak on error path in `sn65dsi83_reset_work` and swallowed errors in `sn65dsi83_reset_pipe`.
- [High] Missing cancel_work_sync(&ctx->reset_work) in the probe error path leads to a use-after-free.
--

--- Patch [1]: [PATCH] drm/bridge: ti-sn65dsi83: Cancel reset_work on remove to avoid use-after-free ---
commit 8ee8b1cf3acf86f0f88ed002c2cab55bb6d809f3
Author: Fan Wu <[email protected]>
    
drm/bridge: ti-sn65dsi83: Cancel reset_work on remove to avoid use-after-free
    
The error recovery code queues ctx->reset_work from the threaded IRQ
handler and the polling monitor_work, but nothing ever cancels it.

> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -1068,6 +1068,12 @@ static void sn65dsi83_remove(struct i2c_client *client)
>  {
>  	struct sn65dsi83 *ctx = i2c_get_clientdata(client);
>  
> +	/* Stop the reset_work producers, then drain, before unplug. */
> +	if (ctx->irq)
> +		disable_irq(ctx->irq);
> +	cancel_delayed_work_sync(&ctx->monitor_work);
> +	cancel_work_sync(&ctx->reset_work);
> +
>  	drm_bridge_unplug(&ctx->bridge);

[Severity: High]
This isn't a bug introduced by this patch, but could this ordering cause a 
use-after-free in polling mode?

Since drm_bridge_unplug() hasn't been called yet, bridge->unplugged is still 
false. If ctx->irq == 0 (polling mode), no IRQ is disabled. If a concurrent 
DRM atomic commit calls sn65dsi83_atomic_enable() in this window, it will 
successfully enter the bridge and call sn65dsi83_monitor_start(), which 
re-arms monitor_work.

When sn65dsi83_remove() finishes, devres frees ctx, and the newly queued work 
executes on freed memory. Should drm_bridge_unplug() be called before 
cancelling the works to act as a barrier?

While looking at the workqueue handling in this driver, I noticed a couple of 
other pre-existing issues. In sn65dsi83_reset_work():

drivers/gpu/drm/bridge/ti-sn65dsi83.c:sn65dsi83_reset_work() {
        ...
        ret = sn65dsi83_reset_pipe(ctx);
        if (ret) {
                dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret));
                return;
        }
        ...
}

[Severity: High]
This is a pre-existing issue, but does this early return leak the SRCU read 
lock? sn65dsi83_reset_work() calls drm_bridge_enter(), but this error path 
returns without calling drm_bridge_exit(idx).

Additionally, in sn65dsi83_reset_pipe():

drivers/gpu/drm/bridge/ti-sn65dsi83.c:sn65dsi83_reset_pipe() {
        ...
        drm_modeset_drop_locks(&ctx);
        drm_modeset_acquire_fini(&ctx);
        
        return 0;
}

The error code from drm_bridge_helper_reset_crtc() is swallowed and 
unconditionally returns 0, which masks the lock leak above, but instead causes 
sn65dsi83_reset_work() to proceed and re-enable the IRQ without the hardware 
being recovered. Could this lead to an immediate IRQ storm and an infinite 
workqueue loop?

Finally, regarding the probe error path in sn65dsi83_probe():

drivers/gpu/drm/bridge/ti-sn65dsi83.c:sn65dsi83_probe() {
        ...
err_remove_bridge:
        drm_bridge_remove(&ctx->bridge);
        return ret;
}

[Severity: High]
This is another pre-existing issue, but is it possible for reset_work to 
execute after ctx is freed here?

The IRQ is requested via devm_request_threaded_irq() before 
sn65dsi83_host_attach() is called. If a spurious interrupt fires while the 
chip is held in reset, an I2C read in the interrupt handler will fail, causing 
it to queue ctx->reset_work. 

If sn65dsi83_host_attach() then returns -EPROBE_DEFER, the code jumps to 
err_remove_bridge: and returns. Devres will free the IRQ and ctx. Because 
drm_bridge_remove() does not set bridge->unplugged = true, and there is no 
cancel_work_sync(&ctx->reset_work) here, the queued work could execute later 
and dereference the freed ctx memory.

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