Re: [PATCH v2 2/4] dmaengine: dw: Fix XFER bit set, but channel not idle error

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] dmaengine: dw: use-after-free in dwc_scan_descriptors() after CLEAR.XFER
--

commit 72138309eb8e38de489c0fc20421b49827977836
Author: Serge Semin <fancer.lancer@gmail.com>

dmaengine: dw: Fix XFER bit set, but channel not idle error

This commit clears pending channel interrupt bits in dwc_chan_disable()
and inlines dwc_complete_all() into dwc_scan_descriptors() so completion
handling runs under a single critical section.

> diff --git a/drivers/dma/dw/core.c b/drivers/dma/dw/core.c
> index af1871646eb9e..fbc46cbfe2591 100644
> --- a/drivers/dma/dw/core.c
> +++ b/drivers/dma/dw/core.c
> @@ -143,6 +143,12 @@ static inline void dwc_chan_disable(struct dw_dma *dw, struct dw_dma_chan *dwc)
>  	channel_clear_bit(dw, CH_EN, dwc->mask);
>  	while (dma_readl(dw, CH_EN) & dwc->mask)
>  		cpu_relax();
> +
> +	dma_writel(dw, CLEAR.XFER, dwc->mask);
> +	dma_writel(dw, CLEAR.BLOCK, dwc->mask);
> +	dma_writel(dw, CLEAR.SRC_TRAN, dwc->mask);
> +	dma_writel(dw, CLEAR.DST_TRAN, dwc->mask);
> +	dma_writel(dw, CLEAR.ERROR, dwc->mask);
>  }

[Severity: Critical]
Could clearing CLEAR.XFER in dwc_chan_disable() cause dwc_scan_descriptors()
to fall through into the hard-LLP scan loop and free an active descriptor
without unlinking it from dwc->active_list?

In drivers/dma/dw/core.c, dw_dma_tasklet() samples RAW.XFER without holding
dwc->lock before calling dwc_scan_descriptors():

	status_xfer = dma_readl(dw, RAW.XFER);
	status_err = dma_readl(dw, RAW.ERROR);
	...
	for (i = 0; i < dw->dma.chancnt; i++) {
		...
		else if (status_xfer & (1 << i))
			dwc_scan_descriptors(dw, dwc);
	}

If a client calls dwc_terminate_all() -> dwc_chan_disable() and immediately
starts a new hard-LLP transfer via dwc_issue_pending() after dw_dma_tasklet()
reads RAW.XFER, but before dwc_scan_descriptors() acquires dwc->lock,
dwc_chan_disable() clears RAW.XFER while dwc->active_list is populated with
the newly started descriptor.

When dwc_scan_descriptors() then acquires dwc->lock and re-reads RAW.XFER,
status_xfer & dwc->mask is 0. Instead of returning, it falls through to the
hard-LLP loop over dwc->active_list (which is also reached via
dwc_handle_error() prior to commit "dmaengine: dw: serialize error recovery
with channel termination"):

	list_for_each_entry_safe(desc, _desc, &dwc->active_list, desc_node) {
		desc->residue = dwc_get_hard_llp_desc_residue(dwc, desc);
		if (desc->residue) {
			spin_unlock_irqrestore(&dwc->lock, flags);
			return;
		}

		/* No data left to be send. Finalize the transfer then */
		spin_unlock_irqrestore(&dwc->lock, flags);
		dwc_descriptor_complete(dwc, desc, true);
		spin_lock_irqsave(&dwc->lock, flags);
	}

	dev_err(chan2dev(&dwc->chan),
		"BUG: All descriptors done, but channel not idle!\n");

	/* Try to continue after resetting the channel... */
	dwc_chan_disable(dw, dwc);

If dwc_get_hard_llp_desc_residue(dwc, desc) returns 0 (for example, once the
DMAC has read the last block's source items into the channel FIFO so
dwc_get_sent(dwc) equals desc->len while the peripheral is still draining the
FIFO, or if the new transfer completes between reading RAW.XFER and reading
LLP/CTL_HI), dwc_scan_descriptors() calls dwc_descriptor_complete(dwc, desc,
true) without removing desc->desc_node from dwc->active_list.

Since dwc_descriptor_complete() -> dwc_desc_put() frees desc back to
dw->desc_pool via dma_pool_free() without unlinking desc->desc_node, doesn't
this leave a freed dw_desc linked on dwc->active_list while also prematurely
disabling the channel, leading to a use-after-free and double-free on
subsequent completions, status queries, or terminations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-submit-dw-dma-synchronize-v1-v2-0-5244e94dca8c@gmail.com?part=2