Re: [PATCH v1 1/6] dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc()

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

Pre-existing issues:
- [Medium] Missing cleanup in the error paths of switchtec_dma_alloc_chan_resources() leads to memory leaks and overwritten DMA allocations.
- [Medium] Leaked switchtec_dma_chan objects and swdma_chans array when device enumeration or registration fails.
--

--- Patch [1]: [PATCH v1 1/6] dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc() ---
commit c1fc554811fc177ea9480bb2fbbc31e95646cd59
Author: Logan Gunthorpe <[email protected]>

dmaengine: switchtec-dma: fix double-free in switchtec_dma_free_desc()

This commit nulls out each pointer as it is freed in switchtec_dma_free_desc()
so a subsequent call is a no-op for anything already released. This prevents
a double-free if the function is called twice.

> switchtec_dma_free_desc() frees swdma_chan->hw_sq, hw_cq, and every
> desc_ring[] entry without clearing the pointers afterward. If
> switchtec_dma_alloc_chan_resources() fails partway through and calls
> it during unwind, then a later retry of alloc_chan_resources() fails

[Severity: Medium]
This is a pre-existing issue, but does switchtec_dma_alloc_chan_resources()
actually call switchtec_dma_free_desc() during unwind?

Looking at switchtec_dma_alloc_chan_resources():

drivers/dma/switchtec_dma.c:switchtec_dma_alloc_chan_resources() {
	rc = switchtec_dma_alloc_desc(swdma_chan);
	if (rc)
		return rc;

	rc = enable_channel(swdma_chan);
	if (rc)
		return rc;

	rc = reset_channel(swdma_chan);
	if (rc)
		return rc;
    ...
}

If enable_channel(), reset_channel(), unhalt_channel(), or rcu_dereference()
fail, the function returns an error directly without freeing the DMA coherent
memory and descriptor rings that were just allocated by
switchtec_dma_alloc_desc().

Because the dmaengine framework does not automatically call
device_free_chan_resources() when device_alloc_chan_resources() fails,
the driver must handle its own cleanup.

Furthermore, if a client or the driver core retries the channel allocation,
switchtec_dma_alloc_desc() will blindly overwrite the dangling pointers with
new allocations, permanently orphaning the previously allocated memory.

Does this need an explicit cleanup path on error?

> in switchtec_dma_alloc_desc() before reallocating one of those
> pointers, its own failure path calls switchtec_dma_free_desc() again
> and frees the same, already-freed pointers a second time.

[ ... ]

While reviewing this code, I noticed another pre-existing issue.

[Severity: Medium]
Does the driver leak the switchtec_dma_chan objects during probe errors?

Looking at switchtec_dma_chans_enumerate():

drivers/dma/switchtec_dma.c:switchtec_dma_chans_enumerate() {
	for (i = 0; i < chan_cnt; i++) {
		rc = switchtec_dma_chan_init(swdma_dev, pdev, i);
		if (rc) {
			dev_err(&pdev->dev, "Channel %d: init channel failed\n",
				i);
			chan_cnt = i;
			goto err_exit;
		}
	}

	return chan_cnt;

err_exit:
	for (i = 0; i < chan_cnt; i++)
		switchtec_dma_chan_free(pdev, swdma_dev->swdma_chans[i]);

	kfree(swdma_dev->swdma_chans);

	return rc;
}

The err_exit loop calls switchtec_dma_chan_free() which stops the hardware
but fails to free the actual swdma_chan memory before freeing the array.
switchtec_dma_chan_free() does not call kfree(swdma_chan).

Additionally, in switchtec_dma_create(), if dma_async_device_register()
fails:

drivers/dma/switchtec_dma.c:switchtec_dma_create() {
    ...
err_chans_release_exit:
	switchtec_dma_chans_release(pdev, swdma_dev);

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

	iounmap(swdma_dev->bar);
	kfree(swdma_dev);
	return rc;
}

It frees swdma_dev but leaks both the swdma_dev->swdma_chans array itself
and the individual channel structures within it, because
switchtec_dma_chans_release() also just calls switchtec_dma_chan_free().

Could these error paths result in a memory leak?

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