Re: [PATCH 5/6] dmaengine: zynqmp_dma: Reject zero-length memcpy transfers

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Dropping the channel lock during descriptor cleanup allows a concurrent `terminate_all` call to silently corrupt list traversal, resulting in double-frees and infinite loops.
- [High] DMA hardware is not stopped in `zynqmp_dma_device_terminate_all`, risking memory corruption when descriptors are freed.
- [High] Unbinding the device via sysfs while a DMA client is active will cause a Use-After-Free due to `devm_kzalloc` misuse.
- [High] Integer overflow and truncation in `desc_cnt` calculation allows an arbitrarily large transfer length to bypass descriptor availability checks, leading to list corruption.
--

--- Patch [5]: [PATCH 5/6] dmaengine: zynqmp_dma: Reject zero-length memcpy transfers ---
commit 6cf35d097f62dcd5d2c9ce20c7eb0a221c5be776
Author: Golla Nagendra <nagendra.golla@amd.com>

dmaengine: zynqmp_dma: Reject zero-length memcpy transfers

Zero-length prep_memcpy() needlessly consumed a descriptor slot. Reject
zero-length memcpy at prep time and document the behaviour.

> diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
> index 2d56ea08ce45a..e70874d651231 100644
> --- a/drivers/dma/xilinx/zynqmp_dma.c
> +++ b/drivers/dma/xilinx/zynqmp_dma.c
[ ... ]
> @@ -851,6 +852,9 @@ static struct dma_async_tx_descriptor *zynqmp_dma_prep_memcpy(
>  
>  	chan = to_chan(dchan);
>  
> +	if (!len)
> +		return NULL;
> +
>  	desc_cnt = DIV_ROUND_UP(len, ZYNQMP_DMA_MAX_TRANS_LEN);
>  
>  	spin_lock_irqsave(&chan->lock, irqflags);

[Severity: High]
This isn't a bug introduced by this patch, but does this size calculation allow
arbitrary lengths to bypass descriptor availability checks?

If a client driver passes a very large len (e.g., SIZE_MAX) to
zynqmp_dma_prep_memcpy(), the calculation
DIV_ROUND_UP(len, ZYNQMP_DMA_MAX_TRANS_LEN) might overflow the 64-bit integer,
resulting in a quotient of 0.

The check (desc_cnt > chan->desc_free_cnt) would then pass, and the subsequent
loop could unconditionally pop from the free_list billions of times, leading to
list underflow and out-of-bounds writes.

While reviewing this code, I also noticed a few other pre-existing issues in
this driver:

[Severity: High]
This is a pre-existing issue, but does zynqmp_dma_device_terminate_all()
safely stop the hardware before freeing descriptors?

drivers/dma/xilinx/zynqmp_dma.c:zynqmp_dma_device_terminate_all() {
    ...
    writel(ZYNQMP_DMA_IDS_DEFAULT_MASK, chan->regs + ZYNQMP_DMA_IDS);
    zynqmp_dma_free_descriptors(chan);
    return 0;
}

If a DMA client calls dmaengine_terminate_all() while a transfer is active,
the driver masks interrupts and recycles the software descriptors back to
free_list.

However, it doesn't appear to write to the hardware registers (e.g., clearing
ZYNQMP_DMA_ENABLE in ZYNQMP_DMA_CTRL2) to stop the physical DMA engine. If a
new transfer is requested, the software will reallocate the freed descriptors
while the hardware concurrently manipulates them, which could cause race
conditions and memory corruption.

[Severity: High]
This is a pre-existing issue, but could unbinding the device via sysfs while a
DMA client is active cause a use-after-free?

drivers/dma/xilinx/zynqmp_dma.c:zynqmp_dma_chan_probe() {
    ...
    chan = devm_kzalloc(zdev->dev, sizeof(*chan), GFP_KERNEL);
    ...
}

The driver allocates its core structures (zdev and chan) using devm_kzalloc().

If unbound via sysfs, zynqmp_dma_remove() is called and
dma_async_device_unregister() is invoked. The devres framework then
automatically frees the zdev and chan structures.

An active client still holds a pointer to the channel because
dma_async_device_unregister() does not wait for clients to release their
references, meaning subsequent client access would dereference freed memory.

[Severity: Critical]
This is a pre-existing issue, but is it safe to drop the channel lock during
descriptor cleanup in zynqmp_dma_chan_desc_cleanup()?

drivers/dma/xilinx/zynqmp_dma.c:zynqmp_dma_chan_desc_cleanup() {
    ...
    list_for_each_entry_safe(desc, next, &chan->done_list, node) {
        ...
        if (dmaengine_desc_callback_valid(&cb)) {
            spin_unlock_irqrestore(&chan->lock, irqflags);
            dmaengine_desc_callback_invoke(&cb, NULL);
            spin_lock_irqsave(&chan->lock, irqflags);
        }
        zynqmp_dma_free_descriptor(chan, desc);
    }
    ...
}

The loop iterates over chan->done_list using list_for_each_entry_safe() and
drops chan->lock to invoke the client callback.

If a client driver concurrently calls zynqmp_dma_device_terminate_all(), which
takes the lock and moves all descriptors from done_list to free_list, the
cached next pointer is now in free_list when the lock is reacquired.

The loop would continue traversing the free_list, double-freeing descriptors
and dereferencing the free_list head, resulting in a system panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806123014.2120447-1-nagendra.golla@amd.com?part=5