Re: [PATCH v21 06/14] dmaengine: qcom: bam_dma: add support for BAM locking

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] `dma_map_sg()` and `dma_unmap_sg()` incorrectly use the client device (`chan->slave`) instead of the BAM DMA controller device.
- [High] Descriptor-specific metadata is incorrectly stored in the global channel state, causing a state leak and potential memory corruption.
- [High] Missing endianness conversion (`cpu_to_le32`) when assigning the physical address to the BAM hardware descriptor.
- [High] Silent fallback to unlocked operation if memory allocation fails for pipe lock descriptors.
- [Medium] Mapping an embedded structure member (`lock_ce`) for DMA violates DMA API cacheline alignment rules.
- [Medium] `bam_metadata_attach` casts and reads the user-provided `data` buffer without validating `len`.
- [Medium] Silent truncation of a physical address to a 24-bit offset.
--

--- Patch [6]: [PATCH v21 06/14] dmaengine: qcom: bam_dma: add support for BAM locking ---
commit 6c206bd6717df7ed7061a889f4998d1b7849b74d
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

dmaengine: qcom: bam_dma: add support for BAM locking

This commit adds support for BAM pipe locking by prepending and appending
dummy command descriptors with LOCK/UNLOCK bits. It uses a client-provided
scratchpad address passed via descriptor metadata.

> diff --git a/drivers/dma/qcom/bam_dma.c b/drivers/dma/qcom/bam_dma.c
> index f3e713a5259c2..f08549ee3872e 100644
> --- a/drivers/dma/qcom/bam_dma.c
> +++ b/drivers/dma/qcom/bam_dma.c
> @@ -72,6 +76,11 @@ struct bam_async_desc {
>  
>  	struct bam_desc_hw *curr_desc;
>  
> +	/* BAM locking infrastructure */
> +	bool is_lock_desc;
> +	struct scatterlist lock_sg;
> +	struct bam_cmd_element lock_ce;

[Severity: Medium]
Is it safe to map lock_ce for DMA here without cacheline alignment?

Since lock_ce is embedded inside the dynamically allocated
struct bam_async_desc, it shares a cacheline with heavily accessed CPU members
like desc_node.

Would this violate DMA API cacheline alignment rules and risk memory
corruption during cache maintenance operations on non-coherent architectures?

[ ... ]

> @@ -686,6 +702,35 @@ static int bam_slave_config(struct dma_chan *chan,
>  	return 0;
>  }
>  
> +static int bam_metadata_attach(struct dma_async_tx_descriptor *desc, void *data, size_t len)
> +{
> +	struct bam_chan *bchan = to_bam_chan(desc->chan);
> +	const struct bam_device_data *bdata = bchan->bdev->dev_data;
> +	struct bam_desc_metadata *metadata = data;
> +
> +	if (!data)
> +		return -EINVAL;

[Severity: Medium]
Does this function need to validate the length of the data buffer?

If a caller accidentally provides a short metadata buffer, could accessing
metadata->scratchpad_addr result in an out-of-bounds memory read?

> +
> +	if (!bdata->pipe_lock_supported)
> +		/*
> +		 * The client wants to use locking but this BAM version doesn't
> +		 * support it. Don't return an error here as this will stop the
> +		 * client from using DMA at all for no reason.
> +		 */
> +		return 0;
> +
> +	guard(spinlock_irqsave)(&bchan->vc.lock);
> +
> +	bchan->scratchpad_addr = metadata->scratchpad_addr;
> +	bchan->direction = metadata->direction;

[Severity: High]
Is it intentional to write transaction-specific metadata directly into the
global channel state here?

If a client queues multiple descriptors with different metadata, won't the
latest call overwrite the state for all pending descriptors?

Also, since bchan->scratchpad_addr is not cleared in bam_free_chan(), if a
new client acquires the channel, could bam_setup_pipe_lock() read a stale
scratchpad_addr and silently inject dummy locks writing to the previous
client's memory?

> +
> +	return 0;
> +}

[ ... ]

> @@ -859,6 +909,15 @@ static int bam_resume(struct dma_chan *chan)
>  	return 0;
>  }
>  
> +static void bam_dma_free_lock_desc(struct virt_dma_desc *vd)
> +{
> +	struct bam_async_desc *async_desc = container_of(vd, struct bam_async_desc, vd);
> +	struct dma_chan *chan = vd->tx.chan;
> +
> +	dma_unmap_sg(chan->slave, &async_desc->lock_sg, 1, DMA_TO_DEVICE);

[Severity: High]
Should this unmap use the BAM DMA controller device instead of chan->slave?

Because the BAM DMA controller is the bus master reading the command element,
using the client device's domain might result in an invalid IOVA or IOMMU
translation faults.

> +	kfree(async_desc);
> +}

