* [PATCH] net: mac80211: fix hard-coding of length check on skb->len in ieee80211_scan_rx() @ 2021-05-10 4:16 Du Cheng 2021-05-11 22:29 ` Shuah Khan 0 siblings, 1 reply; 3+ messages in thread From: Du Cheng @ 2021-05-10 4:16 UTC (permalink / raw) To: Thomas Pedersen, Johannes Berg Cc: linux-wireless, Shuah Khan, Greg Kroah-Hartman, Du Cheng, syzbot+405843667e93b9790fc1 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 Reported-by: syzbot+405843667e93b9790fc1@syzkaller.appspotmail.com Signed-off-by: Du Cheng <ducheng2@gmail.com> --- 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; 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); -- 2.30.2 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] net: mac80211: fix hard-coding of length check on skb->len in ieee80211_scan_rx() 2021-05-10 4:16 [PATCH] net: mac80211: fix hard-coding of length check on skb->len in ieee80211_scan_rx() Du Cheng @ 2021-05-11 22:29 ` Shuah Khan 2021-05-12 0:55 ` Du Cheng 0 siblings, 1 reply; 3+ messages in thread From: Shuah Khan @ 2021-05-11 22:29 UTC (permalink / raw) To: Du Cheng, Thomas Pedersen, Johannes Berg Cc: linux-wireless, Greg Kroah-Hartman, syzbot+405843667e93b9790fc1, Shuah Khan 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@syzkaller.appspotmail.com > Signed-off-by: Du Cheng <ducheng2@gmail.com> > --- > > 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] net: mac80211: fix hard-coding of length check on skb->len in ieee80211_scan_rx() 2021-05-11 22:29 ` Shuah Khan @ 2021-05-12 0:55 ` Du Cheng 0 siblings, 0 replies; 3+ messages in thread From: Du Cheng @ 2021-05-12 0:55 UTC (permalink / raw) To: Shuah Khan Cc: Thomas Pedersen, Johannes Berg, linux-wireless, Greg Kroah-Hartman, syzbot+405843667e93b9790fc1 Le Tue, May 11, 2021 at 04:29:07PM -0600, Shuah Khan a écrit : > 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. 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) > > > Bug reported by syzbot: > > https://syzkaller.appspot.com/bug?id=183869c2f25b1c8692e381d8fcd69771a99221cc > > > > Reported-by: syzbot+405843667e93b9790fc1@syzkaller.appspotmail.com > > Signed-off-by: Du Cheng <ducheng2@gmail.com> > > --- > > > > 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. 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. > > > 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? 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. > > > - } else if (skb->len < 24 || > > Can you explain why it makes sense to remove < 24 check? I replaced 24 with `size_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) > > > - (!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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-05-12 0:55 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-10 4:16 [PATCH] net: mac80211: fix hard-coding of length check on skb->len in ieee80211_scan_rx() Du Cheng 2021-05-11 22:29 ` Shuah Khan 2021-05-12 0:55 ` Du Cheng
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.