| Message ID | 20210517201932.8860-10-wgong@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Mon, 2021-05-17 at 16:19 -0400, Wen Gong wrote: > > + if (is_6ghz) { > + struct ieee802_11_elems elems; > + struct ieee80211_bss_conf *bss_conf; > + u8 i, n; > + > + ieee802_11_parse_elems(ies->data, ies->len, false, &elems, > + NULL, NULL); > + bss_conf = &sdata->vif.bss_conf; > + bss_conf->pwr_reduction = 0; > + if (elems.pwr_constr_elem) > + bss_conf->pwr_reduction = *elems.pwr_constr_elem; > + > + memset(bss_conf->tx_pwr_env, 0, sizeof(bss_conf->tx_pwr_env)); > + bss_conf->tx_pwr_env_num = elems.tx_pwr_env_num; > + n = min_t(u8, elems.tx_pwr_env_num, > + ARRAY_SIZE(elems.tx_pwr_env)); If anything, that min_t would make sense only if you were actually using ARRAY_SIZE(bss_conf->tx_pwr_env), but like this it's quite pointless, just checking again if the element parsing was internally consistent? I'd probably remove it and throw in a BUILD_BUG_ON(ARRAY_SIZE(bss_conf->tx_pwr_env) != ARRAY_SIZE(elems.tx_pwr_env)); instead. > + for (i = 0; i < n; i++) > + memcpy(&bss_conf->tx_pwr_env[i], elems.tx_pwr_env[i], > + elems.tx_pwr_env_len[i]); You also never validated that the element wasn't too long! If you connect to 6 Ghz with this, and then again to another AP that doesn't, you'll have it stuck at the old values. You need to reset at some point (during disconnect). And then two more questions: 1) Could this information change? Should we track it in beacons? 2) Should we at least check it again from the protected beacon or such after association, so we don't blindly trust the probe response or beacon (received during scan, not validated) at least when BIGTK is in use? johannes
On 2021-07-23 17:38, Johannes Berg wrote: > On Mon, 2021-05-17 at 16:19 -0400, Wen Gong wrote: >> >> + if (is_6ghz) { >> + struct ieee802_11_elems elems; >> + struct ieee80211_bss_conf *bss_conf; >> + u8 i, n; >> + >> + ieee802_11_parse_elems(ies->data, ies->len, false, &elems, >> + NULL, NULL); >> + bss_conf = &sdata->vif.bss_conf; >> + bss_conf->pwr_reduction = 0; >> + if (elems.pwr_constr_elem) >> + bss_conf->pwr_reduction = *elems.pwr_constr_elem; >> + >> + memset(bss_conf->tx_pwr_env, 0, sizeof(bss_conf->tx_pwr_env)); >> + bss_conf->tx_pwr_env_num = elems.tx_pwr_env_num; >> + n = min_t(u8, elems.tx_pwr_env_num, >> + ARRAY_SIZE(elems.tx_pwr_env)); > > If anything, that min_t would make sense only if you were actually > using > ARRAY_SIZE(bss_conf->tx_pwr_env), but like this it's quite pointless, > just checking again if the element parsing was internally consistent? > > I'd probably remove it and throw in a > > BUILD_BUG_ON(ARRAY_SIZE(bss_conf->tx_pwr_env) != > ARRAY_SIZE(elems.tx_pwr_env)); > > instead. > >> + for (i = 0; i < n; i++) >> + memcpy(&bss_conf->tx_pwr_env[i], elems.tx_pwr_env[i], >> + elems.tx_pwr_env_len[i]); > > You also never validated that the element wasn't too long! > will change it. > > If you connect to 6 Ghz with this, and then again to another AP that > doesn't, you'll have it stuck at the old values. You need to reset at > some point (during disconnect). > will change to reset it in ieee80211_prep_channel outside is_6ghz{}. Then it will be reset for each connection. > And then two more questions: > > 1) Could this information change? Should we track it in beacons? > The information is from AP side, it should be not changed untill the AP restart. If someone want to change configure of AP, the AP should restart and then take effect by my understand. Is it have some case for this information change? > 2) Should we at least check it again from the protected beacon or such > after association, so we don't blindly trust the probe response or > beacon (received during scan, not validated) at least when BIGTK is in > use? May we add support for BIGTK in future with another patch? The info(pwr_reduction and tx_pwr_env) is used by lower driver such as ath11k. If the info changed after association, then how to notify lower driver? Do it like below in ieee80211_rx_mgmt_beacon()? And use BSS_CHANGED_TXPOWER or a new enum in ieee80211_bss_change? ieee80211_rx_mgmt_beacon{ changed |= ieee80211_handle_pwr_constr(sdata, chan, mgmt, elems.country_elem, elems.country_elem_len, elems.pwr_constr_elem, elems.cisco_dtpc_elem); ieee80211_bss_info_change_notify(sdata, changed); } > > johannes
Hi johannes, Could you see my answer below? please feel free to point out the mistakes :) On 2021-07-30 18:47, Wen Gong wrote: > On 2021-07-23 17:38, Johannes Berg wrote: >> On Mon, 2021-05-17 at 16:19 -0400, Wen Gong wrote: >>> >>> + if (is_6ghz) { >>> + struct ieee802_11_elems elems; >>> + struct ieee80211_bss_conf *bss_conf; >>> + u8 i, n; >>> + >>> + ieee802_11_parse_elems(ies->data, ies->len, false, &elems, >>> + NULL, NULL); >>> + bss_conf = &sdata->vif.bss_conf; >>> + bss_conf->pwr_reduction = 0; >>> + if (elems.pwr_constr_elem) >>> + bss_conf->pwr_reduction = *elems.pwr_constr_elem; >>> + >>> + memset(bss_conf->tx_pwr_env, 0, sizeof(bss_conf->tx_pwr_env)); >>> + bss_conf->tx_pwr_env_num = elems.tx_pwr_env_num; >>> + n = min_t(u8, elems.tx_pwr_env_num, >>> + ARRAY_SIZE(elems.tx_pwr_env)); >> >> If anything, that min_t would make sense only if you were actually >> using >> ARRAY_SIZE(bss_conf->tx_pwr_env), but like this it's quite pointless, >> just checking again if the element parsing was internally consistent? >> >> I'd probably remove it and throw in a >> >> BUILD_BUG_ON(ARRAY_SIZE(bss_conf->tx_pwr_env) != >> ARRAY_SIZE(elems.tx_pwr_env)); >> >> instead. >> >>> + for (i = 0; i < n; i++) >>> + memcpy(&bss_conf->tx_pwr_env[i], elems.tx_pwr_env[i], >>> + elems.tx_pwr_env_len[i]); >> >> You also never validated that the element wasn't too long! >> > will change it. >> >> If you connect to 6 Ghz with this, and then again to another AP that >> doesn't, you'll have it stuck at the old values. You need to reset at >> some point (during disconnect). >> > will change to reset it in ieee80211_prep_channel outside is_6ghz{}. > Then it will be reset for each connection. >> And then two more questions: >> >> 1) Could this information change? Should we track it in beacons? >> > > The information is from AP side, it should be not changed untill the AP > restart. > If someone want to change configure of AP, the AP should restart and > then take effect by my understand. > Is it have some case for this information change? > > >> 2) Should we at least check it again from the protected beacon or such >> after association, so we don't blindly trust the probe response or >> beacon (received during scan, not validated) at least when BIGTK is in >> use? > > May we add support for BIGTK in future with another patch? > The info(pwr_reduction and tx_pwr_env) is used by lower driver such as > ath11k. > If the info changed after association, then how to notify lower driver? > Do it like below in ieee80211_rx_mgmt_beacon()? > And use BSS_CHANGED_TXPOWER or a new enum in ieee80211_bss_change? > > ieee80211_rx_mgmt_beacon{ > changed |= ieee80211_handle_pwr_constr(sdata, chan, mgmt, > elems.country_elem, > elems.country_elem_len, > elems.pwr_constr_elem, > elems.cisco_dtpc_elem); > > ieee80211_bss_info_change_notify(sdata, changed); > } > >> >> johannes
On Fri, 2021-07-30 at 18:47 +0800, Wen Gong wrote: > > > And then two more questions: > > > > 1) Could this information change? Should we track it in beacons? > > > > The information is from AP side, it should be not changed untill the AP > restart. > If someone want to change configure of AP, the AP should restart and > then take effect by my understand. > Is it have some case for this information change? No, I guess that's fine then, I just didn't know. > > 2) Should we at least check it again from the protected beacon or such > > after association, so we don't blindly trust the probe response or > > beacon (received during scan, not validated) at least when BIGTK is in > > use? > > May we add support for BIGTK in future with another patch? We already have BIGTK support in mac80211, so if we don't do that now we're almost certainly not going to do it, so I'd really prefer if you did it here, or if a separate patch still did it now. > The info(pwr_reduction and tx_pwr_env) is used by lower driver such as > ath11k. Sure. > If the info changed after association, then how to notify lower driver? > Do it like below in ieee80211_rx_mgmt_beacon()? > And use BSS_CHANGED_TXPOWER or a new enum in ieee80211_bss_change? Yeah, dunno. Are the drivers assuming now it's set once you get to associated state? johannes
On Fri, 2021-08-13 at 09:19 +0200, Johannes Berg wrote: > > > > 2) Should we at least check it again from the protected beacon or such > > > after association, so we don't blindly trust the probe response or > > > beacon (received during scan, not validated) at least when BIGTK is in > > > use? > > > > May we add support for BIGTK in future with another patch? > > We already have BIGTK support in mac80211, so if we don't do that now > we're almost certainly not going to do it, so I'd really prefer if you > did it here, or if a separate patch still did it now. Actually, I should say though - the question was more whether we even need/want that, rather than whether we can do it later or not. If we should protect this data/information then IMHO we should do it now, but it's not clear to me that we should, given that we also don't have encrypted association response and we still take information from there too, etc. johannes
On 2021-08-13 15:19, Johannes Berg wrote: > On Fri, 2021-07-30 at 18:47 +0800, Wen Gong wrote: >> >> > And then two more questions: >> > >> > 1) Could this information change? Should we track it in beacons? >> > >> >> The information is from AP side, it should be not changed untill the >> AP >> restart. >> If someone want to change configure of AP, the AP should restart and >> then take effect by my understand. >> Is it have some case for this information change? > > No, I guess that's fine then, I just didn't know. > >> > 2) Should we at least check it again from the protected beacon or such >> > after association, so we don't blindly trust the probe response or >> > beacon (received during scan, not validated) at least when BIGTK is in >> > use? >> >> May we add support for BIGTK in future with another patch? > > We already have BIGTK support in mac80211, so if we don't do that now > we're almost certainly not going to do it, so I'd really prefer if you > did it here, or if a separate patch still did it now. > >> The info(pwr_reduction and tx_pwr_env) is used by lower driver such as >> ath11k. > > Sure. > >> If the info changed after association, then how to notify lower >> driver? >> Do it like below in ieee80211_rx_mgmt_beacon()? >> And use BSS_CHANGED_TXPOWER or a new enum in ieee80211_bss_change? > > Yeah, dunno. Are the drivers assuming now it's set once you get to > associated state? yes, driver need this info while associate process. > > johannes
On 2021-08-13 15:25, Johannes Berg wrote: > On Fri, 2021-08-13 at 09:19 +0200, Johannes Berg wrote: >> >> > > 2) Should we at least check it again from the protected beacon or such >> > > after association, so we don't blindly trust the probe response or >> > > beacon (received during scan, not validated) at least when BIGTK is in >> > > use? >> > >> > May we add support for BIGTK in future with another patch? >> >> We already have BIGTK support in mac80211, so if we don't do that now >> we're almost certainly not going to do it, so I'd really prefer if you >> did it here, or if a separate patch still did it now. > > Actually, I should say though - the question was more whether we even > need/want that, rather than whether we can do it later or not. > > If we should protect this data/information then IMHO we should do it > now, but it's not clear to me that we should, given that we also don't > have encrypted association response and we still take information from > there too, etc. > > johannes I prefer to add a new enum(not use BSS_CHANGED_TXPOWER),e.g, BSS_CHANGED_PWR_ENV. And add check in ieee80211_rx_mgmt_beacon() as well as ieee80211_handle_pwr_constr(), when the value of pwr_reduction or content of elems.tx_pwr_env changed, save the pwr_reduction and elems.tx_pwr_env to ieee80211_bss_conf, and notify lower driver with BSS_CHANGED_PWR_ENV, then lower driver will do next action.
On Fri, 2021-08-13 at 16:47 +0800, Wen Gong wrote: > > > > > 2) Should we at least check it again from the protected beacon or such > > > > > after association, so we don't blindly trust the probe response or > > > > > beacon (received during scan, not validated) at least when BIGTK is in > > > > > use? > > > > > > > > May we add support for BIGTK in future with another patch? > > > > > > We already have BIGTK support in mac80211, so if we don't do that now > > > we're almost certainly not going to do it, so I'd really prefer if you > > > did it here, or if a separate patch still did it now. > > > > Actually, I should say though - the question was more whether we even > > need/want that, rather than whether we can do it later or not. > > > > If we should protect this data/information then IMHO we should do it > > now, but it's not clear to me that we should, given that we also don't > > have encrypted association response and we still take information from > > there too, etc. > > > > johannes > I prefer to add a new enum(not use BSS_CHANGED_TXPOWER),e.g, > BSS_CHANGED_PWR_ENV. > And add check in ieee80211_rx_mgmt_beacon() as well as > ieee80211_handle_pwr_constr(), > when the value of pwr_reduction or content of elems.tx_pwr_env changed, > save the pwr_reduction and elems.tx_pwr_env to ieee80211_bss_conf, and > notify lower > driver with BSS_CHANGED_PWR_ENV, then lower driver will do next action. > I don't really have any objection to this, but OTOH it feels like drivers will probably not really listen to this if it can only happen due to BIGTK? And if we always defer this until the first beacon, that also feels wrong and bad? I'm not sure what the right answer here is, TBH. Maybe the right answer is to indeed ignore beacon protection for this, and do exactly what you did here, and say that the TX power envelope thing is just not meant to be protected, because the protection is meant to protect the connection etc. and not the performance (and regulatory?) Do we get this *only* in the beacon, or also in the association response? If it's also in the association response we could use the data from *there*, and basically say that the association response might need some protection (later) anyway? johannes
On 2021-08-13 16:53, Johannes Berg wrote: > On Fri, 2021-08-13 at 16:47 +0800, Wen Gong wrote: >> > > > > 2) Should we at least check it again from the protected beacon or such >> > > > > after association, so we don't blindly trust the probe response or >> > > > > beacon (received during scan, not validated) at least when BIGTK is in >> > > > > use? >> > > > >> > > > May we add support for BIGTK in future with another patch? >> > > >> > > We already have BIGTK support in mac80211, so if we don't do that now >> > > we're almost certainly not going to do it, so I'd really prefer if you >> > > did it here, or if a separate patch still did it now. >> > >> > Actually, I should say though - the question was more whether we even >> > need/want that, rather than whether we can do it later or not. >> > >> > If we should protect this data/information then IMHO we should do it >> > now, but it's not clear to me that we should, given that we also don't >> > have encrypted association response and we still take information from >> > there too, etc. >> > >> > johannes >> I prefer to add a new enum(not use BSS_CHANGED_TXPOWER),e.g, >> BSS_CHANGED_PWR_ENV. >> And add check in ieee80211_rx_mgmt_beacon() as well as >> ieee80211_handle_pwr_constr(), >> when the value of pwr_reduction or content of elems.tx_pwr_env >> changed, >> save the pwr_reduction and elems.tx_pwr_env to ieee80211_bss_conf, and >> notify lower >> driver with BSS_CHANGED_PWR_ENV, then lower driver will do next >> action. >> > I don't really have any objection to this, but OTOH it feels like > drivers will probably not really listen to this if it can only happen > due to BIGTK? yes, it should have some flag/logic to check whether it is BIGTK. If you know it, you can tell me. :) > > And if we always defer this until the first beacon, that also feels > wrong and bad? It can not defer this untill the 1st beacon which pass BIGTK verify. Lower driver need this info to set power before TX data include EAPOL. > > I'm not sure what the right answer here is, TBH. > > Maybe the right answer is to indeed ignore beacon protection for this, > and do exactly what you did here, and say that the TX power envelope > thing is just not meant to be protected, because the protection is > meant > to protect the connection etc. and not the performance (and > regulatory?) Yes, the lower driver also have the max power limit itself. If power calulated from the fake beacon is bigger than the max power limit, then it will be ignored. > > Do we get this *only* in the beacon, or also in the association > response? If it's also in the association response we could use the > data > from *there*, and basically say that the association response might > need > some protection (later) anyway? > The Transmit Power Envelope is not existed in the assoc response, it is existed in beacon. So it can not use assoc response. beacon: IEEE 802.11 wireless LAN Fixed parameters (12 bytes) Timestamp: 0x0000005070684036 Beacon Interval: 0.102400 [Seconds] Capabilities Information: 0x0511 Tagged parameters (264 bytes) Tag: SSID parameter set: Renhui-6G Tag: Supported Rates and BSS Membership Selectors 6.0(B), 9, 12.0(B), 18, 24(B), 36, 48, 54, [Mbit/sec] Tag: Traffic Indication Map (TIM): DTIM 0 of Tag: Country Information: Country Code US, Environment Unknown (0x04) Tag: Power Constraint: 3 Tag: TPC Report Transmit Power: 17, Link Margin: 0 Tag: Extended Supported Rates and BSS Membership Selectors BSS requires support for direct hashing to elements in SAE, [Mbit/sec] Tag: RSN Information Tag: Extended Capabilities (11 octets) Tag: Transmit Power Envelope Tag: Transmit Power Envelope Ext Tag: Reserved (55) Ext Tag: HE Capabilities (IEEE Std 802.11ax/D2.0) Ext Tag: HE Operation (IEEE Std 802.11ax/D2.0) Ext Tag: Spatial Reuse Parameter Set Ext Tag: MU EDCA Parameter Set Ext Tag: 6GHz Band Capabilities assoc response: IEEE 802.11 wireless LAN Fixed parameters (6 bytes) Capabilities Information: 0x0511 Status code: Successful (0x0000) ..00 0000 0001 0001 = Association ID: 0x0011 Tagged parameters (169 bytes) Tag: Supported Rates and BSS Membership Selectors 6.0(B), 9, 12.0(B), 18, 24(B), 36, 48, 54, [Mbit/sec] Tag: Extended Supported Rates and BSS Membership Selectors BSS requires support for direct hashing to elements in SAE, [Mbit/sec] Tag: Extended Capabilities (11 octets) Ext Tag: HE Capabilities (IEEE Std 802.11ax/D2.0) Ext Tag: HE Operation (IEEE Std 802.11ax/D2.0) Ext Tag: Spatial Reuse Parameter Set Ext Tag: MU EDCA Parameter Set Ext Tag: 6GHz Band Capabilities > johannes
On Fri, 2021-08-13 at 17:16 +0800, Wen Gong wrote: > > yes, it should have some flag/logic to check whether it is BIGTK. > If you know it, you can tell me. :) Uh, actually, we don't have a secure indication of BIGTK getting used until after the 4-way-HS. > > > Yes, the lower driver also have the max power limit itself. If power > calulated > from the fake beacon is bigger than the max power limit, then it will be > ignored. Right. > > > The Transmit Power Envelope is not existed in the assoc response, it is > existed > in beacon. So it can not use assoc response. Right. Given this discussion, I think we should just leave it as is, and simply not assume that the TPE is protected by beacon protection or such. There are a number of other similar parameters, and doing some real protection at this level would likely require further spec changes. johannes
On 2021-08-13 18:11, Johannes Berg wrote: > On Fri, 2021-08-13 at 17:16 +0800, Wen Gong wrote: >> >> yes, it should have some flag/logic to check whether it is BIGTK. >> If you know it, you can tell me. :) > > Uh, actually, we don't have a secure indication of BIGTK getting used > until after the 4-way-HS. > >> > >> Yes, the lower driver also have the max power limit itself. If power >> calulated >> from the fake beacon is bigger than the max power limit, then it will >> be >> ignored. > > Right. > >> > >> The Transmit Power Envelope is not existed in the assoc response, it >> is >> existed >> in beacon. So it can not use assoc response. > > Right. > > > Given this discussion, I think we should just leave it as is, and > simply > not assume that the TPE is protected by beacon protection or such. > There > are a number of other similar parameters, and doing some real > protection > at this level would likely require further spec changes. > Thanks. I will leave it as is without change for BIGTK. I will change others patch and send new version. > johannes
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c index 2e33a1263518..5b02d78bd934 100644 --- a/net/mac80211/mlme.c +++ b/net/mac80211/mlme.c @@ -5076,6 +5076,27 @@ static int ieee80211_prep_channel(struct ieee80211_sub_if_data *sdata, else he_oper = NULL; + if (is_6ghz) { + struct ieee802_11_elems elems; + struct ieee80211_bss_conf *bss_conf; + u8 i, n; + + ieee802_11_parse_elems(ies->data, ies->len, false, &elems, + NULL, NULL); + bss_conf = &sdata->vif.bss_conf; + bss_conf->pwr_reduction = 0; + if (elems.pwr_constr_elem) + bss_conf->pwr_reduction = *elems.pwr_constr_elem; + + memset(bss_conf->tx_pwr_env, 0, sizeof(bss_conf->tx_pwr_env)); + bss_conf->tx_pwr_env_num = elems.tx_pwr_env_num; + n = min_t(u8, elems.tx_pwr_env_num, + ARRAY_SIZE(elems.tx_pwr_env)); + for (i = 0; i < n; i++) + memcpy(&bss_conf->tx_pwr_env[i], elems.tx_pwr_env[i], + elems.tx_pwr_env_len[i]); + } + if (!ieee80211_verify_sta_he_mcs_support(sband, he_oper)) ifmgd->flags |= IEEE80211_STA_DISABLE_HE; }
This patch is to save the transmit power envelope element and power constraint in struct ieee80211_bss_conf for 6GHz. Signed-off-by: Wen Gong <wgong@codeaurora.org> --- net/mac80211/mlme.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)