Re: [PATCH v3 04/34] dmaengine: at_xdmac: move irq bottom half to dmaengine BH

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

Pre-existing issues:
- [High] Hard lockup due to premature IRQ registration before software initialization and clock enablement.
- [High] Out-of-bounds access due to lack of BH synchronization and missing empty-list validation in `at_xdmac_tasklet`.
- [High] Out-of-bounds memory access on empty transfer list in `at_xdmac_tx_status()`.
- [High] Contract violation allowing Use-After-Free due to missing `.device_synchronize` callback.
- [High] Hardware access fault in `at_xdmac_remove()` due to invalid teardown order.
- [High] Sleeping PM runtime calls executed while holding an atomic spinlock.
- [High] Use-after-free and memory leak in `at_xdmac_remove()` due to skipped cleanup loop.
--

--- Patch [4]: [PATCH v3 04/34] dmaengine: at_xdmac: move irq bottom half to dmaengine BH ---
commit 2d0fd6f4bc8cb0e5708fc858f4f33ecff4ef0fc8
Author: Allen Pais <[email protected]>

dmaengine: at_xdmac: move irq bottom half 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/at_xdmac.c b/drivers/dma/at_xdmac.c
> index 901971e8bae69..5a970f9f38314 100644
> --- a/drivers/dma/at_xdmac.c
> +++ b/drivers/dma/at_xdmac.c
> @@ -1759,9 +1758,9 @@ static void at_xdmac_handle_error(struct at_xdmac_chan *atchan)
>  	/* Then continue with usual descriptor management */
>  }
>  
> -static void at_xdmac_tasklet(struct tasklet_struct *t)
> +static void at_xdmac_tasklet(struct dma_chan *chan)
>  {
> -	struct at_xdmac_chan	*atchan = from_tasklet(atchan, t, tasklet);
> +	struct at_xdmac_chan	*atchan = to_at_xdmac_chan(chan);
>  	struct at_xdmac		*atxdmac = to_at_xdmac(atchan->chan.device);
>  	struct at_xdmac_desc	*desc;
>  	struct dma_async_tx_descriptor *txd;

[Severity: High]
This is a pre-existing issue, but can this lead to an out-of-bounds access
if the transfer list is empty?

If at_xdmac_device_terminate_all() is called, it removes all pending
transfers from atchan->xfers_list but does not synchronize with or cancel
a pending BH worker. If a hardware interrupt schedules the BH just before
termination, at_xdmac_tasklet() will run concurrently or immediately after
terminate_all().

The BH unconditionally invokes list_first_entry() without a list_empty()
check:

desc = list_first_entry(&atchan->xfers_list, struct at_xdmac_desc,
                        xfer_node);
if (!desc->active_xfer) {

Since the list was emptied, would this return a bogus pointer and corrupt
memory when accessed?

[Severity: High]
This is also a pre-existing issue, but does at_xdmac_tx_status() have a
similar out-of-bounds memory access risk on an empty transfer list?

When at_xdmac_tx_status() is called, it checks if the cookie status is
DMA_IN_PROGRESS. If a client terminates all transfers and then checks the
status of a pending cookie, at_xdmac_tx_status() traverses the empty list
without a list_empty() check:

desc = list_first_entry(&atchan->xfers_list, struct at_xdmac_desc, xfer_node);
if (!desc->active_xfer) {

Would this also result in a bogus pointer dereference?

[Severity: High]
This is a pre-existing issue, but can this code sleep while holding an
atomic spinlock?

Functions like at_xdmac_tasklet() and at_xdmac_issue_pending() acquire the
atomic spinlock atchan->lock using spin_lock_irqsave(). While holding this
lock, they call into execution paths like at_xdmac_advance_work() and
at_xdmac_start_xfer() that eventually invoke pm_runtime_resume_and_get():

spin_lock_irqsave(&atchan->lock, flags);
at_xdmac_advance_work(atchan);
spin_unlock_irqrestore(&atchan->lock, flags);

Since the device is configured with autosuspend and does not use
pm_runtime_irq_safe(), couldn't this PM runtime call sleep when attempting
to resume the device, violating atomic context rules and risking deadlocks?

> [ ... ]
> @@ -2317,7 +2316,7 @@ static int at_xdmac_probe(struct platform_device *pdev)
>  		return PTR_ERR(atxdmac->clk);
>  	}
>  
> -	/* Do not use dev res to prevent races with tasklet */
> +	/* Do not use devm resources to prevent races with the BH worker */
>  	ret = request_irq(atxdmac->irq, at_xdmac_interrupt, 0, "at_xdmac", atxdmac);

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a hard
lockup due to premature IRQ registration before software initialization and
clock enablement?

If an interrupt is asserted right after request_irq() is called in
at_xdmac_probe(), the interrupt handler at_xdmac_interrupt() will execute
while the hardware clock is still disabled. 

Additionally, atxdmac->dma.chancnt is initialized much later in
dma_async_device_register(). Because it evaluates to 0, the handler's
internal processing loop is completely skipped:

for (i = 0; i < atxdmac->dma.chancnt; i++) {
    if (!((1 << i) & pending))
        continue;

This means the device interrupt is never acknowledged or cleared. Would this
result in the ISR spinning infinitely in its "while (pending);" loop,
locking up the CPU?

> [ ... ]
> @@ -2397,7 +2396,7 @@ static int at_xdmac_probe(struct platform_device *pdev)
>  		spin_lock_init(&atchan->lock);
>  		INIT_LIST_HEAD(&atchan->xfers_list);
>  		INIT_LIST_HEAD(&atchan->free_descs_list);
> -		tasklet_setup(&atchan->tasklet, at_xdmac_tasklet);
> +		dmaengine_init_bh(&atchan->chan, at_xdmac_tasklet);
>  
>  		/* Clear pending interrupts. */
>  		while (at_xdmac_chan_read(atchan, AT_XDMAC_CIS))

[Severity: High]
This is a pre-existing issue, but does this violate the dmaengine contract
allowing a use-after-free due to a missing .device_synchronize callback?

According to the dmaengine core rules, any driver utilizing asynchronous
contexts for callbacks (like BH workers) must implement the
.device_synchronize API. at_xdmac does not provide this callback when
registering the DMA device callbacks.

Consequently, dmaengine_synchronize() acts as a no-op. Could clients free
memory buffers associated with a DMA completion while the BH worker is still
accessing them or hasn't finished running?

> [ ... ]
> @@ -2458,7 +2457,7 @@ static void at_xdmac_remove(struct platform_device *pdev)
>  	for (i = 0; i < atxdmac->dma.chancnt; i++) {
>  		struct at_xdmac_chan *atchan = &atxdmac->chan[i];
>  
> -		tasklet_kill(&atchan->tasklet);
> +		dmaengine_kill_bh(&atchan->chan);
>  		at_xdmac_free_chan_resources(&atchan->chan);
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but could this cause a hardware access fault
in at_xdmac_remove() due to an invalid teardown order?

In at_xdmac_remove(), the driver calls clk_disable_unprepare(atxdmac->clk)
before calling free_irq(). If a spurious interrupt fires or if the ISR is
currently running on another CPU, it will attempt to access device
registers after the clock has been turned off. Might this cause the system
to hang or trigger a synchronous external abort?

[Severity: High]
This is also a pre-existing issue, but does this cause a use-after-free
and memory leak because the cleanup loop is skipped?

In at_xdmac_remove(), dma_async_device_unregister() is called before this
channel cleanup loop. dma_async_device_unregister() internally decrements
atxdmac->dma.chancnt to 0. 

Consequently, the loop condition (i < atxdmac->dma.chancnt) evaluates to
false immediately, and the loop is skipped entirely. This prevents
dmaengine_kill_bh() and at_xdmac_free_chan_resources() from executing.

If a BH worker is scheduled, could it run after the device memory is freed
by devres, leading to a use-after-free while also leaking all pre-allocated
descriptors?

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