Re: [PATCH RFC 15/23] ASoC: qcom: lpass: allocate LPM buffer memory to DAIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] ASoC: qcom: lpass: u32 underflow and OOB in
  lpass_platform_allocate_lpm()
- [Medium] ASoC: qcom: lpass: uninitialized capture buffer on
  full-duplex LPM MI2S PCM
- [Medium] ASoC: qcom: lpass: unconditional VA LPM reservation blocks VA
  LPM MI2S
- [Medium] ASoC: qcom: lpass: missing LPM clock parsing in
  asoc_qcom_lpass_cpu_platform_probe()
- [Low] ASoC: qcom: lpass: wrong LPM offset in
  lpass_platform_prealloc_cdc_dma_buffer()
--

commit 038ab20def13fb9da7aca847692783be3d4964b4
Author: Neil Armstrong <neil.armstrong@linaro.org>

ASoC: qcom: lpass: allocate LPM buffer memory to DAIs

This commit dynamically allocates LPASS LPM SRAM buffer offsets for Codec
DMA and non-core LPAIF MI2S DAIs instead of using fixed static offsets.

> diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
> index 9ebbc422c63c3..4bbcd59ebd602 100644
> --- a/sound/soc/qcom/lpass-cpu.c
> +++ b/sound/soc/qcom/lpass-cpu.c
> @@ -1258,16 +1258,6 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev)
>  			dev_err(dev, "failed to get cdc dma clocks %d\n", ret);
>  			return ret;
>  		}
> -
> -		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "lpass-rxtx-cdc-dma-lpm");
> -		if (!res)
> -			return -EINVAL;
> -		drvdata->rxtx_cdc_dma_lpm_buf = res->start;
> -
> -		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "lpass-va-cdc-dma-lpm");
> -		if (!res)
> -			return -EINVAL;
> -		drvdata->va_cdc_dma_lpm_buf = res->start;
>  	}
>  
>  	if (variant->mi2s_desc) {
> @@ -1281,6 +1271,9 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev)
>  						   variant->mi2s_desc[i].reg_name);
>  			if (ret)
>  				return ret;
> +
> +			if (variant->mi2s_desc[i].lpm_type != LPASS_LPM_NONE)
> +				drvdata->mi2s_lpm_enable = true;
>  		}
>  	} else {
>  		drvdata->num_lpaif = 1;
> @@ -1290,6 +1283,20 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev)
>  			return ret;
>  	}
>  
> +	if (drvdata->codec_dma_enable || drvdata->mi2s_lpm_enable) {
> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "lpass-rxtx-cdc-dma-lpm");
> +		if (!res)
> +			return -EINVAL;
> +		drvdata->rxtx_cdc_dma_lpm_buf = res->start;
> +		drvdata->rxtx_cdc_dma_lpm_size = resource_size(res);
> +
> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "lpass-va-cdc-dma-lpm");
> +		if (!res)
> +			return -EINVAL;
> +		drvdata->va_cdc_dma_lpm_buf = res->start;
> +		drvdata->va_cdc_dma_lpm_size = resource_size(res);
> +	}

[Severity: Medium]
Should of_lpass_cdc_dma_clks_parse() also be called when
drvdata->mi2s_lpm_enable is set?

Because of_lpass_cdc_dma_clks_parse() remains inside the
if (drvdata->codec_dma_enable) block above, a board that enables LPM-backed
MI2S DAIs without any Codec DMA DAI leaves drvdata->codec_mem0..3 and
drvdata->va_mem0 as NULL.  Even with commit "ASoC: qcom: lpass: enable the
right codec_mem clock when usin LPM memory" later in this series,
lpass_cpu_daiops_startup() will pass a NULL clk pointer to
clk_prepare_enable(), leaving the LPM SRAM unclocked when accessed by the
CPU or LPAIF DMA.

> diff --git a/sound/soc/qcom/lpass-platform.c b/sound/soc/qcom/lpass-platform.c
> index a953db262aa00..75d2c872f5123 100644
> --- a/sound/soc/qcom/lpass-platform.c
> +++ b/sound/soc/qcom/lpass-platform.c

[ ... ]

