Re: [PATCH v5 05/11] 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 4 potential issue(s) to consider:

New issues:
- [High] The newly added `fsi_stream_is_working()` checks introduce a Time-of-Check to Time-of-Use (TOCTOU) race condition with clock shutdown.

Pre-existing issues:
- [Critical] A pre-existing TOCTOU race condition in PIO transfers causes a NULL pointer dereference because `fsi_stream_is_working()` releases the lock before `io->substream` is accessed.
- [Critical] The DMA transfer handler lacks a stream active check, leading to a fatal NULL pointer dereference if triggered by a shared port interrupt when the stream is stopped.
- [High] The fix fails to add stream activity checks to the PIO data transfer paths called earlier in the same IRQ handler, leaving an active path that still unconditionally triggers system hangs.
--

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

ASoC: renesas: fsi: Fix register access from in-flight IRQ after shutdown

This commit attempts to fix system hangs caused by in-flight IRQs accessing
hardware registers after the SPU clock is disabled. It introduces checks
using fsi_stream_is_working() to return early from IRQ handlers if no
streams are active.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> --- 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?

fsi_stream_is_working() acquires and immediately releases master->lock.
If snd_pcm_stop executes concurrently on another CPU right after this check
returns true, it will call fsi_stream_quit() and fsi_hw_shutdown(), setting
substream = NULL and disabling the hardware clocks.

Would the interrupted CPU then proceed to access hardware registers
(like fsi_reg_read(fsi, DOFF_ST) below) with the clocks disabled, resulting
in the exact system hang this patch was intended to fix?

[Severity: High]
This is a pre-existing issue, but the patch missed it while attempting
to fix the exact same issue. In fsi_interrupt(), before
fsi_count_fifo_err() is reached, the driver calls fsi_stream_transfer()
which routes to fsi_pio_push() or fsi_pio_pop().

Both functions immediately calculate sample space by calling
fsi_get_current_fifo_samples(), which unconditionally performs bare
register reads before any stream activity check is performed:

fsi_get_current_fifo_samples() {
        ...
        status = is_play ?
                fsi_reg_read(fsi, DOFF_ST) :
                fsi_reg_read(fsi, DIFF_ST);
        ...
}

Will an in-flight IRQ therefore access disabled hardware registers during
the PIO transfer phase, bypassing the intended mitigations?

[Severity: Critical]
This is a pre-existing issue, but does a race condition in PIO transfers
cause a NULL pointer dereference because fsi_stream_is_working() releases
the lock before io->substream is accessed?

fsi_pio_transfer() calls fsi_stream_is_working() to ensure the stream is
active. fsi_stream_is_working() safely verifies io->substream under
master->lock but releases the lock immediately.

fsi_pio_transfer() then proceeds to call fsi_pio_get_area() locklessly,
which unconditionally dereferences io->substream->runtime:

fsi_pio_transfer() {
        if (!fsi_stream_is_working(fsi, io))
                return -EINVAL;

        buf = fsi_pio_get_area(fsi, io);
        ...
}

fsi_pio_get_area() {
        struct snd_pcm_runtime *runtime = io->substream->runtime;
        ...
}

If an ALSA SNDRV_PCM_TRIGGER_STOP command executes fsi_stream_quit()
concurrently on another CPU, setting io->substream = NULL, will this
sequence interleave precisely to cause a NULL pointer dereference?

[Severity: Critical]
This is a pre-existing issue, but does the DMA transfer handler lack a
stream active check, leading to a NULL pointer dereference?

When an FSI port is configured with DMA for playback and PIO for capture,
they share the same hardware interrupt line and fsi_interrupt handler.
If the playback stream is stopped, its hardware FIFO remains empty,
continuously asserting the playback data request bit in the shared
int_st register.

The fsi_interrupt handler reads the raw int_st register without applying
software masks and unconditionally routes to fsi_stream_transfer():

fsi_interrupt() {
        ...
        if (int_st & AB_IO(1, AO_SHIFT))
                fsi_stream_transfer(&master->fsia.playback);
        ...
}

The DMA handler fsi_dma_transfer() is invoked without verifying if the
stream is actively working. It immediately calls fsi_get_dai(io->substream).
Since the playback stream is stopped, io->substream is NULL.

Does this result in an immediate NULL pointer dereference in
asoc_substream_to_rtd()?

>  	u32 ostatus = fsi_reg_read(fsi, DOFF_ST);
>  	u32 istatus = fsi_reg_read(fsi, DIFF_ST);
>  
> @@ -681,6 +685,10 @@ static void fsi_irq_clear_status(struct fsi_priv *fsi)
>  	u32 data = 0;
>  	struct fsi_master *master = fsi_get_master(fsi);
>  
> +	if (!fsi_stream_is_working(fsi, &fsi->playback) &&
> +	    !fsi_stream_is_working(fsi, &fsi->capture))
> +		return;
> +

[Severity: High]
Does this also introduce a Time-of-Check to Time-of-Use race condition?

The stream status is checked and the lock is dropped before clearing the
interrupt status locklessly via fsi_core_mask_set(). Could a concurrent
stream stop and clock shutdown occur between the check and the hardware
access, causing a regression?

>  	data |= AB_IO(1, fsi_get_port_shift(fsi, &fsi->playback));
>  	data |= AB_IO(1, fsi_get_port_shift(fsi, &fsi->capture));

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