Re: [PATCH v8 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:

Pre-existing issues:
- [High] Out-of-bounds memory access when the caller passes a 0-length vector array.
- [High] Division by zero kernel panic when processing unaligned DMA_DEV_TO_DEV transfers.
- [High] BUG_ON kernel panic when mapping a resource for DMA_DEV_TO_DEV transfers on a reused channel.
- [High] Silent truncation of unaligned DMA transfers leads to data corruption.
- [Medium] State leakage of the burst size (`nbytes`) reduces performance and can cause transfer rejection for subsequent vectors.
--

--- Patch [1]: [PATCH v8 1/2] dmaengine: fsl-edma: Implement device_prep_peripheral_dma_vec ---
commit eeea1310bfd4138f851a3a95d72a197b35c5d579
Author: Benoît Monin <benoit.monin@bootlin.com>

dmaengine: fsl-edma: Implement device_prep_peripheral_dma_vec

This patch adds the .device_prep_peripheral_dma_vec() callback to the Freescale
eDMA driver. It sets up a scatter/gather DMA transfer from an array of dma_vec
structures and supports cyclic transfers when DMA_PREP_REPEAT 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 if this channel is reused and reconfigured
from DMA_MEM_TO_DEV to DMA_DEV_TO_DEV, fsl_edma_prep_slave_dma() will evaluate
dma_dir as DMA_NONE. This causes it to bypass the early return, unmap the old
resource, and call dma_map_resource() with DMA_NONE:

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

Since DMA_NONE is not a valid mapping direction, does this trigger the
BUG_ON(!valid_dma_direction(dir)) in the DMA mapping core and crash the kernel?

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

[Severity: High]
This isn't a bug introduced by this patch, but if nb is 0, does
fsl_edma_alloc_desc() allocate a descriptor with a 0-length tcd flexible
array?

If so, later when the hardware transfer is initiated in fsl_edma_xfer_desc(),
would the unconditional access to fsl_chan->edesc->tcd[0].vtcd cause an
out-of-bounds memory access?

> +	if (!fsl_desc)
> +		return NULL;

[ ... ]

> +		/*
> +		 * 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: High]
This is a pre-existing issue, but for DMA_DEV_TO_DEV transfers, both the
source offset (soff) and destination offset (doff) are initialized to 0,
resulting in width being assigned 0.

Does this check evaluate to a modulo-by-zero operation and cause a kernel
panic?

> +					nbytes = j * width;

[Severity: Medium]
This isn't a bug introduced by this patch, but nbytes is modified here for an
unaligned vector and never restored to its optimal maximum value at the start
of the next iteration.

Could this cause subsequent aligned vectors to incorrectly inherit this
reduced burst size, potentially degrading performance or causing the transfer
to be rejected if the iteration count exceeds the hardware maximum?

> +					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]
This is a pre-existing issue, but if a vector's length cannot be perfectly
divided by any suitable burst size, nbytes is set to width.

When iter is calculated using integer division here, are any remaining
unaligned bytes silently discarded, causing the hardware to transfer fewer
bytes than requested and leading to silent data corruption?

> +		if (i < nb - 1) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-fsl-edma-dyn-sg-v8-0-dc2b0317206d@bootlin.com?part=1