2019-11-07 11:28:33

by Pan Bian

Subject: [PATCH] dmaengine: sun6i: Fix use after free

The members in the LLI list is released in an incorrect way. Read and
store the next member before releasing it to avoid accessing the freed
memory.

Fixes: a90e173f3faf ("dmaengine: sun6i: Add cyclic capability")

Signed-off-by: Pan Bian <[email protected]>
---
drivers/dma/sun6i-dma.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
index 06cd7f867f7c..096aad7e75bb 100644
--- a/drivers/dma/sun6i-dma.c
+++ b/drivers/dma/sun6i-dma.c
@@ -687,7 +687,7 @@ 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, *next, *prev = NULL;
struct sun6i_desc *txd;
struct scatterlist *sg;
dma_addr_t p_lli;
@@ -752,8 +752,12 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_slave_sg(
return vchan_tx_prep(&vchan->vc, &txd->vd, flags);

err_lli_free:
- for (prev = txd->v_lli; prev; prev = prev->v_lli_next)
- dma_pool_free(sdev->pool, prev, virt_to_phys(prev));
+ v_lli = txd->v_lli;
+ while (v_lli) {
+ next = v_lli->v_lli_next;
+ dma_pool_free(sdev->pool, v_lli, virt_to_phys(v_lli));
+ v_lli = next;
+ }
kfree(txd);
return NULL;
}
@@ -769,7 +773,7 @@ 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, *next, *prev = NULL;
struct sun6i_desc *txd;
dma_addr_t p_lli;
u32 lli_cfg;
@@ -820,8 +824,12 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_dma_cyclic(
return vchan_tx_prep(&vchan->vc, &txd->vd, flags);

err_lli_free:
- for (prev = txd->v_lli; prev; prev = prev->v_lli_next)
- dma_pool_free(sdev->pool, prev, virt_to_phys(prev));
+ v_lli = txd->v_lli;
+ while (v_lli) {
+ next = v_lli->v_lli_next;
+ dma_pool_free(sdev->pool, v_lli, virt_to_phys(v_lli));
+ v_lli = next;
+ }
kfree(txd);
return NULL;
}
--
2.7.4

2019-11-14 06:47:31

by Vinod Koul

Subject: Re: [PATCH] dmaengine: sun6i: Fix use after free

On 07-11-19, 19:26, Pan Bian wrote:
> The members in the LLI list is released in an incorrect way. Read and
> store the next member before releasing it to avoid accessing the freed
> memory.
>
> Fixes: a90e173f3faf ("dmaengine: sun6i: Add cyclic capability")
>
> Signed-off-by: Pan Bian <[email protected]>
> ---
> drivers/dma/sun6i-dma.c | 20 ++++++++++++++------
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 06cd7f867f7c..096aad7e75bb 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -687,7 +687,7 @@ 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, *next, *prev = NULL;
> struct sun6i_desc *txd;
> struct scatterlist *sg;
> dma_addr_t p_lli;
> @@ -752,8 +752,12 @@ static struct dma_async_tx_descriptor *sun6i_dma_prep_slave_sg(
> return vchan_tx_prep(&vchan->vc, &txd->vd, flags);
>
> err_lli_free:
> - for (prev = txd->v_lli; prev; prev = prev->v_lli_next)
> - dma_pool_free(sdev->pool, prev, virt_to_phys(prev));
> + v_lli = txd->v_lli;
> + while (v_lli) {
> + next = v_lli->v_lli_next;
> + dma_pool_free(sdev->pool, v_lli, virt_to_phys(v_lli));
> + v_lli = next;

Have you seen this issue, a panic trace? Has some static checker flagged
this?

I see the code *seems* equivalent. The prev is assigned to txd->v_lli,
checked and then code block will be executed and updated. When do you
see the case when we access freed memory?

--
~Vinod