Re: [PATCH v1 3/6] dmaengine: switchtec-dma: fix channel leak on registration failure

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch introduces a race condition leading to a use-after-free because it frees channel structures without removing them from `dma_dev->channels` while a global interrupt is still active.

Pre-existing issues:
- [High] In `switchtec_dma_remove()`, `swdma_dev` is used to unmap resources after it has already been freed.
- [High] Broken cleanup of `chan_status_irq` resulting in a memory leak, wrong IRQ being freed, and a dangling interrupt handler leading to a use-after-free.
--

--- Patch [3]: [PATCH v1 3/6] dmaengine: switchtec-dma: fix channel leak on registration failure ---
commit 26b72b89d5c52be6bb3a650d3492040c2465768c
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: switchtec-dma: fix channel leak on registration failure

If dma_async_device_register() fails during probe,
switchtec_dma_chans_release() only stops hardware and frees IRQs and
tasklets, but never frees the per-channel swdma_chan structures. This patch
frees the channels and the array directly in the registration-failure path.

Link: https://lore.kernel.org/dmaengine/20260707165555.350951F000E9@smtp.kernel.org/T/#u

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index f77da31aeb65e..5768948ef4662 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c

[ ... ]

> @@ -1317,6 +1317,11 @@ static int switchtec_dma_create(struct pci_dev *pdev)
>  err_chans_release_exit:
>  	switchtec_dma_chans_release(pdev, swdma_dev);
>  
> +	for (i = 0; i < swdma_dev->chan_cnt; i++)
> +		kfree(swdma_dev->swdma_chans[i]);

[Severity: High]
Does this free the channel structures while they are still linked in
dma_dev->channels?

During initialization, the embedded dma_chan nodes are added to
dma->channels. When freeing them here, the nodes are not removed from the
list, and the global chan_status_irq is still registered and active until
the subsequent err_exit block.

If the hardware asserts the interrupt, or if the interrupt is already being
processed concurrently, could switchtec_dma_chan_status_isr() iterate over
dma_dev->channels through this freed memory?

> +
> +	kfree(swdma_dev->swdma_chans);
> +
>  err_exit:
>  	if (swdma_dev->chan_status_irq)
>  		free_irq(swdma_dev->chan_status_irq, swdma_dev);

[Severity: High]
This is a pre-existing issue, but does this cleanup block use a vector
index rather than the global Linux IRQ number?

During probe, swdma_dev->chan_status_irq stores the MSI-X vector index
(e.g., 0 or 1). If it is 0, the condition evaluates to false, skipping
cleanup. If it is > 0, it calls free_irq() with a small integer, which
attempts to free an unrelated system IRQ.

This leaves the actual MSI-X interrupt handler registered. If the Linux IRQ
number is later reused by another device, could the kernel invoke
switchtec_dma_chan_status_isr() with a dev_id pointing to the freed
swdma_dev structure?


[Severity: High]
This is also a pre-existing issue, but is swdma_dev used to unmap resources
after it has already been freed in switchtec_dma_remove()?

switchtec_dma_remove() calls dma_async_device_unregister(&swdma_dev->dma_dev).
This synchronously drops a reference to the DMA device, which can trigger the
driver's switchtec_dma_release() callback and execute kfree(swdma_dev).

Once dma_async_device_unregister() returns, the swdma_dev pointer can be
invalid, but switchtec_dma_remove() immediately proceeds to call
iounmap(swdma_dev->bar). Could this result in a use-after-free on device
teardown?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717221001.361421-1-logang@deltatee.com?part=3