Skip to contentSkip to conversation

dmaengine: fsl_raid: check fsl_re_chan_probe() return value

PATCH Superseded Rosen Penev 4 participants 4 messages Sep 10 – Sep 15 dmaengine

v4 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Rosen Penev

author
#0 lore raw

PATCH v4 1/1 dmaengine: fsl_raid: check fsl_re_chan_probe() return value

fsl_re_probe() ignores the return value of fsl_re_chan_probe() and unconditionally increments total_chans. When a channel fails to probe (for example, an IRQ mapping failure) its re_jrs[] slot is left NULL, yet total_chans still advances, so fsl_re_remove_chan() later dereferences the NULL pointer during device removal.

Check return value and only count successfully probed channels, and guard fsl_re_remove() against NULL entries.

Fixes ad80da658bbc ("dmaengine: Driver support for FSL RaidEngine device.")
Assisted-by opencode:hy3-free
Signed-off-by Rosen Penev <[email protected]>

---
v4: use break;
v3: fix sashiko review
v2: fix description

 drivers/dma/fsl_raid.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)
drivers/dma/fsl_raid.c +21 -6
index 9cc289f7129e..6167fa652e96 100644
@@ -836,18 +836,32 @@ static int fsl_re_probe(struct platform_device *ofdev)
 		}
 		/* Find out the Job Rings present under each JQ */
 		for_each_child_of_node(np, child) {
+			if (ridx >= FSL_RE_MAX_CHANS) {
+				dev_warn(dev,
+					"too many job rings, max %d\n",
+					FSL_RE_MAX_CHANS);
+				of_node_put(child);
+				break;
+			}
+
 			rc = of_device_is_compatible(child,
 					     "fsl,raideng-v1.0-job-ring");
+			if (!rc)
+				continue;
+
+			rc = fsl_re_chan_probe(ofdev, child, ridx, off);
 			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++;
 		}
 	}
 
-	dma_async_device_register(dma_dev);
-
-	return 0;
+	return dma_async_device_register(dma_dev);
 }
 
 static void fsl_re_remove_chan(struct fsl_re_chan *chan)

@@ -872,7 +886,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]);
 
 	/* Unregister the driver */
 	dma_async_device_unregister(&re_priv->dma_dev);
Signature · 1 line
2.55.0
AI review
#1 lore raw
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

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

Pre-existing issues:
- [High] The driver never frees requested IRQs or unregisters child platform devices, causing resource leaks and use-after-free on error paths and driver unbind.
- [High] Returning an error from dma_async_device_register() or early returns in fsl_re_probe() fail to unwind successfully probed channels, causing resource leaks and UAF.
- [High] DMA channels are never added to the DMA device channels list, rendering the driver non-functional.
- [High] Interrupt is requested before private data and hardware are fully initialized, risking a NULL pointer dereference in the interrupt handler.
- [High] Channel resources are destroyed before the DMA device is unregistered, allowing concurrent use-after-free.
--

commit df14fae70b7983f30255454748eb310d0c4f9ad9
Author: Rosen Penev <[email protected]>

dmaengine: fsl_raid: check fsl_re_chan_probe() return value

The commit fixes a potential NULL pointer dereference in fsl_re_remove_chan() by checking the return value of fsl_re_chan_probe() during device probe and guarding against NULL entries during removal.

