Re: [PATCH v1 3/6] dmaengine: switchtec-dma: fix channel leak on registration failure

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

New issues:
- [High] The patch introduces a Use-After-Free in the error path by freeing swdma_chan structures while the global chan_status_irq is still registered and active.

Pre-existing issues:
- [High] Use-after-free on swdma_dev during device removal.
- [High] Preexisting Use-After-Free on dma_async_device_register() failure due to chan->dev being freed while the IRQ is active.
- [High] Failure of ioremap() is not checked, leading to an immediate null pointer dereference.
- [Medium] Leaked channel structures during partial enumeration failure in switchtec_dma_chans_enumerate().
- [Medium] Missing put_device() for the DMA parent device on dma_async_device_register() failure.
--

--- Patch [3]: [PATCH v1 3/6] dmaengine: switchtec-dma: fix channel leak on registration failure ---
commit 7464b8c763dc5bb4e72b11ab5698233a70070171
Author: Logan Gunthorpe <[email protected]>

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

If dma_async_device_register() fails during probe,
switchtec_dma_chans_release() only stops hardware and frees IRQs and
tasklets; it never frees the per-channel swdma_chan structures or the
swdma_chans array itself.

[ ... ]

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

> --- a/drivers/dma/switchtec_dma.c
> +++ b/drivers/dma/switchtec_dma.c

[ ... ]

> @@ -1317,6 +1317,11 @@ static int switchtec_dma_create(struct pci_dev *pdev)
>  err_chans_release_exit:
>  	switchtec_dma_chans_release(pdev, swdma_dev);
>  
> +	for (i = 0; i < swdma_dev->chan_cnt; i++)
> +		kfree(swdma_dev->swdma_chans[i]);
> +

[Severity: High]
Could this newly introduced kfree() loop cause a use-after-free?

Since chan_status_irq is registered earlier and remains active until
free_irq() is called in the err_exit block below, an interrupt could fire
while this error path is executing.

If the status interrupt fires after these structures are freed,
switchtec_dma_chan_status_isr() will iterate over dma_dev->channels and
dereference the freed swdma_chan structures.

Should the IRQ be freed, or the channels removed from the list, before we
free this memory?

> +	kfree(swdma_dev->swdma_chans);
> +
>  err_exit:
>  	if (swdma_dev->chan_status_irq)
>  		free_irq(swdma_dev->chan_status_irq, swdma_dev);


While reviewing this patch, I also noticed a few pre-existing issues in
this driver:

[Severity: Medium]
This is a pre-existing issue, but does switchtec_dma_chans_enumerate()
leak memory on its error path?

If switchtec_dma_chan_init() fails, the error handling cleans up the array
but does not appear to free the individual swdma_chan elements:

drivers/dma/switchtec_dma.c:switchtec_dma_chans_enumerate() {
    ...
	for (i = 0; i < chan_cnt; i++)
		switchtec_dma_chan_free(pdev, swdma_dev->swdma_chans[i]);

	kfree(swdma_dev->swdma_chans);
    ...
}

Since switchtec_dma_chan_free() only disables hardware and frees IRQs,
the memory allocated for the channel structures via kzalloc_obj() seems
to be permanently leaked here.


[Severity: Medium]
This isn't a bug introduced by this patch, but is there a reference leak if
dma_async_device_register() fails in switchtec_dma_create()?

The driver acquires a reference to the parent PCI device:

drivers/dma/switchtec_dma.c:switchtec_dma_create() {
    ...
	dma->dev = get_device(&pdev->dev);
    ...
}

If registration fails and the code jumps to err_chans_release_exit,
swdma_dev is eventually freed without calling put_device(dma->dev).
Because the device failed registration, its release callback isn't
invoked, which appears to leave the parent PCI device reference leaked.


[Severity: High]
This is a pre-existing issue, but can switchtec_dma_remove() trigger a
use-after-free when unmapping the bar?

When unregistering the DMA device, if there are no outstanding channel
references, the core synchronously invokes the release callback
(switchtec_dma_release) which frees swdma_dev.

drivers/dma/switchtec_dma.c:switchtec_dma_remove() {
    ...
	dma_async_device_unregister(&swdma_dev->dma_dev);

	iounmap(swdma_dev->bar);
    ...
}

Since iounmap() dereferences swdma_dev->bar immediately after the
unregister call may have freed swdma_dev, could this lead to a crash
during device removal?


[Severity: High]
This isn't a bug introduced by this patch, but is there another
use-after-free risk during dma_async_device_register() failure?

If registration fails after partially registering channels, the DMA core
automatically frees the chan->dev structures via device_unregister().

Because chan_status_irq remains active at this point, a spurious or
concurrent interrupt can fire:

drivers/dma/switchtec_dma.c:switchtec_dma_chan_status_isr() {
    ...
	chan_dev = &swdma_chan->dma_chan.dev->device;
    ...
}

This unconditionally dereferences the core-freed chan->dev structure.
Could this cause a crash if the interrupt fires before free_irq() is
reached in the error path?


[Severity: High]
This is a pre-existing issue, but does switchtec_dma_create() safely
handle ioremap() failures?

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

Since ioremap() can return null, skipping a validation check here
seems to guarantee a null pointer dereference on the very next readw()
if the mapping fails.

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