Re: [PATCH v2 13/19] dmaengine: dw-edma: Make the LL ring reset a full channel resync

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Caching `desc` prior to channel reset in `dw_edma_start_transfer()` leads to starting hardware DMA on an aborted and completed descriptor, causing Use-After-Free and Rogue DMA.
--

--- Patch [13]: [PATCH v2 13/19] dmaengine: dw-edma: Make the LL ring reset a full channel resync ---
commit 4087cc9178be36e7f92be792abfbec15027f1b49
Author: Koichiro Den <[email protected]>

dmaengine: dw-edma: Make the LL ring reset a full channel resync

Reset currently clears the ring but leaves issued descriptors unchanged.
This patch updates the LL ring reset to abort descriptors with published
entries to prevent unsafe replay and to allow unpublished descriptors to use
the fresh ring.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 3fc929a6bcf52..963abecdc59ed 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -100,8 +100,27 @@ static void dw_hdma_set_callback_result(struct virt_dma_desc *vd,
>  
>  static void dw_edma_core_reset_ll(struct dw_edma_chan *chan)
>  {
> +	struct virt_dma_desc *vd, *tmp;
>  	u32 i;
>  
> +	/*
> +	 * Software cannot tell which published entries completed before the
> +	 * reset. Replaying one could duplicate a transfer after its target
> +	 * changed ownership, while keeping descriptors with uncertain entries
> +	 * would block later in-order completions. Abort descriptors with published
> +	 * entries; untouched descriptors remain available for republishing.
> +	 */
> +	list_for_each_entry_safe(vd, tmp, &chan->vc.desc_issued, node) {
> +		struct dw_edma_desc *desc = vd2dw_edma_desc(vd);
> +
> +		if (desc->start_burst == desc->done_burst)
> +			continue;
> +
> +		dw_hdma_set_callback_result(vd, DMA_TRANS_ABORTED);
> +		list_del(&vd->node);
> +		vchan_cookie_complete(vd);

[Severity: High]
If this aborts partially issued descriptors by unlinking and completing them,
doesn't it cause a use-after-free when called from dw_edma_start_transfer()?

In dw_edma_start_transfer(), the first descriptor is cached before calling the
reset function:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_start_transfer() {
    ...
	vd = vchan_next_desc(&chan->vc);
	if (!vd)
		return 0;

	desc = vd2dw_edma_desc(vd);
	if (!desc)
		return 0;

	if (!chan->non_ll && !chan->ll_valid)
		dw_edma_core_reset_ll(chan);

	dw_edma_core_start(desc);
    ...
}

Because dw_edma_core_reset_ll() may complete the cached descriptor, the client
could free the DMA buffer and the descriptor while dw_edma_core_start() is
subsequently passing it to the hardware. Can this result in rogue hardware DMA
and memory corruption?

> +	}
> +
>  	chan->ll_head = 0;
>  	chan->ll_done = 0;
>  	dw_edma_ll_irq_idx_discard(chan);

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