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

[PATCH] 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.

Propagate the error, 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>
---
 drivers/dma/fsl_raid.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
index bfaef6245695..0b0b4e8fc821 100644
--- a/drivers/dma/fsl_raid.c
+++ b/drivers/dma/fsl_raid.c
@@ -842,7 +842,14 @@ static int fsl_re_probe(struct platform_device *ofdev)
 			rc = of_device_is_compatible(child,
 					     "fsl,raideng-v1.0-job-ring");
 			if (rc) {
-				fsl_re_chan_probe(ofdev, child, ridx++, off);
+				rc = fsl_re_chan_probe(ofdev, child, ridx, off);
+				if (rc) {
+					dev_err(dev,
+						"job ring %d probe failed: %d\n",
+						ridx, rc);
+					continue;
+				}
+				ridx++;
 				re_priv->total_chans++;
 			}
 		}
@@ -875,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]);
 
 	/* Unregister the driver */
 	dma_async_device_unregister(&re_priv->dma_dev);
-- 
2.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver fails to add its initialized DMA channels to the `dma_dev->channels` list, meaning the dmaengine framework sees zero channels for this device.
- [High] The driver requests an interrupt handler using `request_irq()` but never calls `free_irq()`, leading to a Use-After-Free on device removal or a dangling IRQ if probe fails.
- [High] Interrupt is requested before channel driver data is initialized, leading to a race condition and NULL pointer dereference.
- [High] The driver blindly writes to the fixed-size `re_jrs` array based on the number of child nodes in the device tree, leading to a buffer overflow.
- [High] Failure midway through `fsl_re_probe()` leaks resources for successfully probed channels, preventing the release of devm managed DMA pools.
- [High] Recursive spinlock deadlock during DMA callback invocation.
- [High] Missing endianness conversions when writing to hardware descriptors and CDBs.
--

--- Patch [1]: [PATCH] dmaengine: fsl_raid: check fsl_re_chan_probe() return value ---
commit d2ed6dc415b8714a0178137e3830a2d57f27aa00
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: fsl_raid: check fsl_re_chan_probe() return value

This patch fixes a NULL pointer dereference in fsl_re_remove() by properly
checking the return value of fsl_re_chan_probe(). It ensures that only
successfully probed channels increment the total_chans count and guards
against NULL entries during cleanup.

> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 99945845d8b5e..6266ef1e6f098 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -833,7 +833,14 @@ static int fsl_re_probe(struct platform_device *ofdev)
>  			rc = of_device_is_compatible(child,
>  					     "fsl,raideng-v1.0-job-ring");
>  			if (rc) {
> -				fsl_re_chan_probe(ofdev, child, ridx++, off);
> +				rc = fsl_re_chan_probe(ofdev, child, ridx, off);

[Severity: High]
This is a pre-existing issue, but can the array re_jrs[] overflow here?

If the device tree provides more than FSL_RE_MAX_CHANS (4) job rings, ridx
will grow beyond the bounds of the array, leading to memory corruption when
fsl_re_chan_probe() executes:

drivers/dma/fsl_raid.c:fsl_re_chan_probe() {
    ...
    re_priv->re_jrs[q] = chan;
    ...
}

> +				if (rc) {
> +					dev_err(dev,
> +						"job ring %d probe failed: %d\n",
> +						ridx, rc);
> +					continue;

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for a
dangling interrupt to remain active after an error?

If fsl_re_chan_probe() fails after successfully calling request_irq(), the
interrupt is never freed because there is no free_irq() in the error handling
paths. The continue statement allows the driver to stay loaded while a dangling
IRQ remains permanently active, potentially causing a use-after-free if
triggered.

This also happens during device removal:

drivers/dma/fsl_raid.c:fsl_re_remove_chan() {
    tasklet_kill(&chan->irqtask);

    dma_pool_free(chan->re_dev->hw_desc_pool, chan->inb_ring_virt_addr,
                  chan->inb_phys_addr);
    ...
}

Could a hardware interrupt firing after fsl_re_remove_chan() destroys the DMA
rings cause the ISR to access freed memory?

[ ... ]

> @@ -866,7 +873,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);

[Severity: High]
This is a pre-existing issue, but are the channels successfully registered
with the dmaengine framework here?

drivers/dma/fsl_raid.c:fsl_re_chan_probe() {
    ...
    chan->chan.device = dma_dev;
    chan->chan.private = chan;
    chan->dev = chandev;
    ...
}

Since list_add_tail(&chan->chan.device_node, &dma_dev->channels) and
dma_cookie_init() are completely omitted, the framework sees zero channels
for this device, making it unusable by clients. Should they be added?

[Severity: High]
This is a pre-existing issue, but could an interrupt firing during this
window cause a NULL pointer dereference?

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);
    ...
}

The interrupt is requested before channel driver data is initialized. If an
interrupt fires in this small window, or if probe fails and leaves the IRQ
active, the ISR blindly dereferences the NULL pointer:

drivers/dma/fsl_raid.c:fsl_re_isr() {
    ...
    re_chan = dev_get_drvdata((struct device *)data);
    irqstate = in_be32(&re_chan->jrregs->jr_interrupt_status);
    ...
}

[Severity: High]
This is a pre-existing issue, but might this error path leak resources for
already initialized channels?

drivers/dma/fsl_raid.c:fsl_re_probe() {
    ...
    for_each_compatible_node_scoped(np, NULL, "fsl,raideng-v1.0-job-queue") {
        rc = of_property_read_u32(np, "reg", &off);
        if (rc) {
            dev_err(dev, "Reg property not found in JQ node\n");
            return -ENODEV;
        }
    ...
}

If of_property_read_u32() fails after some channels were already successfully
probed, it returns -ENODEV directly without calling fsl_re_remove_chan() for
the initialized channels, leaking devm DMA pool memory and leaving IRQs active.

[Severity: High]
This is a pre-existing issue, but could this pattern lead to a recursive
spinlock deadlock?

In fsl_re_dequeue(), the driver holds desc_lock while invoking the client's
DMA callback:

drivers/dma/fsl_raid.c:fsl_re_dequeue() {
    ...
    if (found) {
        fsl_re_desc_done(desc);
        list_move_tail(&desc->node, &re_chan->ack_q);
    }
    ...
}

If the client submits a new transaction from this callback, which is a
standard dmaengine pattern, it will attempt to acquire the same lock:

drivers/dma/fsl_raid.c:fsl_re_tx_submit() {
    ...
    spin_lock_irqsave(&re_chan->desc_lock, flags);
    ...
}

Would this result in an immediate deadlock on the same CPU?

[Severity: High]
This is a pre-existing issue, but is an endianness conversion required for
these hardware descriptor assignments?

drivers/dma/fsl_raid.c:fsl_re_init_desc() {
    ...
    desc->hwdesc.fmt32 = FSL_RE_FRAME_FORMAT << FSL_RE_HWDESC_FMT_SHIFT;
    desc->hwdesc.lbea32 = upper_32_bits(paddr);
    desc->hwdesc.addr_low = lower_32_bits(paddr);
    ...
}

And similarly in fill_cfd_frame():

drivers/dma/fsl_raid.c:fill_cfd_frame() {
    ...
    cf[index].efrl32 = efrl;
    cf[index].addr_high = upper_32_bits(addr);
    cf[index].addr_low = lower_32_bits(addr);
    ...
}

Host-endian values are assigned directly to __be32 hardware descriptor fields.
Could this result in corrupted hardware descriptors being sent to the device
on little-endian architectures?

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