* [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.