[v2,2/2] dmaengine: sun6i: fix undefined behaviour in sun6i_dma_tx_status

sun6i_dma_tx_status() calls vchan_find_desc() to look up the virtual
descriptor for a given cookie, before checking whether the pointer
vd is NULL:

    vd = vchan_find_desc(&vchan->vc, cookie);
    txd = to_sun6i_desc(&vd->tx);   /* vd may be NULL here */

    if (vd) {
        for (lli = txd->v_lli; ...)

vchan_find_desc() returns NULL when the descriptor has already been
completed or is in-flight on a physical channel and no longer present
in the virtual channel's descriptor list. When vd is NULL,
to_sun6i_desc() is called unconditionally on &vd->tx before the NULL
check, which is undefined behaviour. Move the call inside the if (vd)
guard to ensure it is only reached with a valid pointer.

    vd = vchan_find_desc(&vchan->vc, cookie);
    if (vd) {
        struct sun6i_desc *txd = to_sun6i_desc(&vd->tx);
        for (lli = txd->v_lli; ...)

Fixes: 555859308723 ("dmaengine: sun6i: Add driver for the Allwinner A31 DMA controller")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-sonnet-4-6
Signed-off-by: Christian Lugnberg <christian.lugnberg@soundtrack.io>
---
 drivers/dma/sun6i-dma.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
  

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

Pre-existing issues:
- [Critical] Lockless read of `vchan->phy` leads to Time-of-Check to Time-of-Use (TOCTOU) Use-After-Free and state corruption.
- [Critical] Interrupt handler races with terminate_all, causing a NULL pointer dereference.
- [High] sun6i_dma_tx_status incorrectly reports the residue of the active descriptor for unissued descriptors due to missing cookie validation.
- [High] Deadlock risk due to plain spin_lock() usage on sdev->lock which is shared with a softirq context.
--

--- Patch [2]: [PATCH v2 2/2] dmaengine: sun6i: fix undefined behaviour in sun6i_dma_tx_status ---
commit fc20cb9c5238894b0536bcd9408e43b44dedb04e
Author: Christian Lugnberg <christian.lugnberg@soundtrack.io>

dmaengine: sun6i: fix undefined behaviour in sun6i_dma_tx_status

sun6i_dma_tx_status() calls vchan_find_desc() to look up the virtual
descriptor for a given cookie, before checking whether the pointer
vd is NULL. This patch moves the check inside the guard to ensure it is
only reached with a valid pointer.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index eb060ba2d4738..070101c109a3f 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -984,7 +984,6 @@ static enum dma_status sun6i_dma_tx_status(struct dma_chan *chan,
>  	struct sun6i_pchan *pchan = vchan->phy;

[Severity: Critical]
This is a pre-existing issue, but does this lockless read of vchan->phy
introduce a time-of-check to time-of-use (TOCTOU) use-after-free risk?

If sun6i_dma_tasklet concurrently frees this physical channel and allocates
it to a different stream, by the time sun6i_dma_tx_status() acquires the lock
below and calls sun6i_get_chan_size(pchan), the descriptor might belong to
another channel and could be concurrently freed. Could this cause a
use-after-free when iterating txd->v_lli?

>  	struct sun6i_dma_lli *lli;
>  	struct virt_dma_desc *vd;
[ ... ]
> @@ -996,9 +995,9 @@ static enum dma_status sun6i_dma_tx_status(struct dma_chan *chan,
>  	spin_lock_irqsave(&vchan->vc.lock, flags);
>  
>  	vd = vchan_find_desc(&vchan->vc, cookie);
> -	txd = to_sun6i_desc(&vd->tx);
>  
>  	if (vd) {
> +		struct sun6i_desc *txd = to_sun6i_desc(&vd->tx);
>  		for (lli = txd->v_lli; lli != NULL; lli = lli->v_lli_next)
>  			bytes += lli->len;
>  	} else if (!pchan || !pchan->desc) {

[Severity: High]
This is a pre-existing issue, but does this fallback logic incorrectly report
the residue of the active descriptor for unissued descriptors?

When queried for a submitted but unissued descriptor, vchan_find_desc() returns
NULL because the descriptor is in desc_submitted instead of desc_issued. The
code then falls back to calculating residue for the currently running transfer
using pchan->desc in the else block, without verifying if
pchan->desc->vd.tx.cookie matches the requested cookie.

While reviewing the file, I noticed a couple of other pre-existing issues:

[Severity: Critical]
This is a pre-existing issue, but is there a race condition between the
interrupt handler and sun6i_dma_terminate_all() that can lead to a NULL
pointer dereference?

If sun6i_dma_interrupt() reads vchan locklessly and proceeds to handle it,
while sun6i_dma_terminate_all() concurrently runs on another CPU, acquires
vchan->vc.lock, and clears pchan->desc to NULL:

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

Will the lockless call to vchan_cyclic_callback() or the subsequent call to
vchan_cookie_complete() dereference a NULL pointer since the descriptor could
have just been cleared?

[Severity: High]
This is a pre-existing issue, but is there a deadlock risk due to plain
spin_lock() usage on sdev->lock?

In sun6i_dma_terminate_all() and sun6i_dma_pause():

drivers/dma/sun6i-dma.c:sun6i_dma_pause() {
    ...
    spin_lock(&sdev->lock);
    ...
}

These functions acquire sdev->lock using a plain spin_lock() from process
context, leaving softirqs enabled. If a softirq interrupts the CPU while the
lock is held, and sun6i_dma_tasklet (running in softirq context) attempts to
acquire the same lock via spin_lock_irq(&sdev->lock), might this result in a
deadlock?
  

On Fri, Aug 14, 2026 at 04:21:11PM +0200, Christian Lugnberg wrote:
> [You don't often get email from christian.lugnberg@soundtrack.io. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> sun6i_dma_tx_status() calls vchan_find_desc() to look up the virtual
> descriptor for a given cookie, before checking whether the pointer
> vd is NULL:
>
>     vd = vchan_find_desc(&vchan->vc, cookie);
>     txd = to_sun6i_desc(&vd->tx);   /* vd may be NULL here */
>
>     if (vd) {
>         for (lli = txd->v_lli; ...)
>
> vchan_find_desc() returns NULL when the descriptor has already been
> completed or is in-flight on a physical channel and no longer present
> in the virtual channel's descriptor list. When vd is NULL,
> to_sun6i_desc() is called unconditionally on &vd->tx before the NULL
> check, which is undefined behaviour. Move the call inside the if (vd)
> guard to ensure it is only reached with a valid pointer.
>
>     vd = vchan_find_desc(&vchan->vc, cookie);
>     if (vd) {
>         struct sun6i_desc *txd = to_sun6i_desc(&vd->tx);
>         for (lli = txd->v_lli; ...)
>
> Fixes: 555859308723 ("dmaengine: sun6i: Add driver for the Allwinner A31 DMA controller")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-sonnet-4-6
> Signed-off-by: Christian Lugnberg <christian.lugnberg@soundtrack.io>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/dma/sun6i-dma.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 04fe1f5042e9..7704b016aed8 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -981,7 +981,6 @@ static enum dma_status sun6i_dma_tx_status(struct dma_chan *chan,
>         struct sun6i_pchan *pchan = vchan->phy;
>         struct sun6i_dma_lli *lli;
>         struct virt_dma_desc *vd;
> -       struct sun6i_desc *txd;
>         enum dma_status ret;
>         unsigned long flags;
>         size_t bytes = 0;
> @@ -993,9 +992,9 @@ static enum dma_status sun6i_dma_tx_status(struct dma_chan *chan,
>         spin_lock_irqsave(&vchan->vc.lock, flags);
>
>         vd = vchan_find_desc(&vchan->vc, cookie);
> -       txd = to_sun6i_desc(&vd->tx);
>
>         if (vd) {
> +               struct sun6i_desc *txd = to_sun6i_desc(&vd->tx);
>                 for (lli = txd->v_lli; lli != NULL; lli = lli->v_lli_next)
>                         bytes += lli->len;
>         } else if (!pchan || !pchan->desc) {
> --
> 2.54.0 (Apple Git-156)
>