net: mac80211: fix hard-coding of length check on skb->len in ieee80211_scan_rx()
PATCH 1/1 net: mac80211: fix hard-coding of length check on skb->len in ieee80211_scan_rx()
Replace hard-coding with compile-time constants for header length check on skb->len. This skb->len will be checked again further down the callstack in cfg80211_inform_bss_frame_data() in net/wireless/scan.c (which has a proper length check with WARN_ON()). If the kernel is configure to panic_on_warn(), the insuffient check of skb->len in ieee80211_scan_rx() causes kernel crash in cfg80211_inform_bss_frame_data().
Bug reported by syzbot:
https://syzkaller.appspot.com/bug?id=183869c2f25b1c8692e381d8fcd69771a99221cc
---
This patch has passed syzbot testing.
net/mac80211/scan.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
net/mac80211/scan.c +13 -5
@@ -251,13 +251,21 @@ void ieee80211_scan_rx(struct ieee80211_local *local, struct sk_buff *skb)struct ieee80211_mgmt *mgmt = (void *)skb->data;struct ieee80211_bss *bss;struct ieee80211_channel *channel; + size_t min_hdr_len = offsetof(struct ieee80211_mgmt, u.probe_resp.variable); + +if (!ieee80211_is_probe_resp (mgmt->frame_control) && + !ieee80211_is_beacon (mgmt->frame_control) && + !ieee80211_is_s1g_beacon (mgmt->frame_control)) +return ;if (ieee80211_is_s1g_beacon (mgmt->frame_control)) { -if (skb->len <15 ) -return ; - }else if (skb->len <24 || - (!ieee80211_is_probe_resp (mgmt->frame_control) && - !ieee80211_is_beacon (mgmt->frame_control))) +if (ieee80211_is_s1g_short_beacon (mgmt->frame_control)) + min_hdr_len = offsetof(struct ieee80211_ext, u.s1g_short_beacon.variable); +else + min_hdr_len = offsetof(struct ieee80211_ext, u.s1g_beacon); + } + +if (skb->len < min_hdr_len)return ; sdata1 =rcu_dereference (local->scan_sdata);
Signature · 1 line
2.30.2
Quoted 8 lines · Du Cheng
Replace hard-coding with compile-time constants for header length check on
skb->len. Thisskb->lenwill be checked again further down the callstack incfg80211_inform_bss_frame_data()innet/wireless/scan.c(which has a proper length check withWARN_ON()). If the kernel is configure topanic_on_warn(), the insuffient check ofskb->leninieee80211_scan_rx()causes kernel crash incfg80211_inform_bss_frame_data().
Where is this WARN_ON? I didn't see it cfg80211_inform_bss_frame_data()
Please add more information on why the values of min_hdr_len in this patch make sense for each of these cases.
Quoted 17 lines
Bug reported by syzbot:
https://syzkaller.appspot.com/bug?id=183869c2f25b1c8692e381d8fcd69771a99221ccReported-by [email protected]Signed-off-by Du Cheng <[email protected]>---
This patch has passed syzbot testing.
net/mac80211/scan.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)net/mac80211/scan.c +0
-0
net/mac80211/scan.c void ieee80211_scan_rx(struct ieee80211_local *local, struct sk_buff *skb) · line 260
Quoted 10 lines
net/mac80211/scan.c +6
-0@@ -251,13 +251,21 @@ void ieee80211_scan_rx(struct ieee80211_local *local, struct sk_buff *skb)struct ieee80211_mgmt *mgmt = (void *)skb->data;struct ieee80211_bss *bss;struct ieee80211_channel *channel; + size_t min_hdr_len = offsetof(struct ieee80211_mgmt, u.probe_resp.variable); + +if (!ieee80211_is_probe_resp (mgmt->frame_control) && + !ieee80211_is_beacon (mgmt->frame_control) && + !ieee80211_is_s1g_beacon (mgmt->frame_control)) +return ;
Is the above check necessary? This doesn't look right. This skips the ieee80211_is_s1g_beacon() all together.
if (ieee80211_is_s1g_beacon(mgmt->frame_control)) {
net/mac80211/scan.c void ieee80211_scan_rx(struct ieee80211_local *local, struct sk_buff *skb) · line 257
Quoted 2 lines
net/mac80211/scan.c +0
-2-if (skb->len <15 ) -return ;
Why not compare the header offset you expect here instead of 15 and return?
net/mac80211/scan.c void ieee80211_scan_rx(struct ieee80211_local *local, struct sk_buff *skb) · line 258
Quoted 1 line
net/mac80211/scan.c +0
-1- } else if (skb->len < 24 ||
Can you explain why it makes sense to remove < 24 check?
net/mac80211/scan.c void ieee80211_scan_rx(struct ieee80211_local *local, struct sk_buff *skb)
Quoted 13 lines
net/mac80211/scan.c +7
-2- (!ieee80211_is_probe_resp (mgmt->frame_control) && - !ieee80211_is_beacon(mgmt->frame_control))) +if (ieee80211_is_s1g_short_beacon (mgmt->frame_control)) + min_hdr_len = offsetof(struct ieee80211_ext, u.s1g_short_beacon.variable); +else + min_hdr_len = offsetof(struct ieee80211_ext, u.s1g_beacon); + } + +if (skb->len < min_hdr_len)return ; sdata1 =rcu_dereference (local->scan_sdata);
thanks,
-- Shuah
Quoted 10 lines · Shuah Khan
Quoted 8 lines · Du Cheng
Replace hard-coding with compile-time constants for header length check on
skb->len. Thisskb->lenwill be checked again further down the callstack incfg80211_inform_bss_frame_data()innet/wireless/scan.c(which has a proper length check withWARN_ON()). If the kernel is configure topanic_on_warn(), the insuffient check ofskb->leninieee80211_scan_rx()causes kernel crash incfg80211_inform_bss_frame_data().
Quoted 4 lines
Where is this
WARN_ON? I didn't see itcfg80211_inform_bss_frame_data()Please add more information on why the values of
min_hdr_lenin this patch make sense for each of these cases.
Hi Shuah,
The WARN_ON() is located here:linux/net/wireless/scan.c: 2331
```
if (ieee80211_is_s1g_beacon(mgmt->frame_control)) {
ext = (void *) mgmt;
min_hdr_len = offsetof(struct ieee80211_ext, u.s1g_beacon);
if (ieee80211_is_s1g_short_beacon(mgmt->frame_control))
min_hdr_len = offsetof(struct ieee80211_ext,
u.s1g_short_beacon.variable);
}
if (WARN_ON(len < min_hdr_len)) // <- warn_on line
return NULL;
```the min_hdr_len that I added in was following the setup of min_hdr_len before that WARN_ON(len < min_hdr_len)
Quoted 22 lines
Quoted 22 lines
Bug reported by syzbot:
https://syzkaller.appspot.com/bug?id=183869c2f25b1c8692e381d8fcd69771a99221ccReported-by [email protected]Signed-off-by Du Cheng <[email protected]>---
This patch has passed syzbot testing.
net/mac80211/scan.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)net/mac80211/scan.c +1
-0@@ -251,13 +251,21 @@ void ieee80211_scan_rx(struct ieee80211_local *local, struct sk_buff *skb)struct ieee80211_mgmt *mgmt = (void *)skb->data;struct ieee80211_bss *bss;struct ieee80211_channel *channel; + size_t min_hdr_len = offsetof(struct ieee80211_mgmt, u.probe_resp.variable);
Quoted 8 lines
net/mac80211/scan.c void ieee80211_scan_rx(struct ieee80211_local *local, struct sk_buff *skb) · line 259
Quoted 5 lines
net/mac80211/scan.c +5
-0+ +if (!ieee80211_is_probe_resp (mgmt->frame_control) && + !ieee80211_is_beacon (mgmt->frame_control) && + !ieee80211_is_s1g_beacon (mgmt->frame_control)) +return ;Is the above check necessary? This doesn't look right. This skips the
ieee80211_is_s1g_beacon()all together.
the original check only has the first two conditions (ieee80211_is_probe_resp() and ieee80211_is_beacon()), but they were placed after condition of ieee80211_is_s1g_beacon() not being met. Since I moved these checks at the above, _before_ the if(ieee80211_is_s1g_beacon()), hence I joined ieee80211_is_s1g_beacon() with the two orginal conditions.
Quoted 8 lines
if (ieee80211_is_s1g_beacon(mgmt->frame_control)) {net/mac80211/scan.c void ieee80211_scan_rx(struct ieee80211_local *local, struct sk_buff *skb) · line 257
Quoted 2 lines
net/mac80211/scan.c +0
-2-if (skb->len <15 ) -return ;Why not compare the header offset you expect here instead of 15 and return?
In fact, I do not understand where 15 (and the 24 shortly after) comes from. They were there more than 10 years ago, but the more recent guard code in cfg80211_inform_single_bss_frame_data() on the same header length seems more correct, therefore I followed examples and copied the calculation from there, for consistency.
Quoted 4 lines
net/mac80211/scan.c void ieee80211_scan_rx(struct ieee80211_local *local, struct sk_buff *skb) · line 258
Quoted 2 lines
net/mac80211/scan.c +0
-1- } else if (skb->len < 24 ||Can you explain why it makes sense to remove < 24 check?
I replaced 24 withsize_t min_hdr_len = offsetof(struct ieee80211_mgmt, u.probe_resp.variable); which was found in cfg80211_inform_single_bss_frame_data(), for conditions:
ieee80211_is_probe_resp(mgmt->frame_control) or ieee80211_is_beacon(mgmt->frame_control)
Quoted 16 lines
net/mac80211/scan.c void ieee80211_scan_rx(struct ieee80211_local *local, struct sk_buff *skb)
Quoted 13 lines
net/mac80211/scan.c +7
-2- (!ieee80211_is_probe_resp (mgmt->frame_control) && - !ieee80211_is_beacon(mgmt->frame_control))) +if (ieee80211_is_s1g_short_beacon (mgmt->frame_control)) + min_hdr_len = offsetof(struct ieee80211_ext, u.s1g_short_beacon.variable); +else + min_hdr_len = offsetof(struct ieee80211_ext, u.s1g_beacon); + } + +if (skb->len < min_hdr_len)return ; sdata1 =rcu_dereference (local->scan_sdata);thanks,
-- Shuah
Best regards,
Du Cheng