diff mbox series

[1/4] wifi: mac80211: mlme: fix verification of puncturing bitmap obtained from AP

Message ID 20230928055022.9670-2-quic_kangyang@quicinc.com
State New
Headers show
Series dynamically update puncturing bitmap | expand

Commit Message

Kang Yang Sept. 28, 2023, 5:50 a.m. UTC 
Puncturing bitmap and bandwidth is included in beacon's EHT Operation
element. After receiving beacon, mac80211 will verify if they are match.
But the bandwidth used for verification is incorrect. Because bandwidth
in link->conf->chandef is a negotiated bandwidth, it may be downgraded.
Should use bandwidth included in beacon. Otherwise when bandwidth
downgrade occurs, even if the received values match, an error may be
returned.

Also, checking if bitmap and bandwidth match should be done before
extraction. But here extract first and then check.

So fix these two issues.

Fixes: aa87cd8b3573 ("wifi: mac80211: mlme: handle EHT channel puncturing")
Signed-off-by: Kang Yang <quic_kangyang@quicinc.com>
---
 net/mac80211/mlme.c | 54 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 44 insertions(+), 10 deletions(-)

Comments

Johannes Berg Oct. 18, 2023, 11:39 a.m. UTC | #1 
On Thu, 2023-09-28 at 13:50 +0800, Kang Yang wrote:
> 
> +static enum nl80211_chan_width
> +ieee80211_rx_bw_to_nlwidth(enum ieee80211_sta_rx_bandwidth bw)
> +{
> +	switch (bw) {
> +	case IEEE80211_STA_RX_BW_20:
> +		return NL80211_CHAN_WIDTH_20;

So for a while now I was actually not responding to this because I was
scratching my head over how this function ever could be needed or make
sense ...


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



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

> +		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)) {
> +		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
>                                                      &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!
 
> -	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?

johannes
Kang Yang Oct. 19, 2023, 3:25 a.m. UTC | #2 
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
>
diff mbox series

Patch

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index f93eb38ae0b8..16e15ced28a5 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -5670,11 +5670,33 @@  static bool ieee80211_rx_our_beacon(const u8 *tx_bssid,
 	return ether_addr_equal(tx_bssid, bss->transmitted_bss->bssid);
 }
 
+static enum nl80211_chan_width
+ieee80211_rx_bw_to_nlwidth(enum ieee80211_sta_rx_bandwidth bw)
+{
+	switch (bw) {
+	case IEEE80211_STA_RX_BW_20:
+		return NL80211_CHAN_WIDTH_20;
+	case IEEE80211_STA_RX_BW_40:
+		return NL80211_CHAN_WIDTH_40;
+	case IEEE80211_STA_RX_BW_80:
+		return NL80211_CHAN_WIDTH_80;
+	case IEEE80211_STA_RX_BW_160:
+		return NL80211_CHAN_WIDTH_160;
+	case IEEE80211_STA_RX_BW_320:
+		return NL80211_CHAN_WIDTH_320;
+	default:
+		WARN_ON(1);
+		return NL80211_CHAN_WIDTH_20;
+	}
+}
+
 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);
+
+		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)) {
+		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,
@@ -5695,16 +5739,6 @@  static bool ieee80211_config_puncturing(struct ieee80211_link_data *link,
 	    extracted == link->conf->eht_puncturing)
 		return true;
 
-	if (!cfg80211_valid_disable_subchannel_bitmap(&bitmap,
-						      &link->conf->chandef)) {
-		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,
-			  link->conf->chandef.width);
-		return false;
-	}
-
 	ieee80211_handle_puncturing_bitmap(link, eht_oper, bitmap, changed);
 	return true;
 }