Re: [PATCH v9 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:
- [Critical] Kernel panic due to division by zero when adjusting the burst length for unaligned DMA_DEV_TO_DEV transfers.
- [Critical] Missing parameter validation for `nb == 0` (or `sg_len == 0`) allows the allocation of an empty descriptor, leading to critical out-of-bounds access.
- [Critical] Kernel panic (BUG_ON) triggered in `dma_map_phys` when mapping a DMA_DEV_TO_DEV resource because the driver passes `DMA_NONE` to `dma_map_resource()`.
- [High] The local `nbytes` variable is permanently reduced during vector length alignment, affecting all subsequent vectors.
- [High] Lockless modification of shared channel state during descriptor preparation causes race conditions and resource leaks.
--

commit bc7a9931f6cf0321b75f8853ea3938f42947649d
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..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: Critical]
This is a pre-existing issue, but if the direction is DMA_DEV_TO_DEV, does
fsl_edma_prep_slave_dma() eventually pass DMA_NONE to dma_map_resource()?

drivers/dma/fsl-edma-common.c:fsl_edma_prep_slave_dma() {
    ...
    switch (dir) {
    ...
    default:
        dma_dir = DMA_NONE;
        break;
    }
    ...
    fsl_chan->dma_dev_addr = dma_map_resource(dev, addr, size, dma_dir, 0);
    ...
}

Will passing DMA_NONE to dma_map_resource() trigger a BUG_ON() panic in the
core DMA mapping subsystem?

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

[Severity: Critical]
This is a pre-existing issue, but does this allow the creation of an empty
descriptor if nb == 0?

If nb == 0, a descriptor with a 0-length flexible array is allocated.
When the DMA engine later issues the transfer, for example in
fsl_edma_xfer_desc():

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

could this unconditionally access the nonexistent zeroth element tcd[0],
leading to out-of-bounds access?

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

[Severity: High]
This isn't a bug introduced by this patch, but does this code modify the
shared channel state fsl_chan->cfg.src_addr_width without holding the channel
lock?

If multiple threads concurrently call fsl_edma_prep_peripheral_dma_vec() on
the same channel, could they race when mutating fsl_chan->cfg.src_addr_width
and when calling dma_map_resource() in fsl_edma_prep_slave_dma() earlier,
causing a resource leak or use-after-free of active hardware mappings?

[ ... ]

> +		} 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) {
> +			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, but if the transfer direction is DMA_DEV_TO_DEV,
both soff and doff are 0. Since width evaluates to 0, does the modulo
operation vecs[i].len % (j * width) cause a division by zero and kernel panic?

> +					nbytes = j * width;

[Severity: High]
This is a pre-existing issue, but does setting nbytes = j * width
permanently reduce nbytes for all subsequent vectors in the loop?

If the transfer contains an unaligned vector followed by a very large vector,
the first unaligned vector permanently downgrades nbytes for the rest of the
loop since it is never reset to its original value. Could this cause the
iteration count iter to exceed hardware limits FIELD_MAX(EDMA_TCD_ITER_MASK),
rejecting valid transfers?

> +					break;
> +				}
> +			}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-fsl-edma-dyn-sg-v9-0-60bb2e678e8f@bootlin.com?part=1