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

Pre-existing issues:
- [High] The driver is vulnerable to divide-by-zero kernel panics during DMA preparation, primarily when handling unaligned DMA_DEV_TO_DEV transfers or when slave configuration attributes are uninitialized (zero).
- [High] Passing a zero-length array (`nb == 0` or `sg_len == 0`) leads to a zero-length allocation for a flexible array, causing an out-of-bounds read/write during descriptor submission and transfer execution.
- [High] Multi-FIFO configuration is ignored for offsets in SG/vec transfers, leading to negative address progression and hardware state corruption.
--

commit 19b9121b085ec8ee890fa0c25990d53b2a25758c
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;
> +
> +	fsl_desc = fsl_edma_alloc_desc(fsl_chan, nb);

[Severity: High]
This is a pre-existing issue, but does this allocation lack a check for nb > 0?

If nb is 0, fsl_edma_alloc_desc() allocates a descriptor with a 0-length
flexible array for tcd. Later, when the descriptor is submitted and executed:

drivers/dma/fsl-edma-common.c:fsl_edma_xfer_desc() {
    ...
    fsl_edma_set_tcd_regs(fsl_chan, fsl_chan->edesc->tcd[0].vtcd);
    ...
}

This unconditionally accesses tcd[0], resulting in an out-of-bounds read.

Also, fsl_edma_link_sg() accesses prev_desc->tcd[prev_desc->n_tcds - 1], which
would evaluate to index -1. Could this cause memory corruption or a crash if
nb is 0?

> +	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++) {
> +		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]
This is a pre-existing issue, but does hardcoding doff to 0 break multi-FIFO
devices?

In fsl_edma_prep_dma_cyclic(), doff is set to 4 if fsl_chan->is_multi_fifo is
true. Here it is hardcoded to 0. When fsl_edma_fill_tcd() calculates a
negative minor loop offset for multi-FIFO configurations:

drivers/dma/fsl-edma-common.c:fsl_edma_fill_tcd() {
    ...
    nbytes |= EDMA_V3_TCD_NBYTES_MLOFF(-burst);
    ...
}

Since doff is 0, the address does not advance during the minor loop, and the
negative offset continually rewinds the DMA address backwards in memory.
Can this corrupt adjacent memory or hardware registers?

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

[Severity: High]
This is a pre-existing issue, but can this code cause a divide-by-zero panic?

If direction is DMA_DEV_TO_DEV, both doff and soff are hardcoded to 0, which
makes width evaluate to 0. Then, the fallback loop calculates:
    vecs[i].len % (j * width)
resulting in a modulo by zero.

Additionally, if the vector length is perfectly aligned and the block is
skipped, the later calculation:
    iter = vecs[i].len / nbytes;
could divide by zero if nbytes evaluates to 0 due to uninitialized slave
address width configuration.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-fsl-edma-dyn-sg-v5-0-16787185be49@bootlin.com?part=1