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

[Frank Li <Frank.li@oss.nxp.com>]

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