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

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

New issues:
- [High] Missing `disable_irq()` in `sn65dsi83_detach()` causes unbalanced IRQ state and Use-After-Free of DRM pipeline objects.

Pre-existing issues:
- [Medium] `sn65dsi83_reset_pipe()` silently discards errors from `drm_bridge_helper_reset_crtc()`.
--

--- Patch [1]: [PATCH v3] drm/bridge: ti-sn65dsi83: Cancel reset_work on remove to avoid use-after-free ---
commit 71f81476ddab3b26071a54fac540bd7518b097ca
Author: Fan Wu <[email protected]>

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

This commit addresses a use-after-free vulnerability by properly canceling
reset_work and monitor_work during device removal and probe failure paths.
It also ensures the IRQ is disabled before unplugging the bridge to
prevent handlers from executing after teardown.

> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index f9fdbf48c6b32..8f17fd319cf90 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -298,9 +298,14 @@ static int sn65dsi83_attach(struct drm_bridge *bridge,
>  			    enum drm_bridge_attach_flags flags)
>  {
>  	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
> +	int ret;
> +
> +	ret = drm_bridge_attach(encoder, ctx->panel_bridge, &ctx->bridge,
> +				flags);
> +	if (!ret && ctx->irq)
> +		enable_irq(ctx->irq);
>  
> -	return drm_bridge_attach(encoder, ctx->panel_bridge,
> -				 &ctx->bridge, flags);
> +	return ret;
>  }
>  
>  static void sn65dsi83_detach(struct drm_bridge *bridge)
>  {
>  	struct sn65dsi83 *ctx = bridge_to_sn65dsi83(bridge);
>  
>  	if (!ctx->dsi)
>  		return;
>  
>  	ctx->dsi = NULL;
>  }

[Severity: High]
Does this require a symmetric disable_irq() in sn65dsi83_detach()?

If the master DRM driver unbinds, drm_bridge_detach() is called, removing
the bridge from the encoder chain. If we don't disable the IRQ here, the
hardware interrupt remains active.

If the master DRM driver binds again, sn65dsi83_attach() will call
enable_irq() on an already-enabled IRQ, which could trigger an unbalanced
enable warning.

More critically, if a hardware interrupt triggers while the bridge is
detached, sn65dsi83_irq() schedules reset_work. The work executes
drm_bridge_helper_reset_crtc(), which could dereference a freed encoder:

drm_bridge_helper_reset_crtc()
{
	struct drm_encoder *encoder = bridge->encoder;
	struct drm_device *dev = encoder->dev;
...

[ ... ]

[Severity: Medium]
This is a pre-existing issue, but sn65dsi83_reset_pipe() appears to silently
discard errors from drm_bridge_helper_reset_crtc():

sn65dsi83_reset_pipe() {
	err = drm_bridge_helper_reset_crtc(&sn65dsi83->bridge, &ctx);
	if (err == -EDEADLK) {
		drm_modeset_backoff(&ctx);
		goto retry;
	}

	drm_modeset_drop_locks(&ctx);
	drm_modeset_acquire_fini(&ctx);

	return 0;
}

Since sn65dsi83_reset_work() assumes the pipe reset was successful and
calls enable_irq(), could this immediately re-trigger the hardware interrupt
if the reset actually failed, potentially causing an IRQ storm?

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