Re: [PATCH v5 07/10] ASoC: qcom: Add QAIF PCM operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Dangling pointers left in persistent substream->dma_buffer on close and error paths, leading to a Double-Free memory corruption.
- [Medium] Large order-5 contiguous DMA buffer allocation in open() callback is highly prone to failure due to memory fragmentation.
--

commit e38dec114f51cf2a17be9cb465b247a4bd24cc25
Author: Harendra Gautam <harendra.gautam@oss.qualcomm.com>

ASoC: qcom: Add QAIF PCM operations

This commit adds ALSA SoC PCM platform driver callbacks for Qualcomm Audio 
Interface (QAIF) devices. It provides open, close, hw_params, prepare, and 
pointer operations, and handles DMA buffer allocation and mmap/copy support 
for playback and capture streams.

> diff --git a/sound/soc/qcom/qaif-platform.c b/sound/soc/qcom/qaif-platform.c
> new file mode 100644
> index 0000000000000..a64bbd7f6fe56
> --- /dev/null
> +++ b/sound/soc/qcom/qaif-platform.c
[ ... ]
> +static int qaif_platform_pcmops_open(struct snd_soc_component *component,
> +				     struct snd_pcm_substream *substream)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct snd_soc_pcm_runtime *soc_runtime = snd_soc_substream_to_rtd(substream);
> +	struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(soc_runtime, 0);
> +	struct qaif_drv_data *drvdata = snd_soc_component_get_drvdata(component);
> +	const struct qaif_variant *v = drvdata->variant;
> +	unsigned int dai_id = cpu_dai->driver->id;
> +	struct snd_dma_buffer *buf = &substream->dma_buffer;
> +	struct qaif_dma_mem_info *dma_mem_info;
> +	struct qaif_pcm_data *data;
> +	int ret, stream_dma_idx, dma_reg_idx, dir = substream->stream;
[ ... ]
> +	dma_mem_info = kzalloc(sizeof(*dma_mem_info), GFP_KERNEL);
> +	if (!dma_mem_info) {
> +		ret = -ENOMEM;
> +		goto err_free_data;
> +	}
> +	dma_mem_info->alloc_size = qaif_platform_hardware.buffer_bytes_max;
> +	dma_mem_info->vaddr = dma_alloc_coherent(component->dev,
> +						 dma_mem_info->alloc_size,
> +						 &dma_mem_info->dma_addr,
> +						 GFP_KERNEL);

[Severity: Medium]
Is it safe to perform a large contiguous allocation dynamically during the 
stream open callback? 

On a system that has been running for some time, memory fragmentation might 
cause this order-5 (120 KB) dma_alloc_coherent() call to fail with -ENOMEM, 
which would prevent the stream from opening. ALSA patterns generally avoid 
this by pre-allocating DMA buffers during driver probe via 
snd_pcm_set_managed_buffer_all().

> +	if (!dma_mem_info->vaddr) {
> +		ret = -ENOMEM;
> +		goto err_free_mem_info;
> +	}
[ ... ]
> +	buf->bytes = qaif_platform_hardware.buffer_bytes_max;
> +	buf->addr = dma_mem_info->dma_addr;
> +	buf->area = (unsigned char *)dma_mem_info->vaddr;
[ ... ]
> +	snd_pcm_set_runtime_buffer(substream, buf);
[ ... ]
> +static int qaif_platform_pcmops_close(struct snd_soc_component *component,
> +				      struct snd_pcm_substream *substream)
> +{
> +	struct snd_soc_pcm_runtime *soc_runtime = snd_soc_substream_to_rtd(substream);
> +	struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(soc_runtime, 0);
> +	struct qaif_drv_data *drvdata = snd_soc_component_get_drvdata(component);
> +	const struct qaif_variant *v = drvdata->variant;
> +	struct qaif_pcm_data *data = substream->runtime->private_data;
> +	struct qaif_dma_mem_info *dma_mem_info;
> +	unsigned int dai_id = cpu_dai->driver->id;
[ ... ]
> +	if (dma_mem_info) {
> +		dma_free_coherent(component->dev, dma_mem_info->alloc_size,
> +				  dma_mem_info->vaddr, dma_mem_info->dma_addr);
> +		kfree(dma_mem_info);
> +	}
> +
> +	snd_pcm_set_runtime_buffer(substream, NULL);

[Severity: High]
Does this leave a dangling pointer in the persistent substream->dma_buffer 
structure that could result in a double-free memory corruption?

The dynamically allocated memory is assigned to the persistent 
substream->dma_buffer (via the local buf pointer) during 
qaif_platform_pcmops_open().

While snd_pcm_set_runtime_buffer(substream, NULL) correctly clears the 
pointers in the runtime structure, it leaves substream->dma_buffer.area 
pointing to the memory that was just freed here by dma_free_coherent(). 

When the ALSA PCM device is later unregistered (e.g. during driver unbind 
or module unload), the ALSA core calls snd_pcm_lib_preallocate_free_for_all(), 
which sees that dma_buffer.area != NULL and attempts to free it again using 
snd_dma_free_pages(). 

Should the persistent buffer pointers be explicitly cleared here to avoid 
the double-free?

> +	substream->runtime->private_data = NULL;
> +
> +	kfree(data);
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-b4-qaif-shikra-audio-v5-0-6d8ad9ba5c9f@oss.qualcomm.com?part=7