Re: [PATCH v3 08/11] dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove()

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

Pre-existing issues:
- [High] Missing check for ioremap() failure in switchtec_dma_create() can cause a null pointer dereference.
- [High] Client callbacks are invoked while holding the channel's `complete_lock`, leading to a self-deadlock if the callback calls DMA engine APIs.
- [High] The channel status IRQ is requested before the `dma_dev->channels` list is initialized, which will cause a NULL pointer dereference if an interrupt fires early.
--

--- Patch [8]: [PATCH v3 08/11] dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove() ---
commit 29e71ea6cf1f57028cf682c7cc5c705361a852e2
Author: Logan Gunthorpe <[email protected]>

dmaengine: switchtec-dma: fix use-after-free of swdma_dev in remove()

This commit fixes a use-after-free of swdma_dev in switchtec_dma_remove().
It caches the bar variable locally before calling
dma_async_device_unregister(), which can synchronously free swdma_dev.

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

> diff --git a/drivers/dma/switchtec_dma.c b/drivers/dma/switchtec_dma.c
> index 800d8ecd07171..d73506b5cabc1 100644
> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does switchtec_dma_create()
check if ioremap() fails?

If ioremap() fails due to resource exhaustion, it returns NULL. The code
appears to immediately dereference this pointer:

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);
    ...
}

Can this lead to a NULL pointer dereference during device probe?

[Severity: High]
This is a pre-existing issue, but does the driver hold complete_lock while
invoking client callbacks?

In switchtec_dma_cleanup_completed(), the lock is acquired:

drivers/dma/switchtec_dma.c:switchtec_dma_cleanup_completed() {
    spin_lock_bh(&swdma_chan->complete_lock);
    ...
    dmaengine_desc_get_callback_invoke(&desc->txd, &res);
    ...
}

The same pattern appears in switchtec_dma_abort_desc().

If the client callback calls back into the DMA engine API (such as 
dmaengine_terminate_all()), could this cause a self-deadlock when the 
driver attempts to acquire complete_lock again?

[Severity: High]
This is a pre-existing issue, but is it safe to request the channel status
IRQ before the channels list is initialized?

In switchtec_dma_create(), pci_request_irq() is called before the channel
list is populated:

drivers/dma/switchtec_dma.c:switchtec_dma_create() {
    ...
    rc = pci_request_irq(pdev, irq, NULL, switchtec_dma_chan_status_isr,
                         swdma_dev, KBUILD_MODNAME);
    ...
    chan_cnt = switchtec_dma_chans_enumerate(swdma_dev, pdev, chan_cnt);
    ...
}

If an interrupt fires early, switchtec_dma_chan_status_isr() will iterate
over the uninitialized dma_dev->channels list:

drivers/dma/switchtec_dma.c:switchtec_dma_chan_status_isr() {
    ...
    list_for_each_entry(chan, &dma_dev->channels, device_node) {
    ...
}

Could this loop dereference a NULL pointer and crash if the list head is
completely zeroed before initialization?

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