dmaengine: sun6i: fix use-after-free in descriptor error path

Message ID 20260810102044.1725649-1-nagachaithanya9911@gmail.com (mailing list archive)
State New
Headers
Series dmaengine: sun6i: fix use-after-free in descriptor error path |

Commit Message

Chaithanya Lagisetty Aug. 10, 2026, 10:20 a.m. UTC
The err_lli_free cleanup loops in sun6i_dma_prep_slave_sg() and
sun6i_dma_prep_dma_cyclic() free each LLI node with dma_pool_free()
and then dereference the freed node to obtain v_lli_next and
p_lli_next for the next loop iteration.

Save the next pointers before freeing, matching the pattern already used
in sun6i_dma_free_desc().

Fixes: 9aa48806edb8 ("dmaengine: sun6i: Do not use virt_to_phys")
Signed-off-by: Chaithanya Lagisetty <nagachaithanya9911@gmail.com>
---
 drivers/dma/sun6i-dma.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)
  

Comments

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

Pre-existing issues:
- [High] Missing validation for `period_len` and `buf_len` in `sun6i_dma_prep_dma_cyclic` leads to a division by zero or a NULL pointer dereference.
- [High] Missing validation for `sg_len == 0` in `sun6i_dma_prep_slave_sg` leads to hardware being programmed with physical address 0x0.
- [High] Integer truncation in the interrupt handler loop bound causes interrupts from channels >= 8 to be permanently ignored on SoCs with non-multiple-of-8 channels (e.g., sun8i-h3).
--

--- Patch [1]: [PATCH] dmaengine: sun6i: fix use-after-free in descriptor error path ---
commit 815343fddc5f554759b857f782dbda4a7c8b1a7a
Author: Chaithanya Lagisetty <nagachaithanya9911@gmail.com>

dmaengine: sun6i: fix use-after-free in descriptor error path

