Re: [PATCH v12 18/20] dmaengine: ti: k3-udma-v2: Add support for PKTDMA V2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] dmaengine: ti: hardware ring UAF on unstoppable DMA channel error path
- [Medium] dmaengine: ti: udma_v2_ring_irq_handler returns IRQ_NONE for valid RX flush
- [Medium] dmaengine: ti: DMA mapping leaks on probe and setup error paths
--

commit 49c152d7719d193b9ac7700439e03ff1c85f702b
Author: Sai Sree Kartheek Adivi <s-adivi@ti.com>

dmaengine: ti: k3-udma-v2: Add support for PKTDMA V2

The patch adds support for PKTDMA V2 to the k3-udma driver. It introduces
changes such as static channel mapping, direct IRQs to the GIC, unified
channel space, and remote side configuration handled directly by DMA without
needing to write to PEER registers.

> diff --git a/drivers/dma/ti/k3-udma-v2.c b/drivers/dma/ti/k3-udma-v2.c
> index 0bbd7e41a8f9b..feac531a24f47 100644
> --- a/drivers/dma/ti/k3-udma-v2.c
> +++ b/drivers/dma/ti/k3-udma-v2.c
[ ... ]
> @@ -803,6 +803,155 @@ static int bcdma_v2_alloc_chan_resources(struct dma_chan *chan)
>  	return ret;
>  }
>  
> +static int pktdma_v2_alloc_chan_resources(struct dma_chan *chan)
> +{
[ ... ]
> +	if (udma_is_chan_running(uc)) {
> +		dev_warn(ud->dev, "chan%d: is running!\n", uc->id);
> +		ud->reset_chan(uc, false);
> +		if (udma_is_chan_running(uc)) {
> +			dev_err(ud->dev, "chan%d: won't stop!\n", uc->id);
> +			ret = -EBUSY;
> +			goto err_res_free;
> +		}
> +	}
[ ... ]
> +	ret = request_irq(uc->irq_num_ring, udma_v2_ring_irq_handler,
> +			  IRQF_SHARED | IRQF_TRIGGER_HIGH, uc->name, uc);
> +
> +	if (ret) {

[Severity: Medium]
Will returning IRQ_NONE for a valid hardware event (like an RX flush descriptor
successfully popped) break the IRQF_SHARED contract?

In udma_v2_ring_irq_handler(), if udma_pop_from_ring() returns -ENOENT, it
evaluates as true and returns IRQ_NONE:

    if (udma_pop_from_ring(uc, &paddr) || !paddr)
        return IRQ_NONE;

Since accumulating these spurious interrupts during teardown can cause the
spurious IRQ detector to permanently disable the interrupt line, does this
break all other devices sharing the IRQ?

[ ... ]
> +err_irq_free:
> +	uc->irq_num_ring = 0;
> +err_res_free:
> +	udma_free_tx_resources(uc);
> +	udma_free_rx_resources(uc);

[Severity: High]
Does this code free the coherent DMA memory back to the kernel pool without
disabling the hardware ring accelerator?

If the DMA channel is still running (as checked earlier in the function) and
the ring accelerator points to this freed memory, will the hardware perform
arbitrary DMA writes/reads and corrupt kernel memory?


[Severity: Medium]
Does the error path in udma_v2_probe() permanently leak coherent DMA memory?

If of_dma_controller_register() fails, it returns without calling
udma_teardown_rx_flush():

    ret = of_dma_controller_register(dev->of_node, udma_v2_of_xlate, ud);
    if (ret) {
        dev_err(dev, "failed to register of_dma controller\n");
        dma_async_device_unregister(&ud->ddev);
    }
    return ret;

Additionally, if dma_map_single() fails inside udma_setup_rx_flush(), does it
return -ENOMEM without unmapping the already-mapped resources like
rx_flush->buffer_paddr?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007102936.2402427-1-s-adivi@ti.com?part=18