Re: [PATCH v2 2/3] ASoC: qcom: q6apm-dai: add VMID-based SCM assignment for mDSP buffers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The DMA buffer allocation strictly limits the size to `reserved_buf_size`, entirely omitting the required `POS_BUFFER_BYTES` for push-pull mode streams. The position buffer calculation then points out-of-bounds, corrupting adjacent allocations in the DMA pool.
- [High] Compressed audio streams bypass SCM memory assignment completely, causing Stage-2 faults on protected targets.
- [High] Hardcoded Q6APM_MAX_CARVEOUTS limit breaks sound card probe for topologies with more than 2 DAIs.
- [High] A failed SCM unassign in the non-reserved memory path allows the Stage-2 protected memory to be returned to the system page allocator, leading to kernel panics upon reallocation.
- [Medium] SCM VMID assignment leak on probe error path permanently locks out HLOS memory access.
- [Medium] Missing runtime buffer size constraint for capture streams when using reserved memory.
--

commit 57510541fbc0081167c27ca31e55e5b8fd89730c
Author: Ajay Kumar Nandam <ajay.nandam@oss.qualcomm.com>
Subject: ASoC: qcom: q6apm-dai: add VMID-based SCM assignment for mDSP buffers

This commit adds VMID-based SCM assignment for memory buffers on platforms
with an mDSP. It parses the qcom,vmids property and assigns memory to HLOS
and consumer VMIDs to allow the DSP to access PCM buffers.

diff --git a/sound/soc/qcom/qdsp6/q6apm-dai.c b/sound/soc/qcom/qdsp6/q6apm-dai.c
--- a/sound/soc/qcom/qdsp6/q6apm-dai.c
+++ b/sound/soc/qcom/qdsp6/q6apm-dai.c
@@ -84,9 +96,123 @@ struct q6apm_dai_rtd {
 };
[ ... ]
> +static void q6apm_dai_scm_unassign_addr(struct q6apm_dai_data *pdata,
> +					phys_addr_t addr)
> +{
> +	int i;
> +
> +	for (i = 0; i < pdata->num_carveouts; i++) {
> +		if (pdata->carveout_regions[i].addr != addr)
> +			continue;
> +
> +		q6apm_dai_scm_unassign(&pdata->carveout_regions[i], pdata);
> +		if (pdata->carveout_regions[i].assigned)
> +			return;	/* reclaim failed: keep tracking, warned already */

[Severity: High]
If restoring HLOS permissions fails in q6apm_dai_scm_unassign_addr(), it returns
early while leaving the buffer marked as assigned to the DSP.

However, looking at q6apm_dai_pcm_free(), which calls this function during PCM
destruction, the failure is ignored since the callback returns void.
Won't the ALSA core then proceed to blindly free this Stage-2 protected memory
back to the system page allocator? This could lead to a kernel panic when the
kernel later attempts to reuse the memory.

[ ... ]
> @@ -409,8 +535,11 @@ static int q6apm_dai_open(struct snd_soc_component *component,
>  	}
>  
>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		size_t buf_max = pdata->has_reserved_mem ?
> +				 pdata->reserved_buf_size : BUFFER_BYTES_MAX;
> +
>  		ret = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_BUFFER_BYTES,
> -						   BUFFER_BYTES_MIN, BUFFER_BYTES_MAX);
> +						   BUFFER_BYTES_MIN, buf_max);

[Severity: Medium]
This constraint restricts the buffer size to the reserved memory limit, but it
appears to be gated entirely behind a check for playback streams.

Are capture streams also supposed to be constrained when using reserved memory?
Without it, userspace might request a capture buffer larger than the reserved
DMA pool slice, leading to unexpected -ENOMEM errors.

[ ... ]
> @@ -431,17 +560,18 @@ static int q6apm_dai_open(struct snd_soc_component *component,
>  	}
>  
>  	runtime->private_data = prtd;
> -	runtime->dma_bytes = BUFFER_BYTES_MAX;
>  	if (pdata->sid < 0)
>  		prtd->phys = substream->dma_buffer.addr;
>  	else
>  		prtd->phys = substream->dma_buffer.addr | (pdata->sid << 32);
>  
>  	if (q6apm_is_graph_in_push_pull_mode(prtd->graph)) {
> +		size_t buf_sz = pdata->has_reserved_mem ?
> +				pdata->reserved_buf_size : BUFFER_BYTES_MAX;
>  		void *pos_buffer;
>  
> -		prtd->pos_phys = prtd->phys + BUFFER_BYTES_MAX;
> -		pos_buffer = (void *)(substream->dma_buffer.area + BUFFER_BYTES_MAX);
> +		prtd->pos_phys = prtd->phys + buf_sz;
> +		pos_buffer = (void *)(substream->dma_buffer.area + buf_sz);
>  		prtd->pos_buffer = (struct sh_mem_pull_push_mode_position_buffer *)(pos_buffer);
>  	}

