| Message ID | 20210510041649.589754-1-ducheng2@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | net: mac80211: fix hard-coding of length check on skb->len in ieee80211_scan_rx() | expand |
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
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
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);
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(-)