Re: [PATCH v2 2/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The error handling branch patched by this commit is dead code because sn65dsi83_reset_pipe() unconditionally returns 0.
- [High] Missing cancellation of reset_work on driver removal leads to a potential Use-After-Free.
--

--- Patch [2]: [PATCH v2 2/3] drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work() ---
commit da59dc684f40189c9b3344ad029c137b580d644a
Author: Esben Haabendal <esben@geanix.com>

drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work()

This commit updates the error handling path in sn65dsi83_reset_work() to
ensure the DRM bridge critical section is cleanly exited when resetting the
pipe fails.

> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index b4b220eee7909..7e73035d77986 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -419,11 +419,12 @@ static void sn65dsi83_reset_work(struct work_struct *ws)
>  	ret = sn65dsi83_reset_pipe(ctx);
>  	if (ret) {
>  		dev_err(ctx->dev, "reset pipe failed %pe\n", ERR_PTR(ret));
> -		return;
> +		goto err_exit;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but is this error handling block
actually dead code?

Looking at sn65dsi83_reset_pipe(), it appears to unconditionally return 0,
even if drm_bridge_helper_reset_crtc() returns an error like -EINVAL or
-ENOMEM:

static int sn65dsi83_reset_pipe(struct sn65dsi83 *sn65dsi83)
{
	...
retry:
	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 ret is always 0, this error branch will never execute. If an actual
hardware error persists (e.g., PLL unlock), the driver will unconditionally
proceed to enable_irq(ctx->irq) below. 

Could this result in an immediate subsequent interrupt and cause an interrupt
storm that locks up the CPU?

>  	if (ctx->irq)
>  		enable_irq(ctx->irq);
>  
> +err_exit:
>  	drm_bridge_exit(idx);
>  }

[Severity: High]
This is also a pre-existing issue, but does this workqueue function have a
use-after-free risk during driver removal?

When the driver is unbound (e.g., via sysfs), sn65dsi83_remove() unregisters
the bridge but does not appear to call cancel_work_sync(&ctx->reset_work).

If an interrupt fires right before cleanup and schedules this work,
devm_drm_bridge_alloc cleanup will free the ctx structure. The workqueue
could then execute sn65dsi83_reset_work() asynchronously, causing a UAF
when dereferencing ctx->bridge.

Should sn65dsi83_remove() be updated to cancel the pending reset work?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-ti-sn65dsi83-fixes-v2-0-ebc4c3fe29b6@geanix.com?part=2