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

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

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

[Toke Høiland-Jørgensen]

[ This had the wrong mailing list in Cc, adding back linux-wireless ]

Pavel Skripkin <paskripkin@gmail.com> writes:
> 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

[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

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