Quoted 12 lines
drivers/dma/fsl_raid.c +4 -0
index 2d86f61105e52..66b7ff0f31f70 100644
@@ -828,18 +828,32 @@ static int fsl_re_probe(struct platform_device *ofdev)
 		}
 		/* Find out the Job Rings present under each JQ */
 		for_each_child_of_node(np, child) {
+			if (ridx >= FSL_RE_MAX_CHANS) {
+				dev_warn(dev,
+					"too many job rings, max %d\n",
+					FSL_RE_MAX_CHANS);

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

Quoted 10 lines
drivers/dma/fsl_raid.c +7 -0
+				of_node_put(child);
+				break;
+			}
+
 			rc = of_device_is_compatible(child,
 					     "fsl,raideng-v1.0-job-ring");
+			if (!rc)
+				continue;
+
 			rc = fsl_re_chan_probe(ofdev, child, ridx, off);

[Severity: High]
This is a pre-existing issue, but does fsl_re_chan_probe() leak resources on failure and driver unbind?

In fsl_re_chan_probe(), a platform device is created and an interrupt is requested without devres management:

    chan_ofdev = of_platform_device_create(np, NULL, dev);
    ...
    ret = request_irq(chan->irq, fsl_re_isr, 0, chan->name, chandev);

The error path just returns without unwinding these:

err_free:

    return ret;

Furthermore, fsl_re_remove_chan() destroys the tasklet but does not free the interrupt or unregister the child device. Since devres will automatically free the chan structure on unbind, wouldn't the leaked active interrupt cause a use-after-free if it fires and accesses the freed chan structure?

[Severity: High]
This is another pre-existing issue, but are the channels ever linked to the device's channels list?

fsl_re_chan_probe() sets the device pointer:

    chan->chan.device = dma_dev;
    chan->chan.private = chan;

However, it seems to miss linking the channel into the parent device's channel list. When dma_async_device_register() is called later, wouldn't it see an empty channels list and register zero channels, rendering the driver non-functional?

[Severity: High]
This is a pre-existing issue, but is there a race condition between requesting the interrupt and setting the driver data?

fsl_re_chan_probe() requests the interrupt before setting the driver data:

    ret = request_irq(chan->irq, fsl_re_isr, 0, chan->name, chandev);
    ...
    dev_set_drvdata(chandev, chan);

If the interrupt fires in this window, fsl_re_isr() will read a NULL pointer from dev_get_drvdata() and unconditionally dereference it:

    re_chan = dev_get_drvdata((struct device *)data);
    irqstate = in_be32(&re_chan->jrregs->jr_interrupt_status);

Can this lead to a NULL pointer dereference panic?

Quoted 7 lines
drivers/dma/fsl_raid.c +4 -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;

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

Quoted 10 lines
drivers/dma/fsl_raid.c +3 -3
 			}
+			ridx++;
+			re_priv->total_chans++;
 		}
 	}
 
-	dma_async_device_register(dma_dev);
-
-	return 0;
+	return dma_async_device_register(dma_dev);

[Severity: High]
This is a pre-existing issue, but if dma_async_device_register() fails, are the successfully probed channels abandoned?

When dma_async_device_register() returns an error, the function exits without calling fsl_re_remove() to clean up any channels initialized during earlier job queue iterations.

Since the interrupts for these channels are not devres-managed, they would remain active. Once devres frees the associated chan memory, could an interrupt firing trigger a use-after-free crash?

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

Quoted 13 lines
drivers/dma/fsl_raid.c +2 -1
 }
 
 static void fsl_re_remove_chan(struct fsl_re_chan *chan)

@@ -864,7 +878,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]);
 
 	/* Unregister the driver */
 	dma_async_device_unregister(&re_priv->dma_dev);

[Severity: High]
This is a pre-existing issue, but are channel resources destroyed prematurely during driver unbind?

fsl_re_remove_chan() is called for each channel, killing tasklets and freeing dma pools:

    for (i = 0; i < re_priv->total_chans; i++)
        if (re_priv->re_jrs[i])
            fsl_re_remove_chan(re_priv->re_jrs[i]);

Afterward, the device is unregistered:

    dma_async_device_unregister(&re_priv->dma_dev);

Since the subsystem still considers the device active until unregistration completes, could concurrent or pending requests attempt to use the already freed pools or schedule the killed tasklets?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Reviewed-by on v4 1/1
#2 lore raw
In reply to Rosen Penev
Quoted 6 lines · Rosen Penev

fsl_re_probe() ignores the return value of fsl_re_chan_probe() and unconditionally increments total_chans. When a channel fails to probe (for example, an IRQ mapping failure) its re_jrs[] slot is left NULL, yet total_chans still advances, so fsl_re_remove_chan() later dereferences the NULL pointer during device removal.

Quoted 7 lines

Check return value and only count successfully probed channels, and guard fsl_re_remove() against NULL entries.

