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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing validation for maximum segment transfer length causes silent 16-bit truncation of the loop iteration count.

Pre-existing issues:
- [Critical] Unconditional division and modulo by zero occur during DMA_DEV_TO_DEV transfers due to hardcoded zero offsets.
- [Critical] Missing `is_multi_fifo` and `port_window_size` handling causes the peripheral address pointer to permanently walk backwards.
- [High] Missing validation for `nb == 0` leads to a zero-length flexible array allocation and a subsequent out-of-bounds access.
- [High] Missing validation for slave configuration parameters allows clients to trigger a divide-by-zero crash.
- [High] Calling `dma_map_resource` with `DMA_NONE` triggers a BUG_ON in the DMA mapping subsystem.
--

--- Patch [1]: [PATCH v6 1/2] dmaengine: fsl-edma: Implement device_prep_peripheral_dma_vec ---
commit 70924f5c721616d3d99a0c89b36974c0edfc571b
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
> @@ -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)
> +{
> +	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 isn't a bug introduced by this patch, but does calling
fsl_edma_prep_slave_dma() with DMA_DEV_TO_DEV trigger a BUG_ON()?

fsl_edma_prep_slave_dma() translates DEV_TO_DEV to DMA_NONE, which is then
passed to dma_map_resource(). The DMA subsystem enforces a BUG_ON for
invalid directions like DMA_NONE, which would crash the kernel.

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

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for nb to be
0 here?

If so, fsl_edma_alloc_desc() will allocate a descriptor with a 0-length
tcd flexible array, and later fsl_edma_xfer_desc() might dereference it
out of bounds.

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

[Severity: High]
This is a pre-existing issue, but what happens if the client misconfigures
dst_maxburst or dst_addr_width and leaves them at 0?

It looks like nbytes evaluates to 0, which would cause a divide-by-zero
kernel panic later when evaluating vecs[i].len % nbytes.

[ ... ]
> +	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;

[Severity: Critical]
This isn't a bug introduced by this patch, but doesn't the lack of
is_multi_fifo and port_window_size handling here cause the address to
permanently walk backwards?

fsl_edma_fill_tcd() applies a negative minor loop offset if these are
enabled, but since doff and soff are hardcoded to 0 here, the address doesn't
increment during the minor loop and still gets subtracted at the end.

[ ... ]
> +		/*
> +		 * 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))) {

[Severity: Critical]
This is a pre-existing issue copied from fsl_edma_prep_slave_sg(), but
could this cause a divide-by-zero panic during a DMA_DEV_TO_DEV transfer?

For DEV_TO_DEV, both soff and doff are hardcoded to 0 earlier in the loop,
so width becomes 0. If the vector length isn't a multiple of nbytes,
this modulo operation will divide by zero.

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

[Severity: High]
Does this properly handle vectors that exceed the hardware's maximum segment
size?

The dma_vec API passes raw vector lengths directly from the client, and
iter is a u16. If vecs[i].len / nbytes exceeds 65535, will this silently
truncate and lead to a partial transfer and data corruption?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710-fsl-edma-dyn-sg-v6-0-831b96be3f31@bootlin.com?part=1