| Message ID | 20221018044341.5453-1-quic_ramess@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: mac80211: Allow NSS change only up to capability | expand |
On Tue, 2022-10-18 at 10:13 +0530, Rameshkumar Sundaram wrote: > Stations can update bandwidth/NSS change in > VHT action frame with action type Operating Mode Notification. > (IEEE Std 802.11-2020 - 9.4.1.53 Operating Mode field) > > For Operating Mode Notification, an RX NSS change to a value > greater than AP's maximum NSS should not be allowed. > Hence allow NSS change only up to maximum NSS that is negotiated > and capped to AP's capability during association. That seems reasonable. Might be worth noting in a comment in the code which AP has such bugs though, just so we know it later. > if (link_sta->pub->rx_nss != nss) { > - link_sta->pub->rx_nss = nss; > - sta_opmode.rx_nss = nss; > - changed |= IEEE80211_RC_NSS_CHANGED; > - sta_opmode.changed |= STA_OPMODE_N_SS_CHANGED; > + cur_nss = link_sta->pub->rx_nss; > + link_sta->pub->rx_nss = 0; > + ieee80211_sta_set_rx_nss(link_sta); > + /* Do not allow an nss change to rx_nss greater than max_nss > + * negotiated and capped to APs capability during association. > + */ > + if (nss <= link_sta->pub->rx_nss) { > + link_sta->pub->rx_nss = nss; That, however, doesn't seem right. It means that you can only ever reduce the RX NSS, not switch it around within the originally negotiated range. johannes
> -----Original Message----- > From: Johannes Berg <johannes@sipsolutions.net> > Sent: Friday, October 21, 2022 5:13 PM > To: Rameshkumar Sundaram (QUIC) <quic_ramess@quicinc.com> > Cc: linux-wireless@vger.kernel.org > Subject: Re: [PATCH] wifi: mac80211: Allow NSS change only up to capability > > On Tue, 2022-10-18 at 10:13 +0530, Rameshkumar Sundaram wrote: > > Stations can update bandwidth/NSS change in VHT action frame with > > action type Operating Mode Notification. > > (IEEE Std 802.11-2020 - 9.4.1.53 Operating Mode field) > > > > For Operating Mode Notification, an RX NSS change to a value greater > > than AP's maximum NSS should not be allowed. > > Hence allow NSS change only up to maximum NSS that is negotiated and > > capped to AP's capability during association. > > That seems reasonable. Might be worth noting in a comment in the code > which AP has such bugs though, just so we know it later. > These are found during fuzz testing by forcefully sending VHT Op. mode notif. frames from STA with Random rx_nss values. We accepted all the values. > > if (link_sta->pub->rx_nss != nss) { > > - link_sta->pub->rx_nss = nss; > > - sta_opmode.rx_nss = nss; > > - changed |= IEEE80211_RC_NSS_CHANGED; > > - sta_opmode.changed |= STA_OPMODE_N_SS_CHANGED; > > + cur_nss = link_sta->pub->rx_nss; > > + link_sta->pub->rx_nss = 0; > > + ieee80211_sta_set_rx_nss(link_sta); > > + /* Do not allow an nss change to rx_nss greater than > max_nss > > + * negotiated and capped to APs capability during > association. > > + */ > > + if (nss <= link_sta->pub->rx_nss) { > > + link_sta->pub->rx_nss = nss; > > That, however, doesn't seem right. It means that you can only ever reduce > the RX NSS, not switch it around within the originally negotiated range. > Not sure if I understood you comment. We reset Sta's rx_nss and make a call to ieee80211_sta_set_rx_nss() which will set the same to max nss negotiated, so this Will allow any nss change within negotiated range, isn’t it ? > johannes
On Tue, 2022-12-27 at 08:04 +0000, Rameshkumar Sundaram (QUIC) wrote: > > > > > + if (nss <= link_sta->pub->rx_nss) { > > > + link_sta->pub->rx_nss = nss; > > > > That, however, doesn't seem right. It means that you can only ever > > reduce > > the RX NSS, not switch it around within the originally negotiated > > range. > > > Not sure if I understood you comment. > We reset Sta's rx_nss I don't see where it's being reset? The way I'm reading this, you check nss<=rx_nss and then set rx_nss=nss. I didn't see any code that sets rx_nss higher, but maybe I missed it? So if say rx_nss is 4, and nss is 2, then we set rx_nss to 2. But now if the AP wants to switch back to 4, nss will be 4, rx_nss will be 2, and the change is ignored, no? johannes
> -----Original Message----- > From: Johannes Berg <johannes@sipsolutions.net> > Sent: Tuesday, December 27, 2022 9:13 PM > To: Rameshkumar Sundaram (QUIC) <quic_ramess@quicinc.com> > Cc: linux-wireless@vger.kernel.org > Subject: Re: [PATCH] wifi: mac80211: Allow NSS change only up to capability > > On Tue, 2022-12-27 at 08:04 +0000, Rameshkumar Sundaram (QUIC) wrote: > > > > > > > + if (nss <= link_sta->pub->rx_nss) { > > > > + link_sta->pub->rx_nss = nss; > > > > > > That, however, doesn't seem right. It means that you can only ever > > > reduce the RX NSS, not switch it around within the originally > > > negotiated range. > > > > > Not sure if I understood you comment. > > We reset Sta's rx_nss > > I don't see where it's being reset? > > The way I'm reading this, you check nss<=rx_nss and then set rx_nss=nss. > I didn't see any code that sets rx_nss higher, but maybe I missed it? > + cur_nss = link_sta->pub->rx_nss; + link_sta->pub->rx_nss = 0; + ieee80211_sta_set_rx_nss(link_sta); Here we take copy of current rx_nss of STA (that sent the VHT Op. mode notif frame to AP), reset it and call ieee80211_sta_set_rx_nss() which will set link_sta->pub->rx_nss to Max nss that was capped and stored for this STA during association. Hence Below check will allow an nss change until max_nss at any point. + if (nss <= link_sta->pub->rx_nss) { + link_sta->pub->rx_nss = nss; > So if say rx_nss is 4, and nss is 2, then we set rx_nss to 2. But now if the AP > wants to switch back to 4, nss will be 4, rx_nss will be 2, and the change is > ignored, no? > > johannes
diff --git a/net/mac80211/vht.c b/net/mac80211/vht.c index c804890dc623..fb1c55109884 100644 --- a/net/mac80211/vht.c +++ b/net/mac80211/vht.c @@ -623,7 +623,7 @@ u32 __ieee80211_vht_handle_opmode(struct ieee80211_sub_if_data *sdata, enum ieee80211_sta_rx_bandwidth new_bw; struct sta_opmode_info sta_opmode = {}; u32 changed = 0; - u8 nss; + u8 nss, cur_nss; /* ignore - no support for BF yet */ if (opmode & IEEE80211_OPMODE_NOTIF_RX_NSS_TYPE_BF) @@ -634,10 +634,22 @@ u32 __ieee80211_vht_handle_opmode(struct ieee80211_sub_if_data *sdata, nss += 1; if (link_sta->pub->rx_nss != nss) { - link_sta->pub->rx_nss = nss; - sta_opmode.rx_nss = nss; - changed |= IEEE80211_RC_NSS_CHANGED; - sta_opmode.changed |= STA_OPMODE_N_SS_CHANGED; + cur_nss = link_sta->pub->rx_nss; + link_sta->pub->rx_nss = 0; + ieee80211_sta_set_rx_nss(link_sta); + /* Do not allow an nss change to rx_nss greater than max_nss + * negotiated and capped to APs capability during association. + */ + if (nss <= link_sta->pub->rx_nss) { + link_sta->pub->rx_nss = nss; + sta_opmode.rx_nss = nss; + changed |= IEEE80211_RC_NSS_CHANGED; + sta_opmode.changed |= STA_OPMODE_N_SS_CHANGED; + } else { + link_sta->pub->rx_nss = cur_nss; + pr_warn_ratelimited("Ignoring NSS change in VHT Operating Mode Notification from %pM with invalid nss %d", + link_sta->pub->addr, nss); + } } switch (opmode & IEEE80211_OPMODE_NOTIF_CHANWIDTH_MASK) {
Stations can update bandwidth/NSS change in VHT action frame with action type Operating Mode Notification. (IEEE Std 802.11-2020 - 9.4.1.53 Operating Mode field) For Operating Mode Notification, an RX NSS change to a value greater than AP's maximum NSS should not be allowed. Hence allow NSS change only up to maximum NSS that is negotiated and capped to AP's capability during association. Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com> --- net/mac80211/vht.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)