Linux-Wireless Archive on lore.kernel.org
 help / Atom feed
* [PATCH wireless-drivers 0/3] fix multiple issues in mt76u error path
@ 2019-02-10 21:49 Lorenzo Bianconi
  2019-02-10 21:49 ` [PATCH wireless-drivers 1/3] mt76: usb: fix possible NULL pointer dereference in mt76u_mcu_deinit Lorenzo Bianconi
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Lorenzo Bianconi @ 2019-02-10 21:49 UTC (permalink / raw)
  To: linux-wireless; +Cc: nbd, sgruszka, stefan.wahren

Fix following issues in mt76u initialization error path:
- NULL pointer dereference in mt76u_mcu_deinit
- possible memory leak in mt76u_buf_free
- use-after-free warning since mt76u_queues_deinit is run twice

This series has been tested running mt76x0u driver on rpi3+
(dwc_otg controller does not support SG I/O)

Lorenzo Bianconi (3):
  mt76: usb: fix possible NULL pointer dereference in mt76u_mcu_deinit
  mt76: usb: fix possible memory leak in mt76u_buf_free
  mt76: usb: do not run mt76u_queues_deinit twice

 drivers/net/wireless/mediatek/mt76/usb.c     | 25 ++++++++++----------
 drivers/net/wireless/mediatek/mt76/usb_mcu.c |  8 ++++---
 2 files changed, 17 insertions(+), 16 deletions(-)

-- 
2.20.1


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

* [PATCH wireless-drivers 1/3] mt76: usb: fix possible NULL pointer dereference in mt76u_mcu_deinit
  2019-02-10 21:49 [PATCH wireless-drivers 0/3] fix multiple issues in mt76u error path Lorenzo Bianconi
@ 2019-02-10 21:49 ` Lorenzo Bianconi
  2019-02-10 21:49 ` [PATCH wireless-drivers 2/3] mt76: usb: fix possible memory leak in mt76u_buf_free Lorenzo Bianconi
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Lorenzo Bianconi @ 2019-02-10 21:49 UTC (permalink / raw)
  To: linux-wireless; +Cc: nbd, sgruszka, stefan.wahren

Fix possible NULL pointer dereference in mt76u_mcu_deinit routine that
can occur if initialization path fails before calling mt76u_mcu_init_rx

Fixes: ee676cd5017c ("mt76: add driver code for MT76x2u based devices")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 drivers/net/wireless/mediatek/mt76/usb_mcu.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/usb_mcu.c b/drivers/net/wireless/mediatek/mt76/usb_mcu.c
index 036be4163e69..9527e1216f3d 100644
--- a/drivers/net/wireless/mediatek/mt76/usb_mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/usb_mcu.c
@@ -48,9 +48,11 @@ EXPORT_SYMBOL_GPL(mt76u_mcu_init_rx);
 
 void mt76u_mcu_deinit(struct mt76_dev *dev)
 {
-	struct mt76_usb *usb = &dev->usb;
+	struct mt76u_buf *buf = &dev->usb.mcu.res;
 
-	usb_kill_urb(usb->mcu.res.urb);
-	mt76u_buf_free(&usb->mcu.res);
+	if (buf->urb) {
+		usb_kill_urb(buf->urb);
+		mt76u_buf_free(buf);
+	}
 }
 EXPORT_SYMBOL_GPL(mt76u_mcu_deinit);
-- 
2.20.1


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

* [PATCH wireless-drivers 2/3] mt76: usb: fix possible memory leak in mt76u_buf_free
  2019-02-10 21:49 [PATCH wireless-drivers 0/3] fix multiple issues in mt76u error path Lorenzo Bianconi
  2019-02-10 21:49 ` [PATCH wireless-drivers 1/3] mt76: usb: fix possible NULL pointer dereference in mt76u_mcu_deinit Lorenzo Bianconi
