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

[PATCH v5 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 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 6b45e63fae4b..e3d546ef71dd 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 ff61ae34ecdf..07ac88fb1c57 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 v5 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:
	- 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 518deb5098a2..7f220a245985 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(hiv_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 e3d546ef71dd..30f0765fb9fd 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 a23eaca0326d..672789e3c55d 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 v5 2/2] ath9k: htc: clean up statistics macros

[kernel test robot]

Hi Pavel,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on wireless-next/main]
[also build test ERROR on next-20220520]
[cannot apply to wireless/main linus/master v5.18-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Pavel-Skripkin/ath9k-fix-use-after-free-in-ath9k_hif_usb_rx_cb/20220522-053020
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git main
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20220522/202205220831.I9Nd8bqU-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/712472af928db8726d53f2c63ea05430e57f4727
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Pavel-Skripkin/ath9k-fix-use-after-free-in-ath9k_hif_usb_rx_cb/20220522-053020
        git checkout 712472af928db8726d53f2c63ea05430e57f4727
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=alpha SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/net/wireless/ath/ath9k/hif_usb.c:18:
   drivers/net/wireless/ath/ath9k/hif_usb.c: In function '__hif_usb_tx':
>> drivers/net/wireless/ath/ath9k/hif_usb.c:372:29: error: 'hiv_dev' undeclared (first use in this function); did you mean 'hif_dev'?
     372 |                 TX_STAT_INC(hiv_dev, buf_queued);
         |                             ^~~~~~~
   drivers/net/wireless/ath/ath9k/htc.h:330:43: note: in definition of macro '__STAT_SAFE'
     330 | #define __STAT_SAFE(hif_dev, expr)      ((hif_dev)->htc_handle->drv_priv ? (expr) : 0)
         |                                           ^~~~~~~
   drivers/net/wireless/ath/ath9k/hif_usb.c:372:17: note: in expansion of macro 'TX_STAT_INC'
     372 |                 TX_STAT_INC(hiv_dev, buf_queued);
         |                 ^~~~~~~~~~~
   drivers/net/wireless/ath/ath9k/hif_usb.c:372:29: note: each undeclared identifier is reported only once for each function it appears in
     372 |                 TX_STAT_INC(hiv_dev, buf_queued);
         |                             ^~~~~~~
   drivers/net/wireless/ath/ath9k/htc.h:330:43: note: in definition of macro '__STAT_SAFE'
     330 | #define __STAT_SAFE(hif_dev, expr)      ((hif_dev)->htc_handle->drv_priv ? (expr) : 0)
         |                                           ^~~~~~~
   drivers/net/wireless/ath/ath9k/hif_usb.c:372:17: note: in expansion of macro 'TX_STAT_INC'
     372 |                 TX_STAT_INC(hiv_dev, buf_queued);
         |                 ^~~~~~~~~~~


vim +372 drivers/net/wireless/ath/ath9k/hif_usb.c

   308	
   309	/* TX lock has to be taken */
   310	static int __hif_usb_tx(struct hif_device_usb *hif_dev)
   311	{
   312		struct tx_buf *tx_buf = NULL;
   313		struct sk_buff *nskb = NULL;
   314		int ret = 0, i;
   315		u16 tx_skb_cnt = 0;
   316		u8 *buf;
   317		__le16 *hdr;
   318	
   319		if (hif_dev->tx.tx_skb_cnt == 0)
   320			return 0;
   321	
   322		/* Check if a free TX buffer is available */
   323		if (list_empty(&hif_dev->tx.tx_buf))
   324			return 0;
   325	
   326		tx_buf = list_first_entry(&hif_dev->tx.tx_buf, struct tx_buf, list);
   327		list_move_tail(&tx_buf->list, &hif_dev->tx.tx_pending);
   328		hif_dev->tx.tx_buf_cnt--;
   329	
   330		tx_skb_cnt = min_t(u16, hif_dev->tx.tx_skb_cnt, MAX_TX_AGGR_NUM);
   331	
   332		for (i = 0; i < tx_skb_cnt; i++) {
   333			nskb = __skb_dequeue(&hif_dev->tx.tx_skb_queue);
   334	
   335			/* Should never be NULL */
   336			BUG_ON(!nskb);
   337	
   338			hif_dev->tx.tx_skb_cnt--;
   339	
   340			buf = tx_buf->buf;
   341			buf += tx_buf->offset;
   342			hdr = (__le16 *)buf;
   343			*hdr++ = cpu_to_le16(nskb->len);
   344			*hdr++ = cpu_to_le16(ATH_USB_TX_STREAM_MODE_TAG);
   345			buf += 4;
   346			memcpy(buf, nskb->data, nskb->len);
   347			tx_buf->len = nskb->len + 4;
   348	
   349			if (i < (tx_skb_cnt - 1))
   350				tx_buf->offset += (((tx_buf->len - 1) / 4) + 1) * 4;
   351	
   352			if (i == (tx_skb_cnt - 1))
   353				tx_buf->len += tx_buf->offset;
   354	
   355			__skb_queue_tail(&tx_buf->skb_queue, nskb);
   356			TX_STAT_INC(hif_dev, skb_queued);
   357		}
   358	
   359		usb_fill_bulk_urb(tx_buf->urb, hif_dev->udev,
   360				  usb_sndbulkpipe(hif_dev->udev, USB_WLAN_TX_PIPE),
   361				  tx_buf->buf, tx_buf->len,
   362				  hif_usb_tx_cb, tx_buf);
   363	
   364		ret = usb_submit_urb(tx_buf->urb, GFP_ATOMIC);
   365		if (ret) {
   366			tx_buf->len = tx_buf->offset = 0;
   367			ath9k_skb_queue_complete(hif_dev, &tx_buf->skb_queue, false);
   368			__skb_queue_head_init(&tx_buf->skb_queue);
   369			list_move_tail(&tx_buf->list, &hif_dev->tx.tx_buf);
   370			hif_dev->tx.tx_buf_cnt++;
   371		} else {
 > 372			TX_STAT_INC(hiv_dev, buf_queued);
   373		}
   374	
   375		return ret;
   376	}
   377	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[kernel test robot]

Hi Pavel,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on wireless-next/main]
[also build test ERROR on next-20220520]
[cannot apply to wireless/main linus/master v5.18-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Pavel-Skripkin/ath9k-fix-use-after-free-in-ath9k_hif_usb_rx_cb/20220522-053020
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git main
config: arm64-allmodconfig (https://download.01.org/0day-ci/archive/20220522/202205221713.VsmyJA1I-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 1443dbaba6f0e57be066995db9164f89fb57b413)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/712472af928db8726d53f2c63ea05430e57f4727
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Pavel-Skripkin/ath9k-fix-use-after-free-in-ath9k_hif_usb_rx_cb/20220522-053020
        git checkout 712472af928db8726d53f2c63ea05430e57f4727
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm64 SHELL=/bin/bash drivers/net/wireless/ath/ath9k/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/net/wireless/ath/ath9k/hif_usb.c:372:15: error: use of undeclared identifier 'hiv_dev'; did you mean 'hif_dev'?
                   TX_STAT_INC(hiv_dev, buf_queued);
                               ^~~~~~~
                               hif_dev
   drivers/net/wireless/ath/ath9k/htc.h:335:16: note: expanded from macro 'TX_STAT_INC'
                   __STAT_SAFE((hif_dev), (hif_dev)->htc_handle->drv_priv->debug.tx_stats.c++)
                                ^
   drivers/net/wireless/ath/ath9k/htc.h:330:38: note: expanded from macro '__STAT_SAFE'
   #define __STAT_SAFE(hif_dev, expr)      ((hif_dev)->htc_handle->drv_priv ? (expr) : 0)
                                             ^
   drivers/net/wireless/ath/ath9k/hif_usb.c:310:48: note: 'hif_dev' declared here
   static int __hif_usb_tx(struct hif_device_usb *hif_dev)
                                                  ^
>> drivers/net/wireless/ath/ath9k/hif_usb.c:372:15: error: use of undeclared identifier 'hiv_dev'; did you mean 'hif_dev'?
                   TX_STAT_INC(hiv_dev, buf_queued);
                               ^~~~~~~
                               hif_dev
   drivers/net/wireless/ath/ath9k/htc.h:335:27: note: expanded from macro 'TX_STAT_INC'
                   __STAT_SAFE((hif_dev), (hif_dev)->htc_handle->drv_priv->debug.tx_stats.c++)
                                           ^
   drivers/net/wireless/ath/ath9k/htc.h:330:72: note: expanded from macro '__STAT_SAFE'
   #define __STAT_SAFE(hif_dev, expr)      ((hif_dev)->htc_handle->drv_priv ? (expr) : 0)
                                                                               ^
   drivers/net/wireless/ath/ath9k/hif_usb.c:310:48: note: 'hif_dev' declared here
   static int __hif_usb_tx(struct hif_device_usb *hif_dev)
                                                  ^
   2 errors generated.


vim +372 drivers/net/wireless/ath/ath9k/hif_usb.c

   308	
   309	/* TX lock has to be taken */
   310	static int __hif_usb_tx(struct hif_device_usb *hif_dev)
   311	{
   312		struct tx_buf *tx_buf = NULL;
   313		struct sk_buff *nskb = NULL;
   314		int ret = 0, i;
   315		u16 tx_skb_cnt = 0;
   316		u8 *buf;
   317		__le16 *hdr;
   318	
   319		if (hif_dev->tx.tx_skb_cnt == 0)
   320			return 0;
   321	
   322		/* Check if a free TX buffer is available */
   323		if (list_empty(&hif_dev->tx.tx_buf))
   324			return 0;
   325	
   326		tx_buf = list_first_entry(&hif_dev->tx.tx_buf, struct tx_buf, list);
   327		list_move_tail(&tx_buf->list, &hif_dev->tx.tx_pending);
   328		hif_dev->tx.tx_buf_cnt--;
   329	
   330		tx_skb_cnt = min_t(u16, hif_dev->tx.tx_skb_cnt, MAX_TX_AGGR_NUM);
   331	
   332		for (i = 0; i < tx_skb_cnt; i++) {
   333			nskb = __skb_dequeue(&hif_dev->tx.tx_skb_queue);
   334	
   335			/* Should never be NULL */
   336			BUG_ON(!nskb);
   337	
   338			hif_dev->tx.tx_skb_cnt--;
   339	
   340			buf = tx_buf->buf;
   341			buf += tx_buf->offset;
   342			hdr = (__le16 *)buf;
   343			*hdr++ = cpu_to_le16(nskb->len);
   344			*hdr++ = cpu_to_le16(ATH_USB_TX_STREAM_MODE_TAG);
   345			buf += 4;
   346			memcpy(buf, nskb->data, nskb->len);
   347			tx_buf->len = nskb->len + 4;
   348	
   349			if (i < (tx_skb_cnt - 1))
   350				tx_buf->offset += (((tx_buf->len - 1) / 4) + 1) * 4;
   351	
   352			if (i == (tx_skb_cnt - 1))
   353				tx_buf->len += tx_buf->offset;
   354	
   355			__skb_queue_tail(&tx_buf->skb_queue, nskb);
   356			TX_STAT_INC(hif_dev, skb_queued);
   357		}
   358	
   359		usb_fill_bulk_urb(tx_buf->urb, hif_dev->udev,
   360				  usb_sndbulkpipe(hif_dev->udev, USB_WLAN_TX_PIPE),
   361				  tx_buf->buf, tx_buf->len,
   362				  hif_usb_tx_cb, tx_buf);
   363	
   364		ret = usb_submit_urb(tx_buf->urb, GFP_ATOMIC);
   365		if (ret) {
   366			tx_buf->len = tx_buf->offset = 0;
   367			ath9k_skb_queue_complete(hif_dev, &tx_buf->skb_queue, false);
   368			__skb_queue_head_init(&tx_buf->skb_queue);
   369			list_move_tail(&tx_buf->list, &hif_dev->tx.tx_buf);
   370			hif_dev->tx.tx_buf_cnt++;
   371		} else {
 > 372			TX_STAT_INC(hiv_dev, buf_queued);
   373		}
   374	
   375		return ret;
   376	}
   377	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[Takashi Iwai]

On Sat, 21 May 2022 23:28:01 +0200,
Pavel Skripkin 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>

Hi Toke, any update on this?

I'm asking it because this is a fix for a security issue
(CVE-2022-1679 [*]), and distros have been waiting for the fix getting
merged to the upstream for weeks.

[*] https://nvd.nist.gov/vuln/detail/CVE-2022-1679


thanks,

Takashi

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

[Toke Høiland-Jørgensen]

Takashi Iwai <tiwai@suse.de> writes:

> On Sat, 21 May 2022 23:28:01 +0200,
> Pavel Skripkin 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>
>
> Hi Toke, any update on this?

It's marked as "Changes Requested" in patchwork, due to the kernel test
robot comments on patch 2[0]. So it's waiting for Pavel to resubmit
fixing that. There's also a separate comment on patch 1, which I just
noticed didn't have the mailing list in Cc; will reply to that and try
to get it back on the list.

> I'm asking it because this is a fix for a security issue
> (CVE-2022-1679 [*]), and distros have been waiting for the fix getting
> merged to the upstream for weeks.
>
> [*] https://nvd.nist.gov/vuln/detail/CVE-2022-1679

In general, if a patch is marked as "changes requested", the right thing
to do is to bug the submitter to resubmit. Which I guess you just did,
so hopefully we'll get an update soon :)

-Toke

[0] https://patchwork.kernel.org/project/linux-wireless/patch/7bb006bb88e280c596d0e86ece7d251a21b8ed1f.1653168225.git.paskripkin@gmail.com/

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

[Pavel Skripkin]

[-- Attachment #1.1: Type: text/plain, Size: 562 bytes --]

Hi Toke,

On 6/10/22 21:30, Toke Høiland-Jørgensen wrote:
> 
> In general, if a patch is marked as "changes requested", the right thing
> to do is to bug the submitter to resubmit. Which I guess you just did,
> so hopefully we'll get an update soon :)
> 


I agree here. The build fix is trivial, I just wanted to reply to Hillf 
like 2 weeks ago, but an email got lost in my inbox.

So, i don't know what is correct thing to do rn: wait for Hillf's reply 
or to quickly respin with build error addressed?




With regards,
Pavel Skripkin

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Toke Høiland-Jørgensen]

Pavel Skripkin <paskripkin@gmail.com> writes:

> Hi Toke,
>
> On 6/10/22 21:30, Toke Høiland-Jørgensen wrote:
>> 
>> In general, if a patch is marked as "changes requested", the right thing
>> to do is to bug the submitter to resubmit. Which I guess you just did,
>> so hopefully we'll get an update soon :)
>> 
>
>
> I agree here. The build fix is trivial, I just wanted to reply to
> Hillf like 2 weeks ago, but an email got lost in my inbox.
>
> So, i don't know what is correct thing to do rn: wait for Hillf's
> reply or to quickly respin with build error addressed?

Up to you. If you respin it now we can just let it sit in patchwork over
the weekend and see if it attracts any further comment; or you can wait
and respin on Monday...

-Toke

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

[Pavel Skripkin]

[-- Attachment #1.1: Type: text/plain, Size: 1084 bytes --]

Hi Hillf,

On 5/22/22 07:15, Hillf Danton wrote:
> 
> In the call chain below
> 
> 	ath9k_hif_usb_firmware_cb()
> 	  ath9k_htc_hw_alloc()
> 	  ath9k_hif_usb_dev_init()
> 	  ret = ath9k_htc_hw_init()
> 	          ath9k_htc_probe_device()
> 		    htc_handle->drv_priv = priv;
> 		    ret = ath9k_htc_wait_for_target(priv);
> 		    if (ret)
> 			goto err_free;
> 	  if (ret)
> 		goto err_htc_hw_init;
> 
> err_free:
> 	ieee80211_free_hw(hw);
> 
> 
> err_htc_hw_init:
> 	ath9k_hif_usb_dev_deinit(hif_dev);
> 	  ath9k_hif_usb_dealloc_urbs()
> err_dev_init:
> 	ath9k_htc_hw_free(hif_dev->htc_handle);
> err_dev_alloc:
> 	release_firmware(fw);
> err_fw:
> 	ath9k_hif_usb_firmware_fail(hif_dev);
> 
> 
> hw should survive deallocating urbs, and changes should be added instead to
> the rollback in ath9k_htc_probe_device() by deferring cleanup of hw to its
> callsite in addition to urbs.
> 

Don't get it, sorry. I am not changing the life time of `hw`, I am just 
deferring htc_handle->drv_priv initialization.




With regards,
Pavel Skripkin

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]