dmaengine: fsl_raid: check fsl_re_chan_probe() return value

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

New issues:
- [High] The newly added FSL_RE_MAX_CHANS check aborts probe directly, leaking previously probed channels and causing a Use-After-Free.

Pre-existing issues:
- [High] The driver requests unmanaged IRQs but never frees them, leading to a permanent leak and Use-After-Free on module unload.
- [High] The driver never registers its DMA channels with the DMA engine framework, rendering it completely non-functional.
- [High] Channel resources are torn down before unregistering the DMA device, introducing a Use-After-Free race condition.
- [High] The interrupt handler is registered before its private data is fully initialized, causing a potential NULL pointer dereference.
- [High] The physical addresses for inbound and outbound rings are incorrectly shifted and masked, truncating 40-bit DMA addresses. --

--- Patch [1]: [PATCHv2] dmaengine: fsl_raid: check fsl_re_chan_probe() return value --- commit 12f631d0fec9947222a58ad5b976eca2ba2a47f9
Author: Rosen Penev <[email protected]>

dmaengine: fsl_raid: check fsl_re_chan_probe() return value

This patch updates fsl_re_probe() to check the return value of fsl_re_chan_probe(). It ensures that the driver only increments the channel count for successfully probed channels and avoids dereferencing NULL pointers during device removal.

Quoted 7 lines

drivers/dma/fsl_raid.c +0 -0

index 99945845d8b5e..052218b224943 100644
@@ -832,10 +832,26 @@ static int fsl_re_probe(struct platform_device *ofdev)
 		for_each_child_of_node(np, child) {
 			rc = of_device_is_compatible(child,

drivers/dma/fsl_raid.c static int fsl_re_probe(struct platform_device *ofdev) · line 843

Quoted 10 lines

drivers/dma/fsl_raid.c +9 -0

 					     "fsl,raideng-v1.0-job-ring");
+			if (!rc)
+				continue;
+
+			if (ridx >= FSL_RE_MAX_CHANS) {
+				dev_err(dev,
+					"too many job rings, max %d\n",
+					FSL_RE_MAX_CHANS);
+				of_node_put(child);
+				return -EINVAL;

[Severity: High]
Does returning directly here leak previously probed channels?

If the device tree contains more than FSL_RE_MAX_CHANS (4) job rings, this aborts the probe and frees devm-managed memory. However, successfully probed channels leave behind unmanaged resources (like dma_pool_alloc() and request_irq()), and the hardware remains enabled.

Could this lead to a use-after-free if an interrupt fires and the ISR dereferences the freed channel structure?

drivers/dma/fsl_raid.c static int fsl_re_probe(struct platform_device *ofdev) · line 855

Quoted 4 lines

drivers/dma/fsl_raid.c +3 -0

+			}
+
+			rc = fsl_re_chan_probe(ofdev, child, ridx, off);

[Severity: High]
This is a pre-existing issue, but does the driver ever register its DMA channels with the DMA engine framework?

Looking at fsl_re_chan_probe(), the channel is configured but is never added to dma_dev->channels (e.g., via vchan_init() or list_add_tail()). Because dma_async_device_register() iterates over an empty list, it registers a DMA device with zero channels, rendering the driver unusable.

[Severity: High]
This is a pre-existing issue, but does fsl_re_chan_probe() register the interrupt handler before its private data is fully initialized?

drivers/dma/fsl_raid.c:fsl_re_chan_probe() {
    ...
    ret = request_irq(chan->irq, fsl_re_isr, 0, chan->name, chandev);
    ...
    dev_set_drvdata(chandev, chan);
    ...
}

If an interrupt fires between request_irq() and dev_set_drvdata(), wouldn't fsl_re_isr() receive NULL when calling dev_get_drvdata(chandev) and crash when dereferencing it?

[Severity: High]
This is a pre-existing issue, but are the physical addresses for inbound and outbound rings incorrectly shifted in fsl_re_chan_probe()?

drivers/dma/fsl_raid.c:fsl_re_chan_probe() {
    ...
    out_be32(&chan->jrregs->inbring_base_h,
             chan->inb_phys_addr & FSL_RE_ADDR_BIT_MASK);
    ...
    out_be32(&chan->jrregs->inbring_base_l,
             chan->inb_phys_addr >> FSL_RE_ADDR_BIT_SHIFT);
    ...
}

Assuming FSL_RE_ADDR_BIT_MASK is a lower-bit mask and FSL_RE_ADDR_BIT_SHIFT is 4, inbring_base_h receives the lowest 4 bits instead of the upper bits, and inbring_base_l receives the address shifted right by 4.

Doesn't this truncate 40-bit DMA addresses and cause IOMMU faults or memory corruption?

Quoted 12 lines

drivers/dma/fsl_raid.c +6 -2

 			if (rc) {
-				fsl_re_chan_probe(ofdev, child, ridx++, off);
-				re_priv->total_chans++;
+				dev_err(dev,
+					"job ring %d probe failed: %d\n",
+					ridx, rc);
+				continue;
 			}
+			ridx++;
+			re_priv->total_chans++;
 		}
 	}

[ ... ]

drivers/dma/fsl_raid.c static void fsl_re_remove(struct platform_device *ofdev) · line 886

Quoted 7 lines

drivers/dma/fsl_raid.c +2 -1

@@ -866,7 +882,8 @@ static void fsl_re_remove(struct platform_device *ofdev)
 
 	/* Cleanup chan related memory areas */
 	for (i = 0; i < re_priv->total_chans; i++)
-		fsl_re_remove_chan(re_priv->re_jrs[i]);
+		if (re_priv->re_jrs[i])
+			fsl_re_remove_chan(re_priv->re_jrs[i]);

[Severity: High]
This is a pre-existing issue, but does fsl_re_remove_chan() leak the unmanaged IRQ?

The interrupt is allocated with request_irq() during probe, but neither the probe error path nor fsl_re_remove_chan() ever calls free_irq().

Could this cause a use-after-free on module unload if the hardware asserts an interrupt after the devm structures are freed?

drivers/dma/fsl_raid.c static void fsl_re_remove(struct platform_device *ofdev) · line 898

Quoted 3 lines

drivers/dma/fsl_raid.c +0 -0

 	/* Unregister the driver */
 	dma_async_device_unregister(&re_priv->dma_dev);

[Severity: High]
This is a pre-existing issue, but does fsl_re_remove() tear down channel resources before unregistering the DMA device?

By freeing the DMA pools and killing tasklets before calling dma_async_device_unregister(), couldn't concurrent DMA clients or core subsystems access already-freed DMA pools?

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