dmaengine.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] fix mediatek UART APDMA desc logic
@ 2021-05-13 19:26 Guillaume Ranquet
  2021-05-13 19:26 ` [PATCH v2 1/3] dmaengine: mediatek: free the proper desc in desc_free handler Guillaume Ranquet
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Guillaume Ranquet @ 2021-05-13 19:26 UTC (permalink / raw)
  Cc: Sean Wang, Vinod Koul, Matthias Brugger, dmaengine,
	linux-arm-kernel, linux-mediatek, linux-kernel

The logic used in the apdma driver to handle the virt_dma_desc caused
panics and various memory corruption.
This is an attempt at sanitizing the logic a bit.

Sending a v2 as the previous mails were ill formatted and not threaded
properly.
I'm also removing the last patch from the series as the fix is
alread on mainline.

Guillaume Ranquet (3):
  dmaengine: mediatek: free the proper desc in desc_free handler
  dmaengine: mediatek: do not issue a new desc if one is still current
  dmaengine: mediatek: use GFP_NOWAIT instead of GFP_ATOMIC in prep_dma

 drivers/dma/mediatek/mtk-uart-apdma.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

-- 
2.26.3


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2 1/3] dmaengine: mediatek: free the proper desc in desc_free handler
  2021-05-13 19:26 [PATCH v2 0/3] fix mediatek UART APDMA desc logic Guillaume Ranquet
@ 2021-05-13 19:26 ` Guillaume Ranquet
  2021-05-13 19:26 ` [PATCH v2 2/3] dmaengine: mediatek: do not issue a new desc if one is still current Guillaume Ranquet
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Guillaume Ranquet @ 2021-05-13 19:26 UTC (permalink / raw)
  Cc: Sean Wang, Vinod Koul, Matthias Brugger, dmaengine,
	linux-arm-kernel, linux-mediatek, linux-kernel

The desc_free handler assumed that the desc we want to free was always
 the current one associated with the channel.

This is seldom the case and this is causing use after free crashes in
 multiple places (tx/rx/terminate...).

  BUG: KASAN: use-after-free in mtk_uart_apdma_rx_handler+0x120/0x304

  Call trace:
   dump_backtrace+0x0/0x1b0
   show_stack+0x24/0x34
   dump_stack+0xe0/0x150
   print_address_description+0x8c/0x55c
   __kasan_report+0x1b8/0x218
   kasan_report+0x14/0x20
   __asan_load4+0x98/0x9c
   mtk_uart_apdma_rx_handler+0x120/0x304
   mtk_uart_apdma_irq_handler+0x50/0x80
   __handle_irq_event_percpu+0xe0/0x210
   handle_irq_event+0x8c/0x184
   handle_fasteoi_irq+0x1d8/0x3ac
   __handle_domain_irq+0xb0/0x110
   gic_handle_irq+0x50/0xb8
   el0_irq_naked+0x60/0x6c

  Allocated by task 3541:
   __kasan_kmalloc+0xf0/0x1b0
   kasan_kmalloc+0x10/0x1c
   kmem_cache_alloc_trace+0x90/0x2dc
   mtk_uart_apdma_prep_slave_sg+0x6c/0x1a0
   mtk8250_dma_rx_complete+0x220/0x2e4
   vchan_complete+0x290/0x340
   tasklet_action_common+0x220/0x298
   tasklet_action+0x28/0x34
   __do_softirq+0x158/0x35c

  Freed by task 3541:
   __kasan_slab_free+0x154/0x224
   kasan_slab_free+0x14/0x24
   slab_free_freelist_hook+0xf8/0x15c
   kfree+0xb4/0x278
   mtk_uart_apdma_desc_free+0x34/0x44
   vchan_complete+0x1bc/0x340
   tasklet_action_common+0x220/0x298
   tasklet_action+0x28/0x34
   __do_softirq+0x158/0x35c

  The buggy address belongs to the object at ffff000063606800
   which belongs to the cache kmalloc-256 of size 256
  The buggy address is located 176 bytes inside of
   256-byte region [ffff000063606800, ffff000063606900)
  The buggy address belongs to the page:
  page:fffffe00016d8180 refcount:1 mapcount:0 mapping:ffff00000302f600 index:0x0 compound_mapcount: 0
  flags: 0xffff00000010200(slab|head)
  raw: 0ffff00000010200 dead000000000100 dead000000000122 ffff00000302f600
  raw: 0000000000000000 0000000080100010 00000001ffffffff 0000000000000000
  page dumped because: kasan: bad access detected

Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>

diff --git a/drivers/dma/mediatek/mtk-uart-apdma.c b/drivers/dma/mediatek/mtk-uart-apdma.c
index 27c07350971d..e38b67fc0c0c 100644
--- a/drivers/dma/mediatek/mtk-uart-apdma.c
+++ b/drivers/dma/mediatek/mtk-uart-apdma.c
@@ -131,10 +131,7 @@ static unsigned int mtk_uart_apdma_read(struct mtk_chan *c, unsigned int reg)
 
 static void mtk_uart_apdma_desc_free(struct virt_dma_desc *vd)
 {
-	struct dma_chan *chan = vd->tx.chan;
-	struct mtk_chan *c = to_mtk_uart_apdma_chan(chan);
-
-	kfree(c->desc);
+	kfree(container_of(vd, struct mtk_uart_apdma_desc, vd));
 }
 
 static void mtk_uart_apdma_start_tx(struct mtk_chan *c)
-- 
2.26.3


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v2 2/3] dmaengine: mediatek: do not issue a new desc if one is still current
  2021-05-13 19:26 [PATCH v2 0/3] fix mediatek UART APDMA desc logic Guillaume Ranquet
  2021-05-13 19:26 ` [PATCH v2 1/3] dmaengine: mediatek: free the proper desc in desc_free handler Guillaume Ranquet
