| Message ID | 80962aae265995d1cdb724f5362c556d494c7566.1644265120.git.paskripkin@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb | expand |
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"?
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
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
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
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
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(-)