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

<[email protected]> writes:

> Thank you for your contribution! Sashiko AI review found 2 potential issu=
e(s) to consider:
>
> Pre-existing issues:
> - [Critical] Missing cancel_work_sync() for ctx->reset_work during driver=
 teardown leads to a Use-After-Free.
> - [High] Skipping enable_irq() on the error path causes an IRQ disable de=
pth leak, leaving the interrupt permanently disabled.
> --
>
> --- Patch [2]: [PATCH v3 2/3] drm/bridge: ti-sn65dsi83: Fix error handlin=
g in sn65dsi83_reset_work() ---
> commit 58246b5ef131d7aeea7b5c2b693088e7255aa435
> Author: Esben Haabendal <[email protected]>
>
> drm/bridge: ti-sn65dsi83: Fix error handling in sn65dsi83_reset_work()
>
> This commit fixes the error path in sn65dsi83_reset_work() by adding a
> goto to ensure drm_bridge_exit() is called when sn65dsi83_reset_pipe()
> fails, safely closing the critical section.
>
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bri=
dge/ti-sn65dsi83.c
>> --- 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_struc=
t *ws)
>
> [Severity: Critical]
> This is a pre-existing issue, but could this work function execute after
> the device is unbound, resulting in a use-after-free?
>
> If the driver is removed and devres frees the ctx structure, the work
> could still be pending since sn65dsi83_remove() does not appear to call
> cancel_work_sync() before returning:
>
> sn65dsi83_remove() {
>         ...
>         drm_bridge_unplug(&ctx->bridge);
>         // missing cancel_work_sync(&ctx->reset_work);
> }
>
> If the workqueue executes sn65dsi83_reset_work() after teardown,
> dereferencing ctx for drm_bridge_enter() might access freed memory.
>
>>  	ret =3D 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 doesn't jumping to err_exit
> here leave the interrupt permanently disabled?
>
> If a hardware error triggers the IRQ handler, sn65dsi83_handle_errors()
> calls disable_irq_nosync(), which increments the IRQ disable depth. By
> skipping the enable_irq() call on this error path, the depth counter
> remains unbalanced.
>
> Would it be better to mask the interrupt source via hardware registers
> on failure, and still call enable_irq() so the kernel's depth counter
> remains balanced?

Nah. I would rather just balance the counter here.
Calling enable_irq() on error should be the safe choice.
As sn65dsi83_reset_pipe() have always been returning 0 until now, we
have in practise been doing that unconditionally anyway.

I will do that in next version.

>
>>  	}
>>  	if (ctx->irq)
>>  		enable_irq(ctx->irq);
>>
>> +err_exit:
>>  	drm_bridge_exit(idx);
>>  }
>
> --
> Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803-ti-sn65d=
[email protected]?part=3D2