Re: [PATCH v2 4/4] ASoC: codecs: wcd939x: fix ref leak and NULL deref with USB-C audio

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ASoC: wcd939x: uninitialized __free() pointer in wcd939x_populate_dt_data()
- [High] ASoC: wcd939x: use-after-free race in wcd939x_mbhc_deinit() with typec mux
--

commit 263e4868f28e72190c4609440782fec0a736dcae
Author: Val Packett <val@packett.cool>

ASoC: codecs: wcd939x: fix ref leak and NULL deref with USB-C audio

This commit addresses three issues with USB-C audio support in wcd939x:
preventing wcd939x->wcd_mbhc from holding an error pointer or stale pointer
for Type-C callbacks, releasing wcd939x->typec_switch on unbind, and
adding a __free(device_node) cleanup annotation for the node returned by
of_graph_get_remote_node().

> diff --git a/sound/soc/codecs/wcd939x.c b/sound/soc/codecs/wcd939x.c
> index 2d463f062e6d7..17503e4785e2c 100644
> --- a/sound/soc/codecs/wcd939x.c
> +++ b/sound/soc/codecs/wcd939x.c
[ ... ]
> @@ -2503,6 +2507,8 @@ static void wcd939x_mbhc_deinit(struct snd_soc_component *component)
>  	struct wcd939x_priv *wcd939x = snd_soc_component_get_drvdata(component);
>  
>  	wcd_mbhc_deinit(wcd939x->wcd_mbhc);
> +	/* May be accessed by platform dev typec callbacks, clear */
> +	wcd939x->wcd_mbhc = NULL;
>  }

[Severity: High]
Can wcd939x_typec_mux_set() race with wcd939x_mbhc_deinit() and access a
freed wcd939x->wcd_mbhc?

In wcd939x_mbhc_deinit(), wcd939x->wcd_mbhc is only set to NULL after
wcd_mbhc_deinit() has already returned, and without synchronization against
wcd939x_typec_mux_set(). Inside wcd_mbhc_deinit(), execution sleeps across
free_irq() and cancel_work_sync(&mbhc->mbhc_plug_detect_work) before calling
kfree(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);
}

During that window, wcd939x_typec_mux_set() can still see a non-NULL
wcd939x->wcd_mbhc:

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

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

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

Wouldn't a concurrent wcd939x_typec_mux_set() call dereference freed mbhc
memory or reschedule mbhc_plug_detect_work after cancel_work_sync() has
completed?

>  
>  /* END MBHC */
> @@ -3175,7 +3181,7 @@ static int wcd939x_populate_dt_data(struct wcd939x_priv *wcd939x, struct device
>  {
>  	struct wcd_mbhc_config *cfg = &wcd939x->mbhc_cfg;
>  #if IS_ENABLED(CONFIG_TYPEC)
> -	struct device_node *np;
> +	struct device_node *np __free(device_node);
>  #endif /* CONFIG_TYPEC */
>  	int ret;

[Severity: High]
Does wcd939x_populate_dt_data() pass an uninitialized np pointer to
of_node_put() on early return?

Here np is declared with __free(device_node) without being initialized to
NULL, while three early return paths precede its assignment at
of_graph_get_remote_node():

sound/soc/codecs/wcd939x.c:wcd939x_populate_dt_data() {
	...
	wcd939x->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
	if (IS_ERR(wcd939x->reset_gpio))
		return dev_err_probe(dev, PTR_ERR(wcd939x->reset_gpio),
				     "Failed to get reset gpio\n");

	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(wcd939x_supplies),
					     wcd939x_supplies);
	if (ret)
		return dev_err_probe(dev, ret, "Failed to get and enable supplies\n");

	ret = wcd_dt_parse_micbias_info(&wcd939x->common);
	if (ret)
		return dev_err_probe(dev, ret, "Failed to get micbias\n");
	...
	np = of_graph_get_remote_node(dev->of_node, 0, 0);
}

If any of those three calls fails or defers probe when CONFIG_TYPEC is
enabled, the cleanup handler will run on an uninitialized np pointer. Should
np be initialized to NULL (or declared at the point of assignment)?

[ ... ]

>dari

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