| Message ID | 80962aae265995d1cdb724f5362c556d494c7566.1644265120.git.paskripkin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v3,1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb | expand |
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
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
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 :)
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
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?
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
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.
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
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 :)
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
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
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
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?
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
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:
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(-)