Re: [PATCH 11/13] ASoC: qcom: Add QAIF PCM operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] Incorrect DMA buffer type `SNDRV_DMA_TYPE_CONTINUOUS` for `dma_alloc_coherent` memory leads to arbitrary physical memory mappings.
- [High] Missing `free_stream_dma_idx` cleanup in the `qaif_init` failure path.
- [High] The ALSA pointer callback returns raw negative error codes directly, corrupting the ALSA core pointer math.
- [High] Mismatched device structs between coherent DMA memory allocation and mapping for CIF streams.
- [High] Missing constraint to enforce period and buffer sizes as multiples of 8 bytes.
- [High] Concurrent `prepare` calls unconditionally overwrite global DMA clock rates.
- [Medium] Memory leak and uninitialized buffer values used for unrecognized DAIs.
- [Medium] Data race / TOCTOU on `qaif_init_ref_cnt` leading to multiple simultaneous hardware initializations.
- [Low] Bitwise OR operations are performed on negative error codes, scrambling the returned error values.
--

commit 282c91bcb4ac4fdd53a4566b6b00d7be3a79bcbc
Author: Harendra Gautam <harendra.gautam@oss.qualcomm.com>
Subject: ASoC: qcom: Add QAIF PCM operations

This commit adds PCM operation callbacks for the QAIF platform driver
to manage the DMA stream lifecycle for AIF and CIF DAIs.

> diff --git a/sound/soc/qcom/qaif-platform.c b/sound/soc/qcom/qaif-platform.c
> index b298ee9d865ee..beebec55222a5 100644
> --- a/sound/soc/qcom/qaif-platform.c
> +++ b/sound/soc/qcom/qaif-platform.c
> @@ -280,3 +280,570 @@ static int qaif_init(struct snd_soc_component *component)
[ ... ]
> +static int qaif_platform_pcmops_open(struct snd_soc_component *component,
> +				      struct snd_pcm_substream *substream)
> +{
[ ... ]
> +	buf = &substream->dma_buffer;
> +	buf->dev.dev = component->dev;
> +	buf->private_data = NULL;
> +	buf->dev.type = SNDRV_DMA_TYPE_CONTINUOUS;

[Severity: Critical]
Does this code assign the wrong DMA buffer type?

Because the memory is allocated with dma_alloc_coherent() (via
qaif_mem_alloc_attach()), tagging it as SNDRV_DMA_TYPE_CONTINUOUS causes
ALSA's default mmap handler to assume the virtual address is linearly
mapped. The handler then attempts to use virt_to_page() to resolve
physical addresses, which can return incorrect page frames for vmalloc
or IOVA-backed addresses on ARM64. This could potentially allow
unprivileged userspace mapping of arbitrary physical memory.

[ ... ]
> +	ret = qaif_init(component);
> +	if (ret) {
> +		dev_err(soc_runtime->dev, "qaif_init failed: %d\n", ret);
> +		clk_disable_unprepare(drvdata->aud_dma_mem_clk);
> +		clk_disable_unprepare(drvdata->aud_dma_clk);
> +		qaif_mem_dealloc_detach(component->dev, dma_mem_info);
> +		kfree(data);
> +		return -EINVAL;
> +	}

[Severity: High]
Does this error path leak the DMA stream index?

The stream_dma_idx was dynamically allocated earlier, but it is not
released via v->free_stream_dma_idx() here. A few initialization
failures might exhaust the stream index pool and permanently break the
DAI until module reload.

> +	drvdata->qaif_init_ref_cnt++;

[Severity: Medium]
Could this refcount increment lead to a race condition?

The qaif_init() function checks if qaif_init_ref_cnt is zero before
executing the hardware setup. However, the increment happens here, after
qaif_init() has returned successfully. Since PCM devices have individual
locks, open() might be called simultaneously across different DAIs,
allowing multiple threads to see a zero refcount and simultaneously
execute hardware setup.

[ ... ]
> +	switch (dai_id) {
> +	case QAIF_MI2S_TDM_AIF0 ... QAIF_MI2S_TDM_AIF12:
[ ... ]
> +	case QAIF_CDC_DMA_RX0 ... QAIF_CDC_DMA_RX9:
[ ... ]
> +	default:
> +		break;
> +	}
> +
> +	snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);

[Severity: Medium]
Does this code proceed with uninitialized buffer values for unrecognized
DAIs?

The switch ignores unrecognized DAIs via the default branch. The function
then proceeds to configure the ALSA runtime buffer with an uninitialized
buf->bytes and buf->addr. The memory allocated for dma_mem_info earlier
in the function also appears to be leaked.