@ 2019-02-10 21:49 ` Lorenzo Bianconi
  2019-02-10 21:49 ` [PATCH wireless-drivers 3/3] mt76: usb: do not run mt76u_queues_deinit twice Lorenzo Bianconi
  2019-02-12 18:59 ` [PATCH wireless-drivers 0/3] fix multiple issues in mt76u error path Kalle Valo
  3 siblings, 0 replies; 5+ messages in thread
From: Lorenzo Bianconi @ 2019-02-10 21:49 UTC (permalink / raw)
  To: linux-wireless; +Cc: nbd, sgruszka, stefan.wahren

Move q->ndesc initialization before the for loop in mt76u_alloc_rx
since otherwise allocated urbs will not be freed in mt76u_buf_free
Double-check scatterlist pointer in mt76u_buf_free

Fixes: b40b15e1521f ("mt76: add usb support to mt76 layer")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index b061263453d4..31c18ac1c94a 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -331,10 +331,16 @@ EXPORT_SYMBOL_GPL(mt76u_buf_alloc);
 void mt76u_buf_free(struct mt76u_buf *buf)
 {
 	struct urb *urb = buf->urb;
+	struct scatterlist *sg;
 	int i;
 
-	for (i = 0; i < urb->num_sgs; i++)
-		skb_free_frag(sg_virt(&urb->sg[i]));
+	for (i = 0; i < urb->num_sgs; i++) {
+		sg = &urb->sg[i];
+		if (!sg)
+			continue;
+
+		skb_free_frag(sg_virt(sg));
+	}
 	usb_free_urb(buf->urb);
 }
 EXPORT_SYMBOL_GPL(mt76u_buf_free);
@@ -540,7 +546,8 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)
 		nsgs = 1;
 	}
 
-	for (i = 0; i < MT_NUM_RX_ENTRIES; i++) {
+	q->ndesc = MT_NUM_RX_ENTRIES;
+	for (i = 0; i < q->ndesc; i++) {
 		err = mt76u_buf_alloc(dev, &q->entry[i].ubuf,
 				      nsgs, q->buf_size,
 				      SKB_WITH_OVERHEAD(q->buf_size),
@@ -548,7 +555,6 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)
 		if (err < 0)
 			return err;
 	}
-	q->ndesc = MT_NUM_RX_ENTRIES;
 
 	return mt76u_submit_rx_buffers(dev);
 }
-- 
2.20.1


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

* [PATCH wireless-drivers 3/3] mt76: usb: do not run mt76u_queues_deinit twice
  2019-02-10 21:49 [PATCH wireless-drivers 0/3] fix multiple issues in mt76u error path Lorenzo Bianconi
  2019-02-10 21:49 ` [PATCH wireless-drivers 1/3] mt76: usb: fix possible NULL pointer dereference in mt76u_mcu_deinit Lorenzo Bianconi
  2019-02-10 21:49 ` [PATCH wireless-drivers 2/3] mt76: usb: fix possible memory leak in mt76u_buf_free Lorenzo Bianconi
@ 2019-02-10 21:49 ` Lorenzo Bianconi
  2019-02-12 18:59 ` [PATCH wireless-drivers 0/3] fix multiple issues in mt76u error path Kalle Valo
  3 siblings, 0 replies; 5+ messages in thread
From: Lorenzo Bianconi @ 2019-02-10 21:49 UTC (permalink / raw)
  To: linux-wireless; +Cc: nbd, sgruszka, stefan.wahren

Do not call mt76u_queues_deinit routine in mt76u_alloc_queues error path
since it will be run in mt76x0u_register_device or
mt76x2u_register_device error path. Current implementation triggers the
following kernel warning:

[   67.005516] WARNING: CPU: 2 PID: 761 at lib/refcount.c:187 refcount_sub_and_test_checked+0xa4/0xb8
[   67.019513] refcount_t: underflow; use-after-free.
[   67.099872] Hardware name: BCM2835
[   67.106268] Backtrace:
[   67.111584] [<8010c91c>] (dump_backtrace) from [<8010cc00>] (show_stack+0x20/0x24)
[   67.124974]  r6:60000013 r5:ffffffff r4:00000000 r3:a50bade6
[   67.132226] [<8010cbe0>] (show_stack) from [<807ca5f4>] (dump_stack+0xc8/0x114)
[   67.141225] [<807ca52c>] (dump_stack) from [<8011e65c>] (__warn+0xf4/0x120)
[   67.149849]  r9:000000bb r8:804d0138 r7:00000009 r6:8099dc84 r5:00000000 r4:b66c7b58
[   67.160767] [<8011e568>] (__warn) from [<8011e6d0>] (warn_slowpath_fmt+0x48/0x50)
[   67.171436]  r9:7f65e128 r8:80d1419c r7:80c0bac4 r6:b97b3044 r5:b7368e00 r4:00000000
[   67.182433] [<8011e68c>] (warn_slowpath_fmt) from [<804d0138>] (refcount_sub_and_test_checked+0xa4/0xb8)
[   67.195221]  r3:80c91c25 r2:8099dc94
[   67.200370]  r4:00000000
[   67.204397] [<804d0094>] (refcount_sub_and_test_checked) from [<804d0164>] (refcount_dec_and_test_checked+0x18/0x1c)
[   67.218046]  r4:b7368e00 r3:00000001
[   67.223125] [<804d014c>] (refcount_dec_and_test_checked) from [<805db49c>] (usb_free_urb+0x20/0x4c)
[   67.235358] [<805db47c>] (usb_free_urb) from [<7f639804>] (mt76u_buf_free+0x98/0xac [mt76_usb])
[   67.247302]  r4:00000001 r3:00000001
[   67.252468] [<7f63976c>] (mt76u_buf_free [mt76_usb]) from [<7f639ef8>] (mt76u_queues_deinit+0x44/0x100 [mt76_usb])
[   67.266102]  r8:b8fe8600 r7:b5dac480 r6:b5dace20 r5:00000001 r4:00000000 r3:00000080
[   67.277132] [<7f639eb4>] (mt76u_queues_deinit [mt76_usb]) from [<7f65c040>] (mt76x0u_cleanup+0x40/0x4c [mt76x0u])
[   67.290737]  r7:b5dac480 r6:b8fe8600 r5:ffffffea r4:b5dace20
[   67.298069] [<7f65c000>] (mt76x0u_cleanup [mt76x0u]) from [<7f65c564>] (mt76x0u_probe+0x1f0/0x354 [mt76x0u])
[   67.311174]  r4:b5dace20 r3:00000000
[   67.316312] [<7f65c374>] (mt76x0u_probe [mt76x0u]) from [<805e0b6c>] (usb_probe_interface+0x104/0x240)
[   67.328915]  r7:00000000 r6:7f65e034 r5:b6634800 r4:b8fe8620
[   67.336276] [<805e0a68>] (usb_probe_interface) from [<8056a8bc>] (really_probe+0x224/0x2f8)
[   67.347965]  r10:b65f0a00 r9:00000019 r8:7f65e034 r7:80d3e124 r6:00000000 r5:80d3e120
[   67.359175]  r4:b8fe8620 r3:805e0a68
[   67.364384] [<8056a698>] (really_probe) from [<8056ab60>] (driver_probe_device+0x6c/0x180)
[   67.375974]  r10:b65f0a00 r9:7f65e2c0 r8:b8fe8620 r7:00000000 r6:7f65e034 r5:7f65e034
[   67.387170]  r4:b8fe8620 r3:00000000
[   67.392378] [<8056aaf4>] (driver_probe_device) from [<8056ad54>] (__driver_attach+0xe0/0xe4)
[   67.404097]  r9:7f65e2c0 r8:7f65d22c r7:00000000 r6:b8fe8654 r5:7f65e034 r4:b8fe8620
[   67.415122] [<8056ac74>] (__driver_attach) from [<8056880c>] (bus_for_each_dev+0x68/0xa0)
[   67.426628]  r6:8056ac74 r5:7f65e034 r4:00000000 r3:00000027
[   67.434017] [<805687a4>] (bus_for_each_dev) from [<8056a1cc>] (driver_attach+0x28/0x30)
[   67.445394]  r6:80c6ddc8 r5:b7368f80 r4:7f65e034
[   67.451703] [<8056a1a4>] (driver_attach) from [<80569c24>] (bus_add_driver+0x194/0x21c)
[   67.463081] [<80569a90>] (bus_add_driver) from [<8056b504>] (driver_register+0x8c/0x124)
[   67.474560]  r7:80c6ddc8 r6:7f65e034 r5:00000000 r4:7f65e034
[   67.481964] [<8056b478>] (driver_register) from [<805df510>] (usb_register_driver+0x74/0x140)
[   67.493901]  r5:00000000 r4:7f65e000
[   67.499131] [<805df49c>] (usb_register_driver) from [<7f661024>] (mt76x0_driver_init+0x24/0x1000 [mt76x0u])
[   67.512258]  r9:00000001 r8:7f65e308 r7:00000000 r6:80c08d48 r5:7f661000 r4:7f65e2c0
[   67.523404] [<7f661000>] (mt76x0_driver_init [mt76x0u]) from [<80102f6c>] (do_one_initcall+0x4c/0x210)
[   67.536142] [<80102f20>] (do_one_initcall) from [<801ae63c>] (do_init_module+0x6c/0x21c)
[   67.547639]  r8:7f65e308 r7:80c08d48 r6:b65f0ac0 r5:7f65e2c0 r4:7f65e2c0
[   67.556129] [<801ae5d0>] (do_init_module) from [<801ad68c>] (load_module+0x1d10/0x2304)

Fixes: b40b15e1521f ("mt76: add usb support to mt76 layer")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 drivers/net/wireless/mediatek/mt76/usb.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 31c18ac1c94a..8920add3fdfa 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -844,16 +844,9 @@ int mt76u_alloc_queues(struct mt76_dev *dev)
 
 	err = mt76u_alloc_rx(dev);
 	if (err < 0)
-		goto err;
-
-	err = mt76u_alloc_tx(dev);
-	if (err < 0)
-		goto err;
+		return err;
 
-	return 0;
-err:
-	mt76u_queues_deinit(dev);
-	return err;
+	return mt76u_alloc_tx(dev);
 }
 EXPORT_SYMBOL_GPL(mt76u_alloc_queues);
 
-- 
2.20.1


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

* Re: [PATCH wireless-drivers 0/3] fix multiple issues in mt76u error path
  2019-02-10 21:49 [PATCH wireless-drivers 0/3] fix multiple issues in mt76u error path Lorenzo Bianconi
                   ` (2 preceding siblings ...)
  2019-02-10 21:49 ` [PATCH wireless-drivers 3/3] mt76: usb: do not run mt76u_queues_deinit twice Lorenzo Bianconi
@ 2019-02-12 18:59 ` Kalle Valo
  3 siblings, 0 replies; 5+ messages in thread
