dmaengine: fsl_raid: check fsl_re_chan_probe() return value
PATCH v7 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 guard fsl_re_remove() against NULL entries.
---
v7: handle differently based on sashiko review
v6: increment ridx before continue;
v5: put some stuff on one line for easier readability.
v4: use break;
v3: fix sashiko review
v2: fix description
drivers/dma/fsl_raid.c | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-)
drivers/dma/fsl_raid.c +21 -10
@@ -687,7 +687,6 @@ static int fsl_re_chan_probe(struct platform_device *ofdev, goto err_free; } - re_priv->re_jrs[q] = chan; chan->chan.device = dma_dev; chan->chan.private = chan; chan->dev = chandev;
@@ -741,6 +740,11 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,/* Enable RE/CHAN */ out_be32 (&chan->jrregs->jr_command,FSL_RE_ENABLE ); +/* Only record the channel once it is fully initialized, so the +* cleanup paths never touch a partially-probed ring. +*/ + re_priv->re_jrs[q] = chan; +return 0 ; err_free_1:
@@ -836,18 +840,24 @@ 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) { - rc =of_device_is_compatible (child, -"fsl,raideng-v1.0-job-ring" ); -if (rc) { -fsl_re_chan_probe (ofdev, child, ridx++, off); - re_priv->total_chans++; +if (!of_device_is_compatible (child,"fsl,raideng-v1.0-job-ring" )) +continue ; + +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 =fsl_re_chan_probe (ofdev, child, ridx, off); +if (rc) +dev_err (dev,"job ring %d probe failed: %d\n" , ridx, rc); + re_priv->total_chans++; + ridx++; } } -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 +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]);/* Unregister the driver */ dma_async_device_unregister (&re_priv->dma_dev);
Signature · 1 line
2.55.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] dmaengine: fsl_raid: Use-After-Free due to missing unwind on dma_async_device_register failure --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dmaengine: fsl_raid: Use-After-Free due to missing unwind on dma_async_device_register failure --
commit d02780e2d0d4fd4ead4e7e3dafdee9eccab11cf0
Author: Rosen Penev <[email protected]>
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 guard fsl_re_remove() against NULL entries.
Quoted 4 lines
drivers/dma/fsl_raid.c +0
-0
[ ... ]
Quoted 20 lines
drivers/dma/fsl_raid.c +10
-5@@ -828,18 +832,24 @@ 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) { - rc =of_device_is_compatible (child, -"fsl,raideng-v1.0-job-ring" ); -if (rc) { -fsl_re_chan_probe (ofdev, child, ridx++, off); - re_priv->total_chans++; +if (!of_device_is_compatible (child,"fsl,raideng-v1.0-job-ring" )) +continue ; + +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 =fsl_re_chan_probe (ofdev, child, ridx, off); +if (rc)
drivers/dma/fsl_raid.c static int fsl_re_probe(struct platform_device *ofdev) · line 852
Quoted 10 lines
drivers/dma/fsl_raid.c +4
-3+dev_err (dev,"job ring %d probe failed: %d\n" , ridx, rc); + re_priv->total_chans++; + ridx++; } } -dma_async_device_register (dma_dev); - -return 0 ; +return dma_async_device_register (dma_dev);
[Severity: High]
Does propagating the error from dma_async_device_register() leave the hardware interrupts active?
If dma_async_device_register() fails, the probe function now returns the error and aborts. The driver core will then automatically free all devm-managed memory, including the chan structures allocated in fsl_re_chan_probe().
However, because fsl_re_chan_probe() requests a hardware IRQ via a non-devm request_irq() without any unwind path in fsl_re_probe() to call free_irq(), the hardware interrupts will remain active.
If the hardware asserts an interrupt after the probe has failed and the memory is freed, fsl_re_isr() could be invoked, which would dereference the freed chan pointer and schedule a freed tasklet, causing a use-after-free and kernel crash.
This isn't a bug introduced by this patch, but there is also a pre-existing early return in fsl_re_probe() that demonstrates the same lack of channel cleanup:
drivers/dma/fsl_raid.c:fsl_re_probe() {
...
rc = of_property_read_u32(np, "reg", &off);
if (rc) {
dev_err(dev, "Reg property not found in JQ node\n");
return -ENODEV;
}
...
}Should fsl_re_probe() provide a cleanup path to manually free the interrupts and other resources if dma_async_device_register() or subsequent job queue probes fail?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1