Re: [PATCH 2/2] dmaengine: qcom: bam_dma: SCM-assign descriptor FIFOs to a remote VMID

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Critical] Passing an IOVA to the TrustZone SCM call instead of a strictly required physical address.
- [High] Skipping hardware reset during channel termination causes DMA Use-After-Free and persistent state corruption.
- [High] Skipping interrupt disable during channel free leaves hardware interrupts unmasked for a destroyed channel.

Pre-existing issues:
- [High] Unconditional MMIO register access in driver removal causes a synchronous external abort on remote-powered BAMs.
--

--- Patch [2]: [PATCH 2/2] dmaengine: qcom: bam_dma: SCM-assign descriptor FIFOs to a remote VMID ---
commit 91f2fbb1c7215cade81c38c8732865591d19254f
Author: Vishnu Santhosh <vishnu.santhosh@oss.qualcomm.com>

dmaengine: qcom: bam_dma: SCM-assign descriptor FIFOs to a remote VMID

On Qualcomm SoCs where the BAM is powered by a remote processor that
enforces XPU access control, the BAM reads the descriptor FIFO as an
AXI master under the remote execution environment. This patch handles
such BAMs by keeping the descriptor FIFO as a persistent resource, 
SCM-assigning it once and reclaiming it during remove.

> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index 1bb26af0405f3..389ca1a9d70e8 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
[ ... ]
> @@ -559,6 +567,126 @@ static void bam_chan_init_hw(struct bam_chan *bchan,
[ ... ]
> +static int bam_assign_fifo(struct bam_device *bdev, struct bam_chan *bchan)
> +{
> +	struct qcom_scm_vmperm *dst __free(kfree) = NULL;
> +	u64 src = BIT_ULL(QCOM_SCM_VMID_HLOS);
> +	int i, ret;
[ ... ]
> +	ret = qcom_scm_assign_mem(bchan->fifo_phys, BAM_DESC_FIFO_SIZE,
> +				  &src, dst, bdev->num_vmids + 1);

[Severity: Critical]
Is it safe to pass the IOVA returned by the DMA allocation API to TrustZone
here?

When the device is behind an IOMMU, bchan->fifo_phys contains a dma_addr_t
IOVA. However, TrustZone operates on raw physical memory and expects a physical
address to program hardware XPUs, completely bypassing the local IOMMU
translation.

Could this cause TrustZone to configure XPU memory permissions for the wrong
physical address?

> +	if (ret) {
> +		dev_err(bdev->dev, "SCM assign fifo chan %u failed: %d\n",
> +			bchan->id, ret);
> +		return ret;
> +	}
> +
> +	bchan->fifo_src_perms = src;
> +
> +	return 0;
> +}
> +
> +/**
> + * bam_fifo_can_free - Reclaim a channel's descriptor FIFO to HLOS
[ ... ]
> +static bool bam_fifo_can_free(struct bam_device *bdev, struct bam_chan *bchan)
> +{
> +	struct qcom_scm_vmperm hlos = {
> +		.vmid = QCOM_SCM_VMID_HLOS,
> +		.perm = QCOM_SCM_PERM_RW,
> +	};
> +	int ret;
> +
> +	if (!bchan->fifo_src_perms)
> +		return true;
> +
> +	ret = qcom_scm_assign_mem(bchan->fifo_phys, BAM_DESC_FIFO_SIZE,
> +				  &bchan->fifo_src_perms, &hlos, 1);

[Severity: Critical]
Since bchan->fifo_phys is an IOVA when an IOMMU is present, will passing it
back during the reclaim operation compound the memory permission corruption
noted above?

> +	if (ret) {
> +		dev_err(bdev->dev, "SCM reclaim fifo chan %u failed: %d; leaking\n",
> +			bchan->id, ret);
> +		return false;
> +	}
[ ... ]
> @@ -613,12 +754,29 @@ static void bam_free_chan(struct dma_chan *chan)
>  		goto err;
>  	}
>  
> +	/*
> +	 * Remote-owned BAMs (qcom,vmid) keep the descriptor FIFO allocated and
> +	 * SCM-assigned across power cycles: the remote may already have cut
> +	 * power, so pipe-register access would fault, and TZ still holds the
> +	 * grant for the next restart (the FIFO is reclaimed and freed once in
> +	 * bam_dma_remove). Only drop local channel state here so the block and
> +	 * pipe are re-initialised on the next power-on; skip all MMIO.
> +	 */
> +	if (bdev->num_vmids) {
> +		scoped_guard(spinlock_irqsave, &bchan->vc.lock)
> +			bchan->initialized = 0;
> +		bdev->active_channels--;
> +		goto err;
> +	}

[Severity: High]
Does this early exit skip masking and disabling the channel's interrupts in the
BAM controller?

If the hardware reset was also skipped during termination, the BAM might still
be actively running. Could this assert an unmasked interrupt when a pending
transaction completes after the channel is freed, resulting in an interrupt
storm or use-after-free in the interrupt handler?

> +
>  	scoped_guard(spinlock_irqsave, &bchan->vc.lock)
>  		bam_reset_channel(bchan);
[ ... ]
> @@ -767,7 +925,9 @@ static int bam_dma_terminate_all(struct dma_chan *chan)
>  		if (!list_empty(&bchan->desc_list)) {
>  			async_desc = list_first_entry(&bchan->desc_list,
>  						      struct bam_async_desc, desc_node);
> -			bam_chan_init_hw(bchan, async_desc->dir);
> +			/* Remote-owned BAM: pipe reset may fault, skip it. */
> +			if (!bchan->bdev->num_vmids)
> +				bam_chan_init_hw(bchan, async_desc->dir);

[Severity: High]
By skipping the hardware reset for remote-owned BAMs without providing an
alternative mechanism to halt the active hardware pipe, won't the BAM continue
performing DMA?

When the client subsequently frees its data buffers, the still-running DMA
hardware could overwrite the freed memory. Furthermore, since the internal
software state flag for initialization isn't cleared, might future transfers
start with desynchronized FIFO pointers?

>  		}
[ ... ]
> @@ -1418,9 +1582,10 @@ static void bam_dma_remove(struct platform_device *pdev)
>  		if (!bdev->channels[i].fifo_virt)
>  			continue;
>  
> -		dma_free_wc(bdev->dev, BAM_DESC_FIFO_SIZE,
> -			    bdev->channels[i].fifo_virt,
> -			    bdev->channels[i].fifo_phys);
> +		if (bam_fifo_can_free(bdev, &bdev->channels[i]))
> +			dma_free_wc(bdev->dev, BAM_DESC_FIFO_SIZE,
> +				    bdev->channels[i].fifo_virt,
> +				    bdev->channels[i].fifo_phys);
>  	}

[Severity: High]
This is a pre-existing issue, but I noticed an unconditional MMIO register
access in bam_dma_remove() happens right before this loop:

	/* mask all interrupts for this execution environment */
	writel_relaxed(0, bam_addr(bdev, 0,  BAM_IRQ_SRCS_MSK_EE));

If the BAM driver is unbound while the remote firmware has cut power to the
hardware, wouldn't this unconditional MMIO write in the removal function fault?

The patch intentionally skipped MMIO operations in other teardown paths (like
channel free and terminate) to prevent this exact crash on remote-powered BAMs.
Could we also fix this in the removal path?

>  
>  	tasklet_kill(&bdev->task);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-qcom-bam-dma-vmid-ext-v1-0-cef87c57b7dc@oss.qualcomm.com?part=2