[PATCH v6 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb

[PATCH v6 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb

[Pavel Skripkin]

Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb() [0]. The
problem was in incorrect htc_handle->drv_priv initialization.

Probable call trace which can trigger use-after-free:

ath9k_htc_probe_device()
  /* htc_handle->drv_priv = priv; */
  ath9k_htc_wait_for_target()      <--- Failed
  ieee80211_free_hw()		   <--- priv pointer is freed

<IRQ>
...
ath9k_hif_usb_rx_cb()
  ath9k_hif_usb_rx_stream()
   RX_STAT_INC()		<--- htc_handle->drv_priv access

In order to not add fancy protection for drv_priv we can move
htc_handle->drv_priv initialization at the end of the
ath9k_htc_probe_device() and add helper macro to make
all *_STAT_* macros NULL safe, since syzbot has reported related NULL
deref in that macros [1]

Link: https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60 [0]
Link: https://syzkaller.appspot.com/bug?id=b8101ffcec107c0567a0cd8acbbacec91e9ee8de [1]
Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
Reported-and-tested-by: syzbot+03110230a11411024147@syzkaller.appspotmail.com
Reported-and-tested-by: syzbot+c6dde1f690b60e0b9fbe@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---

Changes since v5:
	- No changes

Changes since v4:
	s/save/safe/ in commit message

Changes since v3:
	- s/SAVE/SAFE/
	- Added links to syzkaller reports

Changes since v2:
	- My send-email script forgot, that mailing lists exist.
	  Added back all related lists

Changes since v1:
	- Removed clean-ups and moved them to 2/2

---
 drivers/net/wireless/ath/ath9k/htc.h          | 10 +++++-----
 drivers/net/wireless/ath/ath9k/htc_drv_init.c |  3 ++-
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h
index 6b45e63fa..e3d546ef7 100644
--- a/drivers/net/wireless/ath/ath9k/htc.h
+++ b/drivers/net/wireless/ath/ath9k/htc.h
@@ -327,11 +327,11 @@ static inline struct ath9k_htc_tx_ctl *HTC_SKB_CB(struct sk_buff *skb)
 }
 
 #ifdef CONFIG_ATH9K_HTC_DEBUGFS
-
-#define TX_STAT_INC(c) (hif_dev->htc_handle->drv_priv->debug.tx_stats.c++)
-#define TX_STAT_ADD(c, a) (hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a)
-#define RX_STAT_INC(c) (hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++)
-#define RX_STAT_ADD(c, a) (hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a)
+#define __STAT_SAFE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0)
+#define TX_STAT_INC(c) __STAT_SAFE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++)
+#define TX_STAT_ADD(c, a) __STAT_SAFE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a)
+#define RX_STAT_INC(c) __STAT_SAFE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++)
+#define RX_STAT_ADD(c, a) __STAT_SAFE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a)
 #define CAB_STAT_INC   priv->debug.tx_stats.cab_queued++
 
 #define TX_QSTAT_INC(q) (priv->debug.tx_stats.queue_stats[q]++)
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_init.c b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
index ff61ae34e..07ac88fb1 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_init.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_init.c
@@ -944,7 +944,6 @@ int ath9k_htc_probe_device(struct htc_target *htc_handle, struct device *dev,
 	priv->hw = hw;
 	priv->htc = htc_handle;
 	priv->dev = dev;
-	htc_handle->drv_priv = priv;
 	SET_IEEE80211_DEV(hw, priv->dev);
 
 	ret = ath9k_htc_wait_for_target(priv);
@@ -965,6 +964,8 @@ int ath9k_htc_probe_device(struct htc_target *htc_handle, struct device *dev,
 	if (ret)
 		goto err_init;
 
+	htc_handle->drv_priv = priv;
+
 	return 0;
 
 err_init:
-- 
2.36.1

[PATCH v6 2/2] ath9k: htc: clean up statistics macros

[Pavel Skripkin]

I've changed *STAT_* macros a bit in previous patch and I seems like
they become really unreadable. Align these macros definitions to make
code cleaner and fix folllowing checkpatch warning

ERROR: Macros with complex values should be enclosed in parentheses

Also, statistics macros now accept an hif_dev as argument, since
macros that depend on having a local variable with a magic name
don't abide by the coding style.

No functional change

Suggested-by: Jeff Johnson <quic_jjohnson@quicinc.com>
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---

Changes since v4:
	- Fixed build error caused by wrong variable name
	  passed to newly added macros

Changes since v4:
	- Rebased on top of ath-next branch

Changes since v3:
	- Added additional clean up related to relying on magical
	  name from outside of the macro

Changes since v2:
	- My send-email script forgot, that mailing lists exist.
	  Added back all related lists
	- Fixed checkpatch warning

Changes since v1:
	- Added this patch

---
 drivers/net/wireless/ath/ath9k/hif_usb.c      | 26 +++++++--------
 drivers/net/wireless/ath/ath9k/htc.h          | 32 +++++++++++--------
 drivers/net/wireless/ath/ath9k/htc_drv_txrx.c | 10 +++---
 3 files changed, 36 insertions(+), 32 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
index 518deb509..4d9002a9d 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.c
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
@@ -244,11 +244,11 @@ static inline void ath9k_skb_queue_complete(struct hif_device_usb *hif_dev,
 		ath9k_htc_txcompletion_cb(hif_dev->htc_handle,
 					  skb, txok);
 		if (txok) {
-			TX_STAT_INC(skb_success);
-			TX_STAT_ADD(skb_success_bytes, ln);
+			TX_STAT_INC(hif_dev, skb_success);
+			TX_STAT_ADD(hif_dev, skb_success_bytes, ln);
 		}
 		else
-			TX_STAT_INC(skb_failed);
+			TX_STAT_INC(hif_dev, skb_failed);
 	}
 }
 
@@ -302,7 +302,7 @@ static void hif_usb_tx_cb(struct urb *urb)
 	hif_dev->tx.tx_buf_cnt++;
 	if (!(hif_dev->tx.flags & HIF_USB_TX_STOP))
 		__hif_usb_tx(hif_dev); /* Check for pending SKBs */
-	TX_STAT_INC(buf_completed);
+	TX_STAT_INC(hif_dev, buf_completed);
 	spin_unlock(&hif_dev->tx.tx_lock);
 }
 
@@ -353,7 +353,7 @@ static int __hif_usb_tx(struct hif_device_usb *hif_dev)
 			tx_buf->len += tx_buf->offset;
 
 		__skb_queue_tail(&tx_buf->skb_queue, nskb);
-		TX_STAT_INC(skb_queued);
+		TX_STAT_INC(hif_dev, skb_queued);
 	}
 
 	usb_fill_bulk_urb(tx_buf->urb, hif_dev->udev,
@@ -369,7 +369,7 @@ static int __hif_usb_tx(struct hif_device_usb *hif_dev)
 		list_move_tail(&tx_buf->list, &hif_dev->tx.tx_buf);
 		hif_dev->tx.tx_buf_cnt++;
 	} else {
-		TX_STAT_INC(buf_queued);
+		TX_STAT_INC(hif_dev, buf_queued);
 	}
 
 	return ret;
