Re: [PATCH v2 09/25] ASoC: atmel: atmel-pcm-pdc: Fix atmel_pcm_close() being skipped on rollback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] sound: atmel: Use-after-free and NULL deref in atmel_pcm_hw_free race
--

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

ASoC: atmel: atmel-pcm-pdc: Fix atmel_pcm_close() being skipped on rollback

This commit fixes an issue where atmel_pcm_close() might be skipped on
rollback. It forces atmel_pcm_open() to explicitly return 0 on success,
as snd_pcm_hw_constraint_integer() can return 1, which causes ASoC to skip
marking the component as opened.

> diff --git a/sound/soc/atmel/atmel-pcm-pdc.c b/sound/soc/atmel/atmel-pcm-pdc.c
> index 374f93fa6e0b0..4cedf2ad934d6 100644
> --- a/sound/soc/atmel/atmel-pcm-pdc.c
> +++ b/sound/soc/atmel/atmel-pcm-pdc.c
> @@ -295,6 +295,13 @@ static int atmel_pcm_open(struct snd_soc_component *component,
>  	}
>  	runtime->private_data = prtd;
>  
> +	/*
> +	 * ASoC only marks the component as opened when .open() returns 0.
> +	 * Since snd_pcm_hw_constraint_integer() may return 1, explicitly
> +	 * return 0 to ensure .close() is not skipped on rollback.
> +	 */
> +	return 0;

[Severity: High]
Could this change expose a race condition during teardown? By ensuring
atmel_pcm_close() executes, it seems we can now hit a use-after-free or
NULL pointer dereference.

When an audio stream is closed, ALSA triggers snd_pcm_drop() followed by
trigger(STOP), which disables the PDC but does not disable SSC interrupts
globally. This is followed by atmel_pcm_hw_free(), which clears the
interrupt handler:

sound/soc/atmel/atmel-pcm-pdc.c:atmel_pcm_hw_free() {
    ...
    prtd->params->dma_intr_handler = NULL;
    ...
}

And finally atmel_pcm_close() is called, which frees the prtd structure:

sound/soc/atmel/atmel-pcm-pdc.c:atmel_pcm_close() {
    ...
    kfree(prtd);
    ...
}

Meanwhile, an interrupt can still fire, and atmel_ssc_interrupt() calls the
handler without synchronization:

sound/soc/atmel/atmel_ssc_dai.c:atmel_ssc_interrupt() {
    ...
    dma_params->dma_intr_handler(ssc_sr, dma_params->substream);
    ...
}

Since there is no synchronize_irq() in the PCM teardown path,
atmel_ssc_interrupt() can race with atmel_pcm_hw_free() and
atmel_pcm_close(). This could lead to a NULL pointer dereference if
dma_intr_handler is evaluated after being cleared, or a use-after-free of
prtd if kfree(prtd) executes while atmel_pcm_dma_irq() is still actively
processing the stream on another CPU.

> +
>   out:
>  	return ret;
>  }

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