Re: [PATCH v2 06/11] dmaengine: switchtec-dma: unlink channels before freeing on registration failure

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch introduces a data race by calling `list_del()` concurrently with the active interrupt handler, resulting in a poisoned list pointer dereference.

Pre-existing issues:
- [High] The channel status IRQ is requested prematurely before the `dma->channels` list and `chan->dev` are fully initialized, causing NULL pointer dereferences in the ISR.
- [High] The error path in `switchtec_dma_create()` incorrectly uses `free_irq()` with an MSI-X vector index instead of `pci_free_irq()`, and fails to free vector index 0.
- [High] The number of DMA channels is read directly from an untrusted hardware register and used without bounds validation, leading to out-of-bounds MMIO accesses.
--

--- Patch [6]: [PATCH v2 06/11] dmaengine: switchtec-dma: unlink channels before freeing on registration failure ---
commit 304c38996e78423c61828786ab626c7f864c96fd
Author: Logan Gunthorpe <logang@deltatee.com>

dmaengine: switchtec-dma: unlink 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 channels
are released and freed but never removed from dma_dev->channels.

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 c752a1b05871d..2a1ae27bd1a69 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1184,6 +1184,14 @@ static void switchtec_dma_chans_free(struct switchtec_dma_dev *swdma_dev)
>  	kfree(swdma_dev->swdma_chans);
>  }
>  
> +static void switchtec_dma_chans_unlist(struct switchtec_dma_dev *swdma_dev)
> +{
> +	int i;
> +
> +	for (i = 0; i < swdma_dev->chan_cnt; i++)
> +		list_del(&swdma_dev->swdma_chans[i]->dma_chan.device_node);
> +}
> +

[Severity: High]
Could this regression introduce a data race?

In switchtec_dma_create(), the error paths call switchtec_dma_chans_unlist()
while the channel status irq is still active. If an interrupt fires
concurrently, switchtec_dma_chan_status_isr() will traverse the
dma_dev->channels list locklessly using list_for_each_entry().

Can the isr traverse into the poisoned pointer from list_del() and crash
the system if it runs during or after this call? Should the irq be freed
or disabled before cleaning up the channels?

>  static int switchtec_dma_chans_enumerate(struct switchtec_dma_dev *swdma_dev,
>  					 struct pci_dev *pdev, int chan_cnt)

[ ... ]

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

[Severity: High]
This is a pre-existing issue, but is the channel status irq requested
prematurely in switchtec_dma_create()?

The irq is requested via pci_request_irq() before
switchtec_dma_chans_enumerate() is called and before the dma->channels list
is fully initialized.

If an interrupt fires immediately, switchtec_dma_chan_status_isr() will iterate
over an uninitialized list. Furthermore, the isr accesses chan->dev, which
is not set until dma_async_device_register() runs much later. Could this
cause a null pointer dereference?

[Severity: High]
This is a pre-existing issue, but does the error path in switchtec_dma_create()
fail to cleanly release the irq?

The code checks if swdma_dev->chan_status_irq is valid and calls free_irq().
If the vector index is 0, the check skips freeing, leaking the irq.
Additionally, if it is greater than 0, it passes the msi-x vector index
to free_irq() instead of the global linux irq number, which fails to free it.

If switchtec_dma_create() fails and swdma_dev is freed via kfree(), could a
subsequent interrupt execute the active isr and access the freed swdma_dev,
causing a use-after-free?

[Severity: High]
This is a pre-existing issue, but does switchtec_dma_create() read the channel
count without upper bounds validation?

The chan_cnt is read directly from the hardware register.
During enumeration in switchtec_dma_chan_init(), pointer arithmetic is
performed based on this count:

swdma_chan->mmio_chan_fw = swdma_dev->bar + offset + i * size;

If chan_cnt is excessively large due to untrusted or malfunctioning hardware
input, could this pointer extend beyond the bounds of the mapped pci bar
and cause an out-of-bounds mmio access when dereferenced?

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