@@ -514,7 +514,7 @@ static void hif_usb_sta_drain(void *hif_handle, u8 idx)
 			ath9k_htc_txcompletion_cb(hif_dev->htc_handle,
 						  skb, false);
 			hif_dev->tx.tx_skb_cnt--;
-			TX_STAT_INC(skb_failed);
+			TX_STAT_INC(hif_dev, skb_failed);
 		}
 	}
 
@@ -585,14 +585,14 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
 		pkt_tag = get_unaligned_le16(ptr + index + 2);
 
 		if (pkt_tag != ATH_USB_RX_STREAM_MODE_TAG) {
-			RX_STAT_INC(skb_dropped);
+			RX_STAT_INC(hif_dev, skb_dropped);
 			return;
 		}
 
 		if (pkt_len > 2 * MAX_RX_BUF_SIZE) {
 			dev_err(&hif_dev->udev->dev,
 				"ath9k_htc: invalid pkt_len (%x)\n", pkt_len);
-			RX_STAT_INC(skb_dropped);
+			RX_STAT_INC(hif_dev, skb_dropped);
 			return;
 		}
 
@@ -618,7 +618,7 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
 				goto err;
 			}
 			skb_reserve(nskb, 32);
-			RX_STAT_INC(skb_allocated);
+			RX_STAT_INC(hif_dev, skb_allocated);
 
 			memcpy(nskb->data, &(skb->data[chk_idx+4]),
 			       hif_dev->rx_transfer_len);