> @@ -1315,23 +1348,55 @@ static int lpass_platform_prealloc_cdc_dma_buffer(struct snd_soc_component *comp
>  	switch (dai_id) {
>  	case LPASS_CDC_DMA_RX0 ... LPASS_CDC_DMA_RX9:
>  		buf->bytes = lpass_platform_rxtx_hardware.buffer_bytes_max;
> -		buf->addr = drvdata->rxtx_cdc_dma_lpm_buf;
> +		buf->addr = drvdata->rxtx_cdc_dma_lpm_buf + drvdata->va_lpm_offset;
>  		break;
>  	case LPASS_CDC_DMA_TX0 ... LPASS_CDC_DMA_TX8:
>  		buf->bytes = lpass_platform_rxtx_hardware.buffer_bytes_max;
> -		buf->addr = drvdata->rxtx_cdc_dma_lpm_buf + LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE;
> +		buf->addr = drvdata->rxtx_cdc_dma_lpm_buf +
> +				drvdata->va_lpm_offset + LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE;
>  		break;

[Severity: Low]
Should these two cases use drvdata->rxtx_lpm_offset instead of
drvdata->va_lpm_offset?  lpass_platform_allocate_lpm() populates
drvdata->rxtx_lpm_offset for RXTX Codec DMA, and commit "ASoC: qcom: lpass:
enable the right codec_mem clock when usin LPM memory" uses
drvdata->rxtx_lpm_offset to select the RXTX LPM clock, so using
va_lpm_offset here will desynchronize the buffer address from the clock if
the two offsets ever differ.

>  	case LPASS_CDC_DMA_VA_TX0 ... LPASS_CDC_DMA_VA_TX8:
>  		buf->bytes = lpass_platform_va_hardware.buffer_bytes_max;
> -		buf->addr = drvdata->va_cdc_dma_lpm_buf;
> +		buf->addr = drvdata->va_cdc_dma_lpm_buf + drvdata->va_lpm_offset;
>  		break;
>  	case LPASS_CDC_DMA_WSA_RX0 ... LPASS_CDC_DMA_WSA_RX9:
>  		buf->bytes = lpass_platform_wsa_hardware.buffer_bytes_max;
> -		buf->addr = drvdata->rxtx_cdc_dma_lpm_buf + LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE * 2;
> +		buf->addr = drvdata->rxtx_cdc_dma_lpm_buf + drvdata->wsa_lpm_offset;
>  		break;
>  	case LPASS_CDC_DMA_WSA_TX0 ... LPASS_CDC_DMA_WSA_TX8:
>  		buf->bytes = lpass_platform_wsa_hardware.buffer_bytes_max;
> -		buf->addr = drvdata->rxtx_cdc_dma_lpm_buf + LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE * 3;
> +		buf->addr = drvdata->rxtx_cdc_dma_lpm_buf +
> +				drvdata->wsa_lpm_offset + LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE;
> +		break;
> +	case MI2S_PRIMARY ... MI2S_QUINARY:
> +	case MI2S_SENARY ... MI2S_SEPTENARY:
> +		if (is_lpm_mi2s_port(drvdata, dai_id)) {
> +			unsigned int port_id = mi2s_port_id(dai_id);
> +
> +			buf->bytes = lpass_platform_lpm_mi2s_hardware.buffer_bytes_max;
> +			switch (drvdata->mi2s_lpm[port_id]) {
> +			case LPASS_RXTX_CDC_LPM:
> +				if (pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream)
> +					buf->addr = drvdata->rxtx_cdc_dma_lpm_buf +
> +						    drvdata->mi2s_lpm_offset[port_id];
> +				else
> +					buf->addr = drvdata->rxtx_cdc_dma_lpm_buf +
> +						    drvdata->mi2s_lpm_offset[port_id] +
> +						    LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE;
> +				break;
> +			case LPASS_VA_CDC_LPM:
> +				if (pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream)
> +					buf->addr = drvdata->va_cdc_dma_lpm_buf +
> +						    drvdata->mi2s_lpm_offset[port_id];
> +				else
> +					buf->addr = drvdata->va_cdc_dma_lpm_buf +
> +						    drvdata->mi2s_lpm_offset[port_id] +
> +						    LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE;
> +				break;
> +			default:
> +				break;
> +			}
> +		}
>  		break;
>  	default:
>  		break;

[Severity: Medium]
Does lpass_platform_prealloc_cdc_dma_buffer() leave the capture
substream->dma_buffer uninitialized when an LPM MI2S PCM has both playback
and capture streams?

At the top of lpass_platform_prealloc_cdc_dma_buffer(), only one substream
is selected:

sound/soc/qcom/lpass-platform.c:lpass_platform_prealloc_cdc_dma_buffer() {
	if (pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream)
		substream = pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream;
	else
		substream = pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream;

	buf = &substream->dma_buffer;
...
}

Unlike Codec DMA DAIs (which have separate unidirectional RX and TX DAI
IDs), a single MI2S DAI can have both playback and capture substreams on
the same struct snd_pcm.  When both exist, only the playback dma_buffer is
initialized and memremapped, while the capture substream->dma_buffer stays
zeroed.  Opening the capture stream in lpass_platform_pcmops_open() then
sets runtime->dma_area to NULL via snd_pcm_set_runtime_buffer(), causing a
NULL pointer dereference in lpass_platform_copy().

[ ... ]

> @@ -1464,6 +1527,64 @@ static const struct snd_soc_component_driver lpass_component_driver = {
>  
>  };
>  
> +static int lpass_platform_allocate_lpm(struct lpass_data *drvdata)
> +{
> +	u32 rxtx_cdc_dma_lpm_remain = drvdata->rxtx_cdc_dma_lpm_size;
> +	u32 va_cdc_dma_lpm_remain = drvdata->va_cdc_dma_lpm_size;
> +	u32 rxtx_cdc_dma_lpm_offset = 0;
> +	u32 va_cdc_dma_lpm_offset = 0;
> +	int port_id;
> +
> +	if (drvdata->codec_dma_enable) {
> +		drvdata->rxtx_lpm_offset = rxtx_cdc_dma_lpm_offset;
> +		rxtx_cdc_dma_lpm_offset += LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE * 2;
> +		rxtx_cdc_dma_lpm_remain -= LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE * 2;
> +
> +		drvdata->va_lpm_offset = va_cdc_dma_lpm_offset;
> +		va_cdc_dma_lpm_offset += LPASS_VA_CDC_DMA_LPM_BUFF_SIZE;
> +		va_cdc_dma_lpm_remain -= LPASS_VA_CDC_DMA_LPM_BUFF_SIZE;
> +
> +		if (drvdata->wsa_enable) {
> +			drvdata->wsa_lpm_offset = rxtx_cdc_dma_lpm_offset;
> +			rxtx_cdc_dma_lpm_offset += LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE * 2;
> +			rxtx_cdc_dma_lpm_remain -= LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE * 2;
> +		}
> +	}

[Severity: Medium]
Does reserving LPASS_VA_CDC_DMA_LPM_BUFF_SIZE whenever
drvdata->codec_dma_enable is set prevent using LPASS_VA_CDC_LPM for MI2S
when only RXTX or WSA Codec DMA is enabled?

of_lpass_cpu_parse_dai_data() sets drvdata->codec_dma_enable = 1 for any
RXTX, WSA, or VA Codec DMA DAI.  Because the hardware lpass-va-cdc-dma-lpm
region is 12 KiB (LPASS_VA_CDC_DMA_LPM_BUFF_SIZE), subtracting
LPASS_VA_CDC_DMA_LPM_BUFF_SIZE here even when no VA Codec DMA DAI is in DT
reduces va_cdc_dma_lpm_remain to 0, causing the LPASS_VA_CDC_LPM check
below to always fail with -EINVAL.

> +
> +	if (!drvdata->mi2s_lpm_enable)
> +		return 0;
> +
> +	for (port_id = 0; port_id < LPASS_MAX_MI2S_PORTS; port_id++) {
> +		if (!test_bit(port_id, &drvdata->lpaif_port_enable))
> +			continue;
> +
> +		switch (drvdata->mi2s_lpm[port_id]) {
> +		case LPASS_RXTX_CDC_LPM:
> +			if (rxtx_cdc_dma_lpm_remain < LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE)
> +				return -EINVAL;
> +			drvdata->mi2s_lpm_offset[port_id] = rxtx_cdc_dma_lpm_offset;
> +			rxtx_cdc_dma_lpm_offset += LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE * 2;
> +			rxtx_cdc_dma_lpm_remain -= LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE * 2;
> +			break;
> +		case LPASS_VA_CDC_LPM:
> +			/*
> +			 * VA Codec uses 12KiB, but MI2S uses 8KiB
> +			 * Meaning only VA _or_ a single MI2S can be enabled
> +			 */
> +			if (va_cdc_dma_lpm_remain < LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE)
> +				return -EINVAL;
> +			drvdata->mi2s_lpm_offset[port_id] = va_cdc_dma_lpm_offset;
> +			va_cdc_dma_lpm_offset += LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE * 2;
> +			va_cdc_dma_lpm_remain -= LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE * 2;
> +			break;

[Severity: High]
Can rxtx_cdc_dma_lpm_remain and va_cdc_dma_lpm_remain underflow and assign
out-of-bounds LPM buffer offsets here?

Both cases check for LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE (8 KiB) remaining,
but then advance the offset and decrement the unsigned u32 remaining
counter by LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE * 2 (16 KiB).  On a 12 KiB
lpass-va-cdc-dma-lpm region (0x3000 bytes), enabling one MI2S port on
LPASS_VA_CDC_LPM passes the < 0x2000 check and subtracts 0x4000, wrapping
va_cdc_dma_lpm_remain to 0xfffff000:

1. If a second MI2S port on LPASS_VA_CDC_LPM is enabled (such as both
   MI2S_QUATERNARY and MI2S_QUINARY), 0xfffff000 < 0x2000 is false, so it
   is given an offset 16 KiB past the start of the 12 KiB VA LPM region.
2. Even with a single MI2S port on LPASS_VA_CDC_LPM, a capture stream in
   lpass_platform_prealloc_cdc_dma_buffer() is placed at
   mi2s_lpm_offset[port_id] + LPASS_RXTX_CDC_DMA_LPM_BUFF_SIZE (8 KiB ..
   16 KiB), overflowing the 12 KiB VA LPM region by 4 KiB during
   memremap(), mmap(), and DMA.
3. The same u32 underflow occurs on LPASS_RXTX_CDC_LPM whenever 8 KiB
   remains before subtracting 16 KiB.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-topic-sm8650-el2-v1-0-5183f6aab9fc@linaro.org?part=15