[ ... ]
> +static int qaif_platform_pcmops_prepare(struct snd_soc_component *component,
> +					 struct snd_pcm_substream *substream)
> +{
[ ... ]
> +	clk_set_rate(drvdata->aud_dma_clk, rate * bitwidth * channels * QAIF_DMA_CLK_RATE_MULTIPLIER);
> +	clk_set_rate(drvdata->aud_dma_mem_clk, rate * bitwidth * channels * QAIF_DMA_CLK_RATE_MULTIPLIER);

[Severity: High]
Can concurrent prepare calls break active streams by clobbering these
global clocks?

These global clocks provide the base frequency for the entire QAIF
hardware block. Because they are unconditionally overwritten here based
on the specific rate, channels, and bitwidth of the preparing stream,
concurrent streams with different formats might starve or break streams
that are already running.

[ ... ]
> +	ret = regmap_write(map, QAIF_DMABUFF_REG(v, idx, dir, dai_id),
> +			(snd_pcm_lib_buffer_bytes(substream) >> QAIF_DMA_BYTES_TO_WORDS_SHIFT) - 1);

[Severity: High]
Can this shift operation truncate buffer sizes and cause data corruption?

Because there is no ALSA constraint (like snd_pcm_hw_constraint_step)
enforcing an 8-byte alignment, userspace might request a size that is
not divisible by 8. QAIF_DMA_BYTES_TO_WORDS_SHIFT is 3, so the right
shift will silently truncate the remaining bytes. This makes the hardware
DMA buffer smaller than ALSA's software tracking expects, leading to
wrap-around mismatches.

[ ... ]
> +static int qaif_platform_irq_clear(struct qaif_drv_data *drvdata,
> +				int dir, enum qaif_irq_type irq_type, int idx)
> +{
> +	int ret = 0;
> +	const struct qaif_variant *v = drvdata->variant;
> +	struct regmap *map = drvdata->audio_qaif_map;
> +	unsigned int val_irqclr = BIT(idx);
> +
> +	if (dir == SNDRV_PCM_STREAM_PLAYBACK) {
> +		ret |= regmap_write(map, QAIF_EE_RDDMA_PERIOD_IRQ_CLR_REG(v, irq_type), val_irqclr);
> +		ret |= regmap_write(map, QAIF_EE_RDDMA_UNDERFLOW_IRQ_CLR_REG(v, irq_type), val_irqclr);
> +		ret |= regmap_write(map, QAIF_EE_RDDMA_ERR_RSP_IRQ_CLR_REG(v, irq_type), val_irqclr);

[Severity: Low]
Does this code inadvertently scramble error values?

The regmap_write() functions can return negative error codes. Two's
complement integers combined with bitwise OR operations will produce
incorrect error semantics or completely invalid codes, corrupting the
return value given to the caller.

[ ... ]
> +static snd_pcm_uframes_t qaif_platform_pcmops_pointer(
> +		struct snd_soc_component *component,
> +		struct snd_pcm_substream *substream)
> +{
[ ... ]
> +	idx = v->get_dma_idx(dai_id);
> +
> +	if (idx < 0) {
> +		dev_err(soc_runtime->dev, "%s: Invalid DMA index: %d\n", __func__, idx);
> +		return -EINVAL;
> +	}

[Severity: High]
Does this pointer callback return raw negative error codes?

The ALSA core pointer callback is defined to return snd_pcm_uframes_t
(unsigned long). A negative error code like -EINVAL will cast to an
enormous frame position, completely breaking ALSA's period math and
stream state. Consider returning SNDRV_PCM_POS_XRUN or 0 on error.

[ ... ]
> +static int qaif_platform_cdc_dma_mmap(struct snd_pcm_substream *substream,
> +				       struct vm_area_struct *vma)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +
> +	return dma_mmap_coherent(substream->pcm->card->dev, vma,
> +				 runtime->dma_area, runtime->dma_addr,
> +				 runtime->dma_bytes);
> +}

[Severity: High]
Is the wrong device pointer being used for this DMA mapping?

In qaif_mem_alloc_attach(), the memory is allocated using component->dev:

	dma_mem_info->vaddr = dma_alloc_coherent(dev, alloc_size,
						 &dma_mem_info->dma_addr,
						 GFP_KERNEL);

However, this mapping operation uses substream->pcm->card->dev. DMA API
constraints require mapping operations to use the exact same device
pointer used for memory allocation. If these devices belong to different
IOMMU domains, this could cause IOMMU faults or failed mmaps.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605103739.3557573-1-harendra.gautam@oss.qualcomm.com?part=11