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

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 <[email protected]>

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?

>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2