The err_lli_free cleanup loops in sun6i_dma_prep_slave_sg() and
sun6i_dma_prep_dma_cyclic() free each LLI node with dma_pool_free()
and then dereference the freed node to obtain v_lli_next and
p_lli_next for the next loop iteration.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..2c163ab22af5c 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -720,10 +720,10 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_slave_sg(
>  	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
>  	struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
>  	struct dma_slave_config *sconfig = &vchan->cfg;
> -	struct sun6i_dma_lli *v_lli, *prev = NULL;
> +	struct sun6i_dma_lli *v_lli, *v_next, *prev = NULL;
>  	struct sun6i_desc *txd;
>  	struct scatterlist *sg;
> -	dma_addr_t p_lli;
> +	dma_addr_t p_lli, p_next;
>  	u32 lli_cfg;
>  	int i, ret;
>  

[Severity: High]
This is a pre-existing issue, but I noticed there is no validation for
sg_len == 0.

If a caller provides an empty scatterlist, the txd is allocated but the
for_each_sg() loop is skipped:

drivers/dma/sun6i-dma.c:sun6i_dma_prep_slave_sg() {
    ...
    for_each_sg(sgl, sg, sg_len, i) {
        ...
    }
    ...
}

This leaves txd->p_lli as 0 and txd->v_lli as NULL. When this descriptor is
executed via sun6i_dma_start_desc(), the hardware is programmed with physical
address 0x0.

Could this lead to the hardware accessing physical address 0x0?

[ ... ]

> @@ -806,9 +809,9 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_dma_cyclic(
>  	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
>  	struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
>  	struct dma_slave_config *sconfig = &vchan->cfg;
> -	struct sun6i_dma_lli *v_lli, *prev = NULL;
> +	struct sun6i_dma_lli *v_lli, *v_next, *prev = NULL;
>  	struct sun6i_desc *txd;
> -	dma_addr_t p_lli;
> +	dma_addr_t p_lli, p_next;
>  	u32 lli_cfg;
>  	unsigned int i, periods = buf_len / period_len;

[Severity: High]
This is a pre-existing issue, but is it possible for period_len to be 0 here,
leading to a division by zero?

Additionally, if buf_len < period_len, periods evaluates to 0, which skips
the allocation loop. This leaves prev as NULL, and causes a NULL pointer
dereference later in the function:

drivers/dma/sun6i-dma.c:sun6i_dma_prep_dma_cyclic() {
    ...
    prev->p_lli_next = txd->p_lli;      /* cyclic list */
    ...
}

Should we validate period_len and buf_len before performing this division?

[ ... ]

While reviewing this code, I also noticed another pre-existing issue in the
interrupt handler.

[Severity: High]
This is a pre-existing issue, but I noticed a potential truncation in the
channel group calculation for the interrupt handler.

drivers/dma/sun6i-dma.c:sun6i_dma_interrupt() {
    ...
    for (i = 0; i < sdev->num_pchans / DMA_IRQ_CHAN_NR; i++) {
        ...
    }
    ...
}

For SoCs with non-multiple-of-8 channel counts (like sun8i-h3 with 12
channels), 12 / 8 evaluates to 1. This causes the handler to only process
channels 0-7 and completely skip channels 8-11.

Could this result in unhandled interrupts and system hard lockups if a DMA
transfer completes on channels 8-11?
  
Frank Li Aug. 14, 2026, 7:06 p.m. UTC | #2
On Mon, Aug 10, 2026 at 10:20:44AM +0000, Chaithanya Lagisetty wrote:
> The err_lli_free cleanup loops in sun6i_dma_prep_slave_sg() and
> sun6i_dma_prep_dma_cyclic() free each LLI node with dma_pool_free()
> and then dereference the freed node to obtain v_lli_next and
> p_lli_next for the next loop iteration.
>
> Save the next pointers before freeing, matching the pattern already used
> in sun6i_dma_free_desc().
>
> Fixes: 9aa48806edb8 ("dmaengine: sun6i: Do not use virt_to_phys")
> Signed-off-by: Chaithanya Lagisetty <nagachaithanya9911@gmail.com>
> ---

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

>  drivers/dma/sun6i-dma.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index f47a326dd7ff..2439fa6564f2 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -720,10 +720,10 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_slave_sg(
>  	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
>  	struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
>  	struct dma_slave_config *sconfig = &vchan->cfg;
> -	struct sun6i_dma_lli *v_lli, *prev = NULL;
> +	struct sun6i_dma_lli *v_lli, *v_next, *prev = NULL;
>  	struct sun6i_desc *txd;
>  	struct scatterlist *sg;
> -	dma_addr_t p_lli;
> +	dma_addr_t p_lli, p_next;
>  	u32 lli_cfg;
>  	int i, ret;
>
> @@ -789,8 +789,11 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_slave_sg(
>
>  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 = p_next, v_lli = v_next) {
> +		v_next = v_lli->v_lli_next;
> +		p_next = v_lli->p_lli_next;
>  		dma_pool_free(sdev->pool, v_lli, p_lli);
> +	}
>  	kfree(txd);
>  	return NULL;
>  }
> @@ -806,9 +809,9 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_dma_cyclic(
>  	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
>  	struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
>  	struct dma_slave_config *sconfig = &vchan->cfg;
> -	struct sun6i_dma_lli *v_lli, *prev = NULL;
> +	struct sun6i_dma_lli *v_lli, *v_next, *prev = NULL;
>  	struct sun6i_desc *txd;
> -	dma_addr_t p_lli;
> +	dma_addr_t p_lli, p_next;
>  	u32 lli_cfg;
>  	unsigned int i, periods = buf_len / period_len;
>  	int ret;
> @@ -870,8 +873,11 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_dma_cyclic(
>
>  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 = p_next, v_lli = v_next) {
> +		v_next = v_lli->v_lli_next;
> +		p_next = v_lli->p_lli_next;
>  		dma_pool_free(sdev->pool, v_lli, p_lli);
> +	}
>  	kfree(txd);
>  	return NULL;
>  }
> --
> 2.43.0
>
  
Frank Li Aug. 14, 2026, 9 p.m. UTC | #3
On Fri, Aug 14, 2026 at 03:06:30PM -0400, Frank Li wrote:
> On Mon, Aug 10, 2026 at 10:20:44AM +0000, Chaithanya Lagisetty wrote:
> > The err_lli_free cleanup loops in sun6i_dma_prep_slave_sg() and
> > sun6i_dma_prep_dma_cyclic() free each LLI node with dma_pool_free()
> > and then dereference the freed node to obtain v_lli_next and
> > p_lli_next for the next loop iteration.
> >
> > Save the next pointers before freeing, matching the pattern already used
> > in sun6i_dma_free_desc().
> >
> > Fixes: 9aa48806edb8 ("dmaengine: sun6i: Do not use virt_to_phys")
> > Signed-off-by: Chaithanya Lagisetty <nagachaithanya9911@gmail.com>
> > ---
>
> Reviewed-by: Frank Li <Frank.Li@nxp.com>

Drop review-by, I prefer use below method to fix

https://patchwork.kernel.org/project/linux-dmaengine/patch/20260727061142.44195-1-zenghongling@kylinos.cn/

Frank
>
> >  drivers/dma/sun6i-dma.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> > index f47a326dd7ff..2439fa6564f2 100644
> > --- a/drivers/dma/sun6i-dma.c
> > +++ b/drivers/dma/sun6i-dma.c
> > @@ -720,10 +720,10 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_slave_sg(
> >  	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
> >  	struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
> >  	struct dma_slave_config *sconfig = &vchan->cfg;
> > -	struct sun6i_dma_lli *v_lli, *prev = NULL;
> > +	struct sun6i_dma_lli *v_lli, *v_next, *prev = NULL;
> >  	struct sun6i_desc *txd;
> >  	struct scatterlist *sg;
> > -	dma_addr_t p_lli;
> > +	dma_addr_t p_lli, p_next;
> >  	u32 lli_cfg;
> >  	int i, ret;
> >
> > @@ -789,8 +789,11 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_slave_sg(
> >
> >  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 = p_next, v_lli = v_next) {
> > +		v_next = v_lli->v_lli_next;
> > +		p_next = v_lli->p_lli_next;
> >  		dma_pool_free(sdev->pool, v_lli, p_lli);
> > +	}
> >  	kfree(txd);
> >  	return NULL;
> >  }
> > @@ -806,9 +809,9 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_dma_cyclic(
> >  	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
> >  	struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
> >  	struct dma_slave_config *sconfig = &vchan->cfg;
> > -	struct sun6i_dma_lli *v_lli, *prev = NULL;
> > +	struct sun6i_dma_lli *v_lli, *v_next, *prev = NULL;
> >  	struct sun6i_desc *txd;
> > -	dma_addr_t p_lli;
> > +	dma_addr_t p_lli, p_next;
> >  	u32 lli_cfg;
> >  	unsigned int i, periods = buf_len / period_len;
> >  	int ret;
> > @@ -870,8 +873,11 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_dma_cyclic(
> >
> >  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 = p_next, v_lli = v_next) {
> > +		v_next = v_lli->v_lli_next;
> > +		p_next = v_lli->p_lli_next;
> >  		dma_pool_free(sdev->pool, v_lli, p_lli);
> > +	}
> >  	kfree(txd);
> >  	return NULL;
> >  }
> > --
> > 2.43.0
> >
  
Chaithanya Lagisetty Aug. 15, 2026, 7:24 a.m. UTC | #4
On Fri, Aug 14, 2026 at 05:00:21PM -0400, Frank Li wrote:
> Drop review-by, I prefer use below method to fix
>
> https://patchwork.kernel.org/project/linux-dmaengine/patch/20260727061142.44195-1-zenghongling@kylinos.cn/

Thanks Frank, agreed. Hongling Zeng's series is the better fix since it
reuses the existing sun6i_dma_free_desc() helper instead of open-coding
the save-next-pointer logic in each error path, and it removes the
duplicated cleanup loops entirely.

Please drop this patch in favour of that series; no v2 needed from my
side.

Thanks,
Chaithanya
  

Patch

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index f47a326dd7ff..2439fa6564f2 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -720,10 +720,10 @@  static struct dma_async_tx_descriptor *sun6i_dma_prep_slave_sg(
 	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
 	struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
 	struct dma_slave_config *sconfig = &vchan->cfg;
-	struct sun6i_dma_lli *v_lli, *prev = NULL;
+	struct sun6i_dma_lli *v_lli, *v_next, *prev = NULL;
 	struct sun6i_desc *txd;
 	struct scatterlist *sg;
-	dma_addr_t p_lli;
+	dma_addr_t p_lli, p_next;
 	u32 lli_cfg;
 	int i, ret;
 
@@ -789,8 +789,11 @@  static struct dma_async_tx_descriptor *sun6i_dma_prep_slave_sg(
 
 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 = p_next, v_lli = v_next) {
+		v_next = v_lli->v_lli_next;
+		p_next = v_lli->p_lli_next;
 		dma_pool_free(sdev->pool, v_lli, p_lli);
+	}
 	kfree(txd);
 	return NULL;
 }
@@ -806,9 +809,9 @@  static struct dma_async_tx_descriptor *sun6i_dma_prep_dma_cyclic(
 	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
 	struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
 	struct dma_slave_config *sconfig = &vchan->cfg;
-	struct sun6i_dma_lli *v_lli, *prev = NULL;
+	struct sun6i_dma_lli *v_lli, *v_next, *prev = NULL;
 	struct sun6i_desc *txd;
-	dma_addr_t p_lli;
+	dma_addr_t p_lli, p_next;
 	u32 lli_cfg;
 	unsigned int i, periods = buf_len / period_len;
 	int ret;
@@ -870,8 +873,11 @@  static struct dma_async_tx_descriptor *sun6i_dma_prep_dma_cyclic(
 
 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 = p_next, v_lli = v_next) {
+		v_next = v_lli->v_lli_next;
+		p_next = v_lli->p_lli_next;
 		dma_pool_free(sdev->pool, v_lli, p_lli);
+	}
 	kfree(txd);
 	return NULL;
 }