diff mbox series

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

Message ID 20260817135723.12807-3-christian.lugnberg@soundtrack.io (mailing list archive)
State New
Headers
Series dmaengine: sun6i: Fix two bugs in the DMA status path |

Commit Message

Christian Lugnberg Aug. 17, 2026, 1:51 p.m. UTC 
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(-)

Comments

sashiko-bot@kernel.org Aug. 17, 2026, 2:10 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Cross-channel hardware state corruption due to unprotected read of `vchan->phy`.
- [Critical] Use-After-Free of `pchan->desc` due to tasklet scheduling order during transfer completion.
- [High] Incorrect hardware residue reported for unmatched cookies due to missing validation in the fallback path.
--

--- Patch [2]: [PATCH v3 2/2] dmaengine: sun6i: fix undefined behaviour in sun6i_dma_tx_status ---
commit 6902e79f0976789cfe066fe9007a8898713c0622
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 to_sun6i_desc() call inside the if (vd) 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 caching vchan->phy outside the lock
creates a race window. If the thread is preempted right after this read,
an interrupt could fire, causing sun6i_dma_tasklet() in sun6i-dma.c to clear
vchan->phy and dynamically reassign the physical channel to a different
virtual channel.

When the thread resumes and acquires the lock, it might use a stale pchan
pointer, potentially reading the state of an unrelated transfer locklessly
in sun6i_dma_tx_status() or improperly terminating one in
sun6i_dma_terminate_all().

Could this unprotected read lead to cross-channel hardware state corruption?

[ ... ]

>  	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) {
>  		bytes = 0;
>  	} else {

[Severity: High]
This isn't a bug introduced by this patch, but if a requested cookie is
newly submitted or recently completed, vchan_find_desc() will return NULL.
The driver then falls back to querying the active hardware channel (pchan)
and returns its residue.

Since vchan_find_desc() only searches the desc_issued list, does this
mean the driver assumes pchan is executing the requested cookie without
verifying if pchan->desc->vd.tx.cookie matches? Could this cause the
progress of an unrelated transfer to be reported as residue?

>  		bytes = sun6i_get_chan_size(pchan);

[Severity: Critical]
This is a pre-existing issue, but when a non-cyclic transfer completes,
sun6i_dma_interrupt() in drivers/dma/sun6i-dma.c calls
vchan_cookie_complete() and then schedules the driver's tasklet.

vchan_cookie_complete() schedules the virt-dma tasklet, which executes
first and frees the descriptor memory. If another processor concurrently
queries sun6i_dma_tx_status() for an unrelated cookie, it will fall back
to calling sun6i_get_chan_size(pchan):

drivers/dma/sun6i-dma.c:sun6i_get_chan_size() {
	...
	struct sun6i_desc *txd = pchan->desc;
	...
	for (lli = txd->v_lli; lli; lli = lli->v_lli_next) {
	...
}

Since the driver's tasklet hasn't run yet to clear pchan->desc, won't
this dereference a descriptor that has already been freed by virt-dma?

>  	}
>  
>  	spin_unlock_irqrestore(&vchan->vc.lock, flags);
Frank Li Aug. 17, 2026, 2:32 p.m. UTC | #2 
On Mon, Aug 17, 2026 at 03:51:23PM +0200, Christian Lugnberg wrote:
> 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)
>
diff mbox series

Patch

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) {