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 <[email protected]> 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/[email protected]?part=2
