Re: [PATCH v1 2/3] ASoC: qcom: q6apm-dai: add VMID-based SCM assignment

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Passing a DMA address (IOVA) to TrustZone instead of a physical address causes arbitrary memory permission corruption.
- [Critical] If SCM unassign fails, the physical memory is freed back to the kernel while the DSP retains RW permissions, creating a severe memory corruption vulnerability.
- [High] The SCM memory assignment size truncates the actually allocated buffer size, omitting the required DSP bug workaround page and the push-pull position buffer.
- [High] Compress audio streams are completely excluded from VMID SCM assignment, causing playback/capture faults.
- [High] Component-wide SCM assignment state breaks support for multiple DAIs, leading to SMMU faults and active memory unassignment.
- [High] Double unmap of DSP memory on pcm_new failure due to manual cleanup overriding the ALSA core pcm_free sequence.
--

commit c8f968a876b0a04c1d6a29dfdfb22332fde3da72
Author: Ajay Kumar Nandam <ajay.nandam@oss.qualcomm.com>

ASoC: qcom: q6apm-dai: add VMID-based SCM assignment

This commit adds optional VMID-based SCM memory assignment for q6apm
fixed PCM DMA buffers. It introduces a use_scm_assign flag and
assign/unassign handlers to grant the DSP shared ownership of the DMA
buffers when the qcom,vmid DT property is present.

> diff --git a/sound/soc/qcom/qdsp6/q6apm-dai.c b/sound/soc/qcom/qdsp6/q6apm-dai.c
> index bf1f872a09f45..ae7bcf9802367 100644
> --- a/sound/soc/qcom/qdsp6/q6apm-dai.c
> +++ b/sound/soc/qcom/qdsp6/q6apm-dai.c
[ ... ]
> @@ -83,10 +86,106 @@ struct q6apm_dai_rtd {
>  	bool notify_on_drain;
>  };
>  
> +struct q6apm_scm_region {
> +	phys_addr_t dma_addr;
> +	unsigned int size;
> +	u64 src_perms;
> +	bool assigned;
> +};
> +
>  struct q6apm_dai_data {
>  	long long sid;
> +	int num_vmids;
> +	u32 vmids[Q6APM_MAX_VMIDS];
> +	bool use_scm_assign;
> +	struct q6apm_scm_region scm_regions[SNDRV_PCM_STREAM_LAST + 1];
>  };

[Severity: High]
Is it safe to store the SCM assignment state globally per component?

If multiple playback DAIs are opened, q6apm_dai_assign_memory() might
see scm_region->assigned as true from the first DAI and silently skip
assigning the second DAI's buffer. If the first DAI is subsequently closed,
the memory could be unassigned while the second DAI is still actively
using its buffer. Should this state be tracked per-substream or per-DAI
instead?

[Severity: High]
Are compress audio streams intentionally excluded from VMID SCM assignment?

The VMID assignment appears to be integrated exclusively into the standard
PCM path. Compress audio streams allocate their own DMA buffers in
q6apm_dai_compr_open() using snd_dma_alloc_pages(), but they never seem
to get assigned to the configured VMIDs. Could this trigger a secure Stage-2
memory protection fault on platforms requiring VMID assignment?

[ ... ]
> +static int q6apm_dai_assign_memory(struct snd_pcm_substream *substream,
> +				   struct q6apm_dai_data *pdata)
> +{
[ ... ]
> +	scm_region->dma_addr = substream->dma_buffer.addr;
> +	scm_region->size = ALIGN(BUFFER_BYTES_MAX, PAGE_SIZE);
> +	scm_region->src_perms = BIT_ULL(QCOM_SCM_VMID_HLOS);
> +
> +	ret = qcom_scm_assign_mem(scm_region->dma_addr, scm_region->size,
> +				  &scm_region->src_perms, dst_vmids, dst_count);

[Severity: Critical]
Does substream->dma_buffer.addr contain an IOVA rather than a physical address?

If an IOMMU is present, passing an IOVA to qcom_scm_assign_mem() can cause
issues since TrustZone modifies Stage-2 translation tables, which strictly
operate on physical addresses. Passing an IOVA could instruct the hypervisor
to restrict physical memory that numerically matches the IOVA, leading to
memory permission corruption of arbitrary pages.

[Severity: High]
Does this hardcoded assignment size omit the extra workaround page and the
push-pull position buffer?

In q6apm_dai_pcm_new(), the allocated buffer size is calculated as
BUFFER_BYTES_MAX plus an extra PAGE_SIZE, and an additional POS_BUFFER_BYTES
if push-pull mode is active. Since the SCM assigned size is hardcoded to
ALIGN(BUFFER_BYTES_MAX, PAGE_SIZE), it seems the extra padding page and
position buffer might remain without correct SCM permissions.

[ ... ]
> +static int q6apm_dai_unassign_memory(struct snd_soc_component *component,
> +				     struct snd_pcm_substream *substream,
> +				     struct q6apm_dai_data *pdata)
> +{
[ ... ]
> +	ret = qcom_scm_assign_mem(scm_region->dma_addr, scm_region->size,
> +				  &scm_region->src_perms, &hlos, 1);
> +	if (!ret) {
> +		scm_region->assigned = false;
> +		scm_region->src_perms = BIT_ULL(QCOM_SCM_VMID_HLOS);
> +	} else {
> +		dev_err(dev, "Failed to unassign DMA buffer %pa from VMIDs: %d\n",
> +			&scm_region->dma_addr, ret);
> +	}
> +
> +	return ret;
> +}

[Severity: Critical]
If qcom_scm_assign_mem() fails, does returning the error code allow the ALSA
core to subsequently free the physical DMA buffer back to the kernel?

If the unassign fails, the code merely logs an error and returns. Since
q6apm_dai_pcm_free() ignores this return value, the ALSA core could free
the buffer while the DSP retains architectural RW permissions to the freed
memory. Could this allow the DSP to arbitrarily corrupt kernel or userspace
memory that later reuses these pages? Should the memory be intentionally
leaked instead if it cannot be successfully unassigned?

[ ... ]
> @@ -606,6 +706,14 @@ static int q6apm_dai_pcm_new(struct snd_soc_component *component, struct snd_soc
>  		ret = q6apm_dai_memory_map(component, substream, graph_id, is_push_pull);
>  		if (ret)
>  			return ret;
> +
> +		if (pdata->use_scm_assign) {
> +			ret = q6apm_dai_assign_memory(substream, pdata);
> +			if (ret) {
> +				q6apm_dai_memory_unmap(component, substream);
> +				return ret;
> +			}
> +		}
>  	}

[Severity: High]
Does this manual call to q6apm_dai_memory_unmap() cause a double-free on the
DSP?

If q6apm_dai_assign_memory() fails, the driver manually calls
q6apm_dai_memory_unmap(). However, the ALSA framework inherently handles
pcm_new failures by tearing down the sound card components, which invokes
q6apm_dai_pcm_free(). This callback unconditionally calls
q6apm_dai_memory_unmap() again, which might result in a duplicate unmap
command sent to the DSP firmware.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609064038.492641-1-ajay.nandam@oss.qualcomm.com?part=2