@ 2021-05-13 19:26 ` Guillaume Ranquet
  2021-05-13 19:26 ` [PATCH v2 3/3] dmaengine: mediatek: use GFP_NOWAIT instead of GFP_ATOMIC in prep_dma Guillaume Ranquet
  2021-06-07  6:53 ` [PATCH v2 0/3] fix mediatek UART APDMA desc logic Vinod Koul
  3 siblings, 0 replies; 5+ messages in thread
From: Guillaume Ranquet @ 2021-05-13 19:26 UTC (permalink / raw)
  Cc: Sean Wang, Vinod Koul, Matthias Brugger, dmaengine,
	linux-arm-kernel, linux-mediatek, linux-kernel

Avoid issuing a new desc if one is still being processed as this can
lead to some desc never being marked as completed.

Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>

diff --git a/drivers/dma/mediatek/mtk-uart-apdma.c b/drivers/dma/mediatek/mtk-uart-apdma.c
index e38b67fc0c0c..a09ab2dd3b46 100644
--- a/drivers/dma/mediatek/mtk-uart-apdma.c
+++ b/drivers/dma/mediatek/mtk-uart-apdma.c
@@ -204,14 +204,9 @@ static void mtk_uart_apdma_start_rx(struct mtk_chan *c)
 
 static void mtk_uart_apdma_tx_handler(struct mtk_chan *c)
 {
-	struct mtk_uart_apdma_desc *d = c->desc;
-
 	mtk_uart_apdma_write(c, VFF_INT_FLAG, VFF_TX_INT_CLR_B);
 	mtk_uart_apdma_write(c, VFF_INT_EN, VFF_INT_EN_CLR_B);
 	mtk_uart_apdma_write(c, VFF_EN, VFF_EN_CLR_B);
-
-	list_del(&d->vd.node);
-	vchan_cookie_complete(&d->vd);
 }
 
 static void mtk_uart_apdma_rx_handler(struct mtk_chan *c)
@@ -242,9 +237,17 @@ static void mtk_uart_apdma_rx_handler(struct mtk_chan *c)
 
 	c->rx_status = d->avail_len - cnt;
 	mtk_uart_apdma_write(c, VFF_RPT, wg);
+}
 
-	list_del(&d->vd.node);
-	vchan_cookie_complete(&d->vd);
+static void mtk_uart_apdma_chan_complete_handler(struct mtk_chan *c)
+{
+	struct mtk_uart_apdma_desc *d = c->desc;
+
+	if (d) {
+		list_del(&d->vd.node);
+		vchan_cookie_complete(&d->vd);
+		c->desc = NULL;
+	}
 }
 
 static irqreturn_t mtk_uart_apdma_irq_handler(int irq, void *dev_id)
@@ -258,6 +261,7 @@ static irqreturn_t mtk_uart_apdma_irq_handler(int irq, void *dev_id)
 		mtk_uart_apdma_rx_handler(c);
 	else if (c->dir == DMA_MEM_TO_DEV)
 		mtk_uart_apdma_tx_handler(c);
+	mtk_uart_apdma_chan_complete_handler(c);
 	spin_unlock_irqrestore(&c->vc.lock, flags);
 
 	return IRQ_HANDLED;
@@ -363,7 +367,7 @@ static void mtk_uart_apdma_issue_pending(struct dma_chan *chan)
 	unsigned long flags;
 
 	spin_lock_irqsave(&c->vc.lock, flags);
-	if (vchan_issue_pending(&c->vc)) {
+	if (vchan_issue_pending(&c->vc) && !c->desc) {
 		vd = vchan_next_desc(&c->vc);
 		c->desc = to_mtk_uart_apdma_desc(&vd->tx);
 
-- 
2.26.3


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v2 3/3] dmaengine: mediatek: use GFP_NOWAIT instead of GFP_ATOMIC in prep_dma
  2021-05-13 19:26 [PATCH v2 0/3] fix mediatek UART APDMA desc logic Guillaume Ranquet
  2021-05-13 19:26 ` [PATCH v2 1/3] dmaengine: mediatek: free the proper desc in desc_free handler Guillaume Ranquet
  2021-05-13 19:26 ` [PATCH v2 2/3] dmaengine: mediatek: do not issue a new desc if one is still current Guillaume Ranquet
@ 2021-05-13 19:26 ` Guillaume Ranquet
  2021-06-07  6:53 ` [PATCH v2 0/3] fix mediatek UART APDMA desc logic Vinod Koul
  3 siblings, 0 replies; 5+ messages in thread
From: Guillaume Ranquet @ 2021-05-13 19:26 UTC (permalink / raw)
  Cc: Sean Wang, Vinod Koul, Matthias Brugger, dmaengine,
	linux-arm-kernel, linux-mediatek, linux-kernel

As recommended by the doc in:
Documentation/drivers-api/dmaengine/provider.rst

Use GFP_NOWAIT to not deplete the emergency pool.

Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>

diff --git a/drivers/dma/mediatek/mtk-uart-apdma.c b/drivers/dma/mediatek/mtk-uart-apdma.c
index a09ab2dd3b46..375e7e647df6 100644
--- a/drivers/dma/mediatek/mtk-uart-apdma.c
+++ b/drivers/dma/mediatek/mtk-uart-apdma.c
@@ -349,7 +349,7 @@ static struct dma_async_tx_descriptor *mtk_uart_apdma_prep_slave_sg
 		return NULL;
 
 	/* Now allocate and setup the descriptor */
-	d = kzalloc(sizeof(*d), GFP_ATOMIC);
+	d = kzalloc(sizeof(*d), GFP_NOWAIT);
 	if (!d)
 		return NULL;
 
-- 
2.26.3


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 0/3] fix mediatek UART APDMA desc logic
  2021-05-13 19:26 [PATCH v2 0/3] fix mediatek UART APDMA desc logic Guillaume Ranquet
                   ` (2 preceding siblings ...)
  2021-05-13 19:26 ` [PATCH v2 3/3] dmaengine: mediatek: use GFP_NOWAIT instead of GFP_ATOMIC in prep_dma Guillaume Ranquet
@ 2021-06-07  6:53 ` Vinod Koul
  3 siblings, 0 replies; 5+ messages in thread
