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

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

New issues:
- [High] Divide-by-zero kernel panic on DMA_DEV_TO_DEV transfers with unaligned vector lengths.
- [High] Silent truncation and massive buffer overflow when vector length is smaller than peripheral width.
- [High] Missing multi-fifo and port window offset handling corrupts DMA addresses.
- [High] Out-of-bounds access and crash when called with an empty vector array (nb == 0).
- [Medium] State leak of the `nbytes` variable permanently degrades burst performance for subsequent vectors.

Pre-existing issues:
- [High] Remapping a channel for DMA_DEV_TO_DEV triggers a BUG_ON crash in dma_map_resource.
--

--- Patch [1]: [PATCH v7 1/2] dmaengine: fsl-edma: Implement device_prep_peripheral_dma_vec ---
commit 25e53feb330d50919440f251efabfde609bc03f3
Author: Benoît Monin <[email protected]>

dmaengine: fsl-edma: Implement device_prep_peripheral_dma_vec

This commit implements the .device_prep_peripheral_dma_vec() callback for
the fsl-edma driver. It configures a scatter/gather DMA transfer using an
array of dma_vec structures and supports cyclic transfers.

> 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

[ ... ]

> +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 calling fsl_edma_prep_slave_dma() with
DMA_DEV_TO_DEV trigger a crash in the DMA mapping core?

When fsl_edma_prep_slave_dma() processes DMA_DEV_TO_DEV, it sets the local
variable dma_dir to DMA_NONE. If the channel is being repurposed and needs
remapping, it unmaps and then calls:

    fsl_chan->dma_dev_addr = dma_map_resource(dev, addr, size, DMA_NONE, 0);

The DMA mapping core strictly forbids DMA_NONE and will trigger a BUG_ON if
the direction is not valid.

> +
> +	fsl_desc = fsl_edma_alloc_desc(fsl_chan, nb);

[Severity: High]
Can this code crash if the caller passes an empty vector array where nb == 0?

If nb is 0, fsl_edma_alloc_desc() will allocate a descriptor with a 0-element
flexible array. When the transfer is later initiated, fsl_edma_xfer_desc() 
appears to unconditionally access the first element:

    fsl_edma_set_tcd_regs(fsl_chan, fsl_chan->edesc->tcd[0].vtcd);

Could this result in an out-of-bounds read and subsequent kernel panic?

> +	if (!fsl_desc)
> +		return NULL;
> +	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;

[Severity: High]
Are we missing the multi-fifo and port window offset handling here that is
present in fsl_edma_prep_dma_cyclic()?

In fsl_edma_prep_dma_cyclic(), doff is conditionally set to 4 if
fsl_chan->is_multi_fifo is true, or to dst_addr_width if a port window is
configured. 

Here, doff and soff are hardcoded to 0 for the memory endpoints. When
fsl_edma_fill_tcd() applies a minor loop offset (-burst) for multi-fifo or
port window configurations, it expects the address to have incremented during
the minor loop. With a 0 offset, wouldn't the address increment backwards on
each loop?

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

[Severity: Medium]
Will this permanently reduce the burst size for all remaining vectors in the
array if an early vector has an unaligned length?

nbytes is initialized outside the vector loop to the maximum burst size.
If vecs[i].len is unaligned, nbytes is reduced to a smaller value here.
Because nbytes is not restored to the optimal width * maxburst at the start of
the next iteration, won't subsequent perfectly-aligned vectors be forced to use
the reduced burst size?

> +			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: High]
Can this modulo operation result in a divide-by-zero panic for DMA_DEV_TO_DEV
transfers?

For a DMA_DEV_TO_DEV transfer, the direction is not DMA_DEV_TO_MEM, so width
is set to soff. However, soff is explicitly hardcoded to 0 for DMA_DEV_TO_DEV
transfers just above this block.

This would result in j * width evaluating to 0, causing a divide-by-zero
exception when calculating the modulo.

> +					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;
> +
> +		iter = vecs[i].len / nbytes;

[Severity: High]
Could iter be evaluated to 0 if the vector length is smaller than the
peripheral width?

If vecs[i].len < width, the earlier fallback loop leaves nbytes = width.
The integer division vecs[i].len / nbytes will drop the remainder, resulting
in iter = 0.

Programming the eDMA TCD iteration count to 0 usually causes undefined
hardware behavior, sometimes executing the maximum possible iterations. Should
there be a check to ensure iter > 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1