dmaengine: fsl_raid: check fsl_re_chan_probe() return value
PATCH v5 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.
---
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 | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-)
drivers/dma/fsl_raid.c +18 -8
@@ -836,18 +836,27 @@ 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 (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 +881,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: orphaning partially probed channels causes UAF and leaks --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dmaengine: fsl_raid: orphaning partially probed channels causes UAF and leaks --
commit ee451ab6878ff5841fcdf93363ca664a0b821373
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.
Quoted 16 lines
drivers/dma/fsl_raid.c +6
-2@@ -828,18 +828,27 @@ 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 (ridx >=FSL_RE_MAX_CHANS ) { +dev_warn (dev,"too many job rings, max %d\n" ,FSL_RE_MAX_CHANS ); +of_node_put (child); +break ; + } +
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
-2+ 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 ;
[Severity: High]
Does this code risk orphaning a partially probed channel, leading to a use-after-free?
If fsl_re_chan_probe() in drivers/dma/fsl_raid.c fails late during its execution (for example, if a DMA pool allocation fails), it will have already assigned:
re_priv->re_jrs[ridx] = chan;
and registered an IRQ handler and a tasklet.
By calling continue without incrementing ridx, the next loop iteration in fsl_re_probe() will overwrite re_priv->re_jrs[ridx]. This leaves the partially initialized channel orphaned, meaning its tasklet will not be killed during driver teardown. When devres eventually frees the chan structure, an active tasklet or IRQ will access the freed memory.
drivers/dma/fsl_raid.c static int fsl_re_probe(struct platform_device *ofdev) · line 859
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 isn't a bug introduced by this patch, but does returning this error code expose a use-after-free if dma_async_device_register() fails?
If dma_async_device_register() fails, fsl_re_probe() in drivers/dma/fsl_raid.c now returns the error code, which triggers devres cleanup and frees the chan structures. However, since the driver allocates IRQs using the non-devm request_irq() in fsl_re_chan_probe() and lacks an error unwinding path, the IRQ handlers remain active. When a hardware interrupt fires after the probe failure, the handler will access the freed chan memory.
Quoted 1 line
drivers/dma/fsl_raid.c +0
-0}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1