From: Vinod Koul @ 2021-06-07  6:53 UTC (permalink / raw)
  To: Guillaume Ranquet
  Cc: Sean Wang, Matthias Brugger, dmaengine, linux-arm-kernel,
	linux-mediatek, linux-kernel

On 13-05-21, 21:26, Guillaume Ranquet wrote:
> The logic used in the apdma driver to handle the virt_dma_desc caused
> panics and various memory corruption.
> This is an attempt at sanitizing the logic a bit.

Applied, thanks

-- 
~Vinod

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-06-07  6:54 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-13 19:26 [PATCH v2 0/3] fix mediatek UART APDMA desc logic Guillaume Ranquet
2021-05-13 19:26 ` [PATCH v2 1/3] dmaengine: mediatek: free the proper desc in desc_free handler Guillaume Ranquet
2021-05-13 19:26 ` [PATCH v2 2/3] dmaengine: mediatek: do not issue a new desc if one is still current Guillaume Ranquet
2021-05-13 19:26 ` [PATCH v2 3/3] dmaengine: mediatek: use GFP_NOWAIT instead of GFP_ATOMIC in prep_dma Guillaume Ranquet
2021-06-07  6:53 ` [PATCH v2 0/3] fix mediatek UART APDMA desc logic Vinod Koul

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).