Re: [PATCH v3 3/4] ASoC: codecs: wcd-mbhc: guard against NULL cfg in typec_report_(un)plug

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ASoC: codecs: wcd-mbhc: UAF race between typec_report_(un)plug() and deinit()
- [Medium] ASoC: wcd-mbhc: race between wcd_mbhc_start() and typec_report_(un)plug()
--

commit 6517f3fdf1f47c12dda04120bfa0403f19390817
Author: Val Packett <val@packett.cool>

ASoC: codecs: wcd-mbhc: guard against NULL cfg in typec_report_(un)plug

This commit adds a NULL check for mbhc->cfg in
wcd_mbhc_typec_report_unplug() and wcd_mbhc_typec_report_plug() to guard
against Type-C callbacks firing before wcd_mbhc_start() is called.

> diff --git a/sound/soc/codecs/wcd-mbhc-v2.c b/sound/soc/codecs/wcd-mbhc-v2.c
> index 850be84d65ceb..fd40c1686d66a 100644
> --- a/sound/soc/codecs/wcd-mbhc-v2.c
> +++ b/sound/soc/codecs/wcd-mbhc-v2.c
> @@ -594,7 +594,7 @@ static irqreturn_t wcd_mbhc_mech_plug_detect_irq(int irq, void *data)
>  int wcd_mbhc_typec_report_unplug(struct wcd_mbhc *mbhc)
>  {
>  
> -	if (!mbhc || !mbhc->cfg->typec_analog_mux)
> +	if (!mbhc || !mbhc->cfg || !mbhc->cfg->typec_analog_mux)
>  		return -EINVAL;
>  
>  	if (mbhc->mbhc_cb->clk_setup)
> @@ -611,7 +611,7 @@ EXPORT_SYMBOL_GPL(wcd_mbhc_typec_report_unplug);
>  
>  int wcd_mbhc_typec_report_plug(struct wcd_mbhc *mbhc)
>  {
> -	if (!mbhc || !mbhc->cfg->typec_analog_mux)
> +	if (!mbhc || !mbhc->cfg || !mbhc->cfg->typec_analog_mux)
>  		return -EINVAL;
>  
>  	if (mbhc->mbhc_cb->clk_setup)

[Severity: Medium]
Is checking !mbhc->cfg without holding mbhc->lock sufficient to synchronize
wcd_mbhc_typec_report_plug() and wcd_mbhc_typec_report_unplug() against
wcd_mbhc_start(), wcd_mbhc_initialise(), and wcd_mbhc_stop()?

In sound/soc/codecs/wcd-mbhc-v2.c, wcd_mbhc_start() assigns mbhc->cfg
without holding mbhc->lock, before setting mbhc->jack and before
wcd_mbhc_initialise() initializes the hardware:

sound/soc/codecs/wcd-mbhc-v2.c:wcd_mbhc_start() {
	if (!mbhc || !cfg || !jack)
		return -EINVAL;

	mbhc->cfg = cfg;
	mbhc->jack = jack;

	return wcd_mbhc_initialise(mbhc);
}

wcd_mbhc_initialise() then calls pm_runtime_get_sync() before taking
mbhc->lock and configuring the registers:

sound/soc/codecs/wcd-mbhc-v2.c:wcd_mbhc_initialise() {
	ret = pm_runtime_get_sync(component->dev);
	if (ret < 0 && ret != -EACCES) {
		...
		return ret;
	}

	mutex_lock(&mbhc->lock);
	...
	/* Plug detect is triggered manually if analog goes through USBCC */
	if (mbhc->cfg->typec_analog_mux)
		wcd_mbhc_write_field(mbhc, WCD_MBHC_L_DET_EN, 0);
	else
		wcd_mbhc_write_field(mbhc, WCD_MBHC_L_DET_EN, 1);
	...
	if (mbhc->mbhc_cb->clk_setup)
		mbhc->mbhc_cb->clk_setup(component, false);
	mutex_unlock(&mbhc->lock);
}

If wcd937x_typec_mux_set() or wcd939x_typec_mux_set() calls
wcd_mbhc_typec_report_plug() while wcd_mbhc_start() is in progress,
wcd_mbhc_typec_report_plug() can observe mbhc->cfg != NULL, enable the MBHC
clock, set WCD_MBHC_L_DET_EN to 1, and schedule mbhc_plug_detect_work
before wcd_mbhc_initialise() has configured the MBHC registers or
mbhc->swap_thr. When wcd_mbhc_initialise() then runs under mbhc->lock, it
will clear WCD_MBHC_L_DET_EN back to 0 and disable clk_setup, breaking the
plug detection state.

Additionally, mbhc->cfg is not cleared if wcd_mbhc_initialise() fails or
when wcd_mbhc_stop() runs. Could wcd_mbhc_typec_report_plug() and
wcd_mbhc_typec_report_unplug() still execute when MBHC is uninitialized or
stopped?

[Severity: High]
Can wcd_mbhc_typec_report_plug() and wcd_mbhc_typec_report_unplug() also
race with wcd_mbhc_deinit(), causing a use-after-free on struct wcd_mbhc
and mbhc->mbhc_plug_detect_work?

As the commit message notes, the platform device's Type-C callbacks are not
synchronized with codec ops. In sound/soc/codecs/wcd937x.c (and similarly
in wcd939x.c), wcd937x_typec_mux_set() checks wcd937x->wcd_mbhc and passes
the pointer to wcd_mbhc_typec_report_plug() or
wcd_mbhc_typec_report_unplug() without holding a lock that serializes
against teardown:

sound/soc/codecs/wcd937x.c:wcd937x_typec_mux_set() {
	if (!wcd937x->wcd_mbhc)
		return -EINVAL;

	if (wcd937x->typec_mode != state->mode) {
		wcd937x->typec_mode = state->mode;

		if (wcd937x->typec_mode == TYPEC_MODE_AUDIO)
			return wcd_mbhc_typec_report_plug(wcd937x->wcd_mbhc);
		else if (previous_mode == TYPEC_MODE_AUDIO)
			return wcd_mbhc_typec_report_unplug(wcd937x->wcd_mbhc);
	}
}

During ASoC component removal, wcd937x_soc_codec_remove() calls
wcd937x_mbhc_deinit() -> wcd_mbhc_deinit():

sound/soc/codecs/wcd937x.c:wcd937x_mbhc_deinit() {
	/* May be accessed by platform dev typec callbacks, clear it first */
	wcd937x->wcd_mbhc = NULL;
	wcd_mbhc_deinit(mbhc);
}

sound/soc/codecs/wcd-mbhc-v2.c:wcd_mbhc_deinit() {
	...
	mutex_lock(&mbhc->lock);
	wcd_cancel_hs_detect_plug(mbhc,	&mbhc->correct_plug_swch);
	cancel_work_sync(&mbhc->mbhc_plug_detect_work);
	mutex_unlock(&mbhc->lock);

	kfree(mbhc);
}

If wcd937x_typec_mux_set() has already loaded wcd937x->wcd_mbhc before
wcd937x_mbhc_deinit() clears it, wcd_mbhc_deinit() can finish
cancel_work_sync(&mbhc->mbhc_plug_detect_work) and kfree(mbhc) while
wcd_mbhc_typec_report_plug() or wcd_mbhc_typec_report_unplug() is still
running, dereferencing freed mbhc fields and calling
schedule_work(&mbhc->mbhc_plug_detect_work) on freed memory.

Should the Type-C mux callback and MBHC teardown be synchronized with a
lock that spans the wcd_mbhc_typec_report_plug() /
wcd_mbhc_typec_report_unplug() call and wcd_mbhc_deinit()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020170341.1641325-1-val@packett.cool?part=3