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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Interrupt storm between drm_bridge_unplug() and disable_irq()

Pre-existing issues:
- [High] Spurious IRQ during probe causes NULL pointer dereference in sn65dsi83_reset_work()
- [High] Leaked SRCU read lock in sn65dsi83_reset_work() error path
- [High] Missing IRQ synchronization in sn65dsi83_release_resources() causes race condition
--

--- Patch [1]: [PATCH v2] drm/bridge: ti-sn65dsi83: Cancel reset_work on remove to avoid use-after-free ---
commit 197f183cb3d12c2d5d1810b8f11c167e77438773
Author: Fan Wu <fanwu01@zju.edu.cn>

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

The error recovery code queues reset_work from the threaded IRQ handler
and polling monitor_work. Neither the remove path nor the probe failure
path after the IRQ is registered drains that work before devres releases
the bridge allocation.

> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> index f9fdbf48c6b32..bf263f407cb2c 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> @@ -996,6 +996,18 @@ static irqreturn_t sn65dsi83_irq(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static void sn65dsi83_stop_error_recovery(struct sn65dsi83 *ctx)
> +{
> +	/* Block new bridge users and wait for existing critical sections. */
> +	drm_bridge_unplug(&ctx->bridge);
> +
> +	if (ctx->irq)
> +		disable_irq(ctx->irq);

[Severity: High]
Could this ordering cause an interrupt storm leading to a CPU lockup?

When drm_bridge_unplug() sets bridge->unplugged = true, if an IRQ fires before
disable_irq() is called, sn65dsi83_handle_errors() will see drm_bridge_enter()
fail and return early:

drivers/gpu/drm/bridge/ti-sn65dsi83.c:sn65dsi83_handle_errors() {
    ...
	if (!drm_bridge_enter(&ctx->bridge, &idx))
		return;
    ...
}

Since this threaded IRQ handler returns IRQ_HANDLED without clearing the
hardware interrupt or disabling the IRQ, will the IRQ core unmask the
still-asserted interrupt line, causing it to immediately fire again and
starve the teardown code?

[Severity: High]
This is a pre-existing issue, but will the addition of drm_bridge_unplug()
here cause a system deadlock during driver unbind or probe failure?

In the error path of sn65dsi83_reset_work(), if resetting the pipe fails,
it returns early:

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;
	}
    ...
}

This early return skips drm_bridge_exit(idx), permanently leaking the SRCU
read lock. If that happens, won't this new drm_bridge_unplug() call block
forever in synchronize_srcu()?

> +
> +	cancel_delayed_work_sync(&ctx->monitor_work);
> +	cancel_work_sync(&ctx->reset_work);
> +}
> +
>  static int sn65dsi83_probe(struct i2c_client *client)
>  {
[ ... ]
> @@ -1060,7 +1072,7 @@ static int sn65dsi83_probe(struct i2c_client *client)
>  	return 0;
>  
>  err_remove_bridge:
> -	drm_bridge_remove(&ctx->bridge);
> +	sn65dsi83_stop_error_recovery(ctx);

[Severity: High]
This is a pre-existing issue, but can a spurious interrupt during probe
cause a NULL pointer dereference?

If a spurious interrupt fires after devm_request_threaded_irq() is called
in sn65dsi83_probe() but before the KMS driver calls drm_bridge_attach(),
the threaded handler will schedule reset_work.

The worker then executes sn65dsi83_reset_pipe() which calls:

drivers/gpu/drm/drm_bridge_helper.c:drm_bridge_helper_reset_crtc() {
    ...
	struct drm_device *dev = encoder->dev;
    ...
}

Since the bridge is not attached yet, bridge->encoder is NULL. Does this
unconditionally crash if an early interrupt occurs?

>  	return ret;
>  }
>  
> @@ -1068,7 +1080,7 @@ static void sn65dsi83_remove(struct i2c_client *client)
>  {
>  	struct sn65dsi83 *ctx = i2c_get_clientdata(client);
>  
> -	drm_bridge_unplug(&ctx->bridge);
> +	sn65dsi83_stop_error_recovery(ctx);
>  }

[Severity: High]
This is a pre-existing issue, but does sn65dsi83_release_resources() contain
a race condition with the IRQ handler?

During display teardown, sn65dsi83_release_resources() disables the hardware
interrupt and powers off the chip without synchronizing against the executing
IRQ handler:

drivers/gpu/drm/bridge/ti-sn65dsi83.c:sn65dsi83_release_resources() {
    ...
	if (ctx->irq) {
		/* Disable irq */
		regmap_write(ctx->regmap, REG_IRQ_EN, 0x0);
		regmap_write(ctx->regmap, REG_IRQ_GLOBAL, 0x0);
	}
    ...
}

If the threaded IRQ handler sn65dsi83_handle_errors() runs concurrently,
won't its I2C read fail due to the chip being powered down? It seems it
would misinterpret this as a hardware error and improperly schedule
reset_work while the display pipeline is being torn down.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805115546.152786-1-fanwu01@zju.edu.cn?part=1