Re: [PATCH] dmaengine: fsl_raid: fix 64-bit ring base address programming

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The return value of fsl_re_chan_probe() is ignored, leading to a NULL pointer dereference on driver removal.
- [High] The driver calls request_irq() without a corresponding free_irq(), causing a Use-After-Free panic on module removal.
- [High] Channels are never added to the DMA device's channel list, resulting in a non-functional driver.
- [High] The interrupt handler is registered before critical driver data is initialized, leading to crashes from early interrupts.
- [High] The driver invokes the DMA client completion callback while holding desc_lock, causing an AB-BA self-deadlock if the client submits a new transaction.
- [High] fsl_re_free_chan_resources() modifies the free_q list without holding desc_lock and improperly loops based on alloc_count.
--

commit ac27e4641c6966f6e1eb9d43776e03a8a1dcc825
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: fsl_raid: fix 64-bit ring base address programming

The driver claims a 40-bit DMA mask via dma_set_mask(40), but the
inbound/outbound ring base addresses were programmed by masking the
physical address with FSL_RE_ADDR_BIT_MASK (0xF) into the high register
and shifting it right by FSL_RE_ADDR_BIT_SHIFT (4) into the low register.

> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 2d86f61105e52..e8223ac38e222 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -707,15 +707,19 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,
>  		goto err_free_1;
>  	}
>  
> -	/* Program the Inbound/Outbound ring base addresses and size */
> +	/* Program the Inbound/Outbound ring base addresses and size.
> +	 * The DMA rings are 64-bit addresses split into high/low registers.
> +	 * The driver supports a 40-bit DMA mask, so the low register holds
> +	 * bits [31:0] and the high register holds bits [39:32].
> +	 */
>  	out_be32(&chan->jrregs->inbring_base_h,
> -		 chan->inb_phys_addr & FSL_RE_ADDR_BIT_MASK);
> +		 upper_32_bits(chan->inb_phys_addr) & FSL_RE_ADDR_HIGH_MASK);
>  	out_be32(&chan->jrregs->oubring_base_h,
> -		 chan->oub_phys_addr & FSL_RE_ADDR_BIT_MASK);
> +		 upper_32_bits(chan->oub_phys_addr) & FSL_RE_ADDR_HIGH_MASK);
>  	out_be32(&chan->jrregs->inbring_base_l,
> -		 chan->inb_phys_addr >> FSL_RE_ADDR_BIT_SHIFT);
> +		 lower_32_bits(chan->inb_phys_addr));
>  	out_be32(&chan->jrregs->oubring_base_l,
> -		 chan->oub_phys_addr >> FSL_RE_ADDR_BIT_SHIFT);
> +		 lower_32_bits(chan->oub_phys_addr));
>  	out_be32(&chan->jrregs->inbring_size,
>  		 FSL_RE_RING_SIZE << FSL_RE_RING_SIZE_SHIFT);
>  	out_be32(&chan->jrregs->oubring_size,
> diff --git a/drivers/dma/fsl_raid.h b/drivers/dma/fsl_raid.h
> index adbfede330a77..748047996bbcc 100644
> --- a/drivers/dma/fsl_raid.h
> +++ b/drivers/dma/fsl_raid.h
> @@ -66,8 +66,7 @@
>  #define FSL_RE_RING_SIZE		0x400
>  #define FSL_RE_RING_SIZE_MASK		(FSL_RE_RING_SIZE - 1)
>  #define FSL_RE_RING_SIZE_SHIFT		8
> -#define FSL_RE_ADDR_BIT_SHIFT		4
> -#define FSL_RE_ADDR_BIT_MASK		(BIT(FSL_RE_ADDR_BIT_SHIFT) - 1)
> +#define FSL_RE_ADDR_HIGH_MASK		0xFF
>  #define FSL_RE_ERROR			0x40000000
>  #define FSL_RE_INTR			0x80000000
>  #define FSL_RE_CLR_INTR			0x80000000

[Severity: High]
This is a pre-existing issue, but I noticed the return value of
fsl_re_chan_probe() is ignored, which can lead to a NULL pointer dereference
on driver removal.

drivers/dma/fsl_raid.c:fsl_re_probe() {
    ...
			if (rc) {
				fsl_re_chan_probe(ofdev, child, ridx++, off);
				re_priv->total_chans++;
			}
    ...
}

If fsl_re_chan_probe() fails, re_priv->total_chans is still incremented,
and re_priv->re_jrs[q] remains NULL.  During teardown, fsl_re_remove() calls
fsl_re_remove_chan() which unconditionally dereferences it:

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

Will this cause a kernel panic on driver unbind if any channel failed to
probe?

[Severity: High]
This isn't a bug introduced by this patch, but I noticed request_irq() is
called without a corresponding free_irq(), which can cause a use-after-free
panic on module removal.

drivers/dma/fsl_raid.c:fsl_re_chan_probe() {
    ...
	ret = request_irq(chan->irq, fsl_re_isr, 0, chan->name, chandev);
	if (ret) {
    ...
err_free:
	return ret;
}

The interrupt handler doesn't appear to be freed in error paths or during
teardown in fsl_re_remove_chan():

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

If a hardware interrupt fires after the module is unloaded, will the kernel
jump into freed module memory?

[Severity: High]
This is a pre-existing issue, but it looks like channels are never added to
the DMA device's channel list, which might result in a non-functional driver.

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

The global channel list is initialized via INIT_LIST_HEAD(&dma_dev->channels),
but list_add_tail(&chan->chan.device_node, &dma_dev->channels) is never
called.  Will dma_async_device_register() iterate over an empty list and
register zero channels?

[Severity: High]
This isn't a bug introduced by this patch, but the interrupt handler is
registered before critical driver data is initialized, which could lead to
crashes from early interrupts.

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 a spurious or early interrupt triggers immediately after request_irq(),
the ISR will execute dev_get_drvdata() and receive NULL:

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

Could this cause a NULL pointer dereference panic while reading
jr_interrupt_status?

[Severity: High]
This is a pre-existing issue, but the driver invokes the DMA client completion
callback while holding desc_lock, which can cause an AB-BA self-deadlock.

drivers/dma/fsl_raid.c:fsl_re_dequeue() {
    ...
	spin_lock_irqsave(&re_chan->desc_lock, flags);
    ...
		if (found) {
			fsl_re_desc_done(desc);
    ...
}

If the client submits a chained DMA request within the callback,
fsl_re_tx_submit() attempts to re-acquire the exact same desc_lock:

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

Will this instantly deadlock the CPU?

[Severity: High]
This isn't a bug introduced by this patch, but fsl_re_free_chan_resources()
modifies the free_q list without holding desc_lock and improperly loops based
on alloc_count.

drivers/dma/fsl_raid.c:fsl_re_free_chan_resources() {
    ...
	while (re_chan->alloc_count--) {
		desc = list_first_entry(&re_chan->free_q,
					struct fsl_re_desc,
					node);

		list_del(&desc->node);
    ...
		kfree(desc);
	}
}

If any descriptors are still in flight, free_q will be prematurely empty,
causing list_first_entry() to return an invalid pointer which is then freed.
Should this be protected by desc_lock and use list_empty() checks?

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