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

[PATCHv2] 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: opencode:hy3-free
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: fix description
 drivers/dma/fsl_raid.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
index bfaef6245695..30aaca81d855 100644
--- a/drivers/dma/fsl_raid.c
+++ b/drivers/dma/fsl_raid.c
@@ -841,10 +841,26 @@ static int fsl_re_probe(struct platform_device *ofdev)
 		for_each_child_of_node(np, child) {
 			rc = of_device_is_compatible(child,
 					     "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;
+			}
+
+			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++;
 		}
 	}
 
@@ -875,7 +891,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: [PATCHv2] dmaengine: fsl_raid: check fsl_re_chan_probe() return value

[sashiko-bot]

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

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.

> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 99945845d8b5e..052218b224943 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -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,
>  					     "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?

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

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

[ ... ]

> @@ -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?

>  
>  	/* 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/20260720202040.2531311-1-rosenp@gmail.com?part=1

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

[Frank Li]

On Mon, Jul 20, 2026 at 08:33:38PM +0000, sashiko-bot@kernel.org wrote:
> 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 <rosenp@gmail.com>
>
> 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.
>
> > diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> > index 99945845d8b5e..052218b224943 100644
> > --- a/drivers/dma/fsl_raid.c
> > +++ b/drivers/dma/fsl_raid.c
> > @@ -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,
> >  					     "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?

make sense,

break loop instead of return error here should be good solution, you can
print warning to said dts provide too much job rings.

So previous scaned ring should work.

Frank
>
> > +			}
> > +
> > +			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?
>
> >  			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++;
> >  		}
> >  	}
>
> [ ... ]
>
> > @@ -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?
>
> >
> >  	/* 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/20260720202040.2531311-1-rosenp@gmail.com?part=1