linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] rsi: fix use-after-free, memleak and sleep-while-atomic
@ 2019-11-28 17:21 Johan Hovold
  2019-11-28 17:22 ` [PATCH 1/5] rsi: fix use-after-free on failed probe and unbind Johan Hovold
                   ` (4 more replies)
  0 siblings, 5 replies; 7+ messages in thread
From: Johan Hovold @ 2019-11-28 17:21 UTC (permalink / raw)
  To: Amitkumar Karwar, Siva Rebbagondla, Kalle Valo
  Cc: linux-wireless, netdev, linux-kernel, linux-usb, Johan Hovold

The syzbot fuzzer has reported two separate use-after-free issues,
which are fixed by the first two patches.

Turns out there were more gems in this driver and the next two patches
fixes a memory leak and a potential sleep-while-atomic found through
inspection.

The last one tightens the seemingly broken endpoint sanity check which
would have the driver try to submit a bulk URB to the default pipe (and
fail).

Tested using a mockup device.

Johan


Johan Hovold (5):
  rsi: fix use-after-free on failed probe and unbind
  rsi: fix use-after-free on probe errors
  rsi: fix memory leak on failed URB submission
  rsi: fix non-atomic allocation in completion handler
  rsi: add missing endpoint sanity checks

 drivers/net/wireless/rsi/rsi_91x_hal.c | 12 +++----
 drivers/net/wireless/rsi/rsi_91x_usb.c | 47 ++++++++++++++++++++------
 2 files changed, 43 insertions(+), 16 deletions(-)

-- 
2.24.0


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

* [PATCH 1/5] rsi: fix use-after-free on failed probe and unbind
  2019-11-28 17:21 [PATCH 0/5] rsi: fix use-after-free, memleak and sleep-while-atomic Johan Hovold
@ 2019-11-28 17:22 ` Johan Hovold
  2019-12-18 18:57   ` Kalle Valo
  2019-11-28 17:22 ` [PATCH 2/5] rsi: fix use-after-free on probe errors Johan Hovold
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 7+ messages in thread
From: Johan Hovold @ 2019-11-28 17:22 UTC (permalink / raw)
  To: Amitkumar Karwar, Siva Rebbagondla, Kalle Valo
  Cc: linux-wireless, netdev, linux-kernel, linux-usb, Johan Hovold,
	syzbot+b563b7f8dbe8223a51e8, stable, Siva Rebbagondla,
	Prameela Rani Garnepudi, Amitkumar Karwar, Fariya Fatima

Make sure to stop both URBs before returning after failed probe as well
as on disconnect to avoid use-after-free in the completion handler.

Reported-by: syzbot+b563b7f8dbe8223a51e8@syzkaller.appspotmail.com
Fixes: a4302bff28e2 ("rsi: add bluetooth rx endpoint")
Fixes: dad0d04fa7ba ("rsi: Add RS9113 wireless driver")
Cc: stable <stable@vger.kernel.org>     # 3.15
Cc: Siva Rebbagondla <siva.rebbagondla@redpinesignals.com>
Cc: Prameela Rani Garnepudi <prameela.j04cs@gmail.com>
Cc: Amitkumar Karwar <amit.karwar@redpinesignals.com>
Cc: Fariya Fatima <fariyaf@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/wireless/rsi/rsi_91x_usb.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_usb.c b/drivers/net/wireless/rsi/rsi_91x_usb.c
index 53f41fc2cadf..30bed719486e 100644
--- a/drivers/net/wireless/rsi/rsi_91x_usb.c
+++ b/drivers/net/wireless/rsi/rsi_91x_usb.c
@@ -292,6 +292,15 @@ static void rsi_rx_done_handler(struct urb *urb)
 		dev_kfree_skb(rx_cb->rx_skb);
 }
 
+static void rsi_rx_urb_kill(struct rsi_hw *adapter, u8 ep_num)
+{
+	struct rsi_91x_usbdev *dev = (struct rsi_91x_usbdev *)adapter->rsi_dev;
+	struct rx_usb_ctrl_block *rx_cb = &dev->rx_cb[ep_num - 1];
+	struct urb *urb = rx_cb->rx_urb;
+
+	usb_kill_urb(urb);
+}
+
 /**
  * rsi_rx_urb_submit() - This function submits the given URB to the USB stack.
  * @adapter: Pointer to the adapter structure.
@@ -823,10 +832,13 @@ static int rsi_probe(struct usb_interface *pfunction,
 	if (adapter->priv->coex_mode > 1) {
 		status = rsi_rx_urb_submit(adapter, BT_EP);
 		if (status)
-			goto err1;
+			goto err_kill_wlan_urb;
 	}
 
 	return 0;
+
+err_kill_wlan_urb:
+	rsi_rx_urb_kill(adapter, WLAN_EP);
 err1:
 	rsi_deinit_usb_interface(adapter);
 err:
@@ -857,6 +869,10 @@ static void rsi_disconnect(struct usb_interface *pfunction)
 		adapter->priv->bt_adapter = NULL;
 	}
 
+	if (adapter->priv->coex_mode > 1)
+		rsi_rx_urb_kill(adapter, BT_EP);
+	rsi_rx_urb_kill(adapter, WLAN_EP);
+
 	rsi_reset_card(adapter);
 	rsi_deinit_usb_interface(adapter);
 	rsi_91x_deinit(adapter);
-- 
2.24.0


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

* [PATCH 2/5] rsi: fix use-after-free on probe errors
  2019-11-28 17:21 [PATCH 0/5] rsi: fix use-after-free, memleak and sleep-while-atomic Johan Hovold
  2019-11-28 17:22 ` [PATCH 1/5] rsi: fix use-after-free on failed probe and unbind Johan Hovold
