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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] ASoC: rt5665: infinite hang in rt5665_i2c_shutdown() canceling
  jack_detect_work
- [High] ASoC: rt5665: uncanceled calibrate_work in
  rt5665_i2c_shutdown() causes UAF
- [Medium] ASoC: rt5665: unconditional disable_irq() in
  rt5665_i2c_shutdown() on IRQ error
- [Low] ASoC: rt5665: inaccurate handler name and queuing site in commit
  message
--

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

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

This commit disables the client IRQ and cancels jack_detect_work and
jd_check_work in rt5665_i2c_shutdown(), and wires up an I2C .remove callback
that invokes rt5665_i2c_shutdown().

> rt5665_jack_detect_work() runs from the delayed work jack_detect_work,
> which is queued from the jack IRQ handler and from the jack-detect setup,
> and the jd_check_work polls button state; both dereference the driver
> data via container_of().

[Severity: Low]
In sound/soc/codecs/rt5665.c, the handler for jack_detect_work is named
rt5665_jack_detect_handler(), and it is only queued from rt5665_irq();
rt5665_set_jack_detect() only configures registers and stores hs_jack.

> diff --git a/sound/soc/codecs/rt5665.c b/sound/soc/codecs/rt5665.c
> index 62b21092e352b..bce2fc8c9bc6b 100644
> --- a/sound/soc/codecs/rt5665.c
> +++ b/sound/soc/codecs/rt5665.c
> @@ -4837,9 +4837,19 @@ static void rt5665_i2c_shutdown(struct i2c_client *client)
>  {
>  	struct rt5665_priv *rt5665 = i2c_get_clientdata(client);
>  
> +	if (client->irq)
> +		disable_irq(client->irq);

[Severity: Medium]
Can this disable an IRQ that the driver never acquired?

In rt5665_i2c_probe(), a failure in devm_request_threaded_irq() only logs an
error and lets probe succeed with client->irq still set:

sound/soc/codecs/rt5665.c:rt5665_i2c_probe() {
	if (i2c->irq) {
		ret = devm_request_threaded_irq(&i2c->dev, i2c->irq, NULL,
			rt5665_irq, IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING
			| IRQF_ONESHOT, "rt5665", rt5665);
		if (ret)
			dev_err(&i2c->dev, "Failed to request IRQ: %d\n", ret);

	}
}

If that request fails, rt5665_i2c_shutdown() will still call
disable_irq(client->irq) on remove or shutdown, leaving the IRQ disable depth
unbalanced.

> +	cancel_delayed_work_sync(&rt5665->jack_detect_work);

[Severity: High]
Will cancel_delayed_work_sync(&rt5665->jack_detect_work) hang forever if the
device is unbound or shut down before the sound card is instantiated or
calibrated?

rt5665_i2c_probe() requests the IRQ before registering the component, and
rt5665_jack_detect_handler() spins inside the work callback waiting for the
component, card instantiation, and calibration:

sound/soc/codecs/rt5665.c:rt5665_jack_detect_handler() {
	while (!rt5665->component) {
		pr_debug("%s codec = null\n", __func__);
		usleep_range(10000, 15000);
	}

	while (!snd_soc_card_is_instantiated(rt5665->component->card)) {
		pr_debug("%s\n", __func__);
		usleep_range(10000, 15000);
	}

	while (!rt5665->calibration_done) {
		pr_debug("%s calibration not ready\n", __func__);
		usleep_range(10000, 15000);
	}
}

If an interrupt schedules jack_detect_work while any of those conditions is
false and the device is then unbound or shut down, cancel_delayed_work_sync()
waits indefinitely for a loop that will never make progress.  The sibling
drivers fixed this by rescheduling the delayed work instead of looping inside
the handler; see 4c33de0673ce ("ASoC: rt5682: do not block workqueue if card
is unbound") and a6d78661dc90 ("ASoC: rt5668: do not block workqueue if card
is unbound").

> +	cancel_delayed_work_sync(&rt5665->jd_check_work);

[Severity: High]
Does rt5665->calibrate_work also need to be canceled here?

Unlike rt5682, rt5665_probe() schedules rt5665->calibrate_work with a 100 ms
delay, and rt5665_calibrate_handler() -> rt5665_calibrate() sleeps across
polling loops while accessing rt5665, rt5665->component, and rt5665->regmap.
If the I2C device is unbound while calibrate_work is pending or running,
devres frees rt5665_priv and the work handler accesses freed memory.  Note
that rt5665_calibrate_handler() also has an unbounded while
(!snd_soc_card_is_instantiated(rt5665->component->card)) loop that would block
cancel_delayed_work_sync() if not converted to reschedule.

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