Re: [PATCH v2 10/11] ASoC: fsl: mpc5200_dma: Use guard() for spin locks

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The `psc_dma` structure is freed in `mpc5200_audio_dma_destroy()` before the ALSA components are unregistered, leading to a use-after-free and locking of freed memory if userspace invokes component operations during module unbind.
--

commit d1f00eba902c807bb72ff7810e5332f8b08165b6
Author: bui duc phuc <phucduc.bui@gmail.com>

    ASoC: fsl: mpc5200_dma: Use guard() for spin locks

This commit cleans up the spin lock usage in the mpc5200_dma driver by
converting it to use the scoped_guard() macro.

> diff --git a/sound/soc/fsl/mpc5200_dma.c b/sound/soc/fsl/mpc5200_dma.c
> index 56e2cf2f727ba..bfedb2dea0b32 100644
> --- a/sound/soc/fsl/mpc5200_dma.c
> +++ b/sound/soc/fsl/mpc5200_dma.c
[ ... ]
> @@ -135,19 +136,18 @@ static int psc_dma_trigger(struct snd_soc_component *component,
>  		/* Fill up the bestcomm bd queue and enable DMA.
>  		 * This will begin filling the PSC's fifo.
>  		 */
> -		spin_lock_irqsave(&psc_dma->lock, flags);
> -
> -		if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE)
> -			bcom_gen_bd_rx_reset(s->bcom_task);
> -		else
> -			bcom_gen_bd_tx_reset(s->bcom_task);
> +		scoped_guard(spinlock_irqsave, &psc_dma->lock) {

[Severity: High]
This isn't a bug introduced by this patch, but could this code race with
module unbind and dereference a freed psc_dma pointer?

During driver removal, the callbacks (like psc_i2s_of_remove) manually call
mpc5200_audio_dma_destroy(), which frees the psc_dma object and clears the
driver data:

sound/soc/fsl/mpc5200_dma.c:mpc5200_audio_dma_destroy() {
    ...
	kfree(psc_dma);
	dev_set_drvdata(&op->dev, NULL);
}

However, the ALSA component is registered via devm_snd_soc_register_component(),
meaning it remains registered and accessible to userspace until the devres
teardown runs after the remove callback returns.

If userspace invokes a PCM operation during this window, ALSA could call
psc_dma_trigger(), which fetches the driver data (now NULL or pointing
to freed memory) and attempts to acquire the lock here using scoped_guard(),
leading to a use-after-free or NULL pointer dereference crash.

> +			if (substream->pstr->stream == SNDRV_PCM_STREAM_CAPTURE)
> +				bcom_gen_bd_rx_reset(s->bcom_task);
> +			else
> +				bcom_gen_bd_tx_reset(s->bcom_task);
>  
> -		for (i = 0; i < runtime->periods; i++)
> -			if (!bcom_queue_full(s->bcom_task))
> -				psc_dma_bcom_enqueue_next_buffer(s);
> +			for (i = 0; i < runtime->periods; i++)
> +				if (!bcom_queue_full(s->bcom_task))
> +					psc_dma_bcom_enqueue_next_buffer(s);
>  
> -		bcom_enable(s->bcom_task);
> -		spin_unlock_irqrestore(&psc_dma->lock, flags);
> +			bcom_enable(s->bcom_task);
> +		}
>  
>  		out_8(&regs->command, MPC52xx_PSC_RST_ERR_STAT);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615093824.115751-1-phucduc.bui@gmail.com?part=10