Re: [PATCH v3 18/34] dmaengine: xgene-dma: wire descriptor cleanup to dmaengine BH

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

Pre-existing issues:
- [High] Interrupts are requested and hardware interrupts unmasked before the DMA engine BH is initialized, exposing a race condition.
- [High] Potential self-deadlock in `xgene_dma_free_chan_resources` due to calling `xgene_dma_cleanup_descriptors` without disabling softirqs.
- [High] Use-after-free memory corruption in the error handling paths of `xgene_dma_prep_xor` and `xgene_dma_prep_pq`.
- [High] Failure to mask interrupts and disable hardware before calling `dmaengine_kill_bh()` in the probe error path leaves a race window where an interrupt can reschedule the BH, leading to a Use-After-Free.
- [High] The teardown sequence in `xgene_dma_remove` frees the device IRQs before killing the DMA engine BH, potentially causing `enable_irq()` to be called on a freed IRQ.
--

--- Patch [18]: [PATCH v3 18/34] dmaengine: xgene-dma: wire descriptor cleanup to dmaengine BH ---
commit cdb570000a04c682157da4771610c096505551ac
Author: Allen Pais <[email protected]>

dmaengine: xgene-dma: wire descriptor cleanup to dmaengine BH
    
Replace the per-channel tasklet with the shared dmaengine BH helper.
The handler continues to run in softirq context while dmaengine owns
the common scheduling and teardown mechanism.

> diff --git a/drivers/dma/xgene-dma.c b/drivers/dma/xgene-dma.c
> index f64624ea44add..e1199863a8246 100644
> --- a/drivers/dma/xgene-dma.c
> +++ b/drivers/dma/xgene-dma.c

[ ... ]

> @@ -976,9 +974,10 @@ static enum dma_status xgene_dma_tx_status(struct dma_chan *dchan,
>  	return dma_cookie_status(dchan, cookie, txstate);
>  }
>  
> -static void xgene_dma_tasklet_cb(struct tasklet_struct *t)
> +static void xgene_dma_tasklet_cb(struct dma_chan *c)
>  {
> -	struct xgene_dma_chan *chan = from_tasklet(chan, t, tasklet);
> +	struct xgene_dma_chan *chan = container_of(c, struct xgene_dma_chan,
> +						   dma_chan);
>  
>  	/* Run all cleanup for descriptors which have been completed */
>  	xgene_dma_cleanup_descriptors(chan);

[Severity: High]
This isn't a bug introduced by this patch, but can this lead to a
self-deadlock?

Looking at xgene_dma_free_chan_resources(), it calls 
xgene_dma_cleanup_descriptors() directly from process context, which 
acquires chan->lock without disabling softirqs:

xgene_dma_free_chan_resources()
  xgene_dma_cleanup_descriptors(chan)
    spin_lock(&chan->lock);

If a DMA hardware interrupt arrives on the same CPU while this lock is held,
the ISR will schedule the BH. 

When returning from the hardware interrupt, irq_exit() will process softirqs
and attempt to acquire the same chan->lock, deadlocking against the
interrupted process context.

[ ... ]

> @@ -1540,7 +1539,7 @@ static int xgene_dma_async_register(struct xgene_dma *pdma, int id)
>  	INIT_LIST_HEAD(&chan->ld_pending);
>  	INIT_LIST_HEAD(&chan->ld_running);
>  	INIT_LIST_HEAD(&chan->ld_completed);
> -	tasklet_setup(&chan->tasklet, xgene_dma_tasklet_cb);
> +	dmaengine_init_bh(&chan->dma_chan, xgene_dma_tasklet_cb);

[Severity: High]
This isn't a bug introduced by this patch, but is there a race condition
during probe where the BH is scheduled before initialization?

In xgene_dma_probe(), interrupts are requested and hardware interrupts are
unmasked before xgene_dma_init_async() is called to initialize the BH:

xgene_dma_probe()
  ret = xgene_dma_request_irqs(pdma);
  ...
  xgene_dma_init_hw(pdma);
  ...
  ret = xgene_dma_init_async(pdma);

If a pending interrupt fires in this window, the ISR will call
dmaengine_schedule_bh() on an uninitialized BH.

This triggers a WARN_ON and fails to queue the BH. Because the ISR disables
the IRQ and relies on the BH to re-enable it, the interrupt line will remain
permanently disabled.

>  	chan->pending = 0;
>  	chan->desc_pool = NULL;

[ ... ]

> @@ -1580,7 +1579,7 @@ static int xgene_dma_init_async(struct xgene_dma *pdma)
>  		if (ret) {
>  			for (j = 0; j < i; j++) {
>  				dma_async_device_unregister(&pdma->dma_dev[j]);
> -				tasklet_kill(&pdma->chan[j].tasklet);
> +				dmaengine_kill_bh(&pdma->chan[j].dma_chan);

[Severity: High]
This isn't a bug introduced by this patch, but does this error path leave
a race window that could result in a use-after-free?

During driver probe, xgene_dma_init_hw() unmasks hardware interrupts. If a
subsequent channel registration fails here, dmaengine_kill_bh() is called,
but hardware interrupts are not masked.

An incoming interrupt could fire immediately after dmaengine_kill_bh(),
causing the ISR to re-queue the BH.

When the probe function then returns an error, the device data structures
are freed via devres. When the workqueue subsequently executes the re-queued
BH, it will operate on freed memory.

>  			}
>  
>  			return ret;

[ ... ]

> @@ -1791,7 +1790,7 @@ static void xgene_dma_remove(struct platform_device *pdev)
>  
>  	for (i = 0; i < XGENE_DMA_MAX_CHANNEL; i++) {
>  		chan = &pdma->chan[i];
> -		tasklet_kill(&chan->tasklet);
> +		dmaengine_kill_bh(&chan->dma_chan);

[Severity: High]
This isn't a bug introduced by this patch, but is the teardown ordering
correct here?

In xgene_dma_remove(), xgene_dma_free_irqs() is called to free the interrupt
handlers before the loop that flushes the BH work with dmaengine_kill_bh().

If a BH is currently pending, dmaengine_kill_bh() will wait for it to
execute. The BH handler xgene_dma_tasklet_cb() unconditionally calls
enable_irq(chan->rx_irq).

Because the IRQ has already been freed, calling enable_irq() operates on a
torn-down IRQ.

>  		xgene_dma_delete_chan_rings(chan);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but is there a use-after-free
in the error handling path of xgene_dma_prep_xor() and xgene_dma_prep_pq()?

If descriptor allocation fails during a multi-descriptor transaction, the
driver jumps to the fail label and passes the embedded list head of the
first descriptor to the cleanup function:

xgene_dma_prep_xor()
fail:
    if (!first)
        return NULL;
    xgene_dma_free_desc_list(chan, &first->tx_list);

xgene_dma_free_desc_list() uses list_for_each_entry_safe() to iterate and
free elements. On the first iteration, the first descriptor is freed.

However, list_for_each_entry_safe() evaluates the loop increment step at the
end of the iteration, which accesses head->next. Since head is
&first->tx_list, this reads from the freed first descriptor memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=18