Re: [PATCH v4 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:
- [Critical] Multiple deterministic division by zero kernel panics in burst length and offset calculations.
- [High] Transfer lengths smaller than the bus width result in a 0 iteration count, causing a massive hardware memory overflow.
- [High] Incorrect memory address offsets (`soff` and `doff`) are used when source and destination bus widths differ, causing overlapping memory access.
--

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

dmaengine: fsl-edma: Implement device_prep_peripheral_dma_vec

Add implementation of .device_prep_peripheral_dma_vec() callback to setup
a scatter/gather DMA transfer from an array of dma_vec structures. Setup
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
> --- a/drivers/dma/fsl-edma-common.c
> +++ b/drivers/dma/fsl-edma-common.c
> @@ -673,6 +673,115 @@ 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)
> +{
[ ... ]
> +	if (direction == DMA_MEM_TO_DEV) {
> +		if (!fsl_chan->cfg.src_addr_width)
> +			fsl_chan->cfg.src_addr_width = fsl_chan->cfg.dst_addr_width;
> +		fsl_chan->attr =
> +			fsl_edma_get_tcd_attr(fsl_chan->cfg.src_addr_width,
> +					      fsl_chan->cfg.dst_addr_width);
> +		nbytes = fsl_chan->cfg.dst_addr_width * fsl_chan->cfg.dst_maxburst;

This regression appears to be duplicated from the existing fsl_edma_prep_slave_sg
logic.

If a client leaves dst_addr_width or src_addr_width as 0 (undefined), does
this evaluate to nbytes = 0?

The driver relies on fsl_edma_get_tcd_attr() to handle undefined values:

static unsigned int fsl_edma_get_tcd_attr(...)
{
    if (src_addr_width == DMA_SLAVE_BUSWIDTH_UNDEFINED)
        src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
    ...
}

However, that function takes arguments by value and does not update the
fsl_chan->cfg structures. If nbytes evaluates to 0 here, it will lead to a
divide-by-zero panic shortly after in the modulo operation:

    if (vecs[i].len % nbytes)

> +	} else {
[ ... ]
> +	}
> +
> +	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) {

Is it intentional that the source address offset (soff) is set to
dst_addr_width here?

For a DMA_MEM_TO_DEV transfer, memory is the source. The hardware will perform
reads sized according to src_addr_width (configured in the SSIZE attribute).
If a client configures the memory side with a 4-byte width but the peripheral
with a 1-byte width, the DMA engine will read 4 bytes per minor loop, but
increment the source address by only 1 byte.

Could this regression result in overlapping memory reads and corrupted data
being sent to the peripheral? (And conversely for DMA_DEV_TO_MEM where doff
is tied to src_addr_width).

> +			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 (vecs[i].len % nbytes) {
> +			u32 width = (direction == DMA_DEV_TO_MEM) ? doff : soff;

If the direction is DMA_DEV_TO_DEV, both soff and doff are explicitly set to
0 above. This means width will evaluate to 0.

If vecs[i].len is not a multiple of nbytes, the fallback loop will then
evaluate:

    if (!(vecs[i].len % (j * width)))

Will this trigger a divide-by-zero panic?

Furthermore, if the loop finishes with j == 1, nbytes is set to width (0),
which leads to another divide-by-zero panic at iter = vecs[i].len / nbytes.

> +			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;
> +					break;
> +				}
> +			}
> +			/* Set burst size as 1 if there's no suitable one */
> +			if (j == 1)
> +				nbytes = width;
> +		}
> +
> +		iter = vecs[i].len / nbytes;

If a transfer length (vecs[i].len) is smaller than the bus width, the fallback
block above sets nbytes = width. The integer division iter = vecs[i].len /
nbytes will then truncate to 0.

Does writing 0 to the iteration count registers on Freescale eDMA hardware
cause the counter to decrement before checking for zero?

If so, programming the hardware's CITER and BITER registers with 0 could be
interpreted as the maximum possible iterations (e.g. 32768 or 65536). Instead
of safely rejecting the invalid sub-word transfer, could this regression
initiate a massive DMA transfer that silently accesses thousands of bytes past
the client's buffer boundaries?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-fsl-edma-dyn-sg-v4-0-8ce7d95b1ce9@bootlin.com?part=1