Re: [PATCH v3 1/4] ASoC: rt5668: cancel delayed works on component removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ASoC: rt5668: jack_detect_work UAF on I2C unbind in rt5668_remove()
- [High] ASoC: rt5668: unbalanced disable_irq() in rt5668_remove() on card rebind
- [Medium] ASoC: rt5668: disable_irq() in rt5668_remove() when IRQ request failed
--

commit d5c07e49b35a8b8aca80e062381c33477ad1f222
Author: Tristan Madani <tristmd@gmail.com>

ASoC: rt5668: cancel delayed works on component removal

This commit disables the codec IRQ and cancels jack_detect_work and
jd_check_work in rt5668_remove() to prevent work handlers from running
after removal.

> diff --git a/sound/soc/codecs/rt5668.c b/sound/soc/codecs/rt5668.c
> index cd6245d6b91af..7501c6c6ee128 100644
> --- a/sound/soc/codecs/rt5668.c
> +++ b/sound/soc/codecs/rt5668.c
> @@ -2222,6 +2222,13 @@ static int rt5668_probe(struct snd_soc_component *component)
>  static void rt5668_remove(struct snd_soc_component *component)
>  {
>  	struct rt5668_priv *rt5668 = snd_soc_component_get_drvdata(component);
> +	struct i2c_client *i2c = to_i2c_client(component->dev);
> +
> +	if (i2c->irq)
> +		disable_irq(i2c->irq);

[Severity: High]
Does calling disable_irq(i2c->irq) in the ASoC component remove callback
rt5668_remove() leave the IRQ permanently disabled across sound card
unbind/rebind cycles or deferred probe rollbacks?

In rt5668_i2c_probe(), i2c->irq is requested once at I2C device probe time
via devm_request_threaded_irq(), whereas rt5668_probe() and rt5668_remove()
are snd_soc_component_driver callbacks invoked when an ASoC card binds or
unbinds the component:

sound/soc/codecs/rt5668.c:rt5668_probe() {
	struct rt5668_priv *rt5668 = snd_soc_component_get_drvdata(component);

	rt5668->component = component;

	return 0;
}

If the sound card is unbound and later rebound while the I2C device stays
bound, or if snd_soc_bind_card() fails or defers on a later component after
rt5668_probe() succeeds (causing soc_cleanup_card_resources() ->
soc_remove_link_components() -> snd_soc_component_remove() to run),
rt5668_remove() calls disable_irq(i2c->irq). Because neither rt5668_probe()
nor rt5668_set_jack_detect() calls enable_irq(i2c->irq), the IRQ disable
depth remains incremented on subsequent card binds.

[Severity: Medium]
Also, what happens here if devm_request_threaded_irq() failed during
rt5668_i2c_probe()?

In rt5668_i2c_probe(), a failure to request the IRQ is logged as non-fatal
while i2c->irq remains non-zero:

	if (i2c->irq) {
		ret = devm_request_threaded_irq(&i2c->dev, i2c->irq, NULL,
			rt5668_irq, IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING
			| IRQF_ONESHOT, "rt5668", rt5668);
		if (ret)
			dev_err(&i2c->dev, "Failed to request IRQ: %d\n", ret);

	}

When rt5668_remove() later checks only if (i2c->irq), won't it call
disable_irq(i2c->irq) on an IRQ line that rt5668 never successfully
claimed, causing an unbalanced IRQ disable?

> +
> +	cancel_delayed_work_sync(&rt5668->jack_detect_work);
> +	cancel_delayed_work_sync(&rt5668->jd_check_work);

[Severity: High]
As reported by Sashiko on v2 [1], can rt5668->jack_detect_work still cause
a use-after-free when the I2C device is unbound while no sound card is
bound?

In rt5668_i2c_probe(), rt5668->jack_detect_work and rt5668->jd_check_work
are initialized and rt5668_irq() is registered via
devm_request_threaded_irq() before devm_snd_soc_register_component(). If an
interrupt fires before a sound card is bound or instantiated, rt5668_irq()
queues rt5668->jack_detect_work, and rt5668_jack_detect_handler() re-arms
itself every 15 ms:

	if (!rt5668->component ||
	    !snd_soc_card_is_instantiated(rt5668->component->card)) {
		/* card not yet ready, try later */
		mod_delayed_work(system_power_efficient_wq,
				 &rt5668->jack_detect_work, msecs_to_jiffies(15));
		return;
	}

When the I2C device is unbound while component->card is NULL,
devm_component_release() -> snd_soc_unregister_component_by_driver() ->
snd_soc_del_component_unlocked() skips snd_soc_unbind_card() because card is
NULL, so snd_soc_component_remove() and rt5668_remove() are never called.
Because rt5668_i2c_driver has no i2c_driver.remove callback (or devm action)
to disable the IRQ and cancel the delayed works on I2C unbind, devres frees
rt5668 while rt5668->jack_detect_work remains pending. Should the IRQ
disable and work cancellation be handled at the I2C device lifecycle level
(similar to rt5682-i2c.c and rt5682s.c) rather than only in rt5668_remove()?

[1] https://lore.kernel.org/r/sashiko-outbox-190014@kernel.org

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