@ 2019-11-28 17:22 ` Johan Hovold
  2019-11-28 17:22 ` [PATCH 3/5] rsi: fix memory leak on failed URB submission Johan Hovold
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: Johan Hovold @ 2019-11-28 17:22 UTC (permalink / raw)
  To: Amitkumar Karwar, Siva Rebbagondla, Kalle Valo
  Cc: linux-wireless, netdev, linux-kernel, linux-usb, Johan Hovold,
	syzbot+1d1597a5aa3679c65b9f, stable, Prameela Rani Garnepudi,
	Amitkumar Karwar

The driver would fail to stop the command timer in most error paths,
something which specifically could lead to the timer being freed while
still active on I/O errors during probe.

Fix this by making sure that each function starting the timer also stops
it in all relevant error paths.

Reported-by: syzbot+1d1597a5aa3679c65b9f@syzkaller.appspotmail.com
Fixes: b78e91bcfb33 ("rsi: Add new firmware loading method")
Cc: stable <stable@vger.kernel.org>     # 4.12
Cc: Prameela Rani Garnepudi <prameela.j04cs@gmail.com>
Cc: Amitkumar Karwar <amit.karwar@redpinesignals.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/wireless/rsi/rsi_91x_hal.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_hal.c b/drivers/net/wireless/rsi/rsi_91x_hal.c
index f84250bdb8cf..6f8d5f9a9f7e 100644
--- a/drivers/net/wireless/rsi/rsi_91x_hal.c
+++ b/drivers/net/wireless/rsi/rsi_91x_hal.c
@@ -622,6 +622,7 @@ static int bl_cmd(struct rsi_hw *adapter, u8 cmd, u8 exp_resp, char *str)
 	bl_start_cmd_timer(adapter, timeout);
 	status = bl_write_cmd(adapter, cmd, exp_resp, &regout_val);
 	if (status < 0) {
+		bl_stop_cmd_timer(adapter);
 		rsi_dbg(ERR_ZONE,
 			"%s: Command %s (%0x) writing failed..\n",
 			__func__, str, cmd);
@@ -737,10 +738,9 @@ static int ping_pong_write(struct rsi_hw *adapter, u8 cmd, u8 *addr, u32 size)
 	}
 
 	status = bl_cmd(adapter, cmd_req, cmd_resp, str);
-	if (status) {
-		bl_stop_cmd_timer(adapter);
+	if (status)
 		return status;
-	}
+
 	return 0;
 }
 
@@ -828,10 +828,9 @@ static int auto_fw_upgrade(struct rsi_hw *adapter, u8 *flash_content,
 
 	status = bl_cmd(adapter, EOF_REACHED, FW_LOADING_SUCCESSFUL,
 			"EOF_REACHED");
-	if (status) {
-		bl_stop_cmd_timer(adapter);
+	if (status)
 		return status;
-	}
+
 	rsi_dbg(INFO_ZONE, "FW loading is done and FW is running..\n");
 	return 0;
 }
