[PATCHv5] dmaengine: fsl_raid: check fsl_re_chan_probe() return value

[PATCHv5] dmaengine: fsl_raid: check fsl_re_chan_probe() return value

[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: LLM
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 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(-)

diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
index 9cc289f7129e..b4355eb0098d 100644
--- a/drivers/dma/fsl_raid.c
+++ b/drivers/dma/fsl_raid.c
@@ -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);
-- 
2.55.0

Re: [PATCHv5] dmaengine: fsl_raid: check fsl_re_chan_probe() return value

[sashiko-bot]

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 <rosenp@gmail.com>

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.

> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 2d86f61105e52..8c57250ecea6a 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -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;
> +			}
> +
> +			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.

>  			}
> +			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.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915190047.86218-1-rosenp@gmail.com?part=1