[PATCHv3] dmaengine: fsl_raid: free resources in probe

[PATCHv3] dmaengine: fsl_raid: free resources in probe

[Rosen Penev]

Add free_irq() in the error unwind of fsl_re_chan_probe() and in
fsl_re_remove_chan() so the interrupt is always released.

Also add tasklet_kill(). Present in _remove but not _probe.

Also add of_platform_device_destroy() on failure as
of_platform_device_create() increases the reference count and needs
of_platform_device_destroy() on failure. Requires placing the pointer in
the struct for the _remove function.

Fixes: ad80da658bbc ("dmaengine: Driver support for FSL RaidEngine device.")
Assisted-by: opencode:hy3-free
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v3: pass chan to irq handler
 v2: split platform_get_data change
 drivers/dma/fsl_raid.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
index befb4bb69d54..b931bcc06ff2 100644
--- a/drivers/dma/fsl_raid.c
+++ b/drivers/dma/fsl_raid.c
@@ -212,11 +212,9 @@ static void fsl_re_dequeue(struct tasklet_struct *t)
 /* Per Job Ring interrupt handler */
 static irqreturn_t fsl_re_isr(int irq, void *data)
 {
-	struct fsl_re_chan *re_chan;
+	struct fsl_re_chan *re_chan = data;
 	u32 irqstate, status;
 
-	re_chan = dev_get_drvdata((struct device *)data);
-
 	irqstate = in_be32(&re_chan->jrregs->jr_interrupt_status);
 	if (!irqstate)
 		return IRQ_NONE;
@@ -653,10 +651,11 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,
 	chan_ofdev = of_platform_device_create(np, NULL, dev);
 	if (!chan_ofdev) {
 		dev_err(dev, "Not able to create ofdev for jr %d\n", q);
-		ret = -EINVAL;
-		goto err_free;
+		return -EINVAL;
 	}
 
+	chandev = &chan_ofdev->dev;
+
 	/* read reg property from dts */
 	rc = of_property_read_u32(np, "reg", &ptr);
 	if (rc) {
@@ -677,14 +676,13 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,
 
 	snprintf(chan->name, sizeof(chan->name), "re_jr%02d", q);
 
-	chandev = &chan_ofdev->dev;
 	tasklet_setup(&chan->irqtask, fsl_re_dequeue);
 
-	ret = request_irq(chan->irq, fsl_re_isr, 0, chan->name, chandev);
+	ret = request_irq(chan->irq, fsl_re_isr, 0, chan->name, chan);
 	if (ret) {
 		dev_err(dev, "Unable to register interrupt for JR %d\n", q);
 		ret = -EINVAL;
-		goto err_free;
+		goto err_free_tasklet;
 	}
 
 	re_priv->re_jrs[q] = chan;
@@ -704,7 +702,7 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,
 	if (!chan->inb_ring_virt_addr) {
 		dev_err(dev, "No dma memory for inb_ring_virt_addr\n");
 		ret = -ENOMEM;
-		goto err_free;
+		goto err_free_irq;
 	}
 
 	chan->oub_ring_virt_addr = dma_pool_alloc(chan->re_dev->hw_desc_pool,
@@ -746,7 +744,12 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,
 err_free_1:
 	dma_pool_free(chan->re_dev->hw_desc_pool, chan->inb_ring_virt_addr,
 		      chan->inb_phys_addr);
+err_free_irq:
+	free_irq(chan->irq, chan);
+err_free_tasklet:
+	tasklet_kill(&chan->irqtask);
 err_free:
+	of_platform_device_destroy(chandev, NULL);
 	return ret;
 }
 
@@ -852,6 +855,8 @@ static int fsl_re_probe(struct platform_device *ofdev)
 
 static void fsl_re_remove_chan(struct fsl_re_chan *chan)
 {
+	free_irq(chan->irq, chan);
+
 	tasklet_kill(&chan->irqtask);
 
 	dma_pool_free(chan->re_dev->hw_desc_pool, chan->inb_ring_virt_addr,
@@ -859,6 +864,8 @@ static void fsl_re_remove_chan(struct fsl_re_chan *chan)
 
 	dma_pool_free(chan->re_dev->hw_desc_pool, chan->oub_ring_virt_addr,
 		      chan->oub_phys_addr);
+
+	of_platform_device_destroy(chan->dev, NULL);
 }
 
 static void fsl_re_remove(struct platform_device *ofdev)
-- 
2.55.0

Re: [PATCHv3] dmaengine: fsl_raid: free resources in probe

[sashiko-bot]

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

New issues:
- [High] Double-Free of IRQ and Use-After-Free of platform device due to ignored error unwind path.
- [High] Incorrect error unwind order creates a Use-After-Free race condition.

Pre-existing issues:
- [Critical] Resource Leak and Use-After-Free on subsequent channel probe failure.
- [Critical] Inverted High/Low physical address programming for DMA rings.
- [High] NULL Pointer Dereference during teardown if channel probe fails early.
- [High] Premature IRQ enablement leading to uninitialized lock and list usage.
- [High] Deadlock in DMA callback execution.
- [High] Use-After-Free due to destroying resources before unregistering the DMA device.
- [High] Missing DMA channel registration renders the driver completely useless.
--

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

    dmaengine: fsl_raid: free resources in probe
    
    Add free_irq() in the error unwind of fsl_re_chan_probe() and in
    fsl_re_remove_chan() so the interrupt is always released.
    
    Also add tasklet_kill(). Present in _remove but not _probe.
    
    Also add of_platform_device_destroy() on failure as
    of_platform_device_create() increases the reference count and needs
    of_platform_device_destroy() on failure. Requires placing the pointer in
    the struct for the _remove function.
    
    Fixes: ad80da658bbc ("dmaengine: Driver support for FSL RaidEngine device.")
    Assisted-by: opencode:hy3-free
    Signed-off-by: Rosen Penev <rosenp@gmail.com>

> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 2d86f61105e52..a5b9325028f5f 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -669,14 +668,13 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,
>  
>  	snprintf(chan->name, sizeof(chan->name), "re_jr%02d", q);
>  
> -	chandev = &chan_ofdev->dev;
>  	tasklet_setup(&chan->irqtask, fsl_re_dequeue);
>  
> -	ret = request_irq(chan->irq, fsl_re_isr, 0, chan->name, chandev);
> +	ret = request_irq(chan->irq, fsl_re_isr, 0, chan->name, chan);

[Severity: High]
This is a pre-existing issue, but does requesting the IRQ here make the
interrupt handler live before subsequent initialization steps?

These important structures are initialized too late:

drivers/dma/fsl_raid.c:fsl_re_chan_probe() {
    ...
	spin_lock_init(&chan->desc_lock);
	INIT_LIST_HEAD(&chan->ack_q);
	INIT_LIST_HEAD(&chan->active_q);
    ...
}

Can this cause a kernel crash due to uninitialized spinlock and list
operations if the hardware has a pending interrupt from a previous boot or
kexec when the driver probes?

>  	if (ret) {
>  		dev_err(dev, "Unable to register interrupt for JR %d\n", q);
>  		ret = -EINVAL;
> -		goto err_free;
> +		goto err_free_tasklet;
>  	}
>  
>  	re_priv->re_jrs[q] = chan;

[Severity: High]
If fsl_re_chan_probe() fails after this assignment, the channel pointer is
stored in the driver's private array before the final allocations occur. If
those allocations fail, doesn't this leave a dangling pointer?

Later in fsl_re_probe(), the return value of fsl_re_chan_probe() is ignored:

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

So the driver commits the failed channel to the active channel count.

When the driver is subsequently removed, fsl_re_remove_chan() will be called
again for the already freed channel. Since this patch adds free_irq() and
of_platform_device_destroy() to the error unwind path, will this result in a
double-free of the IRQ and a use-after-free of the platform device?


[Severity: High]
This is a pre-existing issue, but while the driver properly assigns the
framework device pointers:

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

It seems to fail to link the channel into the framework's channels list.
Does this missing DMA channel registration render the driver completely useless
by silently failing to register any channels into the dmaengine framework?


[Severity: Critical]
This is a pre-existing issue, but does this misconfigure the DMA engine?

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->oubring_base_h,
		 chan->oub_phys_addr & FSL_RE_ADDR_BIT_MASK);
	out_be32(&chan->jrregs->inbring_base_l,
		 chan->inb_phys_addr >> FSL_RE_ADDR_BIT_SHIFT);
	out_be32(&chan->jrregs->oubring_base_l,
		 chan->oub_phys_addr >> FSL_RE_ADDR_BIT_SHIFT);
    ...
}

It appears the lower 32 bits are written into the high address register and
the upper 32 bits into the low address register. Can this result in invalid
physical memory accesses by the hardware, leading to silent memory corruption
or IOMMU faults?

[ ... ]

> @@ -738,7 +736,12 @@ static int fsl_re_chan_probe(struct platform_device *ofdev,
>  err_free_1:
>  	dma_pool_free(chan->re_dev->hw_desc_pool, chan->inb_ring_virt_addr,
>  		      chan->inb_phys_addr);
> +err_free_irq:
> +	free_irq(chan->irq, chan);
> +err_free_tasklet:
> +	tasklet_kill(&chan->irqtask);
>  err_free:
> +	of_platform_device_destroy(chandev, NULL);
>  	return ret;
>  }

[Severity: High]
The patch added free_irq() and tasklet_kill() after freeing the DMA memory in
err_free_1.

If an interrupt fires during this window, could the handler schedule the
tasklet which then accesses the freed or unallocated DMA rings? Does this
incorrect error unwind order create a use-after-free race condition?

[ ... ]

> @@ -844,6 +847,8 @@ static int fsl_re_probe(struct platform_device *ofdev)
>  
>  static void fsl_re_remove_chan(struct fsl_re_chan *chan)
>  {
> +	free_irq(chan->irq, chan);
> +

[Severity: High]
This is a pre-existing issue, but if fsl_re_chan_probe() fails early in its
execution, re_priv->re_jrs[q] is left as NULL.

Because total_chans is unconditionally incremented on early error in
fsl_re_probe():

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

Will this dereference the channel pointer without checking if it is NULL,
causing an immediate kernel crash?


[Severity: Critical]
This is a pre-existing issue, but if the device tree has multiple job queues
and a subsequent one fails validation:

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

Returning an error here abandons previously successful channels without tearing
down their active resources. Because re_priv is managed by devres, it will be
freed, leaving the active IRQ handler pointing to freed memory. Can this lead
to a resource leak and use-after-free memory corruption if an interrupt fires?


[Severity: High]
This is a pre-existing issue, but is there a potential deadlock in the DMA
callback execution?

The spinlock is acquired to protect the descriptor queues:

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

Then the client callback is invoked synchronously without dropping the spinlock
first:

drivers/dma/fsl_raid.c:fsl_re_desc_done() {
    ...
	dmaengine_desc_get_callback_invoke(&desc->async_tx, NULL);
    ...
}

Could this lead to an ABBA deadlock if the client attempts to allocate or
submit a new descriptor from its completion callback?


[Severity: High]
This is a pre-existing issue, but are the underlying channel resources (IRQs,
tasklets, DMA pools) completely destroyed here:

drivers/dma/fsl_raid.c:fsl_re_remove() {
    ...
	for (i = 0; i < re_priv->total_chans; i++)
		fsl_re_remove_chan(re_priv->re_jrs[i]);
    ...
}

Before the framework unregistration occurs?

drivers/dma/fsl_raid.c:fsl_re_remove() {
    ...
	dma_async_device_unregister(&re_priv->dma_dev);
}

Does this leave a window for active clients to use freed resources if the
driver is unloaded or unbound while a concurrent DMA client is still active?

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