Re: [PATCH 1/7] ASoC: qcom: audioreach: fix module cfg blob size handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] ASoC: qcom: audioreach: module leak and UAF on priv data error
- [High] ASoC: qcom: audioreach: OOB read from array_size truncation to nwords
- [High] ASoC: qcom: audioreach: unchecked mod_array header and size in loop
--

commit 1de183e156fe039ccb5c136361b5dc13fa6c2cdc
Author: Ravi Hothi <ravi.hothi@oss.qualcomm.com>

ASoC: qcom: audioreach: fix module cfg blob size handling

This commit converts mod_array->size from a byte count to a word count when
allocating and copying module private configuration data in
audioreach_get_module_priv_data(). It also adds bounds and zero-size checks
for the blob and checks for an error pointer before storing it in mod->data.

> diff --git a/sound/soc/qcom/qdsp6/topology.c b/sound/soc/qcom/qdsp6/topology.c
> index faafbc5c0ad00..612d8f03e6e98 100644
> --- a/sound/soc/qcom/qdsp6/topology.c
> +++ b/sound/soc/qcom/qdsp6/topology.c
> @@ -312,22 +312,37 @@ audioreach_get_module_priv_data(const struct snd_soc_tplg_private *private)
>  
>  	for (sz = 0; sz < le32_to_cpu(private->size); ) {
>  		const struct snd_soc_tplg_vendor_array *mod_array;
> +		u32 array_size;
>  
>  		mod_array = (struct snd_soc_tplg_vendor_array *)((u8 *)private->array + sz);
> +		array_size = le32_to_cpu(mod_array->size);
> +
>  		if (le32_to_cpu(mod_array->type) == SND_SOC_AR_TPLG_MODULE_CFG_TYPE) {
>  			struct audioreach_module_priv_data *pdata;
> +			u32 nwords;
> +
> +			/* size is the byte count of the __le32 data[] */
> +			nwords = array_size / sizeof(__le32);
> +
> +			/* the blob must lie within the private data */
> +			if (sizeof(*pdata) + (u64)array_size >
> +			    le32_to_cpu(private->size) - sz)
> +				return ERR_PTR(-EINVAL);
>  
> -			pdata = kzalloc_flex(*pdata, data,
> -					     le32_to_cpu(mod_array->size));
> +			pdata = kzalloc_flex(*pdata, data, nwords);
>  			if (!pdata)
>  				return ERR_PTR(-ENOMEM);
>  
> -			memcpy(pdata, ((u8 *)private->data + sz), struct_size(pdata, data,
> -						le32_to_cpu(mod_array->size)));
> +			memcpy(pdata, ((u8 *)private->data + sz),
> +			       struct_size(pdata, data, nwords));
>  			return pdata;
>  		}

[Severity: High]
What happens here if array_size is not a multiple of sizeof(__le32) (or is 0)?

When array_size is not 4-byte aligned, integer division truncates nwords down,
so kzalloc_flex() allocates nwords * sizeof(__le32) bytes for pdata->data[],
which is 1 to 3 bytes smaller than array_size. However, the subsequent
memcpy() copies the struct audioreach_module_priv_data header directly from
private->data + sz, leaving the untruncated array_size in pdata->size.

When the graph is later opened, audioreach_set_module_config() in
sound/soc/qcom/qdsp6/audioreach.c uses that untruncated pdata->size for
memcpy():

	int size = le32_to_cpu(module->data->size);
	...
	memcpy(p, module->data->data, size);

Wouldn't that read up to 3 bytes past the end of the pdata->data[] slab
allocation? Also, because this branch returns before the !array_size check
below, a zero array_size for SND_SOC_AR_TPLG_MODULE_CFG_TYPE is not rejected.

>  
> -		sz = sz + le32_to_cpu(mod_array->size);
> +		/* a zero size would never advance the walk */
> +		if (!array_size)
> +			return ERR_PTR(-EINVAL);
> +
> +		sz = sz + array_size;
>  	}

