Re: [PATCH v3 04/11] dmaengine: switchtec-dma: fix channel leak on registration failure

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

New issues:
- [High] Freeing DMA channels in error paths leaves dangling pointers in the `dma_dev->channels` list, leading to a Use-After-Free when accessed by the active channel status ISR.

Pre-existing issues:
- [High] The `switchtec_dma_remove()` function dereferences `swdma_dev` after it has been freed by the `device_release` callback.
- [High] Registering the channel status ISR before initializing the channel list head causes a NULL pointer dereference.
- [High] Pre-existing deadlock if a client callback attempts to abort or terminate the DMA channel.
- [High] Incorrect IRQ free API in `switchtec_dma_create()` error path guarantees an IRQ leak and subsequent Use-After-Free.
- [Medium] Missing `put_device()` in `switchtec_dma_create()` error path causes a reference count leak for the PCI device.
- [Medium] Missing `pci_free_irq_vectors()` on error paths in `switchtec_dma_create()` leaks MSI-X vectors.
--

--- Patch [4]: [PATCH v3 04/11] dmaengine: switchtec-dma: fix channel leak on registration failure ---
commit 58dd78b5d9196a368d9b2914fdfb2256ea81eef1
Author: Logan Gunthorpe <[email protected]>

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

switchtec_dma_chans_release() is called in three places but the
underlying memory is not freed in all of those places. In order to
clean this up, introduce a switchtec_dma_chans_free() helper that
will free the memory.

Link: https://lore.kernel.org/dmaengine/[email protected]

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 107769cca772f..13efd4189bbb4 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1176,6 +1176,16 @@ static int switchtec_dma_chans_release(struct pci_dev *pdev,
>  	return 0;
>  }
>  
> +static void switchtec_dma_chans_free(struct switchtec_dma_dev *swdma_dev)
> +{
> +	int i;
> +
> +	for (i = 0; i < swdma_dev->chan_cnt; i++)
> +		kfree(swdma_dev->swdma_chans[i]);

[Severity: High]
Does this code leak dangling pointers in the dma_dev->channels list?

The newly introduced switchtec_dma_chans_free() frees each swdma_chan but fails
to call list_del(&chan->device_node). If an interrupt occurs (e.g. spurious or
shared) before the ISR is disabled (or indefinitely if the ISR cleanup fails),
switchtec_dma_chan_status_isr() will iterate over this list and deference the
freed memory, leading to a Use-After-Free.

[ ... ]

> @@ -1317,6 +1321,7 @@ static int switchtec_dma_create(struct pci_dev *pdev)
>  
>  err_chans_release_exit:
>  	switchtec_dma_chans_release(pdev, swdma_dev);
> +	switchtec_dma_chans_free(swdma_dev);

[Severity: Medium]
This is a pre-existing issue, but does this error path leak the PCI device
reference count?

During probe, switchtec_dma_create() acquires the reference via
get_device(&pdev->dev). If dma_async_device_register() fails, the execution
jumps here. This path cleanly frees memory but fails to call put_device(dma->dev),
which leaks the reference count and prevents clean teardown on hot-unplug.

>  
>  err_exit:
>  	if (swdma_dev->chan_status_irq)

[Severity: High]
This is a pre-existing issue, but does this incorrect IRQ free API guarantee
an IRQ leak and subsequent Use-After-Free?

switchtec_dma_create() requests an MSI-X vector via pci_request_irq() using
the hardware vector index (e.g. 0). However, this error path calls free_irq()
instead of pci_free_irq().

This attempts to free the Linux IRQ number corresponding to the index (0),
which fails silently. Furthermore, because it's index 0, the
if (swdma_dev->chan_status_irq) condition prevents the free from even executing.
The ISR remains registered after swdma_dev is kfreed, leading to a UAF when
the IRQ fires.

[Severity: Medium]
This is a pre-existing issue, but does this error path also leak MSI-X vectors?

If pci_alloc_irq_vectors() succeeds, but subsequent steps like pci_request_irq()
or switchtec_dma_chans_enumerate() fail, the execution jumps to the err_exit
label. The error cleanup omits pci_free_irq_vectors(), leaking the allocated
vectors.

[ ... ]

For switchtec_dma_remove():

> static void switchtec_dma_remove(struct pci_dev *pdev)
> {
> 	struct switchtec_dma_dev *swdma_dev = pci_get_drvdata(pdev);
> 
> 	switchtec_dma_chans_release(pdev, swdma_dev);
> 
> 	rcu_assign_pointer(swdma_dev->pdev, NULL);
> 	synchronize_rcu();
> 
> 	pci_free_irq(pdev, swdma_dev->chan_status_irq, swdma_dev);
> 
> 	pci_free_irq_vectors(pdev);
> 
> 	dma_async_device_unregister(&swdma_dev->dma_dev);
> 
> 	iounmap(swdma_dev->bar);

[Severity: High]
This is a pre-existing issue, but does this dereference swdma_dev after it has
been freed?

During driver unbind, dma_async_device_unregister() drops the last reference
to the DMA device, which synchronously invokes switchtec_dma_release(). This
callback frees swdma_dev via kfree(swdma_dev). Immediately after, this
function attempts to access swdma_dev->bar in the iounmap() call, causing a
Use-After-Free.

For switchtec_dma_create():

> 	rc = pci_request_irq(pdev, irq, NULL, switchtec_dma_chan_status_isr,
> 			     swdma_dev, KBUILD_MODNAME);
> 	if (rc)
> 		goto err_exit;
> 
> 	swdma_dev->chan_status_irq = irq;
> 
> 	chan_cnt = readl(swdma_dev->bar + SWITCHTEC_REG_CHAN_CNT);
> 	if (!chan_cnt) {
> 		pci_err(pdev, "No channel configured.\n");
> 		rc = -ENXIO;
> 		goto err_exit;
> 	}
> 
> 	chan_cnt = switchtec_dma_chans_enumerate(swdma_dev, pdev, chan_cnt);

[Severity: High]
This is a pre-existing issue, but does registering the channel status ISR
before initializing the channel list head cause a NULL pointer dereference?

switchtec_dma_create() calls pci_request_irq() to register the ISR, then
proceeds to switchtec_dma_chans_enumerate() which calls
INIT_LIST_HEAD(&dma->channels).

Because dma_dev is allocated with kzalloc_obj, the channels.next pointer is
initially NULL. If an interrupt fires during this narrow window,
list_for_each_entry() in the ISR will compute a negative offset from NULL and
attempt to dereference it, leading to an immediate kernel panic.

For switchtec_dma_cleanup_completed():

> static void
> switchtec_dma_cleanup_completed(struct switchtec_dma_chan *swdma_chan)
> {
> [ ... ]
> 		do {
> 			dma_cookie_complete(&desc->txd);
> 			dma_descriptor_unmap(&desc->txd);
> 			dmaengine_desc_get_callback_invoke(&desc->txd, &res);

[Severity: High]
This is a pre-existing issue, but can this cause a deadlock if a client callback
attempts to abort or terminate the DMA channel?

switchtec_dma_cleanup_completed() acquires swdma_chan->complete_lock with
spin_lock_bh() and invokes the client callback synchronously here.

If the client calls dmaengine_terminate_all(), it invokes:

static int switchtec_dma_terminate_all(struct dma_chan *chan)
{
	struct switchtec_dma_chan *swdma_chan =
		container_of(chan, struct switchtec_dma_chan, dma_chan);

	spin_lock_bh(&swdma_chan->complete_lock);

This attempts to re-acquire the exact same complete_lock unconditionally, causing
a self-deadlock on a single CPU core.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4