diff mbox series

[2/2] dmaengine: sun6i: fix null pointer dereference in sun6i_dma_tx_status

Message ID 20260814132906.70322-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. 14, 2026, 1:28 p.m. UTC 
sun6i_dma_tx_status() calls vchan_find_desc() to look up the virtual
descriptor for a given cookie, then unconditionally dereferences the
result via to_sun6i_desc() before checking whether the pointer 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. Dereferencing NULL via
to_sun6i_desc() in that case is undefined behaviour and will oops on
any architecture that faults on NULL pointer access.

Move the to_sun6i_desc() call inside the if (vd) block so it is only
reached when vd is known to be non-NULL:

    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. 14, 2026, 1:37 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] The commit message falsely claims to fix a NULL pointer dereference that causes a kernel oops, but the original code performs no memory access at that location.

Pre-existing issues:
- [High] TOCTOU race condition from lockless read of `vchan->phy` causes hardware state corruption, use-after-free, and NULL pointer dereferences.
- [Medium] sun6i_dma_tx_status fails to verify the active descriptor's cookie, causing it to report incorrect residue for unrelated cookies.
--

--- Patch [2]: [PATCH 2/2] dmaengine: sun6i: fix null pointer dereference in sun6i_dma_tx_status ---
commit a4a11017b14d1cd4a177bac4da56b1c3eb480161
Author: Christian Lugnberg <christian.lugnberg@soundtrack.io>

dmaengine: sun6i: fix null pointer dereference in sun6i_dma_tx_status

This commit attempts to fix an alleged null pointer dereference in
sun6i_dma_tx_status() by moving the to_sun6i_desc() call inside the
NULL check for vd.

> 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. Dereferencing NULL via
> to_sun6i_desc() in that case is undefined behaviour and will oops on
> any architecture that faults on NULL pointer access.

[Severity: Medium]
Is this description accurate?

Looking at the original code, to_sun6i_desc() expands to a container_of()
macro on &vd->tx. Since tx is at offset 0 of virt_dma_desc and vd is at
offset 0 of sun6i_desc, calculating the offset on a NULL pointer evaluates
to NULL through pure pointer arithmetic.

Does this actually perform a memory access that would trigger a hardware
fault or oops?

> 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: High]
This is a pre-existing issue, but can this lockless read lead to state
corruption or a use-after-free?

If vchan->phy is read into the local variable pchan outside the lock,
could a concurrent tasklet take the lock, complete the transfer, clear
vchan->phy, and reassign the physical channel to a completely different
virtual channel before we acquire the lock here?

If so, would we be operating on a reassigned pchan without verifying
ownership inside the lock? It looks like this same pattern exists in
sun6i_dma_pause(), sun6i_dma_resume(), and sun6i_dma_terminate_all().

>  	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;

[ ... ]

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

[Severity: Medium]
This is a pre-existing issue, but does this code accurately report the
residue if the queried cookie doesn't match the active descriptor?

If the queried cookie is in the desc_submitted list and therefore not
found by vchan_find_desc(), this fallback branch assumes pchan->desc
corresponds to the requested transfer.

Should we verify if pchan->desc->vd.tx.cookie == cookie before blindly
calling sun6i_get_chan_size(pchan) and returning the residue of a
potentially unrelated transfer?

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