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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Kernel panic: Modulo by zero during unaligned DMA_DEV_TO_DEV burst calculation.
- [Critical] Arbitrary DMA Execution: 0-length vector triggers out-of-bounds read at tcd[0].
- [High] Data corruption: Missing multi-FIFO offset causes negative address drift.
- [Medium] Logic Error: Unreset burst size artificially breaches FIELD_MAX limit.

Pre-existing issues:
- [High] Kernel panic: DMA_NONE direction mapping triggers BUG_ON.
--

commit 084b4b0f402eb171d6adefd8fa18c7fa25e129fb
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 supports cyclic transfers 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..c5f5951c988be 100644
> --- a/drivers/dma/fsl-edma-common.c
> +++ b/drivers/dma/fsl-edma-common.c
> @@ -673,6 +673,122 @@ struct dma_async_tx_descriptor *fsl_edma_prep_dma_cyclic(
>  	return vchan_tx_prep(&fsl_chan->vchan, &fsl_desc->vdesc, flags);
>  }
>  
> +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;

[Severity: High]
This is a pre-existing issue, but does passing DMA_DEV_TO_DEV to
fsl_edma_prep_slave_dma() cause a kernel panic?

If a driver requests a DMA_DEV_TO_DEV transfer, fsl_edma_prep_slave_dma()
handles it by mapping the resource with dma_dir = DMA_NONE. The core DMA
mapping API (dma_map_phys via dma_map_resource) asserts
BUG_ON(!valid_dma_direction(dir)), which leads to an intentional kernel
crash.

> +
> +	fsl_desc = fsl_edma_alloc_desc(fsl_chan, nb);
> +	if (!fsl_desc)
> +		return NULL;

[Severity: Critical]
Is it possible for a caller to pass nb == 0 here?

If nb is 0, fsl_edma_alloc_desc() allocates an empty descriptor with
n_tcds = 0. When the transfer is subsequently submitted via
fsl_edma_xfer_desc(), the engine unconditionally dereferences
fsl_chan->edesc->tcd[0].vtcd. Since tcd[0] is out-of-bounds, this will
result in a heap out-of-bounds read, potentially leading to arbitrary
DMA programming or a crash.

[ ... ]

> +	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;

[Severity: High]
Can this hardcoded offset cause memory corruption for multi-FIFO endpoints?

When fsl_chan->is_multi_fifo is true, fsl_edma_fill_tcd() automatically
applies a negative Minor Loop Offset (MLOFF) equal to -burst. Because doff
is set to 0 here, the destination address does not advance during the minor
loop, but is still incorrectly pulled backwards by -burst at the end of
each minor loop. This causes the destination address to drift negatively.

[ ... ]

> +		} 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;
> +			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))) {

[Severity: Critical]
Could this result in a modulo by zero for DMA_DEV_TO_DEV transfers?

For DMA_DEV_TO_DEV, both soff and doff are 0, which means width evaluates
to 0. If the vector length is unaligned, the modulo operator will evaluate
vecs[i].len % 0, causing an immediate kernel panic via division by zero.

> +					nbytes = j * width;
> +					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))
> +			goto err_free_desc;

[Severity: Medium]
Will this erroneously reject valid transfers if nbytes is shrunk during an
earlier loop iteration?

When a vector length isn't a multiple of nbytes, nbytes is reduced to a
suitable factor. However, nbytes is never restored to its optimal value at
the start of the loop for the next vector. If a later vector in the array
is large, it will be divided by this artificially reduced nbytes, causing
the calculation to exceed FIELD_MAX(EDMA_TCD_ITER_MASK) and falsely aborting
the transfer setup.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911-fsl-edma-dyn-sg-v10-0-700eb72def57@bootlin.com?part=1