Re: [PATCH v5 4/4] ASoC: qcom: q6apm-dai: add SCM buffer assignment for mDSP platforms

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] asoc: qcom: compress stream allocation bypasses reserved pool size limits
- [Medium] asoc: qcom: SCM assignment expands beyond reserved memory bounds
- [Medium] asoc: qcom: memory and graph leak in q6apm_dai_compr_open() error path
- [Medium] asoc: qcom: capture stream lacks dynamic buffer size constraint
--

commit 8016abb309ec8211ded913589c6a8b5656e3fed2
Author: Ajay Kumar Nandam <ajay.nandam@oss.qualcomm.com>
Subject: ASoC: qcom: q6apm-dai: add SCM buffer assignment for mDSP platforms

This commit adds support for mDSP platforms by explicitly assigning PCM buffers
to the appropriate Virtual Machine IDs via SCM calls at probe time and during
stream opening. It manages whole-pool assignment for data-path buffers when a
memory region is provided, falling back to per-buffer assignments when no
data-path pool is present.

> diff --git a/sound/soc/qcom/qdsp6/q6apm-dai.c b/sound/soc/qcom/qdsp6/q6apm-dai.c
> index 033197bdd8705..8f12b860e1250 100644
> --- a/sound/soc/qcom/qdsp6/q6apm-dai.c
> +++ b/sound/soc/qcom/qdsp6/q6apm-dai.c
[ ... ]
> @@ -409,8 +502,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]
Since this constraint is only applied when the stream is
SNDRV_PCM_STREAM_PLAYBACK, capture streams still advertise a static
BUFFER_BYTES_MAX of 512KB. If the reserved memory pool limits reserved_buf_size
to something smaller than 512KB, couldn't userspace request a capture size up
to 512KB, causing hw_params to fail because the preallocated buffer is too small?

>  		if (ret < 0) {
[ ... ]
> @@ -683,6 +814,17 @@ static int q6apm_dai_compr_open(struct snd_soc_component *component,
>  	if (ret)
>  		return ret;
>  
> +	if (pdata->use_scm_assign && !pdata->has_reserved_mem) {

[Severity: High]
When has_reserved_mem is true, the device's DMA allocations are fulfilled from
the memory-region[1] pool. However, this function unconditionally allocates 8MB
(COMPR_PLAYBACK_MAX_FRAGMENT_SIZE * COMPR_PLAYBACK_MAX_NUM_FRAGMENTS) without
capping it to pdata->reserved_buf_size like the PCM streams do. Will this
massive allocation immediately exhaust the shared data-path pool, starving all
other streams, or fail outright if the pool slice is smaller than 8MB?

> +		ret = q6apm_dai_scm_assign(pdata, prtd->dma_buffer.addr,
> +					   ALIGN(size, PAGE_SIZE));
> +		if (ret) {
> +			dev_err(dev, "SCM assign compr buffer failed: %d\n",
> +				ret);
> +			snd_dma_free_pages(&prtd->dma_buffer);
> +			return ret;

[Severity: Medium]
If q6apm_dai_scm_assign() fails here, the driver frees the DMA buffer and
returns directly. Doesn't this bypass freeing the prtd structure (allocated via
kzalloc_obj) and closing the DSP graph (opened via q6apm_graph_open)? The ALSA
core does not invoke the .free callback when .open returns an error, so this
appears to cause a permanent memory and graph resource leak.

> +		}
> +	}
> +
[ ... ]
> @@ -1029,12 +1177,122 @@ static int q6apm_dai_probe(struct platform_device *pdev)
[ ... ]
> +		if (mem_count >= 1) {
> +			struct device_node *mem_node;
> +			struct reserved_mem *rmem;
> +
> +			mem_node = of_parse_phandle(node, "memory-region", 0);
> +			rmem = of_reserved_mem_lookup(mem_node);
> +			of_node_put(mem_node);
> +			if (!rmem) {
> +				dev_err(dev,
> +					"memory-region[0]: lookup failed\n");
> +				return -ENODEV;
> +			}
> +
> +			rc = q6apm_dai_scm_assign(pdata, rmem->base,
> +						  ALIGN(rmem->size, PAGE_SIZE));

[Severity: Medium]
SCM assignments modify memory protections at the TrustZone level. Since the
ALIGN macro indiscriminately rounds up the size, what happens if the device
tree provides a carveout size that is not a multiple of PAGE_SIZE? It seems
the driver will request TrustZone to reassign the trailing bytes that belong
to adjacent, unreserved kernel memory, which revokes HLOS access to that
adjacent memory and could cause a kernel crash when accessed by other
subsystems.

> +			if (rc) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922-vmid-v4-v5-0-e79cfd7af5b3@oss.qualcomm.com?part=4