Re: [PATCH v11 1/2] dmaengine: fsl-edma: Implement device_prep_peripheral_dma_vec

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] dmaengine: fsl-edma: division by zero in unaligned DMA_DEV_TO_DEV transfers
- [High] dmaengine: fsl-edma: loop mutates shared nbytes causing spurious transfer failures
- [High] dmaengine: fsl-edma: missing nb == 0 guard causes out-of-bounds heap read
--

commit 1a23f772b7926fbde5dc0fc2476442d47898d799
Author: Benoît Monin <benoit.monin@bootlin.com>

dmaengine: fsl-edma: Implement device_prep_peripheral_dma_vec

This commit adds the implementation of the .device_prep_peripheral_dma_vec()
callback to setup a scatter/gather DMA transfer from an array of dma_vec
structures. It also sets up a cyclic transfer if the DMA_PREP_REPEAT flag
is set.

> diff --git a/drivers/dma/fsl-edma-common.c b/drivers/dma/fsl-edma-common.c
> index bb7531c456dfa..8ba72b3d0e1af 100644
> --- a/drivers/dma/fsl-edma-common.c
> +++ b/drivers/dma/fsl-edma-common.c
[ ... ]
> @@ -673,6 +674,122 @@ struct dma_async_tx_descriptor *fsl_edma_prep_dma_cyclic(
[ ... ]
> +struct dma_async_tx_descriptor *
> +fsl_edma_prep_peripheral_dma_vec(struct dma_chan *chan, const struct dma_vec *vecs,
> +				 size_t nb, enum dma_transfer_direction direction,
> +				 unsigned long flags)
> +{
> +	struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
> +	dma_addr_t src_addr, dst_addr, last_sg;
> +	struct fsl_edma_desc *fsl_desc;
> +	u16 soff, doff, iter;
> +	u32 nbytes;
> +	int i;
> +
> +	if (!is_slave_direction(direction))
> +		return NULL;
> +
> +	if (!fsl_edma_prep_slave_dma(fsl_chan, direction))
> +		return NULL;
> +
> +	fsl_desc = fsl_edma_alloc_desc(fsl_chan, nb);
> +	if (!fsl_desc)
> +		return NULL;

[Severity: High]
Does this code allow an out-of-bounds heap read if `nb` is 0?

If `nb == 0` is passed, fsl_edma_alloc_desc() will allocate a descriptor with a
0-length flexible array of TCDs. The setup loop `for (i = 0; i < nb; i++)` is
skipped entirely, and the empty descriptor is directly submitted to the virtual
channel.

When the DMA engine tasklet processes this descriptor via
fsl_edma_xfer_desc(), it unconditionally executes fsl_chan->edesc->tcd[0].vtcd,
dereferencing an out-of-bounds heap address. Should there be a check for
`!nb` before allocating?

> +	fsl_desc->iscyclic = flags & DMA_PREP_REPEAT;
> +	fsl_desc->dirn = direction;
[ ... ]
> +	for (i = 0; i < nb; i++) {
> +		if (direction == DMA_MEM_TO_DEV) {
> +			src_addr = vecs[i].addr;
> +			dst_addr = fsl_chan->dma_dev_addr;
> +			soff = fsl_chan->cfg.dst_addr_width;
> +			doff = 0;
> +		} else if (direction == DMA_DEV_TO_MEM) {
> +			src_addr = fsl_chan->dma_dev_addr;
> +			dst_addr = vecs[i].addr;
> +			soff = 0;
> +			doff = fsl_chan->cfg.src_addr_width;
> +		} else {
> +			/* DMA_DEV_TO_DEV */
> +			src_addr = fsl_chan->cfg.src_addr;
> +			dst_addr = fsl_chan->cfg.dst_addr;
> +			soff = 0;
> +			doff = 0;
> +		}
> +
> +		/*
> +		 * Choose the suitable burst length if dma_vec length is not
> +		 * multiple of burst length so that the whole transfer length is
> +		 * multiple of minor loop(burst length).
> +		 */
> +		if (nbytes && vecs[i].len % nbytes) {
> +			u32 width = (direction == DMA_DEV_TO_MEM) ? doff : soff;

[Severity: High]
Can this result in a modulo by zero kernel panic?

During DMA_DEV_TO_DEV configuration, both `soff` and `doff` are explicitly
initialized to 0. The calculation here results in `width = 0`. Then the
loop below evaluates `vecs[i].len % (j * width)`, which performs modulo by
zero.

> +			u32 burst = (direction == DMA_DEV_TO_MEM) ?
> +						fsl_chan->cfg.src_maxburst :
> +						fsl_chan->cfg.dst_maxburst;
> +			int j;
> +
> +			for (j = burst; j > 1; j--) {
> +				if (!(vecs[i].len % (j * width))) {
> +					nbytes = j * width;

[Severity: High]
Does this loop mutate the shared `nbytes` variable causing spurious transfer
failures?

The `nbytes` variable is used to define the optimal burst length for the entire
transfer. Inside the loop, if an early vector is misaligned, the code
permanently shrinks `nbytes` (e.g., to 1 or a smaller factor) to fit that
specific vector without restoring it for the next loop iteration.

This degraded `nbytes` value leaks to all subsequent loop iterations. Large,
perfectly aligned vectors later in the array will incorrectly use the shrunken
`nbytes`.

> +					break;
> +				}
> +			}
> +			/* Set burst size as 1 if there's no suitable one */
> +			if (j == 1)
> +				nbytes = width;
> +		}
> +
> +		if (!nbytes || vecs[i].len / nbytes > FIELD_MAX(EDMA_TCD_ITER_MASK))

[Severity: High]
Does mutating `nbytes` above cause valid transfers to be rejected here?

As a result of permanently shrinking `nbytes` in a previous iteration,
`vecs[i].len / nbytes` can explode in magnitude and spuriously exceed
`FIELD_MAX(EDMA_TCD_ITER_MASK)` for perfectly valid DMA vectors.

> +			goto err_free_desc;
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916-fsl-edma-dyn-sg-v11-0-02792de71b92@bootlin.com?part=1