@@ -849,6 +848,7 @@ static int rsi_hal_prepare_fwload(struct rsi_hw *adapter)
 						  &regout_val,
 						  RSI_COMMON_REG_SIZE);
 		if (status < 0) {
+			bl_stop_cmd_timer(adapter);
 			rsi_dbg(ERR_ZONE,
 				"%s: REGOUT read failed\n", __func__);
 			return status;
-- 
2.24.0


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

* [PATCH 3/5] rsi: fix memory leak on failed URB submission
  2019-11-28 17:21 [PATCH 0/5] rsi: fix use-after-free, memleak and sleep-while-atomic Johan Hovold
  2019-11-28 17:22 ` [PATCH 1/5] rsi: fix use-after-free on failed probe and unbind Johan Hovold
  2019-11-28 17:22 ` [PATCH 2/5] rsi: fix use-after-free on probe errors Johan Hovold
@ 2019-11-28 17:22 ` Johan Hovold
  2019-11-28 17:22 ` [PATCH 4/5] rsi: fix non-atomic allocation in completion handler Johan Hovold
  2019-11-28 17:22 ` [PATCH 5/5] rsi: add missing endpoint sanity checks Johan Hovold
  4 siblings, 0 replies; 7+ messages in thread
From: Johan Hovold @ 2019-11-28 17:22 UTC (permalink / raw)
  To: Amitkumar Karwar, Siva Rebbagondla, Kalle Valo
  Cc: linux-wireless, netdev, linux-kernel, linux-usb, Johan Hovold,
	stable, Prameela Rani Garnepudi

Make sure to free the skb on failed receive-URB submission (e.g. on
disconnect or currently also due to a missing endpoint).

Fixes: a1854fae1414 ("rsi: improve RX packet handling in USB interface")
Cc: stable <stable@vger.kernel.org>     # 4.17
Cc: Prameela Rani Garnepudi <prameela.j04cs@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/wireless/rsi/rsi_91x_usb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_usb.c b/drivers/net/wireless/rsi/rsi_91x_usb.c
index 30bed719486e..2c869df1c62e 100644
--- a/drivers/net/wireless/rsi/rsi_91x_usb.c
+++ b/drivers/net/wireless/rsi/rsi_91x_usb.c
@@ -338,8 +338,10 @@ static int rsi_rx_urb_submit(struct rsi_hw *adapter, u8 ep_num)
 			  rx_cb);
 
 	status = usb_submit_urb(urb, GFP_KERNEL);
-	if (status)
+	if (status) {
 		rsi_dbg(ERR_ZONE, "%s: Failed in urb submission\n", __func__);
+		dev_kfree_skb(skb);
+	}
 
 	return status;
 }
-- 
2.24.0


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

* [PATCH 4/5] rsi: fix non-atomic allocation in completion handler
  2019-11-28 17:21 [PATCH 0/5] rsi: fix use-after-free, memleak and sleep-while-atomic Johan Hovold
                   ` (2 preceding siblings ...)
  2019-11-28 17:22 ` [PATCH 3/5] rsi: fix memory leak on failed URB submission Johan Hovold
@ 2019-11-28 17:22 ` Johan Hovold
  2019-11-28 17:22 ` [PATCH 5/5] rsi: add missing endpoint sanity checks Johan Hovold
  4 siblings, 0 replies; 7+ messages in thread
From: Johan Hovold @ 2019-11-28 17:22 UTC (permalink / raw)
  To: Amitkumar Karwar, Siva Rebbagondla, Kalle Valo
  Cc: linux-wireless, netdev, linux-kernel, linux-usb, Johan Hovold,
	stable, Prameela Rani Garnepudi

USB completion handlers are called in atomic context and must
specifically not allocate memory using GFP_KERNEL.

Fixes: a1854fae1414 ("rsi: improve RX packet handling in USB interface")
Cc: stable <stable@vger.kernel.org> # 4.17
Cc: Prameela Rani Garnepudi <prameela.j04cs@gmail.com>
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/wireless/rsi/rsi_91x_usb.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_usb.c b/drivers/net/wireless/rsi/rsi_91x_usb.c
index 2c869df1c62e..ead75574e10a 100644
--- a/drivers/net/wireless/rsi/rsi_91x_usb.c
+++ b/drivers/net/wireless/rsi/rsi_91x_usb.c
@@ -16,6 +16,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/types.h>
 #include <net/rsi_91x.h>
 #include "rsi_usb.h"
 #include "rsi_hal.h"
@@ -29,7 +30,7 @@ MODULE_PARM_DESC(dev_oper_mode,
 		 "9[Wi-Fi STA + BT LE], 13[Wi-Fi STA + BT classic + BT LE]\n"
 		 "6[AP + BT classic], 14[AP + BT classic + BT LE]");
 
-static int rsi_rx_urb_submit(struct rsi_hw *adapter, u8 ep_num);
+static int rsi_rx_urb_submit(struct rsi_hw *adapter, u8 ep_num, gfp_t flags);
 
 /**
  * rsi_usb_card_write() - This function writes to the USB Card.
@@ -285,7 +286,7 @@ static void rsi_rx_done_handler(struct urb *urb)
 	status = 0;
 
 out:
-	if (rsi_rx_urb_submit(dev->priv, rx_cb->ep_num))
+	if (rsi_rx_urb_submit(dev->priv, rx_cb->ep_num, GFP_ATOMIC))
 		rsi_dbg(ERR_ZONE, "%s: Failed in urb submission", __func__);
 
 	if (status)
@@ -307,7 +308,7 @@ static void rsi_rx_urb_kill(struct rsi_hw *adapter, u8 ep_num)
  *
  * Return: 0 on success, a negative error code on failure.
  */
-static int rsi_rx_urb_submit(struct rsi_hw *adapter, u8 ep_num)
+static int rsi_rx_urb_submit(struct rsi_hw *adapter, u8 ep_num, gfp_t mem_flags)
 {
 	struct rsi_91x_usbdev *dev = (struct rsi_91x_usbdev *)adapter->rsi_dev;
 	struct rx_usb_ctrl_block *rx_cb = &dev->rx_cb[ep_num - 1];
@@ -337,7 +338,7 @@ static int rsi_rx_urb_submit(struct rsi_hw *adapter, u8 ep_num)
 			  rsi_rx_done_handler,
 			  rx_cb);
 
-	status = usb_submit_urb(urb, GFP_KERNEL);
+	status = usb_submit_urb(urb, mem_flags);
 	if (status) {
 		rsi_dbg(ERR_ZONE, "%s: Failed in urb submission\n", __func__);
 		dev_kfree_skb(skb);
@@ -827,12 +828,12 @@ static int rsi_probe(struct usb_interface *pfunction,
 		rsi_dbg(INIT_ZONE, "%s: Device Init Done\n", __func__);
 	}
 
-	status = rsi_rx_urb_submit(adapter, WLAN_EP);
+	status = rsi_rx_urb_submit(adapter, WLAN_EP, GFP_KERNEL);
 	if (status)
 		goto err1;
 
 	if (adapter->priv->coex_mode > 1) {
-		status = rsi_rx_urb_submit(adapter, BT_EP);
+		status = rsi_rx_urb_submit(adapter, BT_EP, GFP_KERNEL);
 		if (status)
 			goto err_kill_wlan_urb;
 	}
-- 
2.24.0


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

* [PATCH 5/5] rsi: add missing endpoint sanity checks
  2019-11-28 17:21 [PATCH 0/5] rsi: fix use-after-free, memleak and sleep-while-atomic Johan Hovold
                   ` (3 preceding siblings ...)
  2019-11-28 17:22 ` [PATCH 4/5] rsi: fix non-atomic allocation in completion handler Johan Hovold
@ 2019-11-28 17:22 ` Johan Hovold
  4 siblings, 0 replies; 7+ messages in thread
From: Johan Hovold @ 2019-11-28 17:22 UTC (permalink / raw)
  To: Amitkumar Karwar, Siva Rebbagondla, Kalle Valo
  Cc: linux-wireless, netdev, linux-kernel, linux-usb, Johan Hovold

The driver expects at least one bulk-in endpoint when in "wifi-alone"
operating mode and two bulk-in endpoints otherwise, and would otherwise
fail to to submit the corresponding bulk URB to the default pipe during
probe with a somewhat cryptic message:

	rsi_91x: rsi_rx_urb_submit: Failed in urb submission
	rsi_91x: rsi_probe: Failed in probe...Exiting
	RSI-USB WLAN: probe of 2-2.4:1.0 failed with error -8

The current endpoint sanity check looks broken and would only bail out
early if there was no bulk-in endpoint but at least one bulk-out
endpoint.

Tighten this check to always require at least one bulk-in and one
bulk-out endpoint, and add the missing sanity check for a Bluetooth
bulk-in endpoint when in a BT operating mode. Also make sure to log an
informative error message when the expected endpoints are missing.

Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/net/wireless/rsi/rsi_91x_usb.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/rsi/rsi_91x_usb.c b/drivers/net/wireless/rsi/rsi_91x_usb.c
index ead75574e10a..396b9b81c1cd 100644
--- a/drivers/net/wireless/rsi/rsi_91x_usb.c
+++ b/drivers/net/wireless/rsi/rsi_91x_usb.c
@@ -149,9 +149,17 @@ static int rsi_find_bulk_in_and_out_endpoints(struct usb_interface *interface,
 			break;
 	}
 
-	if (!(dev->bulkin_endpoint_addr[0]) &&
-	    dev->bulkout_endpoint_addr[0])
+	if (!(dev->bulkin_endpoint_addr[0] && dev->bulkout_endpoint_addr[0])) {
+		dev_err(&interface->dev, "missing wlan bulk endpoints\n");
 		return -EINVAL;
+	}
+
+	if (adapter->priv->coex_mode > 1) {
+		if (!dev->bulkin_endpoint_addr[1]) {
+			dev_err(&interface->dev, "missing bt bulk-in endpoint\n");
+			return -EINVAL;
+		}
+	}
 
 	return 0;
 }