[ ... ]
> @@ -599,13 +741,46 @@ static int q6apm_dai_pcm_new(struct snd_soc_component *component, struct snd_soc
>  		if (is_push_pull)
>  			size += POS_BUFFER_BYTES;
>  
> -		ret = snd_pcm_set_fixed_buffer_all(pcm, SNDRV_DMA_TYPE_DEV, component->dev, size);
> +		if (pdata->has_reserved_mem)
> +			ret = snd_pcm_set_managed_buffer_all(pcm,
> +							     SNDRV_DMA_TYPE_DEV, component->dev,
> +							     pdata->reserved_buf_size,
> +							     pdata->reserved_buf_size);
> +		else
> +			ret = snd_pcm_set_fixed_buffer_all(pcm,
> +							   SNDRV_DMA_TYPE_DEV, component->dev,
> +							   size);

[Severity: High]
When using reserved memory for push-pull streams, the managed buffer size is set
strictly to pdata->reserved_buf_size, completely omitting the extra overhead
required for POS_BUFFER_BYTES.

However, looking back at q6apm_dai_open(), the position buffer is mapped to:
pos_buffer = (void *)(substream->dma_buffer.area + pdata->reserved_buf_size);

Since the allocation doesn't account for this extra size, does this pointer
now access out-of-bounds virtual memory immediately following the allocated
chunk? This seems like it could corrupt adjacent allocations in the DMA pool.

[ ... ]
> +		if (pdata->use_scm_assign && !pdata->has_reserved_mem) {
> +			struct q6apm_scm_region *r;
> +
> +			if (pdata->num_carveouts >= Q6APM_MAX_CARVEOUTS) {
> +				dev_err(component->dev,
> +					"too many SCM carveouts (max %d)\n",
> +					Q6APM_MAX_CARVEOUTS);
> +				q6apm_dai_memory_unmap(component, substream);
> +				return -ENOSPC;
> +			}

[Severity: High]
This dynamically adds a carveout for every PCM substream but rigidly caps the
limit at Q6APM_MAX_CARVEOUTS (which is defined as 2).

Does this arbitrarily restrict the entire sound card to only 2 PCM streams?
Standard configurations often instantiate multiple DAIs with both playback and
capture streams. Would a third PCM stream creation fail probe with -ENOSPC?

[ ... ]
> @@ -1017,23 +1206,161 @@ static const struct snd_soc_component_driver q6apm_fe_dai_component = {
[ ... ]
> +		if (pdata->num_carveouts) {
> +			for (i = 0; i < pdata->num_carveouts; i++) {
> +				rc = q6apm_dai_scm_assign(&pdata->carveout_regions[i],
> +							  pdata);
> +				if (rc) {
> +					dev_err(dev,
> +						"SCM assign carveout[%d] failed: %d\n",
> +						i, rc);
> +					return rc;
> +				}
> +			}
> +			rc = devm_add_action_or_reset(dev,
> +						      q6apm_dai_unassign_all,
> +						      pdata);

[Severity: Medium]
If q6apm_dai_scm_assign() succeeds for the first carveout but fails during a
subsequent iteration, does this early return leak the earlier assignment?

The cleanup action (devm_add_action_or_reset) hasn't been registered yet,
so we might permanently lose HLOS access to that reserved memory region if
probe fails partway through this loop.

[ ... ]

Looking at q6apm_dai_compr_open() which handles compressed audio streams:

static int q6apm_dai_compr_open(struct snd_soc_component *component,
				struct snd_compr_stream *stream)
{
[ ... ]
	size = COMPR_PLAYBACK_MAX_FRAGMENT_SIZE * COMPR_PLAYBACK_MAX_NUM_FRAGMENTS;
	ret = snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, dev, size, &prtd->dma_buffer);
[ ... ]
}

[Severity: High]
For compressed streams, the DMA buffer is allocated from system memory via
snd_dma_alloc_pages() but never appears to be assigned via q6apm_dai_scm_assign().

Since the DSP on these platforms requires explicit VMID assignment to access
memory, will this omission cause a Stage-2 permission fault or DSP crash when
attempting to play compressed audio?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826-a2a-shikra-vmid-v5-v2-0-c3dc62354eee@oss.qualcomm.com?part=2