Re: [PATCHv2 3/4] dma: fsl_raid: keep MMIO bases as void __iomem and cast at access

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing bounds check when parsing device tree Job Ring nodes.
- [High] `fsl_re_probe()` ignores the return value of `fsl_re_chan_probe()` and unconditionally increments `total_chans`, leading to a NULL pointer dereference.
- [High] DMA channels are never registered with the parent DMA device framework.
- [High] Missing cleanup for channel IRQs and platform devices in both error paths and driver removal, plus incorrect teardown order.
- [High] Potential NULL pointer dereference in `fsl_re_isr` due to premature IRQ registration.
--

--- Patch [1]: [PATCHv2 3/4] dma: fsl_raid: keep MMIO bases as void __iomem and cast at access ---
commit d9bcf292f2ff70b568c4dd2362d8ec21ba56f6cd
Author: Rosen Penev <rosenp@gmail.com>

dma: fsl_raid: keep MMIO bases as void __iomem and cast at access

The commit modifies the fsl_re_drv_private and fsl_re_chan structures to
store MMIO bases as void __iomem * instead of typed struct pointers. This
resolves sparse warnings about mismatched address spaces by ensuring typed
pointers are only introduced locally in functions that touch the registers.

Fixes: https://lore.kernel.org/oe-kbuild-all/202008111749.yy85rFMD%25lkp@intel.com/

> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 99945845d8b5e..5bc5bd499ba27 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -656,8 +656,7 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,
>  		goto err_free;
>  	}
>  
> -	chan->jrregs = (struct fsl_re_chan_cfg *)((u8 *)re_priv->re_regs +
> -			off + ptr);
> +	chan->jrregs = re_priv->base + off + ptr;
>  
>  	/* read irq property from dts */
>  	chan->irq = irq_of_parse_and_map(np, 0);

[Severity: High]
This isn't a bug introduced by this patch, but the driver context seems to
be attached to the device too late.

In fsl_re_chan_probe(), request_irq() is called before dev_set_drvdata()
populates the driver data:

    ret = request_irq(chan->irq, fsl_re_isr, 0, chan->name, chandev);
    ...
    dev_set_drvdata(chandev, chan);

If an interrupt triggers immediately, fsl_re_isr() retrieves a NULL pointer
and dereferences it:

    re_chan = dev_get_drvdata((struct device *)data);
    irqstate = in_be32(&re_chan->jrregs->jr_interrupt_status);

Is there a vulnerable window here where an incoming interrupt could lead to
a NULL pointer dereference?

[Severity: High]
This is a pre-existing issue, but it appears the DMA channels are never
registered with the parent DMA device framework.

In fsl_re_chan_probe(), the channels are initialized, but the required list
addition to attach them to dma_dev->channels is omitted:

    re_priv->re_jrs[q] = chan;
    chan->chan.device = dma_dev;
    ...

Without adding the channel to dma_dev->channels, will
dma_async_device_register() end up registering 0 channels and leave the
driver functionally dead?

[Severity: High]
This isn't a bug introduced by this patch, but I noticed potential resource
leaks and incorrect teardown ordering.

On error paths inside fsl_re_chan_probe(), resources like requested IRQs
and platform devices are not freed:

    err_free:
        return ret;

Additionally, in fsl_re_remove_chan(), free_irq() or
of_platform_device_destroy() are not called.

This leaves an active unmanaged IRQ pointing to freed driver memory.

Also, in fsl_re_remove(), the driver frees ring buffers before unregistering
the async DMA device:

    for (i = 0; i < re_priv->total_chans; i++)
        fsl_re_remove_chan(re_priv->re_jrs[i]);

    /* Unregister the driver */
    dma_async_device_unregister(&re_priv->dma_dev);

Could this cause a use-after-free if the IRQ fires, and does the teardown
order violate teardown safety expectations?

[ ... ]

> @@ -763,20 +763,21 @@ static int fsl_re_probe(struct platform_device *ofdev)
>  		return -ENODEV;
>  
>  	/* IOMAP the entire RAID Engine region */
> -	re_priv->re_regs = devm_ioremap(dev, res->start, resource_size(res));
> -	if (!re_priv->re_regs)
> +	re_regs = devm_ioremap(dev, res->start, resource_size(res));
> +	if (!re_regs)
>  		return -EBUSY;
> +	re_priv->base = re_regs;
>  
>  	/* Program the RE mode */
> -	out_be32(&re_priv->re_regs->global_config, FSL_RE_NON_DPAA_MODE);
> +	out_be32(&re_regs->global_config, FSL_RE_NON_DPAA_MODE);

[ ... ]

[Severity: High]
This is a pre-existing issue, but there seems to be a missing bounds check
when parsing device tree Job Ring nodes.

In fsl_re_probe(), the ridx counter is incremented indefinitely and passed
directly to fsl_re_chan_probe() without being bounded against
FSL_RE_MAX_CHANS (4):

    fsl_re_chan_probe(ofdev, child, ridx++, off);
    re_priv->total_chans++;

Then in fsl_re_chan_probe(), the unbounded index q is directly used to write
into the statically sized array:

    re_priv->re_jrs[q] = chan;

If a malformed device tree contains more than 4 job ring child nodes,
would this cause an out-of-bounds write and memory corruption?

[Severity: High]
This isn't a bug introduced by this patch, but fsl_re_probe() appears to
ignore the return value of fsl_re_chan_probe() and unconditionally
increments total_chans.

    fsl_re_chan_probe(ofdev, child, ridx++, off);
    re_priv->total_chans++;

If fsl_re_chan_probe() fails and returns an error, fsl_re_probe() ignores
the error but still increments re_priv->total_chans.

Subsequent calls to fsl_re_remove_chan() on driver remove or error rollback
will then dereference a NULL pointer:

    static void fsl_re_remove_chan(struct fsl_re_chan *chan)
    {
        tasklet_kill(&chan->irqtask);
        ...

Might this result in a kernel panic from a NULL pointer dereference during
teardown?

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