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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Submitting a transfer with zero elements (nb == 0) causes an out-of-bounds memory read and kernel oops.
- [High] Modulo by zero kernel panic for DMA_DEV_TO_DEV transfers with unaligned lengths.
- [High] Missing multi-FIFO support causes backward memory corruption for peripheral transfers.
- [High] The nbytes burst size is never restored per loop iteration, causing subsequent vectors to permanently inherit a heavily degraded burst size.
--

commit 995264f0108e475b7f259a8a9e431ac6988e4a11
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
> index bb7531c456dfa..c10190164926a 100644
> --- a/drivers/dma/fsl-edma-common.c
> +++ b/drivers/dma/fsl-edma-common.c

[ ... ]

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

This problem wasn't introduced by this patch, but does this function need to
validate that nb > 0?

If nb == 0 is provided, fsl_edma_alloc_desc() allocates a descriptor with
zero tcd structures in its flexible array. When the DMA engine later attempts
to execute it in fsl_edma_xfer_desc(), it unconditionally dereferences
fsl_chan->edesc->tcd[0].vtcd. This reads a garbage pointer from adjacent heap
memory and leads to an immediate kernel oops when trying to write to the DMA
hardware registers.

Additionally, the subsequent patch Support dynamic scatter/gather chaining
introduces prev_desc->n_tcds - 1, which underflows to UINT_MAX when nb == 0,
guaranteeing another out-of-bounds crash on tx_submit().

> +	if (!fsl_desc)
> +		return NULL;
> +	fsl_desc->iscyclic = flags & DMA_PREP_REPEAT;
> +	fsl_desc->dirn = direction;
> +
> +	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;
> +	} else {
> +		if (!fsl_chan->cfg.dst_addr_width)
> +			fsl_chan->cfg.dst_addr_width = fsl_chan->cfg.src_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.src_addr_width * fsl_chan->cfg.src_maxburst;
> +	}
> +
> +	for (i = 0; i < nb; i++) {

This isn't a regression from this patch, but does nbytes need to be restored
to its default optimal value at the start of each loop iteration?

If vecs[i].len is not a multiple of nbytes, nbytes is permanently reduced to
a smaller valid multiple (or down to width). However, because nbytes is not
reset to its default optimal value at the start of each loop iteration, all
remaining vectors in the array will be forced to use the heavily degraded
burst size, even if they are perfectly aligned.

This exponentially increases the number of hardware DMA requests (minor loops)
for the remainder of the transfer, which can cause peripheral FIFO overruns
on fast data streams.

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

This problem wasn't introduced by this patch, but should soff and doff handle
multi-FIFO support here?

Unlike fsl_edma_prep_dma_cyclic(), which correctly assigns doff and soff to
support multi-FIFO (e.g., doff = fsl_chan->is_multi_fifo ? 4 : 0), this
function unconditionally hardcodes doff = 0 and soff = 0 for device addresses.

However, fsl_edma_fill_tcd() still applies a negative Minor Loop Offset
(MLOFF) at the end of the minor loop if is_multi_fifo or port_window_size are
enabled on the channel. Because the address never incrementally advanced
during the minor loop, applying MLOFF forces the device address to step
backwards continuously, severely corrupting system memory or registers located
before the target device address.

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

This problem wasn't introduced by this patch, but can this cause a modulo by
zero kernel panic for DMA_DEV_TO_DEV transfers?

For DMA_DEV_TO_DEV transfers, both soff and doff are explicitly set to 0. If
vecs[i].len is not divisible by the initial burst size nbytes, the driver
assigns width = soff, which evaluates to 0.

The loop then evaluates if (!(vecs[i].len % (j * width))), causing an
immediate modulo by zero panic.

Furthermore, if the loop finishes with j == 1, nbytes is set to width (0),
and iter = vecs[i].len / nbytes triggers another division by zero.

> +					nbytes = j * width;
> +					break;
> +				}
> +			}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-fsl-edma-dyn-sg-v3-0-98a181775dae@bootlin.com?part=1