Re: [PATCH v4 09/12] dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove()

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

Pre-existing issues:
- [High] DMA coherent buffers and descriptor structures are permanently leaked in `switchtec_dma_free_chan_resources()` if a channel is released after device hot-unplug.
- [High] Missing NULL check for the result of `ioremap()` in `switchtec_dma_create()`.
--

--- Patch [9]: [PATCH v4 09/12] dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove() ---
commit 07d288b1e2418ad94452e6749769e4b45a0f0978
Author: Logan Gunthorpe <[email protected]>
Subject: dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove()

This commit addresses a use-after-free by caching the bar address before
unregistering the device. During dma_async_device_unregister(), the last
reference to dma_dev can be dropped, synchronously freeing swdma_dev before
the original code used it for iounmap().

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

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index ecf3a44d4dcc4..babb2bcb62084 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);
>  
> @@ -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);
>  }

[Severity: High]
This is a pre-existing issue, but I noticed a missing NULL check for the
result of ioremap() in switchtec_dma_create():

drivers/dma/switchtec_dma.c:switchtec_dma_create() {
    swdma_dev->bar = ioremap(pci_resource_start(pdev, 0),
                             pci_resource_len(pdev, 0));
    ...
    irq = readw(swdma_dev->bar + SWITCHTEC_REG_CHAN_STS_VEC);
}

If ioremap() fails and returns NULL, could this result in a NULL pointer
dereference when we try to read from it?

[Severity: High]
This is a pre-existing issue, but does this code permanently leak DMA
coherent buffers and descriptor structures if a channel is released after
device hot-unplug?

drivers/dma/switchtec_dma.c:halt_channel() {
    pdev = rcu_dereference(swdma_chan->swdma_dev->pdev);
    if (!pdev) {
        ret = -ENODEV;
        goto unlock_and_exit;
    }
}

When pdev is NULL, halt_channel() returns -ENODEV, which propagates to
switchtec_dma_chan_stop(). 

Then in switchtec_dma_free_chan_resources():

    rc = switchtec_dma_chan_stop(swdma_chan);
    switchtec_dma_abort_desc(swdma_chan, 0);
    if (!rc)
        switchtec_dma_free_desc(swdma_chan);

Since switchtec_dma_chan_stop() returned -ENODEV (which is non-zero), the
call to switchtec_dma_free_desc() is skipped. While skipping cleanup might
make sense if hardware times out, is this intentional for hot-unplug cases
where the hardware is physically absent?

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