| Message ID | 20181111110235.14213-3-alexander@wetzel-home.de (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | Extended Key ID support for linux | expand |
> + * @IEEE80211_KEY_FLAG_RX_ONLY: Set by mac80211 to indicate that the key > + * must not be used for TX (yet). I'm not sure that's relevant, since you have one key pointer for TX? > + * @IEEE80211_KEY_FLAG_SET_TX: Set by mac80211 to indicate that a previously > + * installed key with IEEE80211_KEY_FLAG_RX_ONLY should take over TX also. That also doesn't seem relevant ... Oh, all of this is for HW offloads? I _think_ I would prefer to have new key ops instead. Now you'd have SET_KEY / <empty flags> SET_KEY / RX_ONLY SET_KEY / SET_TX but I think maybe SET_KEY SET_KEY_RX_ONLY KEY_ENABLE_TX would make more sense? > + if (pairwise && params->flag == NL80211_KEY_SET_TX) { > + mutex_lock(&local->sta_mtx); > + sta = sta_info_get_bss(sdata, mac_addr); > + > + if (!sta || > + !(key = rcu_dereference(sta->ptk[key_idx])) || indentation here is off by one > + !(key->conf.flags | IEEE80211_KEY_FLAG_RX_ONLY)) { that makes no sense, should be & I guess > - /* PTK only using key ID 0 needs special handling on rekey */ > - if (new_key && sta && ptk0rekey) { > + /* PTK rekey without Extended Key ID needs special handling */ > + if (new_key && pairwise && sta && > + !test_sta_flag(sta, WLAN_STA_EXT_KEY_ID)) { > local = old_key->local; > sdata = old_key->sdata; This seems wrong, even if you have ext key ID support and everything, but you do 0 -> 0 rekeying, then you still need all the special handling (in fact also then if you go 1->1!). So it seems you'd instead want to see if you're going from a TX key to a TX key with the same key ID, and then you don't need this flag at all. > +++ b/net/mac80211/sta_info.c > @@ -350,6 +350,7 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata, > sta->sta.max_rx_aggregation_subframes = > local->hw.max_rx_aggregation_subframes; > > + sta->ptk_idx = NUM_DEFAULT_KEYS - 1; That makes no sense? Why should it be 3? That's invalid anyway? johannes
>> + * @IEEE80211_KEY_FLAG_RX_ONLY: Set by mac80211 to indicate that the key >> + * must not be used for TX (yet). > > I'm not sure that's relevant, since you have one key pointer for TX? > >> + * @IEEE80211_KEY_FLAG_SET_TX: Set by mac80211 to indicate that a previously >> + * installed key with IEEE80211_KEY_FLAG_RX_ONLY should take over TX also. > > That also doesn't seem relevant ... > > Oh, all of this is for HW offloads? > > I _think_ I would prefer to have new key ops instead. Now you'd have > > SET_KEY / <empty flags> > SET_KEY / RX_ONLY > SET_KEY / SET_TX > > but I think maybe > > SET_KEY > SET_KEY_RX_ONLY > KEY_ENABLE_TX > > would make more sense? Fine for me and should make it more understandable. So I'll try that. > >> + if (pairwise && params->flag == NL80211_KEY_SET_TX) { >> + mutex_lock(&local->sta_mtx); >> + sta = sta_info_get_bss(sdata, mac_addr); >> + >> + if (!sta || >> + !(key = rcu_dereference(sta->ptk[key_idx])) || > > indentation here is off by one > Thanks. >> + !(key->conf.flags | IEEE80211_KEY_FLAG_RX_ONLY)) { > > that makes no sense, should be & I guess > yes, I think that was one of the bugs I fixed the last weeks:-) >> - /* PTK only using key ID 0 needs special handling on rekey */ >> - if (new_key && sta && ptk0rekey) { >> + /* PTK rekey without Extended Key ID needs special handling */ >> + if (new_key && pairwise && sta && >> + !test_sta_flag(sta, WLAN_STA_EXT_KEY_ID)) { >> local = old_key->local; >> sdata = old_key->sdata; > > This seems wrong, even if you have ext key ID support and everything, > but you do 0 -> 0 rekeying, then you still need all the special handling > (in fact also then if you go 1->1!). So it seems you'd instead want to > see if you're going from a TX key to a TX key with the same key ID, and > then you don't need this flag at all. > The intention for Extended Key ID is, to have a comparable short time frame where both key IDs can be used. When replacing e.g. key ID 0 again it should be idle for a long time. I guess if someone starts re-keying in 1s intervals it may become an issue, but then anyone re-keying that often can't be helped... With Extended Key IDs it's impossible to directly switch from a TX key with one key ID to another one with the same id. 1) Association 2) key ID 0 installed RX only 3) key Id 0 set_tx 4) rekey timeout passes 5) key ID 1 installed RX only 6) key ID 1 set_tx (also making key ID 0 RX only) 7) rekey timeout passes 8) key ID 0 replaced with new RX only key 9) key ID 0 set_tx 10) rekey timeout passes ... So nobody will use the key being replaced, we don't have to protect against PN poisoning. And when a driver supports Extended Key ID we don't care about if the driver is able to rekey PTK0 correctly. >> +++ b/net/mac80211/sta_info.c >> @@ -350,6 +350,7 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata, >> sta->sta.max_rx_aggregation_subframes = >> local->hw.max_rx_aggregation_subframes; >> >> + sta->ptk_idx = NUM_DEFAULT_KEYS - 1; > > That makes no sense? Why should it be 3? That's invalid anyway? Yes, that's the whole reason for that change:-) Setting it to 2 would also be fine, as long as it's not 0 or 1. ieee80211_tx_h_select_key starts encrypting packets as soon as sta->ptk[tx->sta->ptk_idx] is not null. So installing the first key as RX only will also activate the key for TX. the AP will therefore encrypt EAPOL #3 of the initial connect... To avoid expensive run time checks I simply switched the default setting to make sure sta->ptk[tx->sta->ptk_idx] will be NULL at the initial key install. Alexander
> > > - /* PTK only using key ID 0 needs special handling on rekey */ > > > - if (new_key && sta && ptk0rekey) { > > > + /* PTK rekey without Extended Key ID needs special handling */ > > > + if (new_key && pairwise && sta && > > > + !test_sta_flag(sta, WLAN_STA_EXT_KEY_ID)) { > > > local = old_key->local; > > > sdata = old_key->sdata; > > > > This seems wrong, even if you have ext key ID support and everything, > > but you do 0 -> 0 rekeying, then you still need all the special handling > > (in fact also then if you go 1->1!). So it seems you'd instead want to > > see if you're going from a TX key to a TX key with the same key ID, and > > then you don't need this flag at all. > > > > The intention for Extended Key ID is, to have a comparable short time > frame where both key IDs can be used. When replacing e.g. key ID 0 again > it should be idle for a long time. I guess if someone starts re-keying > in 1s intervals it may become an issue, but then anyone re-keying that > often can't be helped... Sure. But ... not sure how that's related? > With Extended Key IDs it's impossible to directly switch from a TX key > with one key ID to another one with the same id. > > 1) Association > 2) key ID 0 installed RX only > 3) key Id 0 set_tx > 4) rekey timeout passes > 5) key ID 1 installed RX only > 6) key ID 1 set_tx (also making key ID 0 RX only) > 7) rekey timeout passes > 8) key ID 0 replaced with new RX only key > 9) key ID 0 set_tx > 10) rekey timeout passes > ... > > So nobody will use the key being replaced, we don't have to protect > against PN poisoning. Exactly. > And when a driver supports Extended Key ID we > don't care about if the driver is able to rekey PTK0 correctly. Strictly speaking, that's false, since you don't know if wpa_s actually used it, and the peer STA allowed it. It's also not what you implemented, you implemented checking if NL80211_KEY_RX_ONLY was ever used. However, what I'm trying to say is that I'm not sure this makes sense? It seems to me it would be safer, and easier (no station flag), to just check if ("we're replacing the current TX key") and trigger the workarounds in that case. No? Yes, parts of the issue also manifest themselves on the RX side, but if you're not replacing the current key then you were using extended key ID support? > > > +++ b/net/mac80211/sta_info.c > > > @@ -350,6 +350,7 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata, > > > sta->sta.max_rx_aggregation_subframes = > > > local->hw.max_rx_aggregation_subframes; > > > > > > + sta->ptk_idx = NUM_DEFAULT_KEYS - 1; > > > > That makes no sense? Why should it be 3? That's invalid anyway? > > Yes, that's the whole reason for that change:-) Setting it to 2 would > also be fine, as long as it's not 0 or 1. Hmm, ok. So that probably wants a big comment saying that it relies on key idx 2/3 being invalid. I'm not sure I like the NUM_DEFAULT_KEYS-1, better perhaps to do something like /* comment saying why */ BUILD_BUG_ON(ARRAY_SIZE(sta->ptks) > 2); sta->ptk_idx = 2; or so? > ieee80211_tx_h_select_key starts encrypting packets as soon as > sta->ptk[tx->sta->ptk_idx] is not null. Right, so I guess this makes sense. johannes
> >>>> - /* PTK only using key ID 0 needs special handling on rekey */ >>>> - if (new_key && sta && ptk0rekey) { >>>> + /* PTK rekey without Extended Key ID needs special handling */ >>>> + if (new_key && pairwise && sta && >>>> + !test_sta_flag(sta, WLAN_STA_EXT_KEY_ID)) { >>>> local = old_key->local; >>>> sdata = old_key->sdata; >>> >>> This seems wrong, even if you have ext key ID support and everything, >>> but you do 0 -> 0 rekeying, then you still need all the special handling >>> (in fact also then if you go 1->1!). So it seems you'd instead want to >>> see if you're going from a TX key to a TX key with the same key ID, and >>> then you don't need this flag at all. >>> >> >> The intention for Extended Key ID is, to have a comparable short time >> frame where both key IDs can be used. When replacing e.g. key ID 0 again >> it should be idle for a long time. I guess if someone starts re-keying >> in 1s intervals it may become an issue, but then anyone re-keying that >> often can't be helped... > > Sure. But ... not sure how that's related? > >> With Extended Key IDs it's impossible to directly switch from a TX key >> with one key ID to another one with the same id. >> >> 1) Association >> 2) key ID 0 installed RX only >> 3) key Id 0 set_tx >> 4) rekey timeout passes >> 5) key ID 1 installed RX only >> 6) key ID 1 set_tx (also making key ID 0 RX only) >> 7) rekey timeout passes >> 8) key ID 0 replaced with new RX only key >> 9) key ID 0 set_tx >> 10) rekey timeout passes >> ... >> >> So nobody will use the key being replaced, we don't have to protect >> against PN poisoning. > > Exactly. > >> And when a driver supports Extended Key ID we >> don't care about if the driver is able to rekey PTK0 correctly. > > Strictly speaking, that's false, since you don't know if wpa_s actually > used it, and the peer STA allowed it. > > It's also not what you implemented, you implemented checking if > NL80211_KEY_RX_ONLY was ever used. > > However, what I'm trying to say is that I'm not sure this makes sense? > > It seems to me it would be safer, and easier (no station flag), to just > check > > if ("we're replacing the current TX key") > > and trigger the workarounds in that case. No? > > Yes, parts of the issue also manifest themselves on the RX side, but if > you're not replacing the current key then you were using extended key ID > support? > Ah, now I get it:-) Will try that out also. >>>> +++ b/net/mac80211/sta_info.c >>>> @@ -350,6 +350,7 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata, >>>> sta->sta.max_rx_aggregation_subframes = >>>> local->hw.max_rx_aggregation_subframes; >>>> >>>> + sta->ptk_idx = NUM_DEFAULT_KEYS - 1; >>> >>> That makes no sense? Why should it be 3? That's invalid anyway? >> >> Yes, that's the whole reason for that change:-) Setting it to 2 would >> also be fine, as long as it's not 0 or 1. > > Hmm, ok. So that probably wants a big comment saying that it relies on > key idx 2/3 being invalid. I'm not sure I like the NUM_DEFAULT_KEYS-1, > better perhaps to do something like > > /* comment saying why */ > BUILD_BUG_ON(ARRAY_SIZE(sta->ptks) > 2); > sta->ptk_idx = 2; > > or so? > >> ieee80211_tx_h_select_key starts encrypting packets as soon as >> sta->ptk[tx->sta->ptk_idx] is not null. > > Right, so I guess this makes sense. > > johannes >Thank you very much for all the helpful tips and suggestions! Alexander
diff --git a/include/net/mac80211.h b/include/net/mac80211.h index 71985e95d2d9..fb53e7c84c01 100644 --- a/include/net/mac80211.h +++ b/include/net/mac80211.h @@ -1643,6 +1643,10 @@ struct wireless_dev *ieee80211_vif_to_wdev(struct ieee80211_vif *vif); * @IEEE80211_KEY_FLAG_PUT_MIC_SPACE: This flag should be set by the driver for * a TKIP key if it only requires MIC space. Do not set together with * @IEEE80211_KEY_FLAG_GENERATE_MMIC on the same key. + * @IEEE80211_KEY_FLAG_RX_ONLY: Set by mac80211 to indicate that the key + * must not be used for TX (yet). + * @IEEE80211_KEY_FLAG_SET_TX: Set by mac80211 to indicate that a previously + * installed key with IEEE80211_KEY_FLAG_RX_ONLY should take over TX also. */ enum ieee80211_key_flags { IEEE80211_KEY_FLAG_GENERATE_IV_MGMT = BIT(0), @@ -1654,6 +1658,8 @@ enum ieee80211_key_flags { IEEE80211_KEY_FLAG_RX_MGMT = BIT(6), IEEE80211_KEY_FLAG_RESERVE_TAILROOM = BIT(7), IEEE80211_KEY_FLAG_PUT_MIC_SPACE = BIT(8), + IEEE80211_KEY_FLAG_RX_ONLY = BIT(9), + IEEE80211_KEY_FLAG_SET_TX = BIT(10), }; /** diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index 51622333d460..c0af820bc557 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -365,6 +365,25 @@ static int ieee80211_add_key(struct wiphy *wiphy, struct net_device *dev, if (!ieee80211_sdata_running(sdata)) return -ENETDOWN; + if (pairwise && params->flag == NL80211_KEY_SET_TX) { + mutex_lock(&local->sta_mtx); + sta = sta_info_get_bss(sdata, mac_addr); + + if (!sta || + !(key = rcu_dereference(sta->ptk[key_idx])) || + !(key->conf.flags | IEEE80211_KEY_FLAG_RX_ONLY)) { + err = -ENOENT; + } else { + key->conf.flags &= ~IEEE80211_KEY_FLAG_RX_ONLY; + key->conf.flags |= IEEE80211_KEY_FLAG_SET_TX; + err = ieee80211_key_hw_activate_tx(key); + if (!err) + sta->ptk_idx = key_idx; + } + mutex_unlock(&local->sta_mtx); + return err; + } + /* reject WEP and TKIP keys if WEP failed to initialize */ switch (params->cipher) { case WLAN_CIPHER_SUITE_WEP40: @@ -451,9 +470,18 @@ static int ieee80211_add_key(struct wiphy *wiphy, struct net_device *dev, break; } - if (sta) + if (sta) { sta->cipher_scheme = cs; + /* Flag STA correctly when using Extended Key ID */ + if (pairwise && params->flag == NL80211_KEY_RX_ONLY && + !test_sta_flag(sta, WLAN_STA_EXT_KEY_ID)) + set_sta_flag(sta, WLAN_STA_EXT_KEY_ID); + } + + if (params->flag == NL80211_KEY_RX_ONLY) + key->conf.flags |= IEEE80211_KEY_FLAG_RX_ONLY; + err = ieee80211_key_link(key, sdata, sta); out_unlock: diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c index af5185a836e5..d63dca26d504 100644 --- a/net/mac80211/debugfs_sta.c +++ b/net/mac80211/debugfs_sta.c @@ -81,6 +81,7 @@ static const char * const sta_flag_names[] = { FLAG(MPSP_OWNER), FLAG(MPSP_RECIPIENT), FLAG(PS_DELIVER), + FLAG(EXT_KEY_ID), #undef FLAG }; diff --git a/net/mac80211/key.c b/net/mac80211/key.c index 4700718e010f..007f90a1f948 100644 --- a/net/mac80211/key.c +++ b/net/mac80211/key.c @@ -124,6 +124,36 @@ static void decrease_tailroom_need_count(struct ieee80211_sub_if_data *sdata, sdata->crypto_tx_tailroom_needed_cnt -= delta; } +int ieee80211_key_hw_activate_tx(struct ieee80211_key *key) +{ + struct ieee80211_sub_if_data *sdata = key->sdata; + struct sta_info *sta = key->sta; + int ret; + + assert_key_lock(key->local); + + if (!sta) + return -EOPNOTSUPP; + + if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE)) + /* No need to inform driver, report success */ + return 0; + + /* Inform driver that key is no longer RX only */ + ret = drv_set_key(key->local, SET_KEY, sdata, + &sta->sta, &key->conf); + if (ret) { + if (ret == 1) + /* Software crypto, report success */ + return 0; + sdata_err(sdata, + "failed to activate key for TX (%d, %pM) in hardware (%d)\n", + key->conf.keyidx, + sta->sta.addr, ret); + } + return ret; +} + static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key) { struct ieee80211_sub_if_data *sdata = key->sdata; @@ -261,7 +291,7 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key) static int ieee80211_hw_key_replace(struct ieee80211_key *old_key, struct ieee80211_key *new_key, - bool ptk0rekey) + bool pairwise) { struct ieee80211_sub_if_data *sdata; struct ieee80211_local *local; @@ -278,8 +308,9 @@ static int ieee80211_hw_key_replace(struct ieee80211_key *old_key, assert_key_lock(old_key->local); sta = old_key->sta; - /* PTK only using key ID 0 needs special handling on rekey */ - if (new_key && sta && ptk0rekey) { + /* PTK rekey without Extended Key ID needs special handling */ + if (new_key && pairwise && sta && + !test_sta_flag(sta, WLAN_STA_EXT_KEY_ID)) { local = old_key->local; sdata = old_key->sdata; @@ -395,10 +426,6 @@ static int ieee80211_key_replace(struct ieee80211_sub_if_data *sdata, if (old) { idx = old->conf.keyidx; - /* TODO: proper implement and test "Extended Key ID for - * Individually Addressed Frames" from IEEE 802.11-2016. - * Till then always assume only key ID 0 is used for - * pairwise keys.*/ ret = ieee80211_hw_key_replace(old, new, pairwise); } else { /* new must be provided in case old is not */ @@ -415,8 +442,9 @@ static int ieee80211_key_replace(struct ieee80211_sub_if_data *sdata, if (sta) { if (pairwise) { rcu_assign_pointer(sta->ptk[idx], new); - sta->ptk_idx = idx; - if (new) { + if (new && + !(new->conf.flags & IEEE80211_KEY_FLAG_RX_ONLY)) { + sta->ptk_idx = idx; clear_sta_flag(sta, WLAN_STA_BLOCK_BA); ieee80211_check_fast_xmit(sta); } diff --git a/net/mac80211/key.h b/net/mac80211/key.h index ebdb80b85dc3..e0612a0b3adc 100644 --- a/net/mac80211/key.h +++ b/net/mac80211/key.h @@ -146,6 +146,7 @@ ieee80211_key_alloc(u32 cipher, int idx, size_t key_len, int ieee80211_key_link(struct ieee80211_key *key, struct ieee80211_sub_if_data *sdata, struct sta_info *sta); +int ieee80211_key_hw_activate_tx(struct ieee80211_key *key); void ieee80211_key_free(struct ieee80211_key *key, bool delay_tailroom); void ieee80211_key_free_unused(struct ieee80211_key *key); void ieee80211_set_default_key(struct ieee80211_sub_if_data *sdata, int idx, diff --git a/net/mac80211/main.c b/net/mac80211/main.c index 83e71e6b2ebe..8a8ca813494a 100644 --- a/net/mac80211/main.c +++ b/net/mac80211/main.c @@ -574,6 +574,8 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len, wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_FILS_STA); wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211); + /* !! DEVELOPMENT ONLY, must normally be set by driver !! */ + wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_EXT_KEY_ID); if (!ops->hw_scan) { wiphy->features |= NL80211_FEATURE_LOW_PRIORITY_SCAN | diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c index fb8c2252ac0e..2d83d8e13769 100644 --- a/net/mac80211/sta_info.c +++ b/net/mac80211/sta_info.c @@ -350,6 +350,7 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata, sta->sta.max_rx_aggregation_subframes = local->hw.max_rx_aggregation_subframes; + sta->ptk_idx = NUM_DEFAULT_KEYS - 1; sta->local = local; sta->sdata = sdata; sta->rx_stats.last_rx = jiffies; diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h index 9a04327d71d1..7787e773a350 100644 --- a/net/mac80211/sta_info.h +++ b/net/mac80211/sta_info.h @@ -101,6 +101,7 @@ enum ieee80211_sta_info_flags { WLAN_STA_MPSP_OWNER, WLAN_STA_MPSP_RECIPIENT, WLAN_STA_PS_DELIVER, + WLAN_STA_EXT_KEY_ID, NUM_WLAN_STA_FLAGS, };
Allow drivers using mac80211 to support Extended Key IDs. Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de> --- include/net/mac80211.h | 6 +++++ net/mac80211/cfg.c | 30 ++++++++++++++++++++++++- net/mac80211/debugfs_sta.c | 1 + net/mac80211/key.c | 46 ++++++++++++++++++++++++++++++-------- net/mac80211/key.h | 1 + net/mac80211/main.c | 2 ++ net/mac80211/sta_info.c | 1 + net/mac80211/sta_info.h | 1 + 8 files changed, 78 insertions(+), 10 deletions(-)