@@ -639,7 +639,7 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
 				goto err;
 			}
 			skb_reserve(nskb, 32);
-			RX_STAT_INC(skb_allocated);
+			RX_STAT_INC(hif_dev, skb_allocated);
 
 			memcpy(nskb->data, &(skb->data[chk_idx+4]), pkt_len);
 			skb_put(nskb, pkt_len);
@@ -649,10 +649,10 @@ static void ath9k_hif_usb_rx_stream(struct hif_device_usb *hif_dev,
 
 err:
 	for (i = 0; i < pool_index; i++) {
-		RX_STAT_ADD(skb_completed_bytes, skb_pool[i]->len);
+		RX_STAT_ADD(hif_dev, skb_completed_bytes, skb_pool[i]->len);
 		ath9k_htc_rx_msg(hif_dev->htc_handle, skb_pool[i],
 				 skb_pool[i]->len, USB_WLAN_RX_PIPE);
-		RX_STAT_INC(skb_completed);
+		RX_STAT_INC(hif_dev, skb_completed);
 	}
 }
 
diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h
index e3d546ef7..30f0765fb 100644
--- a/drivers/net/wireless/ath/ath9k/htc.h
+++ b/drivers/net/wireless/ath/ath9k/htc.h
@@ -327,14 +327,18 @@ static inline struct ath9k_htc_tx_ctl *HTC_SKB_CB(struct sk_buff *skb)
 }
 
 #ifdef CONFIG_ATH9K_HTC_DEBUGFS
-#define __STAT_SAFE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0)
-#define TX_STAT_INC(c) __STAT_SAFE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++)
-#define TX_STAT_ADD(c, a) __STAT_SAFE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a)
-#define RX_STAT_INC(c) __STAT_SAFE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++)
-#define RX_STAT_ADD(c, a) __STAT_SAFE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a)
-#define CAB_STAT_INC   priv->debug.tx_stats.cab_queued++
-
-#define TX_QSTAT_INC(q) (priv->debug.tx_stats.queue_stats[q]++)
+#define __STAT_SAFE(hif_dev, expr)	((hif_dev)->htc_handle->drv_priv ? (expr) : 0)
+#define CAB_STAT_INC(priv)		((priv)->debug.tx_stats.cab_queued++)
+#define TX_QSTAT_INC(priv, q)		((priv)->debug.tx_stats.queue_stats[q]++)
+
+#define TX_STAT_INC(hif_dev, c) \
+		__STAT_SAFE((hif_dev), (hif_dev)->htc_handle->drv_priv->debug.tx_stats.c++)
+#define TX_STAT_ADD(hif_dev, c, a) \
+		__STAT_SAFE((hif_dev), (hif_dev)->htc_handle->drv_priv->debug.tx_stats.c += a)
+#define RX_STAT_INC(hif_dev, c) \
+		__STAT_SAFE((hif_dev), (hif_dev)->htc_handle->drv_priv->debug.skbrx_stats.c++)
+#define RX_STAT_ADD(hif_dev, c, a) \
+		__STAT_SAFE((hif_dev), (hif_dev)->htc_handle->drv_priv->debug.skbrx_stats.c += a)
 
 void ath9k_htc_err_stat_rx(struct ath9k_htc_priv *priv,
 			   struct ath_rx_status *rs);
@@ -374,13 +378,13 @@ void ath9k_htc_get_et_stats(struct ieee80211_hw *hw,
 			    struct ethtool_stats *stats, u64 *data);
 #else
 
-#define TX_STAT_INC(c) do { } while (0)
-#define TX_STAT_ADD(c, a) do { } while (0)
-#define RX_STAT_INC(c) do { } while (0)
-#define RX_STAT_ADD(c, a) do { } while (0)
-#define CAB_STAT_INC   do { } while (0)
+#define TX_STAT_INC(hif_dev, c)
+#define TX_STAT_ADD(hif_dev, c, a)
+#define RX_STAT_INC(hif_dev, c)
+#define RX_STAT_ADD(hif_dev, c, a)
 
