Re: [PATCH] ASoC: rt5659: cancel jack detect work on removal to avoid use-after-free

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ASoC: rt5659: unguarded cancel_delayed_work_sync() in
  rt5659_i2c_shutdown()
- [Medium] ASoC: rt5659: unguarded disable_irq() in
  rt5659_i2c_shutdown()
--

commit cfae689e1e0a124463e941d5a4ef73f9a8712aac
Author: Hyunjung Ko <hj351016@gmail.com>

ASoC: rt5659: cancel jack detect work on removal to avoid use-after-free

This commit disables the client IRQ and cancels jack_detect_work in
rt5659_i2c_shutdown(), and wires rt5659_i2c_shutdown() into a new i2c
.remove callback.

> diff --git a/sound/soc/codecs/rt5659.c b/sound/soc/codecs/rt5659.c
> index 68f7d5673e190..721c742c0efda 100644
> --- a/sound/soc/codecs/rt5659.c
> +++ b/sound/soc/codecs/rt5659.c
> @@ -4337,9 +4337,18 @@ static void rt5659_i2c_shutdown(struct i2c_client *client)
>  {
>  	struct rt5659_priv *rt5659 = i2c_get_clientdata(client);
>  
> +	if (client->irq)
> +		disable_irq(client->irq);

[Severity: Medium]
Can this cause an unbalanced double disable_irq() on client->irq, or
disable an IRQ that this driver never acquired?

The component remove callback rt5659_remove(), added in ca3efbaa8aeb
("ASoC: rt5659: cancel jack_detect_work on component removal"), already
disables the IRQ:

sound/soc/codecs/rt5659.c:rt5659_remove() {
	...
	if (rt5659->irq_acquired) {
		disable_irq(i2c->irq);
		rt5659->irq_disabled = true;
	}
	...
}

When unbinding an i2c device that is bound to a sound card,
rt5659_i2c_remove() -> rt5659_i2c_shutdown() calls disable_irq(), and
devres teardown of devm_snd_soc_register_component() then invokes
rt5659_remove(), calling disable_irq() a second time.  Conversely, if the
card was already unbound before i2c unbind or system shutdown,
rt5659_remove() has already set irq_disabled = true and
rt5659_i2c_shutdown() disables client->irq again.

In addition, devm_request_threaded_irq() failure in rt5659_i2c_probe() is
non-fatal, leaving client->irq non-zero while rt5659->irq_acquired is
false, so checking client->irq alone disables a line the driver does not
own.

> +	cancel_delayed_work_sync(&rt5659->jack_detect_work);

[Severity: Medium]
Can this call cancel_delayed_work_sync() on an uninitialized
rt5659->jack_detect_work?

In rt5659_i2c_probe(), INIT_DELAYED_WORK(&rt5659->jack_detect_work, ...)
and rt5659->jack_detect_work_inited = true only run when pdata.jd_src is
RT5659_JD3 or RT5659_JD_HDA_HEADER.  When pdata.jd_src is RT5659_JD_NULL
(for example when the optional realtek,jd-src property is omitted),
jack_detect_work is never initialized and unbinding or shutting down the
device triggers WARN_ON(!work->func) in __flush_work().

Guarding this call with if (rt5659->jack_detect_work_inited), as
rt5659_remove() does, avoids canceling uninitialized work.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109172155.1851386-1-hj351016@gmail.com?part=1