[PATCH] dmaengine: fsl_raid: avoid free_q underflow in free_chan_resources

[PATCH] dmaengine: fsl_raid: avoid free_q underflow in free_chan_resources

[Rosen Penev]

fsl_re_free_chan_resources() loops alloc_count times calling
list_first_entry() on free_q without checking whether the list still
has entries. If descriptors remain un-acked or pending in submit_q /
active_q / ack_q when the channel resources are freed, free_q becomes
smaller than alloc_count and list_first_entry() returns the list head,
causing list_del() to corrupt the list head and dereference a bogus
pointer.

Walk free_q with list_for_each_entry_safe() until it is empty,
decrementing alloc_count per freed descriptor, instead of relying on
the alloc_count count.

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 | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
index 30aaca81d855..db86f3e4e460 100644
--- a/drivers/dma/fsl_raid.c
+++ b/drivers/dma/fsl_raid.c
@@ -614,18 +614,15 @@ static int fsl_re_alloc_chan_resources(struct dma_chan *chan)
 static void fsl_re_free_chan_resources(struct dma_chan *chan)
 {
 	struct fsl_re_chan *re_chan;
-	struct fsl_re_desc *desc;
+	struct fsl_re_desc *desc, *_desc;
 
 	re_chan = container_of(chan, struct fsl_re_chan, chan);
-	while (re_chan->alloc_count--) {
-		desc = list_first_entry(&re_chan->free_q,
-					struct fsl_re_desc,
-					node);
-
+	list_for_each_entry_safe(desc, _desc, &re_chan->free_q, node) {
 		list_del(&desc->node);
 		dma_pool_free(re_chan->re_dev->cf_desc_pool, desc->cf_addr,
 			      desc->cf_paddr);
 		kfree(desc);
+		re_chan->alloc_count--;
 	}
 
 	if (!list_empty(&re_chan->free_q))
-- 
2.55.0

Re: [PATCH] dmaengine: fsl_raid: avoid free_q underflow in free_chan_resources

[sashiko-bot]

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

New issues:
- [High] The patch avoids a crash by exclusively iterating over `free_q`, but permanently leaks descriptors in other queues and introduces a dead-code check.

Pre-existing issues:
- [High] Iterating and modifying `free_q` without holding `desc_lock` can race with the hardware tasklet, leading to list corruption.
- [High] The driver never frees the requested IRQ, causing an IRQ leak and a subsequent Use-After-Free on driver removal.
- [High] Hardware descriptor rings are destroyed before the DMA device is unregistered, causing memory corruption if clients submit concurrent requests.
- [High] Papering over the crash without halting the underlying hardware risks severe silent DMA memory corruption.
--

--- Patch [1]: [PATCH] dmaengine: fsl_raid: avoid free_q underflow in free_chan_resources ---
commit 6689b4246e148c9519335c9bfdee192da8124eb9
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: fsl_raid: avoid free_q underflow in free_chan_resources

This commit modifies fsl_re_free_chan_resources() to walk free_q using
list_for_each_entry_safe() rather than relying on a fixed count. This
prevents a list corruption crash when descriptors are still located in
other queues during teardown.

> diff --git a/drivers/dma/fsl_raid.c b/drivers/dma/fsl_raid.c
> index 99945845d8b5e..5d86ff99624c0 100644
> --- a/drivers/dma/fsl_raid.c
> +++ b/drivers/dma/fsl_raid.c
> @@ -602,18 +602,15 @@ static int fsl_re_alloc_chan_resources(struct dma_chan *chan)
>  static void fsl_re_free_chan_resources(struct dma_chan *chan)
>  {
>  	struct fsl_re_chan *re_chan;

[Severity: High]
This is a pre-existing issue, but does this function completely fail to
halt the hardware RAID engine?

If a client frees the channel while DMA transactions are still active, the
client will unmap its data buffers. Because the RAID engine and its
interrupts are not halted here, will it continue performing DMA operations
into memory that has been freed or reassigned?

> -	struct fsl_re_desc *desc;
> +	struct fsl_re_desc *desc, *_desc;
>  
>  	re_chan = container_of(chan, struct fsl_re_chan, chan);
> -	while (re_chan->alloc_count--) {
> -		desc = list_first_entry(&re_chan->free_q,
> -					struct fsl_re_desc,
> -					node);
> -
> +	list_for_each_entry_safe(desc, _desc, &re_chan->free_q, node) {

[Severity: High]
This is a pre-existing issue, but is it safe to iterate and modify free_q
using list_del() without acquiring re_chan->desc_lock?

Since the hardware is not halted during teardown, an interrupt could fire
and schedule the fsl_re_dequeue tasklet on another CPU. If the tasklet
calls fsl_re_cleanup_descs(), which acquires desc_lock and attempts to
move completed descriptors to free_q using list_move_tail(), could this
concurrent modification corrupt the list pointers?

[Severity: High]
By exclusively iterating over free_q, does this deliberately strand and
leak any DMA pool buffers currently sitting in submit_q, active_q, and ack_q?

If a client frees the channel while descriptors remain in those other
queues, they are now ignored and the memory is never freed.

>  		list_del(&desc->node);
>  		dma_pool_free(re_chan->re_dev->cf_desc_pool, desc->cf_addr,
>  			      desc->cf_paddr);
>  		kfree(desc);
> +		re_chan->alloc_count--;
>  	}
>  
>  	if (!list_empty(&re_chan->free_q))

[Severity: High]
Because the list_for_each_entry_safe loop above runs until free_q is empty,
won't this list_empty() check mathematically always evaluate to false?

This makes the error logging dead code, effectively masking the fact that
descriptors were left behind in the other queues.

>  		dev_err(re_chan->dev, "chan resource cannot be cleaned!\n");
>  }

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the driver leak the IRQ requested
in fsl_re_chan_probe()?

Looking at fsl_re_chan_probe(), an interrupt is requested using
request_irq(), but neither fsl_re_remove_chan() nor fsl_re_remove() calls
free_irq().

Because the re_chan structure is allocated with devm_kzalloc() on the
parent device, its memory is automatically freed when the device unbinds.
If an interrupt fires after the driver is unloaded, will the kernel
execute the registered fsl_re_isr and dereference the freed re_chan
structure?

[Severity: High]
This is a pre-existing issue, but does fsl_re_remove() destroy resources
in the wrong order?

Looking at fsl_re_remove(), it destroys the hardware descriptor queues by
calling fsl_re_remove_chan() before calling dma_async_device_unregister():

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]);
    
    dma_async_device_unregister(&re_priv->dma_dev);
    ...
}

By destroying the underlying DMA rings while the device is still visible
to the core framework, will any active or racing DMA transfers attempt to
write to freed memory pools?

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