From: Kalle Valo @ 2019-02-12 18:59 UTC (permalink / raw)
  To: Lorenzo Bianconi; +Cc: linux-wireless, nbd, sgruszka, stefan.wahren

Lorenzo Bianconi <lorenzo.bianconi@redhat.com> writes:

> Fix following issues in mt76u initialization error path:
> - NULL pointer dereference in mt76u_mcu_deinit
> - possible memory leak in mt76u_buf_free
> - use-after-free warning since mt76u_queues_deinit is run twice
>
> This series has been tested running mt76x0u driver on rpi3+
> (dwc_otg controller does not support SG I/O)
>
> Lorenzo Bianconi (3):
>   mt76: usb: fix possible NULL pointer dereference in mt76u_mcu_deinit
>   mt76: usb: fix possible memory leak in mt76u_buf_free
>   mt76: usb: do not run mt76u_queues_deinit twice

As we are in -rc6 now, better to push these via -next. So I assume Felix
will take these.

-- 
Kalle Valo

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

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-10 21:49 [PATCH wireless-drivers 0/3] fix multiple issues in mt76u error path Lorenzo Bianconi
2019-02-10 21:49 ` [PATCH wireless-drivers 1/3] mt76: usb: fix possible NULL pointer dereference in mt76u_mcu_deinit Lorenzo Bianconi
2019-02-10 21:49 ` [PATCH wireless-drivers 2/3] mt76: usb: fix possible memory leak in mt76u_buf_free Lorenzo Bianconi
2019-02-10 21:49 ` [PATCH wireless-drivers 3/3] mt76: usb: do not run mt76u_queues_deinit twice Lorenzo Bianconi
2019-02-12 18:59 ` [PATCH wireless-drivers 0/3] fix multiple issues in mt76u error path Kalle Valo

Linux-Wireless Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-wireless/0 linux-wireless/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-wireless linux-wireless/ https://lore.kernel.org/linux-wireless \
		linux-wireless@vger.kernel.org linux-wireless@archiver.kernel.org
	public-inbox-index linux-wireless


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-wireless


AGPL code for this site: git clone https://public-inbox.org/ public-inbox