-- 
2.24.0


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

* Re: [PATCH 1/5] rsi: fix use-after-free on failed probe and unbind
  2019-11-28 17:22 ` [PATCH 1/5] rsi: fix use-after-free on failed probe and unbind Johan Hovold
@ 2019-12-18 18:57   ` Kalle Valo
  0 siblings, 0 replies; 7+ messages in thread
From: Kalle Valo @ 2019-12-18 18:57 UTC (permalink / raw)
  To: Johan Hovold
  Cc: Amitkumar Karwar, Siva Rebbagondla, linux-wireless, netdev,
	linux-kernel, linux-usb, Johan Hovold,
	syzbot+b563b7f8dbe8223a51e8, stable, Siva Rebbagondla,
	Prameela Rani Garnepudi, Amitkumar Karwar, Fariya Fatima

Johan Hovold <johan@kernel.org> wrote:

> Make sure to stop both URBs before returning after failed probe as well
> as on disconnect to avoid use-after-free in the completion handler.
> 
> Reported-by: syzbot+b563b7f8dbe8223a51e8@syzkaller.appspotmail.com
> Fixes: a4302bff28e2 ("rsi: add bluetooth rx endpoint")
> Fixes: dad0d04fa7ba ("rsi: Add RS9113 wireless driver")
> Cc: stable <stable@vger.kernel.org>     # 3.15
> Cc: Siva Rebbagondla <siva.rebbagondla@redpinesignals.com>
> Cc: Prameela Rani Garnepudi <prameela.j04cs@gmail.com>
> Cc: Amitkumar Karwar <amit.karwar@redpinesignals.com>
> Cc: Fariya Fatima <fariyaf@gmail.com>
> Signed-off-by: Johan Hovold <johan@kernel.org>

