On 5/9/21 10:16 PM, Du Cheng wrote:
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().
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.
Bug reported by syzbot:
https://syzkaller.appspot.com/bug?id=183869c2f25b1c8692e381d8fcd69771a99221cc
Reported-by: syzbot+405843667e93b9790fc1@xxxxxxxxxxxxxxxxxxxxxxxxx
Signed-off-by: Du Cheng <ducheng2@xxxxxxxxx>
---
This patch has passed syzbot testing.
net/mac80211/scan.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index d4cc9ac2d703..562eda13e802 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -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)) {
- if (skb->len < 15)
- return;
Why not compare the header offset you expect here instead of 15 and
return?
- } else if (skb->len < 24 ||
Can you explain why it makes sense to remove < 24 check?
- (!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
