Re: [PATCH v4 05/10] ASoC: renesas: fsi: Fix register access from in-flight IRQ after shutdown

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch fails to completely fix the hardware hang due to improper ordering with clock shutdown and introduces a Time-Of-Check to Time-Of-Use (TOCTOU) race condition.

Pre-existing issues:
- [High] Potential NULL pointer dereference in `fsi_pointer_update()` if a DMA completion callback runs after stream shutdown.
- [High] The driver releases DMA channels in `remove` before unregistering the ALSA component, creating a Use-After-Free race condition.
--

commit c264d5f1cdc59e54f0e7191a60f9c8c00661657c
Author: bui duc phuc <phucduc.bui@gmail.com>
ASoC: renesas: fsi: Fix register access from in-flight IRQ after shutdown

This commit attempts to prevent system hangs by adding checks in the
in-flight IRQ handlers to skip hardware access if the stream is no
longer working. It relies on fsi_stream_is_working to detect if the
stream is active before reading or writing registers.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 429c3c9b6ede4..0f350bddeb1d5 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
> @@ -470,6 +470,10 @@ static int fsi_get_current_fifo_samples(struct fsi_priv *fsi,
>  
>  static void fsi_count_fifo_err(struct fsi_priv *fsi)
>  {
> +	if (!fsi_stream_is_working(fsi, &fsi->playback) &&
> +	    !fsi_stream_is_working(fsi, &fsi->capture))
> +		return;
> +

[Severity: High]
Does this introduce a time-of-check to time-of-use race condition?

Since fsi_stream_is_working acquires and drops the master lock, an
interrupt can check the status and drop the lock. Before the handler
reads the hardware registers below, another thread could call
fsi_hw_shutdown, disabling the clock and causing a hang.

Additionally, when stopping the stream in fsi_dai_trigger, the hardware
shutdown is called before the substream is cleared:

sound/soc/renesas/fsi.c:fsi_dai_trigger() {
    ...
    fsi_stream_stop(fsi, io);
    if (!ret)
        ret = fsi_hw_shutdown(fsi, dai->dev);
    fsi_stream_quit(fsi, io);
    ...
}

Because the clock is disabled before io->substream is cleared, an
in-flight IRQ will still pass the fsi_stream_is_working check
and access the registers. 

The same ordering issue occurs in __fsi_suspend, where io->substream is
never cleared at all.

>  	u32 ostatus = fsi_reg_read(fsi, DOFF_ST);
>  	u32 istatus = fsi_reg_read(fsi, DIFF_ST);

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there a potential NULL pointer
dereference in fsi_pointer_update during stream shutdown?

When stopping a DMA stream, fsi_dma_push_start_stop uses
dmaengine_terminate_all, which terminates asynchronously:

sound/soc/renesas/fsi.c:fsi_dma_push_start_stop() {
    ...
    dmaengine_terminate_all(io->chan);
    ...
}

Immediately after, fsi_stream_quit sets io->substream to NULL. If the
DMA callback fsi_dma_complete fires or is still running concurrently
after io->substream is cleared, it will call fsi_pointer_update which
dereferences the pointer without checking:

sound/soc/renesas/fsi.c:fsi_pointer_update() {
    struct snd_pcm_substream *substream = io->substream;
    struct snd_pcm_runtime *runtime = substream->runtime;
    ...
}

Could this cause a kernel panic if the callback executes after cleanup?


[Severity: High]
This is a pre-existing issue, but does fsi_remove create a use-after-free
race condition by releasing DMA channels before ALSA unregistration?

In fsi_probe, the ALSA component is registered via devres using
devm_snd_soc_register_component, meaning it unregisters automatically
after fsi_remove completes. However, fsi_remove manually destroys
the stream resources:

sound/soc/renesas/fsi.c:fsi_remove() {
    ...
    fsi_stream_remove(&master->fsia);
    fsi_stream_remove(&master->fsib);
    ...
}

Since the ALSA device is still accessible to userspace while the DMA
channels are being freed, an application could trigger audio playback.
If playback is started concurrently, it calls fsi_dma_transfer which
dereferences the freed channel:

sound/soc/renesas/fsi.c:fsi_dma_transfer() {
    ...
    desc = dmaengine_prep_dma_cyclic(io->chan, ...);
    ...
    dma_async_issue_pending(io->chan);
}

Could this lead to accessing a freed DMA channel pointer?

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