-#define TX_QSTAT_INC(c) do { } while (0)
+#define CAB_STAT_INC(priv)
+#define TX_QSTAT_INC(priv, c)
 
 static inline void ath9k_htc_err_stat_rx(struct ath9k_htc_priv *priv,
 					 struct ath_rx_status *rs)
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
index a23eaca03..672789e3c 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_txrx.c
@@ -106,20 +106,20 @@ static inline enum htc_endpoint_id get_htc_epid(struct ath9k_htc_priv *priv,
 
 	switch (qnum) {
 	case 0:
-		TX_QSTAT_INC(IEEE80211_AC_VO);
+		TX_QSTAT_INC(priv, IEEE80211_AC_VO);
 		epid = priv->data_vo_ep;
 		break;
 	case 1:
-		TX_QSTAT_INC(IEEE80211_AC_VI);
+		TX_QSTAT_INC(priv, IEEE80211_AC_VI);
 		epid = priv->data_vi_ep;
 		break;
 	case 2:
-		TX_QSTAT_INC(IEEE80211_AC_BE);
+		TX_QSTAT_INC(priv, IEEE80211_AC_BE);
 		epid = priv->data_be_ep;
 		break;
 	case 3:
 	default:
-		TX_QSTAT_INC(IEEE80211_AC_BK);
+		TX_QSTAT_INC(priv, IEEE80211_AC_BK);
 		epid = priv->data_bk_ep;
 		break;
 	}
@@ -328,7 +328,7 @@ static void ath9k_htc_tx_data(struct ath9k_htc_priv *priv,
 	memcpy(tx_fhdr, (u8 *) &tx_hdr, sizeof(tx_hdr));
 
 	if (is_cab) {
-		CAB_STAT_INC;
+		CAB_STAT_INC(priv);
 		tx_ctl->epid = priv->cab_ep;
 		return;
 	}
-- 
2.36.1

Re: [PATCH v6 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb

[Toke Høiland-Jørgensen]

Pavel Skripkin <paskripkin@gmail.com> writes:

> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb() [0]. The
> problem was in incorrect htc_handle->drv_priv initialization.
>
> Probable call trace which can trigger use-after-free:
>
> ath9k_htc_probe_device()
>   /* htc_handle->drv_priv = priv; */
>   ath9k_htc_wait_for_target()      <--- Failed
>   ieee80211_free_hw()		   <--- priv pointer is freed
>
> <IRQ>
> ...
> ath9k_hif_usb_rx_cb()
>   ath9k_hif_usb_rx_stream()
>    RX_STAT_INC()		<--- htc_handle->drv_priv access
>
> In order to not add fancy protection for drv_priv we can move
> htc_handle->drv_priv initialization at the end of the
> ath9k_htc_probe_device() and add helper macro to make
> all *_STAT_* macros NULL safe, since syzbot has reported related NULL
> deref in that macros [1]
>
> Link: https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60 [0]
> Link: https://syzkaller.appspot.com/bug?id=b8101ffcec107c0567a0cd8acbbacec91e9ee8de [1]
> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
> Reported-and-tested-by: syzbot+03110230a11411024147@syzkaller.appspotmail.com
> Reported-and-tested-by: syzbot+c6dde1f690b60e0b9fbe@syzkaller.appspotmail.com
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>

Alright, since we've heard no more objections and the status quo is
definitely broken, let's get this merged and we can follow up with any
other fixes as necessary...

Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>

Re: [PATCH v6 2/2] ath9k: htc: clean up statistics macros

[Toke Høiland-Jørgensen]

Pavel Skripkin <paskripkin@gmail.com> writes:

> I've changed *STAT_* macros a bit in previous patch and I seems like
> they become really unreadable. Align these macros definitions to make
> code cleaner and fix folllowing checkpatch warning
>
> ERROR: Macros with complex values should be enclosed in parentheses
>
> Also, statistics macros now accept an hif_dev as argument, since
> macros that depend on having a local variable with a magic name
> don't abide by the coding style.
>
> No functional change
>
> Suggested-by: Jeff Johnson <quic_jjohnson@quicinc.com>
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>

Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>

Re: [PATCH v6 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb

[Kalle Valo]

Toke Høiland-Jørgensen <toke@toke.dk> writes:

> Pavel Skripkin <paskripkin@gmail.com> writes:
>
>> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb() [0]. The
>> problem was in incorrect htc_handle->drv_priv initialization.
>>
>> Probable call trace which can trigger use-after-free:
>>
>> ath9k_htc_probe_device()
>>   /* htc_handle->drv_priv = priv; */
>>   ath9k_htc_wait_for_target()      <--- Failed
>>   ieee80211_free_hw()		   <--- priv pointer is freed
>>
>> <IRQ>
>> ...
>> ath9k_hif_usb_rx_cb()
>>   ath9k_hif_usb_rx_stream()
>>    RX_STAT_INC()		<--- htc_handle->drv_priv access
>>
>> In order to not add fancy protection for drv_priv we can move
>> htc_handle->drv_priv initialization at the end of the
>> ath9k_htc_probe_device() and add helper macro to make
>> all *_STAT_* macros NULL safe, since syzbot has reported related NULL
>> deref in that macros [1]
>>
>> Link: https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60 [0]
>> Link: https://syzkaller.appspot.com/bug?id=b8101ffcec107c0567a0cd8acbbacec91e9ee8de [1]
>> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
>> Reported-and-tested-by: syzbot+03110230a11411024147@syzkaller.appspotmail.com
>> Reported-and-tested-by: syzbot+c6dde1f690b60e0b9fbe@syzkaller.appspotmail.com
>> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
>
> Alright, since we've heard no more objections and the status quo is
> definitely broken, let's get this merged and we can follow up with any
> other fixes as necessary...
>
> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>

I'm wondering should these go to -rc or -next? Has anyone actually
tested these with real hardware? (syzbot testing does not count) With
the past bad experience with syzbot fixes I'm leaning towards -next to
have more time to fix any regressions.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

Re: [PATCH v6 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb

[Toke Høiland-Jørgensen]

Kalle Valo <kvalo@kernel.org> writes:

> Toke Høiland-Jørgensen <toke@toke.dk> writes:
>
>> Pavel Skripkin <paskripkin@gmail.com> writes:
>>
>>> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb() [0]. The
>>> problem was in incorrect htc_handle->drv_priv initialization.
>>>
>>> Probable call trace which can trigger use-after-free:
>>>
>>> ath9k_htc_probe_device()
>>>   /* htc_handle->drv_priv = priv; */
>>>   ath9k_htc_wait_for_target()      <--- Failed
>>>   ieee80211_free_hw()		   <--- priv pointer is freed
>>>
>>> <IRQ>
>>> ...
>>> ath9k_hif_usb_rx_cb()
>>>   ath9k_hif_usb_rx_stream()
>>>    RX_STAT_INC()		<--- htc_handle->drv_priv access
>>>
>>> In order to not add fancy protection for drv_priv we can move
>>> htc_handle->drv_priv initialization at the end of the
>>> ath9k_htc_probe_device() and add helper macro to make
>>> all *_STAT_* macros NULL safe, since syzbot has reported related NULL
>>> deref in that macros [1]
>>>
>>> Link: https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60 [0]
>>> Link: https://syzkaller.appspot.com/bug?id=b8101ffcec107c0567a0cd8acbbacec91e9ee8de [1]
>>> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
>>> Reported-and-tested-by: syzbot+03110230a11411024147@syzkaller.appspotmail.com
>>> Reported-and-tested-by: syzbot+c6dde1f690b60e0b9fbe@syzkaller.appspotmail.com
>>> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
>>
>> Alright, since we've heard no more objections and the status quo is
>> definitely broken, let's get this merged and we can follow up with any
>> other fixes as necessary...
>>
>> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
>
> I'm wondering should these go to -rc or -next? Has anyone actually
> tested these with real hardware? (syzbot testing does not count) With
> the past bad experience with syzbot fixes I'm leaning towards -next to
> have more time to fix any regressions.

Hmm, good question. From Takashi's comment on v5, it seems like distros
are going to backport it anyway, so in that sense it probably doesn't
matter that much?

In any case I think it has a fairly low probability of breaking real
users' setup (how often is that error path on setup even hit?), but I'm
OK with it going to -next to be doubleplus-sure :)

-Toke

Re: [PATCH v6 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb

[Takashi Iwai]

On Wed, 15 Jun 2022 11:05:20 +0200,
Toke Høiland-Jørgensen wrote:
> 
> Kalle Valo <kvalo@kernel.org> writes:
> 
> > Toke Høiland-Jørgensen <toke@toke.dk> writes:
> >
> >> Pavel Skripkin <paskripkin@gmail.com> writes:
> >>
> >>> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb() [0]. The
> >>> problem was in incorrect htc_handle->drv_priv initialization.
> >>>
> >>> Probable call trace which can trigger use-after-free:
> >>>
> >>> ath9k_htc_probe_device()
> >>>   /* htc_handle->drv_priv = priv; */
> >>>   ath9k_htc_wait_for_target()      <--- Failed
> >>>   ieee80211_free_hw()		   <--- priv pointer is freed
> >>>
> >>> <IRQ>
> >>> ...
> >>> ath9k_hif_usb_rx_cb()
> >>>   ath9k_hif_usb_rx_stream()
> >>>    RX_STAT_INC()		<--- htc_handle->drv_priv access
> >>>
> >>> In order to not add fancy protection for drv_priv we can move
> >>> htc_handle->drv_priv initialization at the end of the
> >>> ath9k_htc_probe_device() and add helper macro to make
> >>> all *_STAT_* macros NULL safe, since syzbot has reported related NULL
> >>> deref in that macros [1]
> >>>
> >>> Link: https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60 [0]
> >>> Link: https://syzkaller.appspot.com/bug?id=b8101ffcec107c0567a0cd8acbbacec91e9ee8de [1]
> >>> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
> >>> Reported-and-tested-by: syzbot+03110230a11411024147@syzkaller.appspotmail.com
> >>> Reported-and-tested-by: syzbot+c6dde1f690b60e0b9fbe@syzkaller.appspotmail.com
> >>> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> >>
> >> Alright, since we've heard no more objections and the status quo is
> >> definitely broken, let's get this merged and we can follow up with any
> >> other fixes as necessary...
> >>
> >> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
> >
> > I'm wondering should these go to -rc or -next? Has anyone actually
> > tested these with real hardware? (syzbot testing does not count) With
> > the past bad experience with syzbot fixes I'm leaning towards -next to
> > have more time to fix any regressions.
> 
> Hmm, good question. From Takashi's comment on v5, it seems like distros
> are going to backport it anyway, so in that sense it probably doesn't
> matter that much?

Well, it does matter if it really breaks things, of course ;)

> In any case I think it has a fairly low probability of breaking real
> users' setup (how often is that error path on setup even hit?), but I'm
> OK with it going to -next to be doubleplus-sure :)

Queuing to for-next is fine for us.  Backporting immediately or not
will be a decision by each distro, then. 

OTOH, if anyone has tested it beforehand on a real hardware and
confirmed, at least, that it works for normal cases (no error path),
that should suffice for -rc inclusion, too, IMO.


thanks,

Takashi

Re: [PATCH v6 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb

[Kalle Valo]

Takashi Iwai <tiwai@suse.de> writes:

> On Wed, 15 Jun 2022 11:05:20 +0200,
> Toke Høiland-Jørgensen wrote:
>
>> 
>> Kalle Valo <kvalo@kernel.org> writes:
>> 
>> > Toke Høiland-Jørgensen <toke@toke.dk> writes:
>> >
>> >> Pavel Skripkin <paskripkin@gmail.com> writes:
>> >>
>> >>> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb() [0]. The
>> >>> problem was in incorrect htc_handle->drv_priv initialization.
>> >>>
>> >>> Probable call trace which can trigger use-after-free:
>> >>>
>> >>> ath9k_htc_probe_device()
>> >>>   /* htc_handle->drv_priv = priv; */
>> >>>   ath9k_htc_wait_for_target()      <--- Failed
>> >>>   ieee80211_free_hw()		   <--- priv pointer is freed
>> >>>
>> >>> <IRQ>
>> >>> ...
>> >>> ath9k_hif_usb_rx_cb()
>> >>>   ath9k_hif_usb_rx_stream()
>> >>>    RX_STAT_INC()		<--- htc_handle->drv_priv access
>> >>>
>> >>> In order to not add fancy protection for drv_priv we can move
>> >>> htc_handle->drv_priv initialization at the end of the
>> >>> ath9k_htc_probe_device() and add helper macro to make
>> >>> all *_STAT_* macros NULL safe, since syzbot has reported related NULL
>> >>> deref in that macros [1]
>> >>>
>> >>> Link: https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60 [0]
>> >>> Link: https://syzkaller.appspot.com/bug?id=b8101ffcec107c0567a0cd8acbbacec91e9ee8de [1]
>> >>> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
>> >>> Reported-and-tested-by: syzbot+03110230a11411024147@syzkaller.appspotmail.com
>> >>> Reported-and-tested-by: syzbot+c6dde1f690b60e0b9fbe@syzkaller.appspotmail.com
>> >>> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
>> >>
>> >> Alright, since we've heard no more objections and the status quo is
>> >> definitely broken, let's get this merged and we can follow up with any
>> >> other fixes as necessary...
>> >>
>> >> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
>> >
>> > I'm wondering should these go to -rc or -next? Has anyone actually
>> > tested these with real hardware? (syzbot testing does not count) With
>> > the past bad experience with syzbot fixes I'm leaning towards -next to
>> > have more time to fix any regressions.
>> 
>> Hmm, good question. From Takashi's comment on v5, it seems like distros
>> are going to backport it anyway, so in that sense it probably doesn't
>> matter that much?
>
> Well, it does matter if it really breaks things, of course ;)
>
>> In any case I think it has a fairly low probability of breaking real
>> users' setup (how often is that error path on setup even hit?), but I'm
>> OK with it going to -next to be doubleplus-sure :)
>
> Queuing to for-next is fine for us.  Backporting immediately or not
> will be a decision by each distro, then. 
>
> OTOH, if anyone has tested it beforehand on a real hardware and
> confirmed, at least, that it works for normal cases (no error path),
> that should suffice for -rc inclusion, too, IMO.

Ok, I'll take these to -next then. I just don't like taking untested
patches, having them -next gives us more time to fix any issues (or
revert the patches).

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

Re: [PATCH v6 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb

[Kalle Valo]

Pavel Skripkin <paskripkin@gmail.com> wrote:

> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb() [0]. The
> problem was in incorrect htc_handle->drv_priv initialization.
> 
> Probable call trace which can trigger use-after-free:
> 
> ath9k_htc_probe_device()
>   /* htc_handle->drv_priv = priv; */
>   ath9k_htc_wait_for_target()      <--- Failed
>   ieee80211_free_hw()              <--- priv pointer is freed
> 
> <IRQ>
> ...
> ath9k_hif_usb_rx_cb()
>   ath9k_hif_usb_rx_stream()
>    RX_STAT_INC()                <--- htc_handle->drv_priv access
> 
> In order to not add fancy protection for drv_priv we can move
> htc_handle->drv_priv initialization at the end of the
> ath9k_htc_probe_device() and add helper macro to make
> all *_STAT_* macros NULL safe, since syzbot has reported related NULL
> deref in that macros [1]
> 
> Link: https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60 [0]
> Link: https://syzkaller.appspot.com/bug?id=b8101ffcec107c0567a0cd8acbbacec91e9ee8de [1]
> Fixes: fb9987d0f748 ("ath9k_htc: Support for AR9271 chipset.")
> Reported-and-tested-by: syzbot+03110230a11411024147@syzkaller.appspotmail.com
> Reported-and-tested-by: syzbot+c6dde1f690b60e0b9fbe@syzkaller.appspotmail.com
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

2 patches applied to ath-next branch of ath.git, thanks.

0ac4827f78c7 ath9k: fix use-after-free in ath9k_hif_usb_rx_cb
d7fc76039b74 ath9k: htc: clean up statistics macros

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/d57bbedc857950659bfacac0ab48790c1eda00c8.1655145743.git.paskripkin@gmail.com/

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