| Message ID | 961b028f073d0d5541de66c00a517495431981f9.1653168225.git.paskripkin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Toke Høiland-Jørgensen |
| Headers | show |
| Series | [v5,1/2] ath9k: fix use-after-free in ath9k_hif_usb_rx_cb | expand |
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
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/
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
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
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:
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(-)