Re: [PATCH v4 04/10] ASoC: renesas: fsi: Move fsi_stream_is_working()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Time-of-check to time-of-use (TOCTOU) race condition in `fsi_stream_is_working()` leading to a potential NULL pointer dereference or disabled-clock hardware access.
- [High] Asynchronous DMA callbacks (`fsi_dma_complete()`) can execute after hardware clocks are disabled or the stream is stopped, leading to a system hang or NULL pointer dereference.
--

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

ASoC: renesas: fsi: Move fsi_stream_is_working()

Move fsi_stream_is_working() before fsi_count_fifo_err().

> This prepares for a subsequent patch that needs to check stream status
> when handling in-flight IRQ handlers. No functional changwqes intended.

[Severity: Low]
This isn't a bug, but there is a typo (changwqes) in the commit message.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 94ab2e4908108..429c3c9b6ede4 100644
> --- a/sound/soc/renesas/fsi.c
> +++ b/sound/soc/renesas/fsi.c
[ ... ]
> @@ -442,6 +442,16 @@ static int fsi_sample2frame(struct fsi_priv *fsi, int samples)
>  	return samples / fsi->chan_num;
>  }
>  
> +static int fsi_stream_is_working(struct fsi_priv *fsi,
> +				 struct fsi_stream *io)
> +{
> +	struct fsi_master *master = fsi_get_master(fsi);
> +
> +	guard(spinlock_irqsave)(&master->lock);
> +
> +	return !!(io->substream && io->substream->runtime);
> +}
> +

[Severity: High]
This isn't a bug introduced by this patch, but is a pre-existing flaw that
invalidates logic added later in the series.

Can this Time-of-Check to Time-of-Use (TOCTOU) race condition lead to a
NULL pointer dereference?

fsi_stream_is_working() acquires master->lock, checks if io->substream is
valid, and then releases the lock. In callers like fsi_pio_transfer(), this
boolean result allows the code to proceed and unconditionally dereference
io->substream via fsi_pio_get_area():

    if (!fsi_stream_is_working(fsi, io))
        return -EINVAL;
    buf = fsi_pio_get_area(fsi, io);

If fsi_stream_quit() runs concurrently on another CPU during
SNDRV_PCM_TRIGGER_STOP, it sets io->substream = NULL under the lock. Since
the FSI IRQ handler does not hold any stream locks during this path, could
an in-flight fsi_pio_transfer() pass the working check, get preempted while
another CPU clears the pointer, and then dereference NULL?

Additionally, does this invalidate the fix added later in the series for
register access from in-flight IRQs? Since fsi_stream_quit() is called after
fsi_hw_shutdown() in the trigger sequence, io->substream is still non-NULL
when the clocks are disabled. Will this cause fsi_stream_is_working() to
erroneously return true and allow fatal clock-disabled register reads?


[Severity: High]
This is also a pre-existing issue, but involves the broader shutdown sequence
that this series modifies.

Can asynchronous DMA callbacks execute after hardware clocks are disabled or
the stream is stopped, leading to a system hang or NULL pointer dereference?

During SNDRV_PCM_TRIGGER_STOP, fsi_dma_push_start_stop() uses
dmaengine_terminate_all(), which does not synchronously wait for in-flight
DMA callbacks to finish. Immediately after, the driver calls
fsi_hw_shutdown() (disabling the SPU clock) and fsi_stream_quit() (clearing
io->substream).

If a pending fsi_dma_complete() callback executes after fsi_hw_shutdown(),
it unconditionally calls fsi_pointer_update(), which dereferences
io->substream:

    fsi_pointer_update(io, io->period_samples);
    fsi_count_fifo_err(fsi);

If fsi_stream_quit() has already executed, does this cause a NULL pointer
panic?

If the callback executes between shutdown and quit, it continues to
fsi_count_fifo_err() which reads DOFF_ST and DIFF_ST registers. Because
the SPU clock has already been disabled by fsi_hw_shutdown(), will accessing
these registers cause a synchronous external abort and hang the system?

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