diff mbox series

ath9k: fix use-after-free read at ath9k_hif_usb_rx_cb()

Message ID 3ec24824-00b6-4fc3-8bcf-71b9bbcb69c2@I-love.SAKURA.ne.jp
State New
Headers show
Series ath9k: fix use-after-free read at ath9k_hif_usb_rx_cb() | expand

Commit Message

Tetsuo Handa April 29, 2022, 11:18 a.m. UTC 
Although hif_dev->htc_handle is allocated by ath9k_htc_hw_alloc() from
ath9k_hif_usb_firmware_cb(), hif_dev->htc_handle->drv_priv is not assigned
until ieee80211_alloc_hw() from ath9k_htc_probe_device() from
ath9k_htc_hw_init() from ath9k_hif_usb_firmware_cb() returns. However, as
soon as ath9k_hif_usb_alloc_rx_urbs() from ath9k_hif_usb_alloc_urbs() from
ath9k_hif_usb_dev_init() from ath9k_hif_usb_firmware_cb() returns, a timer
interrupt can access hif_dev->htc_handle->drv_priv via RX_STAT_INC() from
ath9k_hif_usb_rx_stream() from ath9k_hif_usb_rx_cb() from
usb_hcd_giveback_urb(), which results in NULL pointer deference problem.

Also, even after htc_handle->drv_priv is assigned, when
ath9k_htc_wait_for_target() from ath9k_htc_probe_device() from
ath9k_htc_hw_init() from ath9k_hif_usb_firmware_cb() fails,
ieee80211_free_hw() (which does not reset hif_dev->htc_handle->drv_priv)
is immediately called due to "goto err_free;". As a result, a timer
interrupt which happens after ieee80211_free_hw() triggers use-after-free
problem at the abovementioned location.

We can flush in-flight requests by calling ath9k_hif_usb_dealloc_urbs()
before calling ieee80211_free_hw(). But we need to take from two choices
for not yet assigned case. One is to change e.g. RX_STAT_INC() to check
for NULL because it depends on CONFIG_ATH9K_HTC_DEBUGFS=y. The other is to
assign a dummy object which is used until initialization. This patch took
the latter.

Link: https://syzkaller.appspot.com/bug?extid=03110230a11411024147
Reported-by: syzbot <syzbot+03110230a11411024147@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Tested-by: syzbot <syzbot+03110230a11411024147@syzkaller.appspotmail.com>
---
Pavel Skripkin has tested "check for NULL" approach, but not yet accepted.
What was wrong with Pavel's approach?

 drivers/net/wireless/ath/ath9k/htc_drv_init.c | 6 +++---
 drivers/net/wireless/ath/ath9k/htc_hst.c      | 5 +++++
 2 files changed, 8 insertions(+), 3 deletions(-)
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_init.c b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
index ff61ae34ecdf..e497a44aff88 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_init.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
@@ -931,7 +931,6 @@  static int ath9k_init_device(struct ath9k_htc_priv *priv,
 int ath9k_htc_probe_device(struct htc_target *htc_handle, struct device *dev,
 			   u16 devid, char *product, u32 drv_info)
 {
-	struct hif_device_usb *hif_dev;
 	struct ath9k_htc_priv *priv;
 	struct ieee80211_hw *hw;
 	int ret;
@@ -969,10 +968,11 @@  int ath9k_htc_probe_device(struct htc_target *htc_handle, struct device *dev,
 
 err_init:
 	ath9k_stop_wmi(priv);
-	hif_dev = (struct hif_device_usb *)htc_handle->hif_dev;
-	ath9k_hif_usb_dealloc_urbs(hif_dev);
+	ath9k_hif_usb_dealloc_urbs((struct hif_device_usb *)htc_handle->hif_dev);
 	ath9k_destroy_wmi(priv);
 err_free:
+	ath9k_hif_usb_dealloc_urbs((struct hif_device_usb *)htc_handle->hif_dev);
+	htc_handle->drv_priv = NULL;
 	ieee80211_free_hw(hw);
 	return ret;
 }
diff --git a/drivers/net/wireless/ath/ath9k/htc_hst.c b/drivers/net/wireless/ath/ath9k/htc_hst.c
index 994ec48b2f66..d461eca389ab 100644
--- a/drivers/net/wireless/ath/ath9k/htc_hst.c
+++ b/drivers/net/wireless/ath/ath9k/htc_hst.c
@@ -468,6 +468,9 @@  void ath9k_htc_rx_msg(struct htc_target *htc_handle,
 	}
 }
 
+/* A dummy object used until device is initialized. */
+static struct ath9k_htc_priv ath9k_uninitialized_htc_priv;
+
 struct htc_target *ath9k_htc_hw_alloc(void *hif_handle,
 				      struct ath9k_htc_hif *hif,
 				      struct device *dev)
@@ -493,6 +496,8 @@  struct htc_target *ath9k_htc_hw_alloc(void *hif_handle,
 
 	atomic_set(&target->tgt_ready, 0);
 
+	target->drv_priv = &ath9k_uninitialized_htc_priv;
+
 	return target;
 }