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

[PATCH v3 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(). 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

[PATCH v3 2/2] ath9k: htc: clean up *STAT_* 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.

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

Re: [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros

[Jeff Johnson]

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"?

Re: [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros

[Toke Høiland-Jørgensen]

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

Re: [PATCH v3 2/2] ath9k: htc: clean up *STAT_* macros

[Pavel Skripkin]

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

Re: [PATCH v3 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(). 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

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

[Pavel Skripkin]

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

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

[Tetsuo Handa]

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 :)

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

[Pavel Skripkin]

[-- 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 --]

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

[Tetsuo Handa]

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?

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

[Pavel Skripkin]

[-- 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 --]

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

[Kalle Valo]

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

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

[Toke Høiland-Jørgensen]

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

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

[Kalle Valo]

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

Re: [PATCH v3 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(). 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

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

[Pavel Skripkin]

[-- 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 --]

Re: [PATCH v3 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 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

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

[Jeff Johnson]

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?

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

[Pavel Skripkin]

[-- 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 --]