Re: [PATCH v4 05/12] dmaengine: switchtec-dma: fix channel leak on registration failure

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

New issues:
- [High] The newly added `switchtec_dma_chans_free()` modifies `dma_dev->channels` locklessly and frees channel objects while the channel status ISR is active, leading to list corruption and Use-After-Free.

Pre-existing issues:
- [Critical] Use-After-Free of `swdma_dev` in `switchtec_dma_remove()` after calling `dma_async_device_unregister()`.
- [High] The `chan_status_irq` is requested before the `dma_dev->channels` list is initialized, leading to a NULL pointer dereference if the ISR runs early.
- [High] The error path in `switchtec_dma_create()` incorrectly uses `free_irq()` instead of `pci_free_irq()`, and uses the flawed condition `if (swdma_dev->chan_status_irq)`.
--

--- Patch [5]: [PATCH v4 05/12] dmaengine: switchtec-dma: fix channel leak on registration failure ---
commit a8d5be8a7824da6cc78512656e71020f75154f88
Author: Logan Gunthorpe <[email protected]>

dmaengine: switchtec-dma: fix channel leak on registration failure

switchtec_dma_chans_release() is called in three places but the
underlying memory is not freed in all of those places. In order to
clean this up, introduce a switchtec_dma_chans_free() helper that
will free the memory.

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 25b988e9b4afb..48da269198f3f 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c
> @@ -1188,6 +1188,18 @@ static int switchtec_dma_chans_release(struct pci_dev *pdev,
>  	return 0;
>  }
>  
> +static void switchtec_dma_chans_free(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);
> +		kfree(swdma_dev->swdma_chans[i]);
> +	}
> +
> +	kfree(swdma_dev->swdma_chans);
> +}

[ ... ]

> @@ -1233,12 +1243,8 @@ static void switchtec_dma_release(struct dma_device *dma_dev)
>  {
>  	struct switchtec_dma_dev *swdma_dev =
>  		container_of(dma_dev, struct switchtec_dma_dev, dma_dev);
> -	int i;
>  
> -	for (i = 0; i < swdma_dev->chan_cnt; i++)
> -		kfree(swdma_dev->swdma_chans[i]);
> -
> -	kfree(swdma_dev->swdma_chans);
> +	switchtec_dma_chans_free(swdma_dev);
>  
>  	put_device(dma_dev->dev);
>  	kfree(swdma_dev);
>  }

[Severity: Critical]
This is a pre-existing issue, but could this lead to a use-after-free when
the driver is unloaded?

If dma_async_device_unregister() is called in switchtec_dma_remove(), it will
drop the reference to the DMA engine device. If there are no active clients,
the release callback switchtec_dma_release() is synchronously invoked, which
calls kfree(swdma_dev) here.

Immediately after the unregister call returns, switchtec_dma_remove() calls
iounmap(swdma_dev->bar), dereferencing the freed swdma_dev pointer:

switchtec_dma_remove() {
    ...
    dma_async_device_unregister(&swdma_dev->dma_dev);
        // eventually calls switchtec_dma_release() -> kfree(swdma_dev)
    iounmap(swdma_dev->bar);
    ...
}

[ ... ]

> @@ -1329,6 +1335,7 @@ static int switchtec_dma_create(struct pci_dev *pdev)
>  
>  err_chans_release_exit:
>  	switchtec_dma_chans_release(pdev, swdma_dev);
> +	switchtec_dma_chans_free(swdma_dev);

[Severity: High]
Does this introduce a race with the active interrupt handler?

In switchtec_dma_create(), the error path calls switchtec_dma_chans_free()
before disabling the interrupt:

err_chans_release_exit:
    switchtec_dma_chans_release(pdev, swdma_dev);
    switchtec_dma_chans_free(swdma_dev);
    
err_exit:
    if (swdma_dev->chan_status_irq)
        free_irq(...);

If the interrupt fires concurrently, switchtec_dma_chan_status_isr() will
locklessly iterate over dma_dev->channels. At the same time,
switchtec_dma_chans_free() calls list_del() and kfree() on those channel
objects, which can cause list corruption or a use-after-free in the ISR.

>  err_exit:
>  	if (swdma_dev->chan_status_irq)

[Severity: High]
This is a pre-existing issue, but are we using the correct function and
condition to free the IRQ here?

chan_status_irq stores the MSI-X vector index (e.g., 0, 1), not the system IRQ
number, and it is requested using pci_request_irq().

Calling free_irq(swdma_dev->chan_status_irq, swdma_dev) passes the vector
index as the system IRQ, attempting to free an unrelated system interrupt
instead of the actual hardware interrupt.

Also, checking if (swdma_dev->chan_status_irq) will skip freeing the IRQ
entirely if the vector index happens to be 0. Should this use pci_free_irq()
and check against a valid vector indicator like >= 0 instead?

[Severity: High]
This is another pre-existing issue, but is it possible for the interrupt
handler to run before the channels list is initialized?

In switchtec_dma_create(), swdma_dev is allocated with kzalloc_obj(), which
leaves the dma_dev->channels list head zeroed (next == NULL). The code then
calls pci_request_irq(), enabling the interrupt.

The list head isn't initialized until INIT_LIST_HEAD(&dma->channels) is called
later inside switchtec_dma_chans_enumerate().

If an interrupt fires in this window, switchtec_dma_chan_status_isr() executes
list_for_each_entry() on the zeroed list head. The loop macro evaluates the
position from head->next (NULL), bypassing the termination condition and
dereferencing the pointer, which can cause a kernel panic.

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