5 patches applied to wireless-drivers-next.git, thanks.

e93cd35101b6 rsi: fix use-after-free on failed probe and unbind
92aafe77123a rsi: fix use-after-free on probe errors
477682974811 rsi: fix memory leak on failed URB submission
b9b9f9fea218 rsi: fix non-atomic allocation in completion handler
960da557f435 rsi: add missing endpoint sanity checks

-- 
https://patchwork.kernel.org/patch/11266455/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

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

end of thread, other threads:[~2019-12-18 18:58 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-28 17:21 [PATCH 0/5] rsi: fix use-after-free, memleak and sleep-while-atomic Johan Hovold
2019-11-28 17:22 ` [PATCH 1/5] rsi: fix use-after-free on failed probe and unbind Johan Hovold
2019-12-18 18:57   ` Kalle Valo
2019-11-28 17:22 ` [PATCH 2/5] rsi: fix use-after-free on probe errors Johan Hovold
2019-11-28 17:22 ` [PATCH 3/5] rsi: fix memory leak on failed URB submission Johan Hovold
2019-11-28 17:22 ` [PATCH 4/5] rsi: fix non-atomic allocation in completion handler Johan Hovold
2019-11-28 17:22 ` [PATCH 5/5] rsi: add missing endpoint sanity checks Johan Hovold

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).