Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Message-ID: <d85bd8f7-a6aa-6ff7-09a8-57005e4d0779@quicinc.com>
Date:   Thu, 19 Oct 2023 11:25:34 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH 1/4] wifi: mac80211: mlme: fix verification of puncturing
 bitmap obtained from AP
To:     Johannes Berg <johannes@sipsolutions.net>,
        <ath12k@lists.infradead.org>
CC:     <linux-wireless@vger.kernel.org>
References: <20230928055022.9670-1-quic_kangyang@quicinc.com>
 <20230928055022.9670-2-quic_kangyang@quicinc.com>
 <08ff36664eb34bce8e7beb425233b1e8d4a4971c.camel@sipsolutions.net>
Content-Language: en-US
From:   Kang Yang <quic_kangyang@quicinc.com>
In-Reply-To: <08ff36664eb34bce8e7beb425233b1e8d4a4971c.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 10/18/2023 7:39 PM, Johannes Berg wrote:
>>   static bool ieee80211_config_puncturing(struct ieee80211_link_data *link,
>>   					const struct ieee80211_eht_operation *eht_oper,
>>   					u64 *changed)
>>   {
>> +	struct cfg80211_chan_def rx_chandef = link->conf->chandef;
>>   	u16 bitmap = 0, extracted;
>> +	u8 bw = 0;
>>   
>>   	if ((eht_oper->params & IEEE80211_EHT_OPER_INFO_PRESENT) &&
>>   	    (eht_oper->params &
>> @@ -5684,6 +5706,28 @@ static bool ieee80211_config_puncturing(struct ieee80211_link_data *link,
>>   		const u8 *disable_subchannel_bitmap = info->optional;
>>   
>>   		bitmap = get_unaligned_le16(disable_subchannel_bitmap);
>> +		bw = u8_get_bits(info->control, IEEE80211_EHT_OPER_CHAN_WIDTH);
>> +		rx_chandef.width = ieee80211_rx_bw_to_nlwidth(bw);
> 
> But looking here, it clearly _doesn't_ make sense. IEEE80211_STA_RX_BW_*
> is a purely internal API, has nothing to do with the spec.
> 
> All this might even be "accidentally correct", but it really isn't right
> at all - the values in IEEE80211_EHT_OPER_CHAN_WIDTH are
> IEEE80211_EHT_OPER_CHAN_WIDTH_*, not IEEE80211_STA_RX_BW_*.
> 


Oh, sorry that i didn't notice IEEE80211_EHT_OPER_CHAN_WIDTH_*, will 
replace them.


> 
> 
> More generally though, I don't even understand the change.


During assoc, downgrade may happen in func ieee80211_config_bw(). In 
this situation, the bandwidth in beacon and the bandwidth after 
downgrade(chandef->width, maybe i should call it local bandwidth during 
below context, will use this name in next version) during assoc will be 
different.

The change is based on this point.


> 
>> +		if (rx_chandef.width == NL80211_CHAN_WIDTH_80)
>> +			rx_chandef.center_freq1 =
>> +				ieee80211_channel_to_frequency(info->ccfs0,
>> +							       rx_chandef.chan->band);
>> +		else if (rx_chandef.width == NL80211_CHAN_WIDTH_160 ||
>> +			 rx_chandef.width == NL80211_CHAN_WIDTH_320)
>> +			rx_chandef.center_freq1 =
>> +				ieee80211_channel_to_frequency(info->ccfs1,
>> +							       rx_chandef.chan->band);
>> +	}
>> +
>> +	if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap,
>> +						      &rx_chandef)) {


Here i change you code 
cfg80211_valid_disable_subchannel_bitmap(&bitmap,&link->conf->chandef) to
cfg80211_valid_disable_subchannel_bitmap(&bitmap,&rx_chandef)

As described above, downgrade may happen before enter 
ieee80211_config_puncturing(), so the bandwidth in link->conf->chandef 
may be different with bandwidth in beacon.

Here, the bitmap you used is from eht_oper in beacon, but the bandwidth 
you used is local bandwidth. They are not match. This is the first issue.


>> +		link_info(link,
>> +			  "Got an invalid disable subchannel bitmap from AP %pM: bitmap = 0x%x, bw = 0x%x. disconnect\n",
>> +			  link->u.mgd.bssid,
>> +			  bitmap,
>> +			  rx_chandef.width);
>> +		return false;
>>   	}
>>   
>>   	extracted = ieee80211_extract_dis_subch_bmap(eht_oper,
> // I've filled in the context here in the patch


Here is move the cfg80211_valid_disable_subchannel_bitmap() before the 
ieee80211_extract_dis_subch_bmap(), cause i think check should done 
before extract.

This is the second issue i said(perhaps not a issue).


>>                                                       &link->conf->chandef,
>>                                                       bitmap);
>>
>>          /* accept if there are no changes */
>>          if (!(*changed & BSS_CHANGED_BANDWIDTH) &&
>>              extracted == link->conf->eht_puncturing)
>>                  return true;
> 
> but ... ieee80211_extract_dis_subch_bmap actually already takes the
> bandwidth from eht_oper into account!
>   

Yes, the ieee80211_extract_dis_subch_bmap realy takes the bandwidth from 
eht_oper into account, and the local_bw in this func is the local 
bandwidth i discuss.

You already notice the difference between bandwidth from eht_oper and 
local bandwidth in ieee80211_extract_dis_subch_bmap(). I think you 
should also take it into account when you use 
cfg80211_valid_disable_subchannel_bitmap(), right?

BTW, do you want to verify the bitmap from eht_oper, or the extracted 
bitmap?

Anyway, whether you want to verify the bitmap from eht_oper or extracted 
bitmap in cfg80211_valid_disable_subchannel_bitmap(), the bitmap and 
bandwidth must correspond.


>> -	if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap,
>> -						      &link->conf->chandef)) {
> 
> So are you saying that the real bug is that we're missing to update the
> link->conf->chandef with the EHT operation from the assoc response?
> 
> But you didn't fix that issue ... so not sure?


I have described my patch with the comments above, maybe i should make 
my commit log more coherent.


Besides, this is you first version, i made some comments on Nov. 21, 
2022, 7:29 a.m. Maybe you already forget them.
https://patchwork.kernel.org/project/linux-wireless/patch/20220325140859.e48bf244f157.I3547481d49f958389f59dfeba3fcc75e72b0aa6e@changeid/


> 
> johannes
>