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

[sashiko-bot@kernel.org]

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