Re: [PATCH v4 08/12] dmaengine: switchtec-dma: disable channels before freeing on registration failure

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

Pre-existing issues:
- [High] The global channel status IRQ is requested too early in `switchtec_dma_create()`, exposing a NULL pointer dereference and race condition in the ISR.
- [High] Use-after-free of `swdma_dev` in `switchtec_dma_remove()` due to synchronous free in the release callback.
- [High] Missing NULL check after `ioremap()` leads to a potential NULL pointer dereference.
- [High] Missing `put_device()` in the error path of `switchtec_dma_create()` causes a reference count leak of the PCI device.
--

--- Patch [8]: [PATCH v4 08/12] dmaengine: switchtec-dma: disable channels before freeing on registration failure ---
commit a7ad01b09e2b3565af60150b4c01bec6d5cae7ad
Author: Logan Gunthorpe <[email protected]>

    dmaengine: switchtec-dma: disable channels before freeing on registration failure
    
    When switchtec_dma_create() fails after channels have been added to
    dma_dev->channels (either from switchtec_dma_chans_enumerate()'s own
    error path, or from dma_async_device_register() failing), the channel
    status IRQ is still live while the channels are released and freed,
    so it can fire mid-teardown against channels that are only partially
    torn down.

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

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 6ab2c62b52fd8..ecf3a44d4dcc4 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1198,6 +1198,15 @@ static void switchtec_dma_chans_free(struct switchtec_dma_dev *swdma_dev)
>  	kfree(swdma_dev->swdma_chans);
>  }
>  
> +static void switchtec_dma_chans_disable(struct pci_dev *pdev,
> +					struct switchtec_dma_dev *swdma_dev)
> +{
> +	if (swdma_dev->chan_status_irq >= 0) {
> +		pci_free_irq(pdev, swdma_dev->chan_status_irq, swdma_dev);
> +		swdma_dev->chan_status_irq = -1;
> +	}
> +}
> +

[Severity: High]
This isn't a bug introduced by this patch, but since we're fixing IRQ
teardown here, is it possible the IRQ is requested too early during setup?

Looking at switchtec_dma_create(), the IRQ is requested before the channel
list is initialized:

switchtec_dma_create()
    rc = pci_request_irq(pdev, irq, NULL, switchtec_dma_chan_status_isr, ...);

If an interrupt fires immediately (e.g., from hardware state left by a
previous boot, or a spurious interrupt), the ISR will execute and iterate
over uninitialized memory:

switchtec_dma_chan_status_isr()
    list_for_each_entry(chan, &dma_dev->channels, device_node)

Could this cause a NULL pointer dereference in the ISR?

[ ... ]

> @@ -1334,6 +1344,7 @@ static int switchtec_dma_create(struct pci_dev *pdev)
>  	return 0;
>  
>  err_chans_release_exit:
> +	switchtec_dma_chans_disable(pdev, swdma_dev);
>  	switchtec_dma_chans_release(pdev, swdma_dev);
>  	switchtec_dma_chans_free(swdma_dev);
>  

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

Earlier in switchtec_dma_create(), a reference is taken:

switchtec_dma_create()
    dma->dev = get_device(&pdev->dev);

When jumping to err_chans_release_exit or err_exit, the code does not seem
to call put_device() to clean up this reference.


[Severity: High]
This is another pre-existing issue, but looking at the earlier portion of
switchtec_dma_create(), could a missing NULL check lead to a crash?

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

    RCU_INIT_POINTER(swdma_dev->pdev, pdev);

If ioremap() fails and returns NULL, subsequent MMIO accesses using
swdma_dev->bar will unconditionally dereference it. Should there be a
validation check here?


[Severity: High]
This is also a pre-existing issue, but does switchtec_dma_remove() have a
use-after-free risk during device teardown?

Looking at switchtec_dma_remove():

switchtec_dma_remove()
    dma_async_device_unregister(&swdma_dev->dma_dev);
    iounmap(swdma_dev->bar);

If dma_async_device_unregister() drops the last reference, it will
synchronously call switchtec_dma_release(), which frees swdma_dev:

switchtec_dma_release()
    kfree(swdma_dev);

Is it safe to pass swdma_dev->bar to iounmap() after the unregister call has
potentially freed swdma_dev?

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