diff mbox series

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

Message ID d57bbedc857950659bfacac0ab48790c1eda00c8.1655145743.git.paskripkin@gmail.com (mailing list archive)
State Accepted
Commit 0ac4827f78c7ffe8eef074bc010e7e34bc22f533
Delegated to: Kalle Valo
Headers show
Series [v6,1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb | expand

Commit Message

Pavel Skripkin June 13, 2022, 6:43 p.m. UTC
Syzbot reported use-after-free Read in ath9k_hif_usb_rx_cb() [0]. The
problem was in incorrect htc_handle->drv_priv initialization.

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

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

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

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

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

Changes since v5:
	- No changes

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

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

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

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

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

Comments

Toke Høiland-Jørgensen June 14, 2022, 10:58 a.m. UTC | #1
Pavel Skripkin <paskripkin@gmail.com> writes:

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

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

Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
Kalle Valo June 15, 2022, 7:10 a.m. UTC | #2
Toke Høiland-Jørgensen <toke@toke.dk> writes:

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

I'm wondering should these go to -rc or -next? Has anyone actually
tested these with real hardware? (syzbot testing does not count) With
the past bad experience with syzbot fixes I'm leaning towards -next to
have more time to fix any regressions.
Toke Høiland-Jørgensen June 15, 2022, 9:05 a.m. UTC | #3
Kalle Valo <kvalo@kernel.org> writes:

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

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

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

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

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

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

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

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


thanks,

Takashi
Kalle Valo June 20, 2022, 8:53 a.m. UTC | #5
Takashi Iwai <tiwai@suse.de> writes:

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

Ok, I'll take these to -next then. I just don't like taking untested
patches, having them -next gives us more time to fix any issues (or
revert the patches).
Kalle Valo June 20, 2022, 10:02 a.m. UTC | #6
Pavel Skripkin <paskripkin@gmail.com> wrote:

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

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

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

Patch

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