Re: [PATCH v2 07/11] dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect IRQ cleanup in `switchtec_dma_create` error path using `free_irq` with a hardware vector index instead of `pci_free_irq`.
- [High] Pending DMA transactions are not aborted during device removal, potentially causing DMA clients to hang indefinitely.
- [High] Missing bounds check on the hardware-provided channel count allows out-of-bounds MMIO accesses.
- [Medium] Missing `put_device(dma->dev)` in the `switchtec_dma_create` error path causes a reference count leak on the PCI device.
--

--- Patch [7]: [PATCH v2 07/11] dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove() ---
commit 0e37b79ac2f6503c0a4dc42c7b0cf19efff70cbd
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove()

This patch fixes a use-after-free regression in switchtec_dma_remove() by
caching the PCI BAR in a local variable before dma_async_device_unregister()
is called, avoiding access to swdma_dev after it is freed.

Link: https://lore.kernel.org/dmaengine/20260717223431.625EE1F000E9@smtp.kernel.org

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 2a1ae27bd1a69..a7e25cb7c0b9f 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
[ ... ]
> @@ -1388,7 +1389,7 @@ static void switchtec_dma_remove(struct pci_dev *pdev)
>  
>  	dma_async_device_unregister(&swdma_dev->dma_dev);
>  
> -	iounmap(swdma_dev->bar);
> +	iounmap(bar);
>  	pci_release_mem_regions(pdev);
>  	pci_disable_device(pdev);
>  }

While reviewing this patch, I noticed a few pre-existing issues in the driver
that might be worth addressing.

drivers/dma/switchtec_dma.c:switchtec_dma_create() {
    ...
    chan_cnt = readl(swdma_dev->bar + SWITCHTEC_REG_CHAN_CNT);

[Severity: High]
This is a pre-existing issue, but is it safe to use this hardware-provided
channel count without bounds checking?

If the endpoint returns an unexpectedly large channel count, this value is
later used in switchtec_dma_chan_init() to calculate MMIO offsets:

    swdma_chan->mmio_chan_fw =
        swdma_dev->bar + SWITCHTEC_DMAC_CHAN_CFG_STS_OFFSET +
        i * SWITCHTEC_DMA_CHAN_FW_REGS_SIZE;

Since chan_cnt is never validated against the mapped length of the PCI BAR,
could this result in out-of-bounds MMIO accesses?

    if (!chan_cnt) {
        pci_err(pdev, "No channel configured.\n");
        rc = -ENXIO;
        goto err_exit;
    }
    ...
err_chans_release_exit:
    switchtec_dma_chans_unlist(swdma_dev);
    switchtec_dma_chans_release(pdev, swdma_dev);
    switchtec_dma_chans_free(swdma_dev);

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

If dma_async_device_register() fails earlier in this function, we jump here,
but there is no call to put_device() to balance the get_device() called
previously. This could leave the PCI device with an elevated reference count
permanently.

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 could this lead to a use-after-free
if an interrupt fires?

The code requests the IRQ using pci_request_irq() with a hardware vector
index, but attempts to free it here with free_irq() using that same vector
index instead of the global Linux IRQ number. Since free_irq() expects a
global IRQ number, it may fail to free the interrupt. The handler would
remain registered while swdma_dev is freed right after this.

Also, if the hardware uses vector 0, the if statement skips cleanup entirely.
Should this use pci_free_irq(pdev, swdma_dev->chan_status_irq, swdma_dev)
instead?

    iounmap(swdma_dev->bar);
    kfree(swdma_dev);
    return rc;
}


drivers/dma/switchtec_dma.c:switchtec_dma_chan_free() {
    ...
    pci_free_irq(pdev, swdma_chan->irq, swdma_chan);
    tasklet_kill(&swdma_chan->desc_task);

    switchtec_dma_chan_stop(swdma_chan);

[Severity: High]
This is a pre-existing issue, but does removing the device without aborting
pending transactions cause DMA clients to hang indefinitely?

If a client is blocking on a completion object (e.g., via
wait_for_completion), and the device is removed, the IRQ is freed and the
hardware is stopped here. However, active descriptor rings aren't traversed
to report DMA_TRANS_ABORTED. Can this prevent the client from ever unbinding?

    return 0;
}

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