Re: [PATCH v4 4/7] ASoC: meson: add AUDIN driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Race condition in PAUSE_RELEASE corrupts hrtimer state
- [High] Use-after-free of hrtimer data during device unbind
- [Medium] Hardware state loss in AUDIN FIFO during system suspend
--

commit 3526087ac379aa85de8aca3b698b812a10b5da4e
Author: Valerio Setti <vsetti@baylibre.com>

ASoC: meson: add AUDIN driver

AUDIN provides audio input support to the Amlogic GX based platforms
(GXBB/GXL). It is the counterpart of AIU for capture and instantiates each of
the FIFOs as a component DAI.

> diff --git a/sound/soc/meson/audin-fifo.c b/sound/soc/meson/audin-fifo.c
> new file mode 100644
> index 0000000000000..cb59bb267e845
> --- /dev/null
> +++ b/sound/soc/meson/audin-fifo.c
[ ... ]
> +static int audin_fifo_dai_trigger(struct snd_pcm_substream *substream, int cmd,
> +				  struct snd_soc_dai *dai)
> +{
> +	struct audin_fifo_dai_data *data = snd_soc_dai_dma_data_get_capture(dai);
> +	struct snd_soc_component *component = dai->component;
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		snd_soc_component_update_bits(component, dai->driver->base + AUDIN_FIFO_CTRL,
> +					      AUDIN_FIFO_CTRL_EN,
> +					      AUDIN_FIFO_CTRL_EN);
> +		WRITE_ONCE(data->running, true);
> +		hrtimer_start(&data->polling_timer,
> +			      audin_fifo_bytes_to_ns(data, data->period_bytes),
> +			      HRTIMER_MODE_REL_SOFT);

[Severity: High]
Does this blindly re-enqueue the timer without synchronizing with a potentially
running callback?

If a user rapidly toggles SNDRV_PCM_IOCTL_PAUSE, PAUSE_PUSH uses a
non-blocking hrtimer_try_to_cancel(). If the timer is running and skips
snd_pcm_period_elapsed() (e.g. because delta < period_bytes), it won't take
the PCM stream lock and will execute hrtimer_forward_now() concurrently with
PAUSE_RELEASE.

PAUSE_RELEASE then blindly calls hrtimer_start(), which enqueues the timer.
When the running callback in audin_fifo_timer_cb() reads running=true, it
returns HRTIMER_RESTART, resulting in a double enqueue of the same timer
which can corrupt the hrtimer rbtree.

> +		break;
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +	case SNDRV_PCM_TRIGGER_STOP:
> +		WRITE_ONCE(data->running, false);
> +		/*
> +		 * PCM stream lock is held here. If the timer callback is running
> +		 * (i.e. it cannot be stopped) there is also hrtimer_cancel()
> +		 * in hw_free().
> +		 */
> +		hrtimer_try_to_cancel(&data->polling_timer);
> +		snd_soc_component_update_bits(component, dai->driver->base + AUDIN_FIFO_CTRL,
> +					      AUDIN_FIFO_CTRL_EN, 0);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static int audin_fifo_dai_remove(struct snd_soc_dai *dai)
> +{
> +	kfree(snd_soc_dai_dma_data_get_capture(dai));
> +
> +	return 0;
> +}

[Severity: High]
Does this free the DAI data without waiting for the polling timer to finish?

If the driver is unbound while an audio stream is open, the ALSA core triggers
a stream stop which uses a non-blocking hrtimer_try_to_cancel(). If the timer
callback is running concurrently, it will write to this freed memory.

[ ... ]
> +int audin_fifo_sync_stop(struct snd_soc_component *component, struct snd_pcm_substream *substream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> +	struct snd_soc_dai *dai = snd_soc_rtd_to_cpu(rtd, 0);
> +	struct audin_fifo_dai_data *data = snd_soc_dai_dma_data_get_capture(dai);
> +
> +	hrtimer_cancel(&data->polling_timer);

[Severity: High]
Could this deferred callback access the freed data pointer?

If the driver was unbound and audin_fifo_dai_remove() freed the DAI data,
when the file descriptor is eventually closed this deferred sync_stop callback
will access the freed data pointer to cancel the timer.

> +	WRITE_ONCE(data->running, false);
> +
> +	return 0;
> +}
> diff --git a/sound/soc/meson/audin.c b/sound/soc/meson/audin.c
> new file mode 100644
> index 0000000000000..808e7d4fd31a2
> --- /dev/null
> +++ b/sound/soc/meson/audin.c
[ ... ]
> +static const struct regmap_config audin_regmap_cfg = {
> +	.reg_bits	= 32,
> +	.val_bits	= 32,
> +	.reg_stride	= 4,
> +	.max_register	= 0x148,
> +};

[Severity: Medium]
Will hardware state like the channel mask and endianness be lost across
system suspend and resume cycles?

ALSA calls the driver's prepare() callback to recover a suspended stream,
but not hw_params(). Because this regmap lacks a cache configuration, it
cannot automatically restore the register state configured during hw_params()
when the system resumes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-audin-v4-0-2476646118b5@baylibre.com?part=4

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic