Re: [PATCH 4/6] dmaengine: sun6i: track paused virtual channels

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] dmaengine: sun6i: UAF and pchan starvation in sun6i_dma_tasklet() when paused
--

commit 668628da200a577f2488b0d26796212d9a34a3f9
Author: James Hilliard <james.hilliard1@gmail.com>

dmaengine: sun6i: track paused virtual channels

This commit adds pause state tracking to virtual channels in the sun6i DMA
driver so sun6i_dma_tx_status() reports DMA_PAUSED for incomplete transfers
and paused channels do not start new descriptors or enter the pending queue.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index cd0562de7def1..7ac6693a041eb 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
[ ... ]
> @@ -538,7 +539,7 @@ static void sun6i_dma_tasklet(struct tasklet_struct *t)
>  
>  		pchan = vchan->phy;
>  
> -		if (pchan && pchan->done) {
> +		if (pchan && pchan->done && !vchan->paused) {

[Severity: High]
What happens if a non-cyclic descriptor completes in sun6i_dma_interrupt()
just before or concurrently with sun6i_dma_pause()?

When a non-cyclic transfer finishes, sun6i_dma_interrupt() marks the cookie
complete and sets pchan->done:

drivers/dma/sun6i-dma.c:sun6i_dma_interrupt() {
	...
	} else {
		vchan_cookie_complete(&pchan->desc->vd);
		pchan->done = pchan->desc;
	}
	...
}

vchan_cookie_complete() places &pchan->desc->vd on vc->desc_completed and
schedules vc->task (vchan_complete()). That tasklet runs independently of
vchan->paused and frees pchan->desc along with its LLI pool entries via
vchan_complete() -> vchan_vdesc_fini() -> sun6i_dma_free_desc_virt() ->
sun6i_dma_free_desc().

Previously, sun6i_dma_tasklet() always entered the pchan && pchan->done block
and called sun6i_dma_start_desc(), which either replaced pchan->desc with the
next issued descriptor or cleared pchan->desc and pchan->done to NULL before
detaching pchan from vchan.

With the added !vchan->paused check, sun6i_dma_tasklet() skips this cleanup
completely while the channel is paused. Doesn't this leave pchan->desc and
pchan->done holding dangling pointers to the freed struct sun6i_desc, while
also keeping the idle physical channel pchan pinned to the paused vchan and
starving other pending virtual channels in sdev->pending?

If a caller then queries sun6i_dma_tx_status() with a non-NULL state for a
submitted-but-unissued cookie (on vc.desc_submitted, which vchan_find_desc()
does not search), vd is NULL while pchan->desc is still non-NULL:

drivers/dma/sun6i-dma.c:sun6i_dma_tx_status() {
	...
	vd = vchan_find_desc(&vchan->vc, cookie);
	txd = to_sun6i_desc(&vd->tx);

	if (vd) {
		for (lli = txd->v_lli; lli != NULL; lli = lli->v_lli_next)
			bytes += lli->len;
	} else if (!pchan || !pchan->desc) {
		bytes = 0;
	} else {
		bytes = sun6i_get_chan_size(pchan);
	}
	...
}

Won't sun6i_get_chan_size(pchan) then dereference the freed pchan->desc and
walk its freed txd->v_lli chain?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-submit-sun6i-dma-lifetime-next-v1-v1-0-01085f684c9f@gmail.com?part=4