Re: [PATCH v5 09/12] 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] DMA descriptor memory and coherent allocations are permanently leaked when channels are released or allocation fails after the PCI device is hot-removed.
- [High] Missing NULL check for ioremap return value can lead to a NULL pointer dereference.
- [High] Tearing down hardware state before RCU synchronization allows concurrent DMA submissions to trigger hardware faults.
- [High] Missing validation of hardware-provided chan_cnt against the mapped BAR length.
--

commit 0f74dee750d8feb49b287883e72b10e222134798
Author: Logan Gunthorpe <logang@deltatee.com>
Subject: dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove()

dma_async_device_unregister() can drop the last reference on dma_dev and free
swdma_dev synchronously. This patch caches the bar locally before unregistering
to avoid a use-after-free during iounmap().

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 c2803af6589d4..3b4ec4ad2bbe9 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1393,6 +1393,7 @@ static int switchtec_dma_probe(struct pci_dev *pdev,
>  static void switchtec_dma_remove(struct pci_dev *pdev)
>  {
>  	struct switchtec_dma_dev *swdma_dev = pci_get_drvdata(pdev);
> +	void __iomem *bar = swdma_dev->bar;
>  
>  	switchtec_dma_chans_release(pdev, swdma_dev);

[Severity: High]
This is a pre-existing issue, but does tearing down the hardware channels here
before RCU synchronization leave a window for concurrent submissions to trigger
hardware faults?

If switchtec_dma_chans_release() zeroes the DMA base registers (via
switchtec_dma_chan_stop()), this happens before swdma_dev->pdev is cleared and
before dma_async_device_unregister() removes the channels from use.

Could a concurrent client in switchtec_dma_issue_pending() check that
pdev != NULL under rcu_read_lock(), and write to the sq_tail register?

switchtec_dma_issue_pending() {
    ...
    spin_lock_bh(&swdma_chan->submit_lock);
    writew(swdma_chan->head, &swdma_chan->mmio_chan_hw->sq_tail);
    spin_unlock_bh(&swdma_chan->submit_lock);
    ...
}

Would this instruct the hardware to fetch descriptors from the now-zeroed base
address, causing DMA or IOMMU faults?

[ ... ]

> @@ -1405,7 +1406,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 driver, I noticed a few other pre-existing issues.

[Severity: High]
This isn't a bug introduced by this patch, but does the error handling in
switchtec_dma_free_chan_resources() and switchtec_dma_alloc_chan_resources()
permanently leak DMA descriptor memory and coherent allocations?

If the PCI device is hot-unplugged, switchtec_dma_chan_stop() can return
-ENODEV. In switchtec_dma_free_chan_resources(), the code conditionally skips
freeing the descriptors:

    if (!rc)
        switchtec_dma_free_desc(swdma_chan);

A similar early return exists in switchtec_dma_alloc_chan_resources():

    if (switchtec_dma_chan_stop(swdma_chan)) {
        disable_channel(swdma_chan);
        return rc;
    }

Do these paths leave the hardware queues (hw_sq and hw_cq) permanently leaked
when allocating or releasing channels after the device is removed?

[Severity: High]
This is a pre-existing issue, but is there a missing check for ioremap()
failures in switchtec_dma_create()?

    swdma_dev->bar = ioremap(pci_resource_start(pdev, 0),
                             pci_resource_len(pdev, 0));

Shortly after, the bar pointer is unconditionally dereferenced:

    irq = readw(swdma_dev->bar + SWITCHTEC_REG_CHAN_STS_VEC);

Could this cause a kernel oops during device initialization if ioremap() fails?

[Severity: High]
This isn't a bug introduced by this patch, but does the driver validate the
hardware-provided channel count against the mapped BAR size?

In switchtec_dma_create(), chan_cnt is read directly from hardware:

    chan_cnt = readl(swdma_dev->bar + SWITCHTEC_REG_CHAN_CNT);

Later, in switchtec_dma_chan_init(), this untrusted count is used for pointer
arithmetic to calculate the MMIO pointers:

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

Could a malicious or malfunctioning PCIe endpoint supply an arbitrarily large
chan_cnt, leading to out-of-bounds MMIO accesses?

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