[Severity: High]
Can a malformed vendor array header or size still read past private->array or
spin in this loop?

At the start of the loop in audioreach_get_module_priv_data(), the condition
sz < le32_to_cpu(private->size) only guarantees at least 1 byte remains, so
reading mod_array->size and mod_array->type can read past the end of
private->array if fewer than 8 bytes remain.

In addition, when mod_array->type is not SND_SOC_AR_TPLG_MODULE_CFG_TYPE,
array_size is only checked for zero and is not bounded against
le32_to_cpu(private->size) - sz or sizeof(*mod_array). Because sz is a signed
int and array_size is u32, advancing sz with sz = sz + array_size at any
sz > 0 (for example, on the second vendor array) with array_size == (u32)-sz
passes the !array_size check and wraps sz back to 0 in 32-bit arithmetic,
causing an infinite loop or an out-of-bounds read.

>  
>  	return NULL;
> @@ -611,6 +626,12 @@ static int audioreach_widget_load_module_common(struct snd_soc_component *compon
>  		return mod ? PTR_ERR(mod) : -ENODEV;
>  
>  	mod->data = audioreach_get_module_priv_data(&tplg_w->priv);
> +	if (IS_ERR(mod->data)) {
> +		int rc = PTR_ERR(mod->data);
> +
> +		mod->data = NULL;
> +		return rc;
> +	}

[Severity: High]
Does returning rc here before setting w->dobj.private = mod leak mod and leave
a dangling mod->widget pointer?

Before audioreach_get_module_priv_data() is called in
audioreach_widget_load_module_common(), audioreach_parse_common_tokens() ->
audioreach_tplg_alloc_module() has already inserted mod into apm->modules_idr,
linked mod->node into cont->modules_list, and set mod->widget = w:

	ret = idr_alloc_u32(&apm->modules_idr, mod, &module_id,
			    INT_MAX, GFP_KERNEL);
	...
	list_add_tail(&mod->node, &cont->modules_list);
	mod->container = cont;
	mod->widget = w;
	cont->num_modules++;

When audioreach_get_module_priv_data() returns an ERR_PTR(), returning rc
before assigning dobj->private = mod causes two issues depending on the caller:

1. For widget loaders whose error is propagated by audioreach_widget_ready()
(such as audioreach_widget_load_pga()), soc_tplg_dapm_widget_create() in
sound/soc/soc-topology.c runs its error path:

ready_err:
	soc_tplg_remove_widget(snd_soc_dapm_to_component(widget->dapm),
			       &widget->dobj, SOC_TPLG_PASS_WIDGET);
	snd_soc_dapm_free_widget(widget);

Because dobj->private is still NULL, audioreach_widget_unload() returns early:

	mod = dobj->private;
	if (!mod)
		return 0;

As a result, mod is leaked in apm->modules_idr and cont->modules_list (which
also prevents cont, sg, and info from being freed), and mod->widget becomes a
dangling pointer to the freed snd_soc_dapm_widget. Subsequent calls to
audioreach_find_module() iterate apm->modules_idr and dereference
module->widget->name, causing a use-after-free:

	idr_for_each_entry(&apm->modules_idr, module, id) {
		if (!strcmp(name, module->widget->name))
			return module;
	}

2. For widget loaders whose return value is ignored by
audioreach_widget_ready() (such as audioreach_widget_load_buffer() and
audioreach_widget_load_enc_dec_cnv()), topology loading succeeds with mod
still linked in cont->modules_list, mod->data == NULL, and
w->dobj.private == NULL. If a MODULE_ID_SMECNS_V2 module is opened with
mod->data == NULL, audioreach_set_media_format() ->
audioreach_set_module_config() dereferences module->data->size without a NULL
check.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023171316.819857-1-srinivas.kandagatla@oss.qualcomm.com?part=1