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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dmaengine: fsl_raid: Use-After-Free due to missing unwind on dma_async_device_register failure
--

commit d02780e2d0d4fd4ead4e7e3dafdee9eccab11cf0
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.
    
    Check return value and guard fsl_re_remove() against NULL entries.

> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 2d86f61105e52..3530546a68e40 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
[ ... ]
> @@ -828,18 +832,24 @@ 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 (rc) {
> -				fsl_re_chan_probe(ofdev, child, ridx++, off);
> -				re_priv->total_chans++;
> +			if (!of_device_is_compatible(child, "fsl,raideng-v1.0-job-ring"))
> +				continue;
> +
> +			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 = fsl_re_chan_probe(ofdev, child, ridx, off);
> +			if (rc)
> +				dev_err(dev, "job ring %d probe failed: %d\n", ridx, rc);
> +			re_priv->total_chans++;
> +			ridx++;
>  		}
>  	}
>  
> -	dma_async_device_register(dma_dev);
> -
> -	return 0;
> +	return dma_async_device_register(dma_dev);

[Severity: High]
Does propagating the error from dma_async_device_register() leave the
hardware interrupts active?

If dma_async_device_register() fails, the probe function now returns the error
and aborts. The driver core will then automatically free all devm-managed
memory, including the chan structures allocated in fsl_re_chan_probe().

However, because fsl_re_chan_probe() requests a hardware IRQ via a non-devm
request_irq() without any unwind path in fsl_re_probe() to call free_irq(), the
hardware interrupts will remain active.

If the hardware asserts an interrupt after the probe has failed and the
memory is freed, fsl_re_isr() could be invoked, which would dereference the
freed chan pointer and schedule a freed tasklet, causing a use-after-free
and kernel crash.

This isn't a bug introduced by this patch, but there is also a pre-existing
early return in fsl_re_probe() that demonstrates the same lack of channel
cleanup:

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

Should fsl_re_probe() provide a cleanup path to manually free the interrupts
and other resources if dma_async_device_register() or subsequent job queue
probes fail?

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