Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] net: mac80211: fix hard-coding of length check on
 skb->len in ieee80211_scan_rx()
To:     Du Cheng <ducheng2@gmail.com>,
        Thomas Pedersen <thomas@adapt-ip.com>,
        Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        syzbot+405843667e93b9790fc1@syzkaller.appspotmail.com,
        Shuah Khan <skhan@linuxfoundation.org>
References: <20210510041649.589754-1-ducheng2@gmail.com>
From:   Shuah Khan <skhan@linuxfoundation.org>
Message-ID: <e52ff0a3-7b64-037d-7825-9cbbcafa21b6@linuxfoundation.org>
Date:   Tue, 11 May 2021 16:29:07 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <20210510041649.589754-1-ducheng2@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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