Fixes ad80da658bbc ("dmaengine: Driver support for FSL RaidEngine device.")
Assisted-by opencode:hy3-free
Signed-off-by Rosen Penev <[email protected]>

---

Reviewed-by Frank Li <[email protected]>
Quoted 61 lines

v4: use break;
v3: fix sashiko review
v2: fix description

 drivers/dma/fsl_raid.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)
drivers/dma/fsl_raid.c +21 -7
index 9cc289f7129e..6167fa652e96 100644
@@ -836,18 +836,32 @@ static int fsl_re_probe(struct platform_device *ofdev)
 		}
 		/* Find out the Job Rings present under each JQ */
 		for_each_child_of_node(np, child) {
+			if (ridx >= FSL_RE_MAX_CHANS) {
+				dev_warn(dev,
+					"too many job rings, max %d\n",
+					FSL_RE_MAX_CHANS);
+				of_node_put(child);
+				break;
+			}
+
 			rc = of_device_is_compatible(child,
 					     "fsl,raideng-v1.0-job-ring");
+			if (!rc)
+				continue;
+
+			rc = fsl_re_chan_probe(ofdev, child, ridx, off);
 			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++;
 		}
 	}
 
-	dma_async_device_register(dma_dev);
-
-	return 0;
+	return dma_async_device_register(dma_dev);
 }
 
 static void fsl_re_remove_chan(struct fsl_re_chan *chan)

@@ -872,7 +886,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]);
 
 	/* Unregister the driver */
 	dma_async_device_unregister(&re_priv->dma_dev);
--

2.55.0

maintainer
#3 lore raw
In reply to Rosen Penev
Quoted 24 lines · Rosen Penev

fsl_re_probe() ignores the return value of fsl_re_chan_probe() and unconditionally increments total_chans. When a channel fails to probe (for example, an IRQ mapping failure) its re_jrs[] slot is left NULL, yet total_chans still advances, so fsl_re_remove_chan() later dereferences the NULL pointer during device removal.

Check return value and only count successfully probed channels, and guard fsl_re_remove() against NULL entries.

Fixes ad80da658bbc ("dmaengine: Driver support for FSL RaidEngine device.")
Assisted-by opencode:hy3-free
Signed-off-by Rosen Penev <[email protected]>

---
v4: use break;
v3: fix sashiko review
v2: fix description

 drivers/dma/fsl_raid.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)
drivers/dma/fsl_raid.c +0 -0
index 9cc289f7129e..6167fa652e96 100644

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

Quoted 7 lines
drivers/dma/fsl_raid.c +4 -0
@@ -836,18 +836,32 @@ static int fsl_re_probe(struct platform_device *ofdev)
 		}
 		/* Find out the Job Rings present under each JQ */
 		for_each_child_of_node(np, child) {
+			if (ridx >= FSL_RE_MAX_CHANS) {
+				dev_warn(dev,
+					"too many job rings, max %d\n",
+					FSL_RE_MAX_CHANS);

what is wrong with:

                                dev_warn(dev, too many job rings, max %d\n", FSL_RE_MAX_CHANS);

exceeds 80chars but makes reading less painful!

Quoted 6 lines
drivers/dma/fsl_raid.c +4 -0
+				of_node_put(child);
+				break;
+			}
+
 			rc = of_device_is_compatible(child,
 					     "fsl,raideng-v1.0-job-ring");

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

Quoted 10 lines
drivers/dma/fsl_raid.c +7 -2
+			if (!rc)
+				continue;
+
+			rc = fsl_re_chan_probe(ofdev, child, ridx, off);
 			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);

here as well

Quoted 26 lines
drivers/dma/fsl_raid.c +6 -4
+				continue;
 			}
+			ridx++;
+			re_priv->total_chans++;
 		}
 	}
 
-	dma_async_device_register(dma_dev);
-
-	return 0;
+	return dma_async_device_register(dma_dev);
 }
 
 static void fsl_re_remove_chan(struct fsl_re_chan *chan)

@@ -872,7 +886,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]);
 
 	/* Unregister the driver */
 	dma_async_device_unregister(&re_priv->dma_dev);
Signature · 1 line
2.55.0
Signature · 1 line
~Vinod