Re: [PATCH v4 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 54b53c52a3c171380da559e1b3114044473f6136
Author: Tristan Madani <tristmd@gmail.com>

ASoC: rt5659: cancel jack_detect_work on component removal

This commit tracks IRQ acquisition and delayed work initialization in
struct rt5659_priv so that rt5659_remove() disables the IRQ and cancels
jack_detect_work before resetting the codec, and rt5659_probe()
re-enables the IRQ on subsequent component binds.

> 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 v3 [1], does re-enabling the host IRQ here on
card rebind actually restore jack detection, or can it trigger a
use-after-free on rt5659->hs_jack?

At the end of rt5659_remove(), the codec is software-reset:

	regmap_write(rt5659->regmap, RT5659_RESET, 0);

Writing RT5659_RESET resets all codec hardware registers to power-on
defaults, wiping out the GPIO1 IRQ pin mux (RT5659_GP1_PIN_IRQ in
RT5659_GPIO_CTRL_1) and the jack-detection and IRQ control registers
(such as RT5659_EJD_CTRL_1, RT5659_RC_CLK_CTRL, RT5659_GPIO_CTRL_3,
RT5659_JD_CTRL_1, RT5659_IRQ_CTRL_1, and RT5659_IRQ_CTRL_2) that were
configured only once in rt5659_i2c_probe(). Because RT5659_RESET is a
volatile register, the regmap cache is not marked dirty or resynced on
re-probe either, leaving the codec unable to generate jack interrupts
after card rebind.

In addition, rt5659_remove() does not clear rt5659->hs_jack or
rt5659->component. When a sound card unbinds and its card-owned struct
snd_soc_jack is freed, rt5659->hs_jack retains the dangling pointer. When
a card subsequently binds again, snd_soc_component_probe() calls
rt5659_probe() before the machine driver's init callback registers a new
jack. Calling enable_irq() at that point unmasks the IRQ while
rt5659->hs_jack still points to the freed jack, so an interrupt firing
before a new jack is set (or if the new card does not call
rt5659_set_jack_detect()) will schedule jack_detect_work and 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);
>  
>  	regmap_write(rt5659->regmap, RT5659_RESET, 0);
>  }

[Severity: High]
As reported by Sashiko on v3 [2], what happens if the I2C device is
unbound while no ASoC sound card is bound to the component, or if
devm_snd_soc_register_component() fails in rt5659_i2c_probe()?

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, requests rt5659_irq() via
devm_request_threaded_irq(), and enables the GPIO1 IRQ output pin before
calling devm_snd_soc_register_component(). Once the IRQ is requested, any
interrupt on i2c->irq runs rt5659_irq() and queues
rt5659->jack_detect_work with a 250 ms delay.

However, in the ASoC core, snd_soc_del_component_unlocked() and
snd_soc_component_remove() only invoke component->driver->remove
(rt5659_remove) when component->card is non-NULL and the component has
been probed:

sound/soc/soc-component.c:snd_soc_component_remove() {
	if (!component->card)
		return;

	if (probed && component->driver->remove)
		component->driver->remove(component);
	...
}

Because rt5659_i2c_driver defines no I2C remove callback and registers no
devm action (such as devm_delayed_work_autocancel()) to cancel
jack_detect_work, unbinding the I2C device without a bound sound card (or
failing devm_snd_soc_register_component()) skips rt5659_remove()
entirely. devres then frees the IRQ, regmap, and struct rt5659_priv while
rt5659->jack_detect_work remains pending, leading to a use-after-free when
the delayed work executes.

Similarly, rt5659_i2c_shutdown() writes RT5659_RESET without disabling the
IRQ or cancelling rt5659->jack_detect_work, allowing the work to run
against a reset codec during system shutdown.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030170416.3561618-1-tristan@talencesecurity.com?part=2