Re: [PATCH v4 3/5] ASoC: fsl_asrc/fsl_easrc: move DMA params into pair/context struct

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] The dynamically allocated `pair` is freed on stream shutdown without clearing the shared CPU DAI's `dma_data` pointer, leaving a dangling pointer that leads to a Use-After-Free on concurrent streams.
- [High] The patch fails to resolve concurrent stream corruption because it assigns the per-pair `dma_params` to a shared CPU DAI, meaning concurrent streams overwrite each other's DMA parameters pointer.
--

--- Patch [3]: [PATCH v4 3/5] ASoC: fsl_asrc/fsl_easrc: move DMA params into pair/context struct ---
commit 6f9d240bd6e939329ded07360e8ef36835e62929
Author: Shengjiu Wang <[email protected]>

ASoC: fsl_asrc/fsl_easrc: move DMA params into pair/context struct
    
The shared dma_params_tx/rx fields in struct fsl_asrc were a single
instance written by every concurrent fsl_asrc_dma_hw_params() call.
With multiple DAIs registered (one per pair/context), parallel stream
open+hw_params races would corrupt addr and maxburst for all active
streams.

Fix by moving dma_params into struct fsl_asrc_pair so each pair owns
its own copy. Initialise them in fsl_asrc_dma_startup() where
the pair is allocated, which makes the per-DAI probe callbacks in
fsl_asrc.c and fsl_easrc.c redundant; remove those as well.

> diff --git a/sound/soc/fsl/fsl_asrc_dma.c b/sound/soc/fsl/fsl_asrc_dma.c
> index 2f662bdf14d07..15f2dccc35840 100644
> --- a/sound/soc/fsl/fsl_asrc_dma.c
> +++ b/sound/soc/fsl/fsl_asrc_dma.c
> @@ -392,6 +392,20 @@ static int fsl_asrc_dma_startup(struct snd_soc_component *component,
>  
>  	runtime->private_data = pair;
>  
> +	/*
> +	 * Point the cpu DAI dma_data at the per-pair params so that
> +	 * concurrent hw_params calls on different pairs each write to
> +	 * their own struct and do not race on addr/maxburst.  Use the
> +	 * per-direction setters so that a concurrent open of the other
> +	 * direction on the same DAI does not NULL out its pointer.
> +	 */
> +	if (tx)
> +		snd_soc_dai_dma_data_set_playback(snd_soc_rtd_to_cpu(rtd, 0),
> +						  &pair->dma_params);
> +	else
> +		snd_soc_dai_dma_data_set_capture(snd_soc_rtd_to_cpu(rtd, 0),
> +						 &pair->dma_params);
> +

[Severity: High]
Does this actually resolve the concurrent stream corruption?

If the driver registers only a single CPU DAI, won't multiple streams opening
in the same direction all assign their per-pair `dma_params` to the same shared
CPU DAI pointer?

The last stream to open would overwrite the pointer. When concurrent streams
then call fsl_asrc_dma_hw_params(), wouldn't they all fetch this same
overwritten pointer and write their DMA configurations into a single shared
struct, continuing to corrupt the data?

[Severity: Critical]
Can this also lead to a Use-After-Free when streams are closed?

If Stream A opens and sets the shared CPU DAI pointer to its dynamically
allocated `pair->dma_params`, and then Stream B opens concurrently and
overwrites the shared pointer with its own `pair->dma_params`:

If Stream B then closes and frees its `pair` struct via kfree() in
fsl_asrc_dma_shutdown(), the pointer in the shared CPU DAI is left dangling.

If Stream A then proceeds to fsl_asrc_dma_hw_params() and calls
snd_soc_dai_get_dma_data(), won't it fetch the dangling pointer and write
`addr` and `maxburst` into freed memory?

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