Re: [PATCH] dmaengine: sun6i-dma: Fix use-after-free in error handling paths

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing `sg_len` validation in `sun6i_dma_prep_slave_sg` leads to NULL pointer dereference and hardware memory corruption.
- [High] Missing bounds validation in `sun6i_dma_prep_dma_cyclic` causes a kernel panic via division by zero or NULL pointer dereference.
- [High] Deadlock risk due to incompatible lock contexts for `sdev->lock`.
- [High] TOCTOU race condition in `sun6i_dma_interrupt` leads to NULL pointer dereference if raced against `sun6i_dma_terminate_all`.
--

commit ca6b94283f2abd1205fd392ecfced542cac0978a
Author: Hongling Zeng <zenghongling@kylinos.cn>

    dmaengine: sun6i-dma: Fix use-after-free in error handling paths

This commit fixes a use-after-free issue in the error handling paths of the
DMA preparation functions. It saves the next physical and virtual pointers
before freeing the current linked list node.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..eb9c4ae87ac8e 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -788,9 +788,15 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_slave_sg(

[Severity: High]
This isn't a bug introduced by this patch, but missing sg_len validation in
sun6i_dma_prep_slave_sg() can lead to a NULL pointer dereference and
hardware memory corruption.

If a 0-length scatterlist is provided, the loop is completely bypassed:

	for_each_sg(sgl, sg, sg_len, i) {
		v_lli = dma_pool_alloc(sdev->pool, GFP_DMA32 | GFP_NOWAIT, &p_lli);

This leaves txd->v_lli as NULL and txd->p_lli as 0. When this descriptor is
later submitted in sun6i_dma_start_desc():

	writel(pchan->desc->p_lli, pchan->base + DMA_CHAN_LLI_ADDR);
	writel(DMA_CHAN_ENABLE_START, pchan->base + DMA_CHAN_ENABLE);

The uninitialized physical address 0 is fed directly to the DMA hardware
registers. Does this code need an explicit sg_len > 0 check to prevent
invalid memory accesses?

>  	return vchan_tx_prep(&vchan->vc, &txd->vd, flags);
>  
>  err_lli_free:
> -	for (p_lli = txd->p_lli, v_lli = txd->v_lli; v_lli;
> -	     p_lli = v_lli->p_lli_next, v_lli = v_lli->v_lli_next)
> +	p_lli = txd->p_lli;
> +	v_lli = txd->v_lli;
> +	while (v_lli) {
> +		struct sun6i_dma_lli *next_v_lli = v_lli->v_lli_next;
> +		dma_addr_t next_p_lli = v_lli->p_lli_next;
>  		dma_pool_free(sdev->pool, v_lli, p_lli);
> +		v_lli = next_v_lli;
> +		p_lli = next_p_lli;
> +	}
>  	kfree(txd);
>  	return NULL;
>  }

[ ... ]

> @@ -869,9 +875,15 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_dma_cyclic(

[Severity: High]
This is a pre-existing issue, but missing bounds validation in
sun6i_dma_prep_dma_cyclic() can cause a kernel panic via division by zero or
a NULL pointer dereference.

The function computes the number of periods without checking if period_len
is zero:

	unsigned int i, periods = buf_len / period_len;

If buf_len is less than period_len, periods evaluates to 0, which skips the
allocation loop and leaves prev as NULL, resulting in an immediate NULL
pointer dereference here:

	prev->p_lli_next = txd->p_lli;		/* cyclic list */

Could this be triggered by a caller passing invalid lengths, and should we
return an error early if period_len is zero or greater than buf_len?

>  	return vchan_tx_prep(&vchan->vc, &txd->vd, flags);
>  
>  err_lli_free:
> -	for (p_lli = txd->p_lli, v_lli = txd->v_lli; v_lli;
> -	     p_lli = v_lli->p_lli_next, v_lli = v_lli->v_lli_next)
> +	p_lli = txd->p_lli;
> +	v_lli = txd->v_lli;
> +	while (v_lli) {
> +		struct sun6i_dma_lli *next_v_lli = v_lli->v_lli_next;
> +		dma_addr_t next_p_lli = v_lli->p_lli_next;
>  		dma_pool_free(sdev->pool, v_lli, p_lli);
> +		v_lli = next_v_lli;
> +		p_lli = next_p_lli;
> +	}
>  	kfree(txd);
>  	return NULL;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but is there a deadlock risk due
to incompatible lock contexts for sdev->lock?

The spinlock sdev->lock is taken by sun6i_dma_tasklet() in softirq context:

	spin_lock_irq(&sdev->lock);

However, sun6i_dma_pause() and sun6i_dma_terminate_all() acquire the lock
without disabling softirqs:

	spin_lock(&sdev->lock);
	list_del_init(&vchan->node);
	spin_unlock(&sdev->lock);

If a process thread holds the lock and a hardware interrupt schedules the
tasklet, could the tasklet execute on the same CPU, attempt to acquire
sdev->lock with spin_lock_irq(), and spin forever because the preempted
process still holds it?

[Severity: High]
This is a pre-existing issue, but does a TOCTOU race condition exist
between sun6i_dma_interrupt() and sun6i_dma_terminate_all()?

In sun6i_dma_terminate_all(), the channel is stopped and pchan->desc is set
to NULL concurrently under vchan->vc.lock:

		vchan->phy = NULL;
		pchan->vchan = NULL;
		pchan->desc = NULL;
		pchan->done = NULL;

Meanwhile, the interrupt handler sun6i_dma_interrupt() processes the
completion:

				if (vchan->cyclic) {
					vchan_cyclic_callback(&pchan->desc->vd);
				} else {
					spin_lock(&vchan->vc.lock);
					vchan_cookie_complete(&pchan->desc->vd);

In the cyclic case, the handler dereferences pchan->desc->vd locklessly. In
the non-cyclic case, it waits on vchan->vc.lock but doesn't re-validate
pchan->desc after acquiring the lock. Could this lead to a NULL pointer
dereference if the descriptor is cleared by sun6i_dma_terminate_all() on
another CPU?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611063631.96566-1-zenghongling@kylinos.cn?part=1