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

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 <[e-mail]>
Signed-off-by: Tetsuo Handa <[e-mail]>
Tested-by: syzbot <[e-mail]>
---
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(-)

Comments

Pavel Skripkin April 29, 2022, 11:23 a.m. UTC | #1

Hi Tetsuo,

On 4/29/22 14:18, Tetsuo Handa wrote:
> 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 <[e-mail]>
> Signed-off-by: Tetsuo Handa <[e-mail]>
> Tested-by: syzbot <[e-mail]>
> ---
> Pavel Skripkin has tested "check for NULL" approach, but not yet accepted.
> What was wrong with Pavel's approach?
> 

I don't know. IIRC the problem is that nobody has tested my patch on 
real hw, so they can't accept it as-is. And maybe it just got lost

You can check out [1] thread. It's the latest version I have posted



[1] 
https://lore.kernel.org/all/80[e-mail]/



With regards,
Pavel Skripkin

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;
 }