net: mac80211: fix hard-coding of length check on skb->len in ieee80211_scan_rx()

Message ID 20210510041649.589754-1-ducheng2@gmail.com
State New
Headers show
Series
  • net: mac80211: fix hard-coding of length check on skb->len in ieee80211_scan_rx()
Related show

Commit Message

Du Cheng May 10, 2021, 4:16 a.m.
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(-)

Comments

Shuah Khan May 11, 2021, 10:29 p.m. | #1
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
Du Cheng May 12, 2021, 12:55 a.m. | #2
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

Patch

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);