Re: [PATCH v3 5/5] ASoC: fsl_easrc: expose individual DAIs per conversion path

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling ALSA hw_params multiple times on the same stream causes a context/pair leak in the ASRC, leading to a Use-After-Free and Denial of Service.
--

--- Patch [5]: [PATCH v3 5/5] ASoC: fsl_easrc: expose individual DAIs per conversion path ---
commit 6518fbf2be545cdd1e3224f3467d1c7ad789391e
Author: Shengjiu Wang <[email protected]>

ASoC: fsl_easrc: expose individual DAIs per conversion path

The driver previously registered a single DAI with generic stream names,
preventing multiple independent conversion paths from being used
simultaneously. Replace the single fsl_easrc_dai instance with an array of
four DAI drivers to allow up to four independent conversion paths to be
active at the same time.

> diff --git a/sound/soc/fsl/fsl_easrc.c b/sound/soc/fsl/fsl_easrc.c
> index f1249da0658cd..17d85d4843588 100644
> --- a/sound/soc/fsl/fsl_easrc.c
> +++ b/sound/soc/fsl/fsl_easrc.c
> @@ -1589,27 +1589,99 @@ static const struct snd_soc_dai_ops fsl_easrc_dai_ops = {
>  	.hw_free	= fsl_easrc_hw_free,

[Severity: High]
This is a pre-existing issue, but if userspace repeatedly calls
SNDRV_PCM_IOCTL_HW_PARAMS on an audio stream without calling hw_free in
between, does fsl_easrc_hw_params() unconditionally request a new context?

Looking at fsl_easrc_hw_params():

sound/soc/fsl/fsl_easrc.c:fsl_easrc_hw_params() {
	...
	ret = fsl_easrc_request_context(channels, ctx);
	if (ret) {
		dev_err(dev, "failed to request context\n");
		return ret;
	}
	...
}

If this happens, fsl_easrc_request_context() finds the next available empty
slot in the easrc->pair[] array, overwrites ctx->index, and deducts channels
from easrc->channel_avail:

sound/soc/fsl/fsl_easrc.c:fsl_easrc_request_context() {
	...
	ctx->index = index;
	ctx->channels = channels;
	easrc->pair[index] = ctx;
	easrc->channel_avail -= channels;
	...
}

Because the previous index is overwritten without being freed, does
easrc->pair[old_index] permanently retain a dangling pointer to the context?

Furthermore, when the stream is closed, fsl_asrc_dma_shutdown() frees the
pair context but only clears asrc->pair[pair->index] using the latest index:

sound/soc/fsl/fsl_asrc_dma.c:fsl_asrc_dma_shutdown() {
	...
	if (asrc->pair[pair->index] == pair)
		asrc->pair[pair->index] = NULL;

	kfree(pair);
	...
}

Would subsequent operations that iterate over all easrc->pair[] slots (such
as fsl_asrc_isr() or fsl_asrc_m2m_suspend()) then dereference these dangling
pointers, resulting in a use-after-free? (It appears the older ASRC driver
shares this same design flaw in fsl_asrc_dai_hw_params).

>  };
>  
> -static struct snd_soc_dai_driver fsl_easrc_dai = {
> -	.playback = {
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5