Re: [PATCH v5 04/11] 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] `fsi_stream_is_working()` acquires a lock to validate a pointer but returns a boolean and drops the lock before the caller uses the pointer, creating a Time-Of-Check to Time-Of-Use (TOCTOU) race condition that can lead to a NULL pointer dereference.
- [High] Sleepable Common Clock Framework (CCF) APIs are called from within the ALSA PCM atomic trigger path, causing a sleep-in-atomic bug.
--

commit da681410389113931660183bb91da994c3ed66c1
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() to prepare for a
subsequent patch. No functional changes are intended.

>     when handling in-flight IRQ handlers. No functional changwqes intended.

This isn't a bug, but there's a typo (changwqes) in the commit message.

> diff --git a/sound/soc/renesas/fsi.c b/sound/soc/renesas/fsi.c
> index 800ac40f9680d..0bd0e0c8c5a30 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 does returning a boolean here
create a Time-Of-Check to Time-Of-Use (TOCTOU) race condition?

When fsi_pio_transfer() calls this, it uses the boolean result to locklessly
call fsi_pio_get_area():

sound/soc/renesas/fsi.c:fsi_pio_transfer() {
	...
	if (!fsi_stream_is_working(fsi, io))
		return -EINVAL;
	buf = fsi_pio_get_area(fsi, io);
	...
}

fsi_pio_get_area() then directly accesses io->substream->runtime:

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

Concurrently, if ALSA invokes fsi_dai_trigger(SNDRV_PCM_TRIGGER_STOP), it
calls fsi_stream_quit() which acquires master->lock and clears the pointer:

sound/soc/renesas/fsi.c:fsi_stream_quit() {
	...
	guard(spinlock_irqsave)(&master->lock);
	...
	io->substream	= NULL;
	...
}

If fsi_stream_quit() executes after fsi_stream_is_working() drops the lock
but before fsi_pio_get_area() dereferences io->substream->runtime, could
this result in a NULL pointer dereference?

[Severity: High]
This isn't a bug introduced by this patch, but does the ALSA trigger path
call sleepable clock APIs while in atomic context?

When ALSA invokes the snd_pcm_trigger callback fsi_dai_trigger() on the
SNDRV_PCM_TRIGGER_START path, it executes in atomic context (with the
substream spinlock held and local interrupts disabled).

This calls fsi_hw_startup(), which subsequently triggers fsi_clk_enable():

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

fsi_hw_startup() then executes:

sound/soc/renesas/fsi.c:fsi_hw_startup() {
	...
	if (fsi_is_clk_master(fsi))
		return fsi_clk_enable(dev, fsi);
	...
}

fsi_clk_enable() calls the driver's dynamic .set_rate callback:

sound/soc/renesas/fsi.c:fsi_clk_enable() {
	...
	ret = clock->set_rate(dev, fsi);
	...
}

For example, fsi_clk_set_rate_cpg() invokes clk_set_rate():

sound/soc/renesas/fsi.c:fsi_clk_set_rate_cpg() {
	...
	ret = clk_set_rate(ick, best_cout);
	...
}

Since clk_set_rate() acquires the prepare_lock mutex, won't this trigger
a sleep-in-atomic bug or deadlock?

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