Re: [PATCH v5 15/17] ASoC: renesas: rz-ssi: Add pause support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Out-of-bounds DMA Access on Resume due to missing bounds check
- [Critical] Catastrophic DMA release on active channels during PAUSE_RELEASE failure
- [Critical] Use-After-Free during STOP/PAUSE in Full-Duplex mode
- [High] Full-Duplex Auto-Restart Loop on Pause
- [High] Data Loss on Resume due to skipped period
- [High] Race condition on asynchronous DMA termination
- [High] Hardware registers modified without lock protection
--

commit e6b2a93963fa9285cbb11812697caca72ab79258
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

ASoC: renesas: rz-ssi: Add pause support

Add pause support as a preparatory step to switch to PCM dmaengine APIs.

> diff --git a/sound/soc/renesas/rz-ssi.c b/sound/soc/renesas/rz-ssi.c
> index 71e434cfe07b9..d4e1dded3a9c1 100644
> --- a/sound/soc/renesas/rz-ssi.c
> +++ b/sound/soc/renesas/rz-ssi.c
> @@ -847,6 +847,7 @@ static int rz_ssi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
>  
>  	switch (cmd) {
>  	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
>  		ret = rz_ssi_trigger_resume(ssi, strm);

If a pause is triggered right when strm->completed_dma_buf_pos is at
runtime->buffer_size - runtime->period_size, could rz_ssi_trigger_resume()
set strm->dma_buffer_pos exactly to runtime->buffer_size?

rz_ssi_trigger_resume() sets:
    strm->dma_buffer_pos = strm->completed_dma_buf_pos + runtime->period_size;

If this happens, will the next DMA transfer read or write out of bounds past
the end of the allocated ALSA ring buffer?

Also, in half-duplex mode where PAUSE_PUSH successfully aborts the current
period, does adding runtime->period_size here permanently discard the
unplayed/uncaptured period and create a gap in the audio data?

Both of these regressions are noted as fixed later in the series by
commit "ASoC: renesas: rz-ssi: Use generic PCM dmaengine APIs".

Since rz_ssi_stop() returns early in full-duplex mode without terminating
the active DMA channel, what happens if PAUSE_RELEASE is issued before the
active descriptors complete?

Execution will fall through to the SNDRV_PCM_TRIGGER_START case and call
rz_ssi_dma_slave_config() on the still-active DMA channel. If the dmaengine
driver rejects this configuration and returns -EBUSY, the fallback to PIO
will call rz_ssi_release_dma_channels(ssi).

Since rz_ssi_release_dma_channels() unconditionally releases both playback
and capture DMA channels, could this cause a use-after-free or panic if the
opposite stream is actively mid-transfer?

This regression is also fixed later in the series by commit "ASoC: renesas:
rz-ssi: Use generic PCM dmaengine APIs".

In half-duplex mode, PAUSE_PUSH terminates the DMA via
dmaengine_terminate_async(). During PAUSE_RELEASE, is there a
synchronization point (like dmaengine_synchronize()) to ensure the
termination has completed before new descriptors are queued?

If the asynchronous termination is still executing when the new descriptors
are queued, could they be inadvertently purged?

This is noted as fixed later in the series by commit "ASoC: renesas: rz-ssi:
Use generic PCM dmaengine APIs".

>  		if (ret)
>  			return ret;

[ ... ]

> @@ -888,6 +889,7 @@ static int rz_ssi_dai_trigger(struct snd_pcm_substream *substream, int cmd,
>  		break;
>  
>  	case SNDRV_PCM_TRIGGER_SUSPEND:
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>  		rz_ssi_stop(ssi, strm);

This isn't a bug introduced by this patch, but adding PAUSE exposes it
further. In rz_ssi_stop(), if one stream is halted but the other is still
running (full-duplex), the function returns early without calling
dmaengine_terminate_async().

If the ALSA subsystem subsequently closes the stream and frees the ring
buffer while the DMA channel continues executing already-queued descriptors,
could this lead to a use-after-free?

This is fixed later in the series by commit "ASoC: renesas: rz-ssi: Use
generic PCM dmaengine APIs".

In full-duplex mode, pausing one stream skips disabling the hardware and does
not terminate the DMA channel. When the DMA channel eventually starves the
hardware FIFO, it triggers an underflow/overflow interrupt.

Since the interrupt handler unconditionally queues new descriptors and calls
rz_ssi_start() (which sets strm->running = 1), will this completely override
the ALSA PAUSE state and allow the stream to continue processing buffers in
an endless interrupt storm?

This logic flaw survives the next patch "ASoC: renesas: rz-ssi: Use generic
PCM dmaengine APIs".

Also not a bug introduced by this patch, but adding PAUSE exposes more paths
invoking these functions. rz_ssi_start() and rz_ssi_stop() perform non-atomic
read-modify-write operations on shared hardware registers (SSICR and SSIFCR)
without acquiring ssi->lock.

If playback and capture triggers happen simultaneously on different CPUs,
could race conditions cause both CPUs to read stale state and fail to enable
SSICR_TEN | SSICR_REN?

This race condition survives the next commit "ASoC: renesas: rz-ssi: Use
generic PCM dmaengine APIs".

>  		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512121219.216159-1-claudiu.beznea.uj@bp.renesas.com?part=15