[ ... ]

> @@ -1046,13 +1113,102 @@ static void bam_apply_new_config(struct bam_chan *bchan,
>  	bchan->reconfigure = 0;
>  }
>  
> +static struct bam_async_desc *
> +bam_make_lock_desc(struct bam_chan *bchan, unsigned long flag)
> +{
> +	struct dma_chan *chan = &bchan->vc.chan;
> +	struct bam_async_desc *async_desc;
> +	struct bam_desc_hw *desc;
> +	struct virt_dma_desc *vd;
> +	struct virt_dma_chan *vc;
> +	unsigned int mapped;
> +
> +	async_desc = kzalloc_flex(*async_desc, desc, 1, GFP_NOWAIT);
> +	if (!async_desc) {
> +		dev_err(bchan->bdev->dev, "failed to allocate the BAM lock descriptor\n");
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	sg_init_table(&async_desc->lock_sg, 1);
> +
> +	async_desc->num_desc = 1;
> +	async_desc->curr_desc = async_desc->desc;
> +	async_desc->dir = DMA_MEM_TO_DEV;
> +	async_desc->is_lock_desc = true;
> +
> +	desc = async_desc->desc;
> +
> +	bam_prep_ce_le32(&async_desc->lock_ce, bchan->scratchpad_addr, BAM_WRITE_COMMAND, 0);

[Severity: Medium]
Does bam_prep_ce_le32() safely handle a full phys_addr_t?

The struct defines scratchpad_addr as a phys_addr_t, but bam_prep_ce_le32()
appears to mask the address with 0xffffff, discarding bits above the lowest
24 bits. If a client provides a 32-bit or 64-bit physical address, will the
dummy write target the wrong address?

> +	sg_set_buf(&async_desc->lock_sg, &async_desc->lock_ce, sizeof(async_desc->lock_ce));
> +
> +	mapped = dma_map_sg(chan->slave, &async_desc->lock_sg, 1, DMA_TO_DEVICE);

[Severity: High]
Similar to the unmap path, should this mapping use the BAM DMA controller
device rather than the client device (chan->slave)?

> +	if (!mapped) {
> +		kfree(async_desc);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	desc->flags |= cpu_to_le16(DESC_FLAG_CMD | flag);
> +	desc->addr = sg_dma_address(&async_desc->lock_sg);

[Severity: High]
Is a cpu_to_le32() conversion missing here when assigning to desc->addr?

Since sg_dma_address() returns a dma_addr_t in host CPU byte order, writing
this directly to the __le32 addr field could result in a byte-swapped address
on Big-Endian systems, causing the BAM DMA engine to read from an invalid
IOVA.

> +	desc->size = cpu_to_le16(sizeof(struct bam_cmd_element));

[ ... ]

> @@ -1072,6 +1229,18 @@ static void bam_start_dma(struct bam_chan *bchan)
>  		return;
>  
>  	while (vd && !IS_BUSY(bchan)) {
> +		/*
> +		 * Open a LOCK/UNLOCK bracket around each fresh sequence.
> +		 * Sentinels inserted by bam_setup_pipe_lock() are skipped: they
> +		 * already have bam_locked set and must not trigger a second pair.
> +		 */
> +		if (!bchan->bam_locked &&
> +		    !container_of(vd, struct bam_async_desc, vd)->is_lock_desc) {
> +			ret = bam_setup_pipe_lock(bchan);
> +			if (ret == 0 && bchan->bam_locked)
> +				vd = vchan_next_desc(&bchan->vc);
> +		}

[Severity: High]
If bam_setup_pipe_lock() fails and returns an error code, is it safe to
proceed to queue the DMA transaction unlocked?

Since the commit message states the lock is strictly required to prevent
config writes while the engine is busy, could silently ignoring the failure
allow unpredictable hardware behavior?

> +
>  		list_del(&vd->node);

[ ... ]

> @@ -1191,8 +1364,11 @@ static void bam_issue_pending(struct dma_chan *chan)
>   */
>  static void bam_dma_free_desc(struct virt_dma_desc *vd)
>  {
> -	struct bam_async_desc *async_desc = container_of(vd,
> -			struct bam_async_desc, vd);
> +	struct bam_async_desc *async_desc = container_of(vd, struct bam_async_desc, vd);
> +	struct dma_chan *chan = vd->tx.chan;
> +
> +	if (async_desc->is_lock_desc)
> +		dma_unmap_sg(chan->slave, &async_desc->lock_sg, 1, DMA_TO_DEVICE);

[Severity: High]
As noted earlier, should this unmap use the BAM DMA controller device instead
of chan->slave?

>  
>  	kfree(async_desc);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-qcom-qce-cmd-descr-v21-0-bc2583e18475@oss.qualcomm.com?part=6