Re: [PATCH v6 2/4] ASoC: rt5659: cancel jack_detect_work on component removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ASoC: rt5659: uncancelled jack_detect_work on I2C teardown without card
- [High] ASoC: rt5659: broken jack detect and dangling hs_jack in rt5659_probe()
--

commit 7e33f0dd93e7476a57b77262485f2364ebdfe711
Author: Tristan Madani <tristan@talencesecurity.com>

ASoC: rt5659: cancel jack_detect_work on component removal

This commit disables the codec IRQ and cancels pending jack_detect_work
in rt5659_remove() before resetting the codec, and re-enables the IRQ
in rt5659_probe().

> diff --git a/sound/soc/codecs/rt5659.c b/sound/soc/codecs/rt5659.c
> index af8051fa69f89..68f7d5673e190 100644
> --- a/sound/soc/codecs/rt5659.c
> +++ b/sound/soc/codecs/rt5659.c
> @@ -3667,6 +3667,11 @@ static int rt5659_probe(struct snd_soc_component *component)
>  
>  	rt5659->component = component;
>  
> +	if (rt5659->irq_disabled) {
> +		enable_irq(to_i2c_client(component->dev)->irq);
> +		rt5659->irq_disabled = false;
> +	}

[Severity: High]
As reported by Sashiko on v5 [1], does re-enabling the IRQ here in
rt5659_probe() work as intended after rt5659_remove() has reset the
codec, and can it race with a dangling rt5659->hs_jack pointer?

At the end of rt5659_remove(), writing 0 to RT5659_RESET resets all
codec hardware registers to their power-on defaults, clearing the GPIO1
IRQ pin mux (RT5659_GP1_PIN_IRQ in RT5659_GPIO_CTRL_1) and the jack
detection and IRQ control registers (RT5659_EJD_CTRL_1,
RT5659_RC_CLK_CTRL, RT5659_GPIO_CTRL_3, RT5659_JD_CTRL_1,
RT5659_IRQ_CTRL_1, RT5659_IRQ_CTRL_2, RT5659_PWR_ANLG_1,
RT5659_PWR_ANLG_2) that were programmed only once in rt5659_i2c_probe().
Because RT5659_RESET is a volatile register in
rt5659_volatile_register(), the REGCACHE_RBTREE regmap cache is also not
marked dirty or resynced on re-probe, leaving the codec unable to
generate jack-detection interrupts after card rebind.

In addition, rt5659_remove() does not clear rt5659->hs_jack or
rt5659->component. When a sound card unbinds, its card-owned struct
snd_soc_jack is freed, leaving rt5659->hs_jack dangling. On card rebind:

snd_soc_component_probe()
  rt5659_probe()
    enable_irq()

runs before snd_soc_component_init() invokes the machine driver's init
callback to register a new snd_soc_jack via rt5659_set_jack_detect().
If an interrupt fires after enable_irq() unmasks the line before a new
jack is set (or if the rebound card does not call
rt5659_set_jack_detect() or fails later during bind), jack_detect_work
will pass the dangling rt5659->hs_jack pointer to
snd_soc_jack_report().

> +
>  	switch (rt5659->pdata.jd_src) {
>  	case RT5659_JD_HDA_HEADER:
>  		break;
> @@ -3684,6 +3689,15 @@ static int rt5659_probe(struct snd_soc_component *component)
>  static void rt5659_remove(struct snd_soc_component *component)
>  {
>  	struct rt5659_priv *rt5659 = snd_soc_component_get_drvdata(component);
> +	struct i2c_client *i2c = to_i2c_client(component->dev);
> +
> +	if (rt5659->irq_acquired) {
> +		disable_irq(i2c->irq);
> +		rt5659->irq_disabled = true;
> +	}
> +
> +	if (rt5659->jack_detect_work_inited)
> +		cancel_delayed_work_sync(&rt5659->jack_detect_work);

[Severity: High]
As reported by Sashiko on v5 [1], can rt5659->jack_detect_work still be
left pending on I2C device teardown when no ASoC card is bound, or
during system shutdown?

In rt5659_i2c_probe(), when rt5659->pdata.jd_src is RT5659_JD3 or
RT5659_JD_HDA_HEADER, the driver initializes rt5659->jack_detect_work,
enables codec jack-detection interrupts, registers rt5659_irq() via
devm_request_threaded_irq(), and enables RT5659_GP1_PIN_IRQ in
RT5659_GPIO_CTRL_1 before calling devm_snd_soc_register_component().
Once devm_request_threaded_irq() succeeds, any interrupt on i2c->irq
invokes rt5659_irq() and queues rt5659->jack_detect_work onto
system_power_efficient_wq with a 250 ms delay.

However, disable_irq() and cancel_delayed_work_sync() are placed only in
rt5659_remove() (the snd_soc_component_driver remove callback), while
rt5659_i2c_driver defines no remove callback and registers no devm
work-cancellation action. In the ASoC core, snd_soc_component_remove()
only calls component->driver->remove() when component->card is non-NULL
and the component was probed.

If devm_snd_soc_register_component() fails in rt5659_i2c_probe(), or if
the I2C device is unbound while no sound card is bound to the component,
rt5659_remove() is skipped and devres frees the IRQ, regmap, and struct
rt5659_priv while rt5659->jack_detect_work remains queued, leading to a
use-after-free in rt5659_jack_detect_work() or
rt5659_jack_detect_intel_hd_header() when the timer expires. Similarly,
rt5659_i2c_shutdown() writes RT5659_RESET without disabling the IRQ or
cancelling jack_detect_work.

[1] https://lore.kernel.org/r/20261030174837.1839402-3-tristan@talencesecurity.com

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