All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb
@ 2022-02-07 20:24 Pavel Skripkin
  2022-02-07 20:24 ` [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros Pavel Skripkin
                   ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: Pavel Skripkin @ 2022-02-07 20:24 UTC (permalink / raw)
  To: ath9k-devel, kvalo, davem, kuba, toke, linville
  Cc: linux-wireless, netdev, linux-kernel, Pavel Skripkin,
	syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe

Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb(). 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 save.

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 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..141642e5e00d 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_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0)
+#define TX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++)
+#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a)
+#define RX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++)
+#define RX_STAT_ADD(c, a) __STAT_SAVE(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.34.1


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

* [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros
  2022-02-07 20:24 [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb Pavel Skripkin
@ 2022-02-07 20:24 ` Pavel Skripkin
  2022-02-07 21:24   ` Jeff Johnson
  2022-02-08 14:47 ` [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb Toke Høiland-Jørgensen
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 19+ messages in thread
From: Pavel Skripkin @ 2022-02-07 20:24 UTC (permalink / raw)
  To: ath9k-devel, kvalo, davem, kuba, toke, linville
  Cc: linux-wireless, netdev, linux-kernel, 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.

Also fixed following checkpatch warning

ERROR: Macros with complex values should be enclosed in parentheses

Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---

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/htc.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h
index 141642e5e00d..b4755e21a501 100644
--- a/drivers/net/wireless/ath/ath9k/htc.h
+++ b/drivers/net/wireless/ath/ath9k/htc.h
@@ -327,14 +327,14 @@ static inline struct ath9k_htc_tx_ctl *HTC_SKB_CB(struct sk_buff *skb)
 }
 
 #ifdef CONFIG_ATH9K_HTC_DEBUGFS
-#define __STAT_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0)
-#define TX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++)
-#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a)
-#define RX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++)
-#define RX_STAT_ADD(c, a) __STAT_SAVE(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_SAVE(expr)	(hif_dev->htc_handle->drv_priv ? (expr) : 0)
+#define TX_STAT_INC(c)		__STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++)
+#define TX_STAT_ADD(c, a)	__STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a)
+#define RX_STAT_INC(c)		__STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++)
+#define RX_STAT_ADD(c, a)	__STAT_SAVE(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]++)
 
 void ath9k_htc_err_stat_rx(struct ath9k_htc_priv *priv,
 			   struct ath_rx_status *rs);
-- 
2.34.1


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

* Re: [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros
  2022-02-07 20:24 ` [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros Pavel Skripkin
@ 2022-02-07 21:24   ` Jeff Johnson
  2022-02-08 15:32     ` Toke Høiland-Jørgensen
  0 siblings, 1 reply; 19+ messages in thread
From: Jeff Johnson @ 2022-02-07 21:24 UTC (permalink / raw)
  To: Pavel Skripkin, ath9k-devel, kvalo, davem, kuba, toke, linville
  Cc: linux-wireless, netdev, linux-kernel

On 2/7/2022 12:24 PM, Pavel Skripkin wrote:
> 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.
> 
> Also fixed following checkpatch warning
> 
> ERROR: Macros with complex values should be enclosed in parentheses
> 
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> ---
> 
> 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/htc.h | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h
> index 141642e5e00d..b4755e21a501 100644
> --- a/drivers/net/wireless/ath/ath9k/htc.h
> +++ b/drivers/net/wireless/ath/ath9k/htc.h
> @@ -327,14 +327,14 @@ static inline struct ath9k_htc_tx_ctl *HTC_SKB_CB(struct sk_buff *skb)
>   }
>   
>   #ifdef CONFIG_ATH9K_HTC_DEBUGFS
> -#define __STAT_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0)
> -#define TX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++)
> -#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a)
> -#define RX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++)
> -#define RX_STAT_ADD(c, a) __STAT_SAVE(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_SAVE(expr)	(hif_dev->htc_handle->drv_priv ? (expr) : 0)
> +#define TX_STAT_INC(c)		__STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++)
> +#define TX_STAT_ADD(c, a)	__STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a)
> +#define RX_STAT_INC(c)		__STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++)
> +#define RX_STAT_ADD(c, a)	__STAT_SAVE(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]++)
>   
>   void ath9k_htc_err_stat_rx(struct ath9k_htc_priv *priv,
>   			   struct ath_rx_status *rs);

It seems that these macros (both the original and the new) aren't 
following the guidance from the Coding Style which tells us under 
"Things to avoid when using macros" that we should avoid "macros that 
depend on having a local variable with a magic name". Wouldn't these 
macros be "better" is they included the hif_dev/priv as arguments rather 
than being "magic"?

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

* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb
  2022-02-07 20:24 [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb Pavel Skripkin
  2022-02-07 20:24 ` [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros Pavel Skripkin
@ 2022-02-08 14:47 ` Toke Høiland-Jørgensen
  2022-02-08 15:48   ` Pavel Skripkin
  2022-05-12 12:49 ` Toke Høiland-Jørgensen
  2022-05-12 16:05 ` Jeff Johnson
  3 siblings, 1 reply; 19+ messages in thread
From: Toke Høiland-Jørgensen @ 2022-02-08 14:47 UTC (permalink / raw)
  To: Pavel Skripkin, ath9k-devel, kvalo, davem, kuba, linville
  Cc: linux-wireless, netdev, linux-kernel, Pavel Skripkin,
	syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe

Pavel Skripkin <paskripkin@gmail.com> writes:

> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb(). 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 save.

I'm not too familiar with how the initialisation flow of an ath9k_htc
device works. Looking at htc_handle->drv_priv there seems to
be three other functions apart from the stat counters that dereference
it:

ath9k_htc_suspend()
ath9k_htc_resume()
ath9k_hif_usb_disconnect()

What guarantees that none of these will be called midway through
ath9k_htc_probe_device() (which would lead to a NULL deref after this
change)?

-Toke

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

* Re: [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros
  2022-02-07 21:24   ` Jeff Johnson
@ 2022-02-08 15:32     ` Toke Høiland-Jørgensen
  2022-02-08 15:49       ` Pavel Skripkin
  0 siblings, 1 reply; 19+ messages in thread
From: Toke Høiland-Jørgensen @ 2022-02-08 15:32 UTC (permalink / raw)
  To: Jeff Johnson, Pavel Skripkin, ath9k-devel, kvalo, davem, kuba, linville
  Cc: linux-wireless, netdev, linux-kernel

Jeff Johnson <quic_jjohnson@quicinc.com> writes:

> On 2/7/2022 12:24 PM, Pavel Skripkin wrote:
>> 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.
>> 
>> Also fixed following checkpatch warning
>> 
>> ERROR: Macros with complex values should be enclosed in parentheses
>> 
>> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
>> ---
>> 
>> 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/htc.h | 16 ++++++++--------
>>   1 file changed, 8 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/net/wireless/ath/ath9k/htc.h b/drivers/net/wireless/ath/ath9k/htc.h
>> index 141642e5e00d..b4755e21a501 100644
>> --- a/drivers/net/wireless/ath/ath9k/htc.h
>> +++ b/drivers/net/wireless/ath/ath9k/htc.h
>> @@ -327,14 +327,14 @@ static inline struct ath9k_htc_tx_ctl *HTC_SKB_CB(struct sk_buff *skb)
>>   }
>>   
>>   #ifdef CONFIG_ATH9K_HTC_DEBUGFS
>> -#define __STAT_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0)
>> -#define TX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++)
>> -#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a)
>> -#define RX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++)
>> -#define RX_STAT_ADD(c, a) __STAT_SAVE(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_SAVE(expr)	(hif_dev->htc_handle->drv_priv ? (expr) : 0)
>> +#define TX_STAT_INC(c)		__STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++)
>> +#define TX_STAT_ADD(c, a)	__STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a)
>> +#define RX_STAT_INC(c)		__STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++)
>> +#define RX_STAT_ADD(c, a)	__STAT_SAVE(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]++)
>>   
>>   void ath9k_htc_err_stat_rx(struct ath9k_htc_priv *priv,
>>   			   struct ath_rx_status *rs);
>
> It seems that these macros (both the original and the new) aren't 
> following the guidance from the Coding Style which tells us under 
> "Things to avoid when using macros" that we should avoid "macros that 
> depend on having a local variable with a magic name". Wouldn't these 
> macros be "better" is they included the hif_dev/priv as arguments rather 
> than being "magic"?

Hmm, yeah, that's a good point; looks like the non-HTC ath9k stats
macros have already been converted to take the container as a parameter,
so taking this opportunity to fix these macros is not a bad idea. While
we're at it, let's switch to the do{} while(0) syntax the other macros
are using instead of that weird usage of ?:. And there's not really any
reason for the duplication between ADD/INC either. So I'm thinking
something like:

#define __STAT_SAVE(_priv, _member, _n) do { if (_priv) (_priv)->_member += (_n); } while(0)

#define TX_STAT_ADD(_priv, _c, _a) __STAT_SAVE(_priv, debug.tx_stats._c, _a)
#define TX_STAT_INC(_priv, _c) TX_STAT_ADD(_priv, _c, 1)

[... etc ...]

-Toke

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

* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb
  2022-02-08 14:47 ` [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb Toke Høiland-Jørgensen
@ 2022-02-08 15:48   ` Pavel Skripkin
  2022-05-02  6:10     ` Tetsuo Handa
  0 siblings, 1 reply; 19+ messages in thread
From: Pavel Skripkin @ 2022-02-08 15:48 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen, ath9k-devel, kvalo, davem,
	kuba, linville
  Cc: linux-wireless, netdev, linux-kernel,
	syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe

Hi Toke,

On 2/8/22 17:47, Toke Høiland-Jørgensen wrote:
> Pavel Skripkin <paskripkin@gmail.com> writes:
> 
>> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb(). 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 save.
> 
> I'm not too familiar with how the initialisation flow of an ath9k_htc
> device works. Looking at htc_handle->drv_priv there seems to
> be three other functions apart from the stat counters that dereference
> it:
> 
> ath9k_htc_suspend()
> ath9k_htc_resume()
> ath9k_hif_usb_disconnect()
> 
> What guarantees that none of these will be called midway through
> ath9k_htc_probe_device() (which would lead to a NULL deref after this
> change)?
> 

IIUC, situation you are talking about may happen even without my change.
I was thinking, that ath9k_htc_probe_device() is the real ->probe() 
function, but things look a bit more tricky.


So, the ->probe() function may be completed before 
ath9k_htc_probe_device() is called, because it's called from fw loader 
callback function. If ->probe() is completed, than we can call 
->suspend(), ->resume() and others usb callbacks, right? And we can meet 
NULL defer even if we leave drv_priv = priv initialization on it's place.

Please, correct me if I am wrong somewhere :)




With regards,
Pavel Skripkin

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

* Re: [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros
  2022-02-08 15:32     ` Toke Høiland-Jørgensen
@ 2022-02-08 15:49       ` Pavel Skripkin
  0 siblings, 0 replies; 19+ messages in thread
From: Pavel Skripkin @ 2022-02-08 15:49 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen, Jeff Johnson, ath9k-devel,
	kvalo, davem, kuba, linville
  Cc: linux-wireless, netdev, linux-kernel

Hi Toke,

On 2/8/22 18:32, Toke Høiland-Jørgensen wrote:
>> It seems that these macros (both the original and the new) aren't 
>> following the guidance from the Coding Style which tells us under 
>> "Things to avoid when using macros" that we should avoid "macros that 
>> depend on having a local variable with a magic name". Wouldn't these 
>> macros be "better" is they included the hif_dev/priv as arguments rather 
>> than being "magic"?
> 
> Hmm, yeah, that's a good point; looks like the non-HTC ath9k stats
> macros have already been converted to take the container as a parameter,
> so taking this opportunity to fix these macros is not a bad idea. While
> we're at it, let's switch to the do{} while(0) syntax the other macros
> are using instead of that weird usage of ?:. And there's not really any
> reason for the duplication between ADD/INC either. So I'm thinking
> something like:
> 
> #define __STAT_SAVE(_priv, _member, _n) do { if (_priv) (_priv)->_member += (_n); } while(0)
> 
> #define TX_STAT_ADD(_priv, _c, _a) __STAT_SAVE(_priv, debug.tx_stats._c, _a)
> #define TX_STAT_INC(_priv, _c) TX_STAT_ADD(_priv, _c, 1)
> 

Good point, thank you. Will redo these macros in v4


With regards,
Pavel Skripkin

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

* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb
  2022-02-08 15:48   ` Pavel Skripkin
@ 2022-05-02  6:10     ` Tetsuo Handa
  2022-05-05 19:09       ` Pavel Skripkin
  0 siblings, 1 reply; 19+ messages in thread
From: Tetsuo Handa @ 2022-05-02  6:10 UTC (permalink / raw)
  To: Pavel Skripkin, Toke Høiland-Jørgensen, ath9k-devel,
	kvalo, davem, kuba, linville
  Cc: linux-wireless, netdev, linux-kernel,
	syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe

On 2022/02/09 0:48, Pavel Skripkin wrote:
>> ath9k_htc_suspend()
>> ath9k_htc_resume()
>> ath9k_hif_usb_disconnect()
>>
>> What guarantees that none of these will be called midway through
>> ath9k_htc_probe_device() (which would lead to a NULL deref after this
>> change)?
>>
> 
> IIUC, situation you are talking about may happen even without my change.
> I was thinking, that ath9k_htc_probe_device() is the real ->probe() function, but things look a bit more tricky.
> 
> 
> So, the ->probe() function may be completed before ath9k_htc_probe_device()
> is called, because it's called from fw loader callback function.

Yes, ath9k_hif_usb_probe() may return before complete_all(&hif_dev->fw_done)
is called by ath9k_hif_usb_firmware_cb() or ath9k_hif_usb_firmware_fail().

> If ->probe() is completed, than we can call ->suspend(), ->resume() and
> others usb callbacks, right?

Yes, but ath9k_hif_usb_disconnect() and ath9k_hif_usb_suspend() are calling
wait_for_completion(&hif_dev->fw_done) before checking HIF_USB_READY flag.
hif_dev->fw_done serves for serialization.

> And we can meet NULL defer even if we leave drv_priv = priv initialization
> on it's place.

I didn't catch the location. As long as "htc_handle->drv_priv = priv;" is done
before complete_all(&hif_dev->fw_done) is done, is something wrong?

> 
> Please, correct me if I am wrong somewhere :)

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

* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb
  2022-05-02  6:10     ` Tetsuo Handa
@ 2022-05-05 19:09       ` Pavel Skripkin
  2022-05-05 23:31         ` Tetsuo Handa
  0 siblings, 1 reply; 19+ messages in thread
From: Pavel Skripkin @ 2022-05-05 19:09 UTC (permalink / raw)
  To: Tetsuo Handa, Toke Høiland-Jørgensen, ath9k-devel,
	kvalo, davem, kuba, linville
  Cc: linux-wireless, netdev, linux-kernel,
	syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe


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

Hi Tetsuo,

On 5/2/22 09:10, Tetsuo Handa wrote:
>> And we can meet NULL defer even if we leave drv_priv = priv initialization
>> on it's place.
> 
> I didn't catch the location. As long as "htc_handle->drv_priv = priv;" is done
> before complete_all(&hif_dev->fw_done) is done, is something wrong?
> 

I don't really remember why I said that, but looks like I just haven't 
opened callbacks' code.

My point was that my patch does not change the logic, but only fixes 2 
problems: UAF and NULL deref.




With regards,
Pavel Skripkin

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

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

* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb
  2022-05-05 19:09       ` Pavel Skripkin
@ 2022-05-05 23:31         ` Tetsuo Handa
  2022-05-10 19:26           ` Pavel Skripkin
  0 siblings, 1 reply; 19+ messages in thread
From: Tetsuo Handa @ 2022-05-05 23:31 UTC (permalink / raw)
  To: Pavel Skripkin, Toke Høiland-Jørgensen, ath9k-devel,
	kvalo, davem, kuba, linville
  Cc: linux-wireless, netdev, linux-kernel,
	syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe

On 2022/05/06 4:09, Pavel Skripkin wrote:
>>> And we can meet NULL defer even if we leave drv_priv = priv initialization
>>> on it's place.
>>
>> I didn't catch the location. As long as "htc_handle->drv_priv = priv;" is done
>> before complete_all(&hif_dev->fw_done) is done, is something wrong?
>>
> 
> I don't really remember why I said that, but looks like I just haven't opened callbacks' code.

OK. Then, why not accept Pavel's patch?

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

* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb
  2022-05-05 23:31         ` Tetsuo Handa
@ 2022-05-10 19:26           ` Pavel Skripkin
  2022-05-11  4:50             ` Kalle Valo
  0 siblings, 1 reply; 19+ messages in thread
From: Pavel Skripkin @ 2022-05-10 19:26 UTC (permalink / raw)
  To: Tetsuo Handa, Toke Høiland-Jørgensen, ath9k-devel,
	kvalo, davem, kuba, linville
  Cc: linux-wireless, netdev, linux-kernel,
	syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe


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

Hi Tetsuo,

On 5/6/22 02:31, Tetsuo Handa wrote:
> On 2022/05/06 4:09, Pavel Skripkin wrote:
>>>> And we can meet NULL defer even if we leave drv_priv = priv initialization
>>>> on it's place.
>>>
>>> I didn't catch the location. As long as "htc_handle->drv_priv = priv;" is done
>>> before complete_all(&hif_dev->fw_done) is done, is something wrong?
>>>
>> 
>> I don't really remember why I said that, but looks like I just haven't opened callbacks' code.
> 
> OK. Then, why not accept Pavel's patch?

As you might expect, I have same question. This series is under review 
for like 7-8 months.

I have no ath9 device, so I can't test it on real hw, so somebody else 
should do it for me. It's requirement to get patch accepted.



With regards,
Pavel Skripkin

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

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

* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb
  2022-05-10 19:26           ` Pavel Skripkin
@ 2022-05-11  4:50             ` Kalle Valo
  2022-05-11  9:53               ` Toke Høiland-Jørgensen
  0 siblings, 1 reply; 19+ messages in thread
From: Kalle Valo @ 2022-05-11  4:50 UTC (permalink / raw)
  To: Pavel Skripkin
  Cc: Tetsuo Handa, Toke Høiland-Jørgensen, ath9k-devel,
	davem, kuba, linville, linux-wireless, netdev, linux-kernel,
	syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe

Pavel Skripkin <paskripkin@gmail.com> writes:

> Hi Tetsuo,
>
> On 5/6/22 02:31, Tetsuo Handa wrote:
>> On 2022/05/06 4:09, Pavel Skripkin wrote:
>>>>> And we can meet NULL defer even if we leave drv_priv = priv initialization
>>>>> on it's place.
>>>>
>>>> I didn't catch the location. As long as "htc_handle->drv_priv = priv;" is done
>>>> before complete_all(&hif_dev->fw_done) is done, is something wrong?
>>>>
>>>
>>> I don't really remember why I said that, but looks like I just haven't opened callbacks' code.
>>
>> OK. Then, why not accept Pavel's patch?
>
> As you might expect, I have same question. This series is under review
> for like 7-8 months.
>
> I have no ath9 device, so I can't test it on real hw, so somebody else
> should do it for me. It's requirement to get patch accepted.

As Toke stepped up to be the ath9k maintainer the situation with ath9k
is now much better. I recommend resubmitting any ath9k patches you might
have.

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

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

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

* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb
  2022-05-11  4:50             ` Kalle Valo
@ 2022-05-11  9:53               ` Toke Høiland-Jørgensen
  2022-05-11  9:59                 ` Kalle Valo
  0 siblings, 1 reply; 19+ messages in thread
From: Toke Høiland-Jørgensen @ 2022-05-11  9:53 UTC (permalink / raw)
  To: Kalle Valo, Pavel Skripkin
  Cc: Tetsuo Handa, ath9k-devel, davem, kuba, linville, linux-wireless,
	netdev, linux-kernel, syzbot+03110230a11411024147,
	syzbot+c6dde1f690b60e0b9fbe

Kalle Valo <kvalo@kernel.org> writes:

> Pavel Skripkin <paskripkin@gmail.com> writes:
>
>> Hi Tetsuo,
>>
>> On 5/6/22 02:31, Tetsuo Handa wrote:
>>> On 2022/05/06 4:09, Pavel Skripkin wrote:
>>>>>> And we can meet NULL defer even if we leave drv_priv = priv initialization
>>>>>> on it's place.
>>>>>
>>>>> I didn't catch the location. As long as "htc_handle->drv_priv = priv;" is done
>>>>> before complete_all(&hif_dev->fw_done) is done, is something wrong?
>>>>>
>>>>
>>>> I don't really remember why I said that, but looks like I just haven't opened callbacks' code.
>>>
>>> OK. Then, why not accept Pavel's patch?
>>
>> As you might expect, I have same question. This series is under review
>> for like 7-8 months.
>>
>> I have no ath9 device, so I can't test it on real hw, so somebody else
>> should do it for me. It's requirement to get patch accepted.
>
> As Toke stepped up to be the ath9k maintainer the situation with ath9k
> is now much better. I recommend resubmitting any ath9k patches you might
> have.

No need to resubmit this one, it's still in patchwork waiting for me to
take a closer look. I have a conference this week, but should hopefully
have some time for this next week.

-Toke

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

* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb
  2022-05-11  9:53               ` Toke Høiland-Jørgensen
@ 2022-05-11  9:59                 ` Kalle Valo
  0 siblings, 0 replies; 19+ messages in thread
From: Kalle Valo @ 2022-05-11  9:59 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Pavel Skripkin, Tetsuo Handa, ath9k-devel, davem, kuba, linville,
	linux-wireless, netdev, linux-kernel,
	syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe

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

> Kalle Valo <kvalo@kernel.org> writes:
>
>> Pavel Skripkin <paskripkin@gmail.com> writes:
>>
>>> Hi Tetsuo,
>>>
>>> On 5/6/22 02:31, Tetsuo Handa wrote:
>>>> On 2022/05/06 4:09, Pavel Skripkin wrote:
>>>>>>> And we can meet NULL defer even if we leave drv_priv = priv initialization
>>>>>>> on it's place.
>>>>>>
>>>>>> I didn't catch the location. As long as "htc_handle->drv_priv = priv;" is done
>>>>>> before complete_all(&hif_dev->fw_done) is done, is something wrong?
>>>>>>
>>>>>
>>>>> I don't really remember why I said that, but looks like I just
>>>>> haven't opened callbacks' code.
>>>>
>>>> OK. Then, why not accept Pavel's patch?
>>>
>>> As you might expect, I have same question. This series is under review
>>> for like 7-8 months.
>>>
>>> I have no ath9 device, so I can't test it on real hw, so somebody else
>>> should do it for me. It's requirement to get patch accepted.
>>
>> As Toke stepped up to be the ath9k maintainer the situation with ath9k
>> is now much better. I recommend resubmitting any ath9k patches you might
>> have.
>
> No need to resubmit this one, it's still in patchwork waiting for me to
> take a closer look.

Ah sorry, I thought this was something which was submitted 7-8 months
ago but I didn't check, I should have.

> I have a conference this week, but should hopefully have some time for
> this next week.

It's great to be able to start meeting people again, have a good one :)

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

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

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

* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb
  2022-02-07 20:24 [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb Pavel Skripkin
  2022-02-07 20:24 ` [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros Pavel Skripkin
  2022-02-08 14:47 ` [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb Toke Høiland-Jørgensen
@ 2022-05-12 12:49 ` Toke Høiland-Jørgensen
  2022-05-12 12:55   ` Pavel Skripkin
  2022-05-12 16:05 ` Jeff Johnson
  3 siblings, 1 reply; 19+ messages in thread
From: Toke Høiland-Jørgensen @ 2022-05-12 12:49 UTC (permalink / raw)
  To: Pavel Skripkin, ath9k-devel, kvalo, davem, kuba, linville
  Cc: linux-wireless, netdev, linux-kernel, Pavel Skripkin,
	syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe

Pavel Skripkin <paskripkin@gmail.com> writes:

> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb(). 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 save.
>
> 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>

Could you link the original syzbot report in the commit message as well,
please? Also that 'tested-by' implies that syzbot run-tested this? How
does it do that; does it have ath9k_htc hardware?

> ---
>
> 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..141642e5e00d 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_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0)
> +#define TX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++)
> +#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a)
> +#define RX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++)
> +#define RX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a)
>  #define CAB_STAT_INC   priv->debug.tx_stats.cab_queued++

s/SAVE/SAFE/ here and in the next patch (and the commit message).

-Toke

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

* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb
  2022-05-12 12:49 ` Toke Høiland-Jørgensen
@ 2022-05-12 12:55   ` Pavel Skripkin
  2022-05-12 13:48     ` Toke Høiland-Jørgensen
  0 siblings, 1 reply; 19+ messages in thread
From: Pavel Skripkin @ 2022-05-12 12:55 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen, ath9k-devel, kvalo, davem,
	kuba, linville
  Cc: linux-wireless, netdev, linux-kernel,
	syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe


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

Hi Toke,

On 5/12/22 15:49, Toke Høiland-Jørgensen wrote:
> Pavel Skripkin <paskripkin@gmail.com> writes:
> 
>> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb(). 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 save.
>>
>> 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>
> 
> Could you link the original syzbot report in the commit message as well,

Sure! See links below

use-after-free bug:
https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60

NULL deref bug:
https://syzkaller.appspot.com/bug?id=b8101ffcec107c0567a0cd8acbbacec91e9ee8de

I can add them in commit message if you want :)

> please? Also that 'tested-by' implies that syzbot run-tested this? How
> does it do that; does it have ath9k_htc hardware?
> 

No, it uses CONFIG_USB_RAW_GADGET and CONFIG_USB_DUMMY_HCD for gadgets 
for emulating usb devices.

Basically these things "connect" fake USB device with random usb ids 
from hardcoded table and try to do various things with usb driver

>> ---

[snip]

>> -#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_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0)
>> +#define TX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++)
>> +#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a)
>> +#define RX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++)
>> +#define RX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a)
>>  #define CAB_STAT_INC   priv->debug.tx_stats.cab_queued++
> 
> s/SAVE/SAFE/ here and in the next patch (and the commit message).
> 

Oh, sorry about that! Will update in next version




With regards,
Pavel Skripkin

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

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

* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb
  2022-05-12 12:55   ` Pavel Skripkin
@ 2022-05-12 13:48     ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 19+ messages in thread
From: Toke Høiland-Jørgensen @ 2022-05-12 13:48 UTC (permalink / raw)
  To: Pavel Skripkin, ath9k-devel, kvalo, davem, kuba, linville
  Cc: linux-wireless, netdev, linux-kernel,
	syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe

Pavel Skripkin <paskripkin@gmail.com> writes:

> Hi Toke,
>
> On 5/12/22 15:49, Toke Høiland-Jørgensen wrote:
>> Pavel Skripkin <paskripkin@gmail.com> writes:
>> 
>>> Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb(). 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 save.
>>>
>>> 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>
>> 
>> Could you link the original syzbot report in the commit message as well,
>
> Sure! See links below
>
> use-after-free bug:
> https://syzkaller.appspot.com/bug?id=6ead44e37afb6866ac0c7dd121b4ce07cb665f60
>
> NULL deref bug:
> https://syzkaller.appspot.com/bug?id=b8101ffcec107c0567a0cd8acbbacec91e9ee8de
>
> I can add them in commit message if you want :)

Yes, please do!

>> please? Also that 'tested-by' implies that syzbot run-tested this? How
>> does it do that; does it have ath9k_htc hardware?
>> 
>
> No, it uses CONFIG_USB_RAW_GADGET and CONFIG_USB_DUMMY_HCD for gadgets 
> for emulating usb devices.
>
> Basically these things "connect" fake USB device with random usb ids 
> from hardcoded table and try to do various things with usb driver

Ah, right, hence the failures I suppose? Makes sense.

> [snip]
>
>>> -#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_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0)
>>> +#define TX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++)
>>> +#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a)
>>> +#define RX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++)
>>> +#define RX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a)
>>>  #define CAB_STAT_INC   priv->debug.tx_stats.cab_queued++
>> 
>> s/SAVE/SAFE/ here and in the next patch (and the commit message).
>> 
>
> Oh, sorry about that! Will update in next version

Thanks! Other than that, I think the patch looks reasonable...

-Toke

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

* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb
  2022-02-07 20:24 [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb Pavel Skripkin
                   ` (2 preceding siblings ...)
  2022-05-12 12:49 ` Toke Høiland-Jørgensen
@ 2022-05-12 16:05 ` Jeff Johnson
  2022-05-12 16:09   ` Pavel Skripkin
  3 siblings, 1 reply; 19+ messages in thread
From: Jeff Johnson @ 2022-05-12 16:05 UTC (permalink / raw)
  To: Pavel Skripkin, ath9k-devel, kvalo, davem, kuba, toke, linville
  Cc: linux-wireless, netdev, linux-kernel,
	syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe

On 2/7/2022 12:24 PM, Pavel Skripkin wrote:
[...snip...]
>   
>   #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_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0)
> +#define TX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++)
> +#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a)
> +#define RX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++)
> +#define RX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a)

it is unfortunate that the existing macros don't abide by the coding style:
	Things to avoid when using macros:
	macros that depend on having a local variable with a magic name

the companion macros in ath9k/debug.h do the right thing

perhaps this could be given to Kernel Janitors for subsequent cleanup?

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

* Re: [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb
  2022-05-12 16:05 ` Jeff Johnson
@ 2022-05-12 16:09   ` Pavel Skripkin
  0 siblings, 0 replies; 19+ messages in thread
From: Pavel Skripkin @ 2022-05-12 16:09 UTC (permalink / raw)
  To: Jeff Johnson, ath9k-devel, kvalo, davem, kuba, toke, linville
  Cc: linux-wireless, netdev, linux-kernel,
	syzbot+03110230a11411024147, syzbot+c6dde1f690b60e0b9fbe


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

Hi Jeff,

On 5/12/22 19:05, Jeff Johnson wrote:
> On 2/7/2022 12:24 PM, Pavel Skripkin wrote:
> [...snip...]
>>   
>>   #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_SAVE(expr) (hif_dev->htc_handle->drv_priv ? (expr) : 0)
>> +#define TX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c++)
>> +#define TX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.tx_stats.c += a)
>> +#define RX_STAT_INC(c) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c++)
>> +#define RX_STAT_ADD(c, a) __STAT_SAVE(hif_dev->htc_handle->drv_priv->debug.skbrx_stats.c += a)
> 
> it is unfortunate that the existing macros don't abide by the coding style:
> 	Things to avoid when using macros:
> 	macros that depend on having a local variable with a magic name
> 
> the companion macros in ath9k/debug.h do the right thing
> 
> perhaps this could be given to Kernel Janitors for subsequent cleanup?

Thanks for pointing that out!

I will clean them up in next version. I think, it will be much easier 
than proposing this task to Kernel Janitors.




With regards,
Pavel Skripkin

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

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

end of thread, other threads:[~2022-05-12 16:09 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-07 20:24 [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb Pavel Skripkin
2022-02-07 20:24 ` [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros Pavel Skripkin
2022-02-07 21:24   ` Jeff Johnson
2022-02-08 15:32     ` Toke Høiland-Jørgensen
2022-02-08 15:49       ` Pavel Skripkin
2022-02-08 14:47 ` [PATCH v3 1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb Toke Høiland-Jørgensen
2022-02-08 15:48   ` Pavel Skripkin
2022-05-02  6:10     ` Tetsuo Handa
2022-05-05 19:09       ` Pavel Skripkin
2022-05-05 23:31         ` Tetsuo Handa
2022-05-10 19:26           ` Pavel Skripkin
2022-05-11  4:50             ` Kalle Valo
2022-05-11  9:53               ` Toke Høiland-Jørgensen
2022-05-11  9:59                 ` Kalle Valo
2022-05-12 12:49 ` Toke Høiland-Jørgensen
2022-05-12 12:55   ` Pavel Skripkin
2022-05-12 13:48     ` Toke Høiland-Jørgensen
2022-05-12 16:05 ` Jeff Johnson
2022-05-12 16:09   ` Pavel Skripkin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.