This is my current development version for Extended Key ID support in
linux and mac80211.
I consider the all patches in this series against nl80211/mac80211 ready
for merge and if they still have defects not mentioned in the patch I
need your help to see them.
There are still some questions if we even want/need all those patches,
and so I've added some remarks to behind some commit message to start the
different discussions.
The driver patches are - with the exception of the hwsim patch -
definitely not ready for merge and mostly here to illustrate how the
different APIs can be used and to start some discussions how to handle HW
specific challenges. Of course if someone wants to play with Extended Key
ID they also should be useful... (I can provide updated mostly working
hostapd/wpa_supplicant patches if someone is interested. Don't try to
use the old ones I sent to hostapd mailing list in November.)
That said I'm now using most of the patches or their predecessor in my
private Wlan with devices both supporting and not supporting Extended
Key ID fine.
Compared to the last RFC patch only the nl80211 patch is still close to
what we discussed. It got the API cleanup/changes and the open sanity
checks and not much more.
The mac80211 patch from RFC v2 had serious defects. The most serious one
was probably to not select the key based on the keyid of the MPDU.
I think outlining all the changes will not be useful here, the initial
patch was too broken for anything but SW crypto. (Which also had
issues...)
It started out with more or less all the fixes we discussed but when
trying to get it really correct and feature complete it became three
different patches we better review from the scratch. They are now
touching much more code and make in some cases drastic changes.
Here a short overview of the patches in the series and why they are in
it:
1) mac80211: Optimize tailroom_needed update checks:
This would be a standalone patch, but some other patches depend on it
to apply cleanly.
2) nl80211/cfg80211: Extended Key ID support
Generic support for Extended Key ID.
3) mac80211: IEEE 802.11 Extended Key ID support
Mac80211 Extended Key ID support for drivers when the hardware is able to
handle Extended Key ID (aka two pairwise keys in HW).
4) mac80211: Compatibility Extended Key ID support
Mac80211 Extended Key ID support for most devices not able to handle
two unicast keys in HW.
5) mac80211: Mark A-MPDU keyid borders for drivers
This is one big question, see the patch for why we may want this or
not...
6) mac80211_hwsim: Ext Key ID support (NATIVE)
Just a one-liner to allow Extended Key ID to be used with hwsim.
--- No patch below this line is ready for merge ---
7) iwlwifi: Extended Key ID support (NATIVE)
Hopefully the seed to support Extended Key ID for all iwlwifi cards,
see the patch description for the (big) issue it has.
As it is it's mostly an example how Native Extended Key ID support
will look like working with only some cards.
8) iwlwifi: dvm - EXT_KEY_ID A-MPDU API update
Stops iwldvm drivers to complain when used together with the
experimental "mac80211: Mark A-MPDU keyid boarders for drivers"
patch.
The following patches in the series are only illustrating the COMPAT
Extended Key ID support:
9) ath: Basic Extended Key ID support
Experimental patch for generic Extended Key ID support for all ath
drivers.
10) ath5k: ath_key_config() API compatibility update
Allows to still compile ath5k drivers with the patch above.
Only provided to not break any drivers if someone wants to test
this.
11) ath9k: Extended Key ID support (COMPAT)
The example for Compatibility Key ID support, works together with
"ath: Basic Extended Key ID support".
12) ath9k: EXT_KEY_ID A-MPDU API update
A mostly untested example how drivers may benefit from "mac80211:
Mark A-MPDU keyid boarders for drivers".
Alexander Wetzel (12):
mac80211: Optimize tailroom_needed update checks
nl80211/cfg80211: Extended Key ID support
mac80211: IEEE 802.11 Extended Key ID support
mac80211: Compatibility Extended Key ID support
mac80211: Mark A-MPDU keyid boarders for drivers
mac80211_hwsim: Ext Key ID support (NATIVE)
iwlwifi: Extended Key ID support (NATIVE)
iwlwifi: dvm - EXT_KEY_ID A-MPDU API update
ath: Basic Extended Key ID support (COMPAT+NATIVE)
ath5k: ath_key_config() API compatibility update
ath9k: Extended Key ID support (COMPAT)
ath9k: EXT_KEY_ID A-MPDU API update
drivers/net/wireless/ath/ath.h | 7 +-
drivers/net/wireless/ath/ath5k/mac80211-ops.c | 2 +-
drivers/net/wireless/ath/ath9k/htc_drv_main.c | 2 +-
drivers/net/wireless/ath/ath9k/init.c | 1 +
drivers/net/wireless/ath/ath9k/main.c | 20 +-
drivers/net/wireless/ath/ath9k/xmit.c | 14 +-
drivers/net/wireless/ath/key.c | 35 ++-
.../net/wireless/intel/iwlwifi/dvm/mac80211.c | 5 +
drivers/net/wireless/intel/iwlwifi/dvm/tx.c | 2 +-
.../net/wireless/intel/iwlwifi/mvm/mac80211.c | 5 +
drivers/net/wireless/mac80211_hwsim.c | 1 +
include/net/cfg80211.h | 2 +
include/net/mac80211.h | 65 ++++-
include/uapi/linux/nl80211.h | 23 +-
net/mac80211/cfg.c | 38 +++
net/mac80211/debugfs.c | 2 +
net/mac80211/ieee80211_i.h | 2 +-
net/mac80211/key.c | 223 +++++++++++++++---
net/mac80211/key.h | 9 +
net/mac80211/main.c | 6 +
net/mac80211/rx.c | 81 ++++---
net/mac80211/sta_info.c | 13 +
net/mac80211/sta_info.h | 6 +-
net/mac80211/tx.c | 77 ++++--
net/wireless/nl80211.c | 32 ++-
net/wireless/rdev-ops.h | 3 +-
net/wireless/trace.h | 31 ++-
net/wireless/util.c | 20 +-
28 files changed, 601 insertions(+), 126 deletions(-)
--
2.20.1
Optimize/cleanup the delay tailroom checks and adds one missing tailroom
update.
Signed-off-by: Alexander Wetzel <[email protected]>
---
It may make sense to write a small macro for support here.
This is only a small tweak/fix of the existing code to fit to how it's
handled in the Extended Key ID patches.
net/mac80211/key.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index 37e372896230..b9f2bfc00263 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -140,6 +140,12 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
* so clear that flag now to avoid trying to remove
* it again later.
*/
+ if (key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE &&
+ !(key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
+ IEEE80211_KEY_FLAG_PUT_MIC_SPACE |
+ IEEE80211_KEY_FLAG_RESERVE_TAILROOM)))
+ increment_tailroom_need_count(sdata);
+
key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
return -EINVAL;
}
@@ -179,9 +185,9 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
if (!ret) {
key->flags |= KEY_FLAG_UPLOADED_TO_HARDWARE;
- if (!((key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
- IEEE80211_KEY_FLAG_PUT_MIC_SPACE)) ||
- (key->conf.flags & IEEE80211_KEY_FLAG_RESERVE_TAILROOM)))
+ if (!(key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
+ IEEE80211_KEY_FLAG_PUT_MIC_SPACE |
+ IEEE80211_KEY_FLAG_RESERVE_TAILROOM)))
decrease_tailroom_need_count(sdata, 1);
WARN_ON((key->conf.flags & IEEE80211_KEY_FLAG_PUT_IV_SPACE) &&
@@ -242,9 +248,9 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
sta = key->sta;
sdata = key->sdata;
- if (!((key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
- IEEE80211_KEY_FLAG_PUT_MIC_SPACE)) ||
- (key->conf.flags & IEEE80211_KEY_FLAG_RESERVE_TAILROOM)))
+ if (!(key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
+ IEEE80211_KEY_FLAG_PUT_MIC_SPACE |
+ IEEE80211_KEY_FLAG_RESERVE_TAILROOM)))
increment_tailroom_need_count(sdata);
key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
@@ -1187,9 +1193,9 @@ void ieee80211_remove_key(struct ieee80211_key_conf *keyconf)
if (key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
- if (!((key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
- IEEE80211_KEY_FLAG_PUT_MIC_SPACE)) ||
- (key->conf.flags & IEEE80211_KEY_FLAG_RESERVE_TAILROOM)))
+ if (!(key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
+ IEEE80211_KEY_FLAG_PUT_MIC_SPACE |
+ IEEE80211_KEY_FLAG_RESERVE_TAILROOM)))
increment_tailroom_need_count(key->sdata);
}
--
2.20.1
Extend the shared ath key cache code to support Extended Key ID.
The key cache code has to accept unicast keys to use key idx 1 and allow
drivers to enable/disable hardware Rx decryption for a key independent
from Tx.
Signed-off-by: Alexander Wetzel <[email protected]>
---
I know this is the wrong audience to discuss ath drivers. It's only
included here as an example and POC that the Compatibility Extended Key
ID means for drivers.
This has so far only got the minimal attention needed to get it working
for my AP used for tests. The idea is, to discuss that with the proper
audience once we know what mac80211 Extended Key ID support will look
like.
drivers/net/wireless/ath/ath.h | 7 ++++++-
drivers/net/wireless/ath/key.c | 35 +++++++++++++++++++++++++++++-----
2 files changed, 36 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/ath/ath.h b/drivers/net/wireless/ath/ath.h
index cc45ccfea5af..465629448fdf 100644
--- a/drivers/net/wireless/ath/ath.h
+++ b/drivers/net/wireless/ath/ath.h
@@ -202,8 +202,13 @@ void ath_key_delete(struct ath_common *common, struct ieee80211_key_conf *key);
int ath_key_config(struct ath_common *common,
struct ieee80211_vif *vif,
struct ieee80211_sta *sta,
- struct ieee80211_key_conf *key);
+ struct ieee80211_key_conf *key,
+ bool rx_accel);
bool ath_hw_keyreset(struct ath_common *common, u16 entry);
+bool ath_hw_rx_crypt(struct ath_common *common,
+ struct ieee80211_key_conf *key,
+ struct ieee80211_sta *sta,
+ bool rx_accel);
void ath_hw_cycle_counters_update(struct ath_common *common);
int32_t ath_hw_get_listen_time(struct ath_common *common);
diff --git a/drivers/net/wireless/ath/key.c b/drivers/net/wireless/ath/key.c
index 689fab9acf10..ced1c89102ad 100644
--- a/drivers/net/wireless/ath/key.c
+++ b/drivers/net/wireless/ath/key.c
@@ -126,6 +126,23 @@ static bool ath_hw_keysetmac(struct ath_common *common,
return true;
}
+bool ath_hw_rx_crypt(struct ath_common *common,
+ struct ieee80211_key_conf *key,
+ struct ieee80211_sta *sta,
+ bool rx_accel)
+{
+ const u8 *mac = NULL;
+
+ if (!sta || !test_bit(key->hw_key_idx, common->keymap))
+ return false;
+
+ if (rx_accel)
+ mac = sta->addr;
+
+ return ath_hw_keysetmac(common, key->hw_key_idx, mac);
+}
+EXPORT_SYMBOL(ath_hw_rx_crypt);
+
static bool ath_hw_set_keycache_entry(struct ath_common *common, u16 entry,
const struct ath_keyval *k,
const u8 *mac)
@@ -473,7 +490,8 @@ static int ath_reserve_key_cache_slot(struct ath_common *common,
int ath_key_config(struct ath_common *common,
struct ieee80211_vif *vif,
struct ieee80211_sta *sta,
- struct ieee80211_key_conf *key)
+ struct ieee80211_key_conf *key,
+ bool rx_accel)
{
struct ath_keyval hk;
const u8 *mac = NULL;
@@ -527,21 +545,28 @@ int ath_key_config(struct ath_common *common,
idx = key->keyidx;
break;
}
- } else if (key->keyidx) {
+ } else if (key->keyidx > 1) {
if (WARN_ON(!sta))
return -EOPNOTSUPP;
mac = sta->addr;
if (vif->type != NL80211_IFTYPE_AP) {
- /* Only keyidx 0 should be used with unicast key, but
- * allow this for client mode for now. */
+ /* Only keyidx 0 and when using Extended Key ID 1 should
+ * be used with a unicast key. But allow this for client
+ * mode for now.
+ */
idx = key->keyidx;
} else
return -EIO;
} else {
if (WARN_ON(!sta))
return -EOPNOTSUPP;
- mac = sta->addr;
+
+ /* Handle sta Tx only keys like GTK keys for now */
+ if (rx_accel)
+ mac = sta->addr;
+ else
+ mac = NULL;
idx = ath_reserve_key_cache_slot(common, key->cipher);
}
--
2.20.1
IEEE 802.11-2016 "9.7.3 A-MPDU contents" forbids aggregating MPDUs with
different keyids.
Extended Key ID support breaks the assumption that all unicast MPDUs for
one station can always be aggregated.
Inform the drivers about a keyid border by dropping the
@IEEE80211_TX_CTL_AMPDU flag for the last MPDU using the current keyid.
Signed-off-by: Alexander Wetzel <[email protected]>
---
This patch here is totally unnecessary when we decide that IEEE 802.11 -
2016 is not meaning what is referenced in the commit message above:-)
The exact wording in the standard is:
"All protected MPDUs within an A-MPDU have the same Key ID."
The intent of the wording was probably written without considering
Extended Key IDs. At least it makes no sense for me to forbid mixing
MPDUs using keyid 0 and 1 in one A-MPDU. Nevertheless it says what it
says and there may now be cards/drivers depending on that and e.g. only
check it for the first MPDU. The lost MPDUs would then be our fault, for
aggregating together what according to the standard must not.
But even with that view we still don't need the patch:
A-MPDU aggregation is the job for the driver. We simply can offload the
problem to the drivers.
On the other side mac80211 is in what I consider a better position to
determine and mark the MPDU keyid border, saving the driver the effort
to parse the MPDUs again and keep track of the "current" keyid. It also
allows the driver to terminate a running A-MPDU frame when it gets the
last packet for the old key instead one frame later.
To get around what looked like a nightmare of synchronisation problems
this patch puts the Tx key switch into the Tx path. Handling idle
connections when we don't want to accept that the first packet of a
rekey may still use the key-before-current makes it more complex.
The code is assuming that the driver is not aggregating MPDUs more than
5s apart. We probably don't have wait nearly so long but I'm not sure
what is the minimum time.
The patch also brought up a interesting problem: We are out of sta_info
flags and skb CB also has no room left to add new ones. I worked around
that by redefining how IEEE80211_TX_CTL_AMPDU has to be handled for
Extended Key ID A-MPDU sessions. If you think that puts too much stain
on the API I would need a pointer what would be considered an acceptable
solution to the problem.
But I also fully understand when you think this patch goes too far and
want it thrown out and want it handled somehow else:-).
net/mac80211/key.c | 10 +++++--
net/mac80211/key.h | 1 +
net/mac80211/sta_info.c | 1 +
net/mac80211/sta_info.h | 2 ++
net/mac80211/tx.c | 64 +++++++++++++++++++++++++++++++++++------
5 files changed, 68 insertions(+), 10 deletions(-)
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index 7f673887ec50..dee18f61fe33 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -309,6 +309,10 @@ int ieee80211_key_activate_tx(struct ieee80211_key *key)
assert_key_lock(local);
+ /* Two key activations must not overlap */
+ if (WARN_ON(sta->ptk_idx_next != INVALID_PTK_KEYIDX))
+ return -EOPNOTSUPP;
+
key->flags &= ~KEY_FLAG_RX_ONLY;
if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) ||
@@ -329,8 +333,9 @@ int ieee80211_key_activate_tx(struct ieee80211_key *key)
}
old = key_mtx_dereference(local, sta->ptk[sta->ptk_idx]);
- sta->ptk_idx = key->conf.keyidx;
- ieee80211_check_fast_xmit(sta);
+
+ sta->ptk_idx_next = key->conf.keyidx;
+ key->force_use_after = jiffies + 5 * HZ;
if (!ext_native && key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
key->flags |= KEY_FLAG_RX_SW_CRYPTO;
@@ -577,6 +582,7 @@ ieee80211_key_alloc(u32 cipher, int idx, size_t key_len,
*/
key->conf.flags = 0;
key->flags = 0;
+ key->force_use_after = 0;
key->conf.cipher = cipher;
key->conf.keyidx = idx;
diff --git a/net/mac80211/key.h b/net/mac80211/key.h
index d74c8c36491a..48975d56e792 100644
--- a/net/mac80211/key.h
+++ b/net/mac80211/key.h
@@ -65,6 +65,7 @@ struct ieee80211_key {
struct ieee80211_local *local;
struct ieee80211_sub_if_data *sdata;
struct sta_info *sta;
+ unsigned long force_use_after;
/* for sdata list */
struct list_head list;
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index a20e05439173..6fe40844f485 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -358,6 +358,7 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
*/
BUILD_BUG_ON(ARRAY_SIZE(sta->ptk) <= INVALID_PTK_KEYIDX);
sta->ptk_idx = INVALID_PTK_KEYIDX;
+ sta->ptk_idx_next = INVALID_PTK_KEYIDX;
sta->local = local;
sta->sdata = sdata;
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 1fd1a349a875..6eff946bc55a 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -450,6 +450,7 @@ struct ieee80211_sta_rx_stats {
* @sdata: virtual interface this station belongs to
* @ptk: peer keys negotiated with this station, if any
* @ptk_idx: peer key index to use for transmissions
+ * @ptk_idx_next: peer key index in activation (Extended Key ID only)
* @ext_key_compat_wk: supports PTK key installs when using EXT_KEY_ID_COMPAT
* @gtk: group keys negotiated with this station, if any
* @rate_ctrl: rate control algorithm reference
@@ -533,6 +534,7 @@ struct sta_info {
struct ieee80211_key __rcu *ptk[NUM_DEFAULT_KEYS];
struct delayed_work ext_key_compat_wk;
u8 ptk_idx;
+ u8 ptk_idx_next;
struct rate_control_ref *rate_ctrl;
void *rate_ctrl_priv;
spinlock_t rate_ctrl_lock;
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 111bd6c490a6..d3825eca9e64 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -586,6 +586,51 @@ ieee80211_tx_h_check_control_port_protocol(struct ieee80211_tx_data *tx)
return TX_CONTINUE;
}
+static struct ieee80211_key debug_noinline
+*ieee80211_select_sta_key(struct ieee80211_tx_data *tx)
+{
+ struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
+ struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
+ struct sta_info *sta = tx->sta;
+ struct ieee80211_key *key;
+ struct ieee80211_key *next_key;
+
+ key = rcu_dereference(tx->sta->ptk[tx->sta->ptk_idx]);
+
+ if (likely(sta->ptk_idx_next == INVALID_PTK_KEYIDX))
+ return key;
+
+ /* Only when using Extended Key ID the code below can be executed */
+
+ if (!ieee80211_is_data_present(hdr->frame_control))
+ return key;
+
+ if (sta->ptk_idx_next == sta->ptk_idx) {
+ /* First packet using new key with A-MPDU active*/
+ sta->ptk_idx_next = INVALID_PTK_KEYIDX;
+ ieee80211_check_fast_xmit(tx->sta);
+ return key;
+ }
+
+ next_key = rcu_dereference(sta->ptk[sta->ptk_idx_next]);
+ if (!key || !(info->flags & IEEE80211_TX_CTL_AMPDU) ||
+ (next_key->force_use_after &&
+ time_is_before_jiffies(next_key->force_use_after))) {
+ /* nothing special to do, just start using the new key */
+ sta->ptk_idx = sta->ptk_idx_next;
+ sta->ptk_idx_next = INVALID_PTK_KEYIDX;
+ next_key->force_use_after = 0;
+ ieee80211_check_fast_xmit(tx->sta);
+ return next_key;
+ }
+
+ /* Last packet with old key with A-MPDU active */
+ sta->ptk_idx = sta->ptk_idx_next;
+ next_key->force_use_after = 0;
+ info->flags &= ~IEEE80211_TX_CTL_AMPDU;
+ return key;
+}
+
static ieee80211_tx_result debug_noinline
ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
{
@@ -595,9 +640,8 @@ ieee80211_tx_h_select_key(struct ieee80211_tx_data *tx)
if (unlikely(info->flags & IEEE80211_TX_INTFL_DONT_ENCRYPT))
tx->key = NULL;
- else if (tx->sta &&
- (key = rcu_dereference(tx->sta->ptk[tx->sta->ptk_idx])))
- tx->key = key;
+ else if (tx->sta)
+ tx->key = ieee80211_select_sta_key(tx);
else if (ieee80211_is_group_privacy_action(tx->skb) &&
(key = rcu_dereference(tx->sdata->default_multicast_key)))
tx->key = key;
@@ -3414,6 +3458,10 @@ static bool ieee80211_xmit_fast(struct ieee80211_sub_if_data *sdata,
if (skb->sk && skb_shinfo(skb)->tx_flags & SKBTX_WIFI_STATUS)
return false;
+ /* ieee80211_key_activate_tx() requests to change key */
+ if (unlikely(sta->ptk_idx_next != INVALID_PTK_KEYIDX))
+ return false;
+
if (hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA)) {
tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
tid_tx = rcu_dereference(sta->ampdu_mlme.tid_tx[tid]);
@@ -3556,6 +3604,11 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
if (txq->sta)
tx.sta = container_of(txq->sta, struct sta_info, sta);
+ if (test_bit(IEEE80211_TXQ_AMPDU, &txqi->flags))
+ info->flags |= IEEE80211_TX_CTL_AMPDU;
+ else
+ info->flags &= ~IEEE80211_TX_CTL_AMPDU;
+
/*
* The key can be removed while the packet was queued, so need to call
* this here to get the current key.
@@ -3566,11 +3619,6 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
goto begin;
}
- if (test_bit(IEEE80211_TXQ_AMPDU, &txqi->flags))
- info->flags |= IEEE80211_TX_CTL_AMPDU;
- else
- info->flags &= ~IEEE80211_TX_CTL_AMPDU;
-
if (info->control.flags & IEEE80211_TX_CTRL_FAST_XMIT) {
struct sta_info *sta = container_of(txq->sta, struct sta_info,
sta);
--
2.20.1
Allow drivers to support Extended Key ID when they are not able to
handle two unicast keys per station for Rx by falling back to software
decryption when replacing keys.
Rx HW decryption activation is delayed till we either get the first MPDU
encrypted with the new key or for max 10s.
Signed-off-by: Alexander Wetzel <[email protected]>
---
Most drivers with the exception of ath10k should be able to support this
mode even without firmware updates from the vendors. (ath10k seems to be
a no-go without some firmware update, but that's not for discussion
here.)
Biggest downside are - besides the added complexity - potential severe
CPU peaks. (Imagine an rebooted AP with hundred of clients using
it with rekeying enabled and a weak CPU.)
Also noteworthy is, that using HW Rx decryption here at all is kind of
breaking the standard: As soon as one Rx key is offloaded to the
hardware we are/may no longer be able to decrypt packets with the "other"
valid keyid. (The hardware may have tried to decrypt it with the wrong
key and then hands over the decryption failure instead of the original
data.) So this will only work correctly as long as the remote sta is
switching to the new keyid at a time within 10s and not mixing old and
new keyids for a while. If that assumption breaks, mac80211 will not be
able to decrypt mangled packets.
That said it seems unlikely that this could be much more than some
retransmits and I get perfect looking rekeys in my test setup with max a
few dozen packets decrypted in software.
include/net/mac80211.h | 46 ++++++++++++++++++++++++++++
net/mac80211/debugfs.c | 1 +
net/mac80211/key.c | 67 +++++++++++++++++++++++++++++++++++++++--
net/mac80211/key.h | 4 +++
net/mac80211/main.c | 3 +-
net/mac80211/rx.c | 7 +++++
net/mac80211/sta_info.c | 3 ++
net/mac80211/sta_info.h | 2 ++
8 files changed, 129 insertions(+), 4 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index e16bc7623dc0..eafad5eb8953 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1814,12 +1814,16 @@ struct ieee80211_cipher_scheme {
* @EXT_SET_KEY: a new key must be set but is only valid for decryption
* @EXT_KEY_RX_TX: a key installed with @EXT_SET_KEY is becoming the
* designated Rx/Tx key for the station
+ * @EXT_DISABLE_KEY_RX: installed key must switch to to software decryption
+ * while not altering Tx encryption. Command only required when driver
+ * is using EXT_KEY_ID_COMPAT for Extended Key ID support.
*/
enum set_key_cmd {
SET_KEY,
DISABLE_KEY,
EXT_SET_KEY,
EXT_KEY_RX_TX,
+ EXT_DISABLE_KEY_RX,
};
/**
@@ -2231,6 +2235,10 @@ struct ieee80211_txq {
* @IEEE80211_HW_EXT_KEY_ID_NATIVE: Driver and hardware are supporting Extended
* Key ID and can handle two unicast keys per station for Rx and Tx.
*
+ * @IEEE80211_HW_EXT_KEY_ID_COMPAT: Driver and hardware support Extended Key ID
+ * when mac80211 handles Rx decryption during transition from one keyid to
+ * the next.
+ *
* @NUM_IEEE80211_HW_FLAGS: number of hardware flags, used for sizing arrays
*/
enum ieee80211_hw_flags {
@@ -2281,6 +2289,7 @@ enum ieee80211_hw_flags {
IEEE80211_HW_STA_MMPDU_TXQ,
IEEE80211_HW_TX_STATUS_NO_AMPDU_LEN,
IEEE80211_HW_EXT_KEY_ID_NATIVE,
+ IEEE80211_HW_EXT_KEY_ID_COMPAT,
/* keep last, obviously */
NUM_IEEE80211_HW_FLAGS
@@ -2641,6 +2650,43 @@ void ieee80211_free_txskb(struct ieee80211_hw *hw, struct sk_buff *skb);
Mac80211 will not queue any new frames for a deleted key to the driver.
*/
+/**
+ * DOC: Extended Key ID support
+ *
+ * Mac80211 can support "Extended Key ID" from IEEE 802.11-2016, allowing to
+ * rekey the in-use unicast key with ideally zero impact for ongoing
+ * transmissions.
+ *
+ * There are two options for a driver to support Extended Key ID:
+ * 1) Native:
+ * The "Native" Extended Key ID mode is using the key commands
+ * %EXT_SET_KEY, %EXT_KEY_RX_TX and %DISABLE_KEY.
+ * Drivers/cards fully compatible can set @IEEE80211_HW_EXT_KEY_ID_NATIVE,
+ * allowing mac80211 to install two unicast keys per station to the driver.
+ * Mac80211 will then inform the driver via %EXT_SET_KEY when a key must be
+ * added for Rx decryption and again with %EXT_KEY_RX_TX when the driver has
+ * to switch Tx to a new key. When the driver returns any other code than 0
+ * for those two commands the key install is aborted and reported as failed.
+ *
+ * 2) Compatibility
+ * This mode is for Drivers and cards which are not able to handle two
+ * unicast key for a station on Rx, but are fine with it for Tx and can
+ * pass trough the still encrypted MPDUs to mac80211.
+ * The "Compatibility" Extended Key ID mode is using the key commands
+ * %EXT_SET_KEY, %EXT_KEY_RX_TX, %EXT_DISABLE_KEY_RX and %DISABLE_KEY.
+ * A driver setting @IEEE80211_HW_EXT_KEY_ID_COMPAT must
+ * - implement %EXT_DISABLE_KEY_RX to switch a running key to Rx software
+ * decryption without changing Tx handling for the key.
+ * - Add a new key for Tx when called with %EXT_SET_KEY for the same station
+ * with another keyid (to have a key ready allowing Tx)
+ * - Optionally activate Rx decryption when called with %EXT_KEY_RX_TX
+ * Only the command %EXT_KEY_RX_TX is allowed to return a value not 0, any
+ * other command failing will abort the key install.
+ *
+ * Additionally any driver/card setting @IEEE80211_HW_EXT_KEY_ID_NATIVE or
+ * @IEEE80211_HW_EXT_KEY_ID_COMPAT must allow keyid 0 and 1 to for unicast keys.
+ */
+
/**
* DOC: Powersave support
*
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 334a9883894f..01849b093287 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -220,6 +220,7 @@ static const char *hw_flag_names[] = {
FLAG(STA_MMPDU_TXQ),
FLAG(TX_STATUS_NO_AMPDU_LEN),
FLAG(EXT_KEY_ID_NATIVE),
+ FLAG(EXT_KEY_ID_COMPAT),
#undef FLAG
};
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index d91503de1e1d..7f673887ec50 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -184,10 +184,32 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
}
}
- if (rx_only)
+ if (rx_only) {
+ /* EXT_KEY_ID_COMPAT drivers may scramble the payload when
+ * using the wrong HW key for decryption. Therefore only use SW
+ * decryption for the critical window.
+ */
+ if (sta && pairwise && !ext_native && !local->wowlan &&
+ sta->ptk_idx != key->conf.keyidx) {
+ struct ieee80211_key *old;
+
+ old = key_mtx_dereference(local,
+ sta->ptk[sta->ptk_idx]);
+ if (old) {
+ if (drv_set_key(local, EXT_DISABLE_KEY_RX,
+ sdata, &sta->sta, &old->conf))
+ return -EINVAL;
+ }
+ }
+
+ /* Install the key to hardware. EXT_KEY_ID_NATIVE drivers can
+ * use it for decryption but EXT_KEY_ID_COMPAT drivers must
+ * prepare it as a not yet used Tx only key.
+ */
cmd = EXT_SET_KEY;
- else
+ } else {
cmd = SET_KEY;
+ }
ret = drv_set_key(local, cmd, sdata,
sta ? &sta->sta : NULL, &key->conf);
@@ -282,6 +304,7 @@ int ieee80211_key_activate_tx(struct ieee80211_key *key)
struct sta_info *sta = key->sta;
struct ieee80211_local *local = key->local;
struct ieee80211_key *old;
+ bool ext_native = ieee80211_hw_check(&local->hw, EXT_KEY_ID_NATIVE);
int ret;
assert_key_lock(local);
@@ -294,7 +317,7 @@ int ieee80211_key_activate_tx(struct ieee80211_key *key)
IEEE80211_KEY_FLAG_RESERVE_TAILROOM))
increment_tailroom_need_count(sdata);
- if (key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
+ if (ext_native && key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
ret = drv_set_key(local, EXT_KEY_RX_TX, sdata,
&sta->sta, &key->conf);
if (ret) {
@@ -309,6 +332,13 @@ int ieee80211_key_activate_tx(struct ieee80211_key *key)
sta->ptk_idx = key->conf.keyidx;
ieee80211_check_fast_xmit(sta);
+ if (!ext_native && key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
+ key->flags |= KEY_FLAG_RX_SW_CRYPTO;
+ /* Activate Rx crypto offload after max 10s when idle */
+ ieee80211_queue_delayed_work(&local->hw, &sta->ext_key_compat_wk,
+ round_jiffies_relative(HZ * 10));
+ }
+
if (old) {
old->flags |= KEY_FLAG_RX_ONLY;
@@ -1109,6 +1139,37 @@ void ieee80211_free_sta_keys(struct ieee80211_local *local,
mutex_unlock(&local->key_mtx);
}
+/* EXT_KEY_ID_COMPAT support can't install PTK keys to the card/driver for
+ * hardware decryption as long as the remote sta may use both keyids. Those
+ * cards are not aware that the keyid must be checked and try to decrypt the
+ * payload with the wrong key, which would effectively scrambling it. This
+ * worker is therefore used to activate Rx hardware decryption when we assume
+ * there will be only packets for the new key.
+ */
+void ext_key_compat_rx_offload_work(struct work_struct *wk)
+{
+ struct sta_info *sta;
+ struct ieee80211_local *local;
+ struct ieee80211_sub_if_data *sdata;
+ struct ieee80211_key *key;
+
+ sta = container_of(wk, struct sta_info, ext_key_compat_wk.work);
+ local = sta->local;
+ sdata = sta->sdata;
+
+ mutex_lock(&local->key_mtx);
+ key = key_mtx_dereference(local, sta->ptk[sta->ptk_idx]);
+
+ if (key->flags & KEY_FLAG_RX_SW_CRYPTO)
+ key->flags &= ~KEY_FLAG_RX_SW_CRYPTO;
+
+ if (drv_set_key(local, EXT_KEY_RX_TX, sdata, &sta->sta, &key->conf)) {
+ sdata_info(sdata, "Could not switch Rx to HW crypto (%d, %pM)\n",
+ key->conf.keyidx, sta->sta.addr);
+ }
+ mutex_unlock(&local->key_mtx);
+}
+
void ieee80211_delayed_tailroom_dec(struct work_struct *wk)
{
struct ieee80211_sub_if_data *sdata;
diff --git a/net/mac80211/key.h b/net/mac80211/key.h
index 1a3da999e0c4..d74c8c36491a 100644
--- a/net/mac80211/key.h
+++ b/net/mac80211/key.h
@@ -32,12 +32,15 @@ struct sta_info;
* @KEY_FLAG_TAINTED: Key is tainted and packets should be dropped.
* @KEY_FLAG_CIPHER_SCHEME: This key is for a hardware cipher scheme
* @KEY_FLAG_RX_ONLY: Pairwise key only allowed to be used on Rx.
+ * @KEY_FLAG_RX_SW_CRYPTO: This key is using Rx SW decryption to work around HW
+ * limitations. Flag can only set when using EXT_KEY_COMPAT for max 10s.
*/
enum ieee80211_internal_key_flags {
KEY_FLAG_UPLOADED_TO_HARDWARE = BIT(0),
KEY_FLAG_TAINTED = BIT(1),
KEY_FLAG_CIPHER_SCHEME = BIT(2),
KEY_FLAG_RX_ONLY = BIT(3),
+ KEY_FLAG_RX_SW_CRYPTO = BIT(4),
};
enum ieee80211_internal_tkip_state {
@@ -167,5 +170,6 @@ void ieee80211_reset_crypto_tx_tailroom(struct ieee80211_sub_if_data *sdata);
rcu_dereference_protected(ref, lockdep_is_held(&((local)->key_mtx)))
void ieee80211_delayed_tailroom_dec(struct work_struct *wk);
+void ext_key_compat_rx_offload_work(struct work_struct *wk);
#endif /* IEEE80211_KEY_H */
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index ea34544985f3..dbabfa58c4c9 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -1052,7 +1052,8 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
}
/* mac80211 supports Extended Key ID when driver does */
- if (ieee80211_hw_check(&local->hw, EXT_KEY_ID_NATIVE))
+ if (ieee80211_hw_check(&local->hw, EXT_KEY_ID_COMPAT) ||
+ ieee80211_hw_check(&local->hw, EXT_KEY_ID_NATIVE))
wiphy_ext_feature_set(local->hw.wiphy,
NL80211_EXT_FEATURE_EXT_KEY_ID);
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index ce786311baf4..95c13f4c7e4a 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -2015,6 +2015,13 @@ ieee80211_rx_h_decrypt(struct ieee80211_rx_data *rx)
if (unlikely(rx->key->flags & KEY_FLAG_TAINTED))
return RX_DROP_MONITOR;
+ if (unlikely(rx->key->flags & KEY_FLAG_RX_SW_CRYPTO)) {
+ rx->key->flags &= ~KEY_FLAG_RX_SW_CRYPTO;
+ cancel_delayed_work(&rx->sta->ext_key_compat_wk);
+ ieee80211_queue_delayed_work(&rx->local->hw,
+ &rx->sta->ext_key_compat_wk, 0);
+ }
+
/* TODO: add threshold stuff again */
} else {
return RX_DROP_MONITOR;
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 09c69955c6e3..a20e05439173 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -132,6 +132,7 @@ static void __cleanup_single_sta(struct sta_info *sta)
if (ieee80211_vif_is_mesh(&sdata->vif))
mesh_sta_cleanup(sta);
+ cancel_delayed_work_sync(&sta->ext_key_compat_wk);
cancel_work_sync(&sta->drv_deliver_wk);
/*
@@ -326,6 +327,8 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
spin_lock_init(&sta->ps_lock);
INIT_WORK(&sta->drv_deliver_wk, sta_deliver_ps_frames);
INIT_WORK(&sta->ampdu_mlme.work, ieee80211_ba_session_work);
+ INIT_DELAYED_WORK(&sta->ext_key_compat_wk,
+ ext_key_compat_rx_offload_work);
mutex_init(&sta->ampdu_mlme.mtx);
#ifdef CONFIG_MAC80211_MESH
if (ieee80211_vif_is_mesh(&sdata->vif)) {
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 304a7ea24757..1fd1a349a875 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -450,6 +450,7 @@ struct ieee80211_sta_rx_stats {
* @sdata: virtual interface this station belongs to
* @ptk: peer keys negotiated with this station, if any
* @ptk_idx: peer key index to use for transmissions
+ * @ext_key_compat_wk: supports PTK key installs when using EXT_KEY_ID_COMPAT
* @gtk: group keys negotiated with this station, if any
* @rate_ctrl: rate control algorithm reference
* @rate_ctrl_lock: spinlock used to protect rate control data
@@ -530,6 +531,7 @@ struct sta_info {
struct ieee80211_sub_if_data *sdata;
struct ieee80211_key __rcu *gtk[NUM_DEFAULT_KEYS + NUM_DEFAULT_MGMT_KEYS];
struct ieee80211_key __rcu *ptk[NUM_DEFAULT_KEYS];
+ struct delayed_work ext_key_compat_wk;
u8 ptk_idx;
struct rate_control_ref *rate_ctrl;
void *rate_ctrl_priv;
--
2.20.1
Add support for IEEE 802.11-2016 "Extended Key ID for Individually
Addressed Frames".
Extends cfg80211 and nl80211 to allow pairwise keys to be installed Rx
only, switch Tx over separately and to use keyidx 1 for pairwise keys.
Signed-off-by: Alexander Wetzel <[email protected]>
---
include/net/cfg80211.h | 2 ++
include/uapi/linux/nl80211.h | 23 ++++++++++++++++++++++-
net/wireless/nl80211.c | 32 ++++++++++++++++++++++++++++----
net/wireless/rdev-ops.h | 3 ++-
net/wireless/trace.h | 31 ++++++++++++++++++++++++++-----
net/wireless/util.c | 20 ++++++++++++++------
6 files changed, 94 insertions(+), 17 deletions(-)
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 7f2739a90bdb..71ebb3492e21 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -485,6 +485,7 @@ struct vif_params {
* with the get_key() callback, must be in little endian,
* length given by @seq_len.
* @seq_len: length of @seq.
+ * @install_mode: key install mode (Rx/Tx, Rx only or set Tx)
*/
struct key_params {
const u8 *key;
@@ -492,6 +493,7 @@ struct key_params {
int key_len;
int seq_len;
u32 cipher;
+ enum nl80211_key_install_mode install_mode;
};
/**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index dd4f86ee286e..8ccede541913 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -4134,6 +4134,21 @@ enum nl80211_channel_type {
NL80211_CHAN_HT40PLUS
};
+/**
+ * enum nl80211_key_install_mode - Key install mode
+ *
+ * @NL80211_KEY_RX_TX: Key must be installed for Rx and Tx
+ * @NL80211_KEY_RX_ONLY: Allowed in combination with @NL80211_CMD_NEW_KEY:
+ * Unicast key has to be installed for Rx only.
+ * @NL80211_KEY_SWITCH_TX: Allowed in combination with @NL80211_CMD_SET_KEY:
+ * Switch Tx to a Rx only, referenced by sta mac and idx.
+ */
+enum nl80211_key_install_mode {
+ NL80211_KEY_RX_TX,
+ NL80211_KEY_RX_ONLY,
+ NL80211_KEY_SWITCH_TX
+};
+
/**
* enum nl80211_chan_width - channel width definitions
*
@@ -4377,6 +4392,9 @@ enum nl80211_key_default_types {
* @NL80211_KEY_DEFAULT_TYPES: A nested attribute containing flags
* attributes, specifying what a key should be set as default as.
* See &enum nl80211_key_default_types.
+ * @NL80211_KEY_INSTALL_MODE: the install mode from
+ * enum nl80211_key_install_mode. Defaults to @NL80211_KEY_RX_TX.
+ *
* @__NL80211_KEY_AFTER_LAST: internal
* @NL80211_KEY_MAX: highest key attribute
*/
@@ -4390,6 +4408,7 @@ enum nl80211_key_attributes {
NL80211_KEY_DEFAULT_MGMT,
NL80211_KEY_TYPE,
NL80211_KEY_DEFAULT_TYPES,
+ NL80211_KEY_INSTALL_MODE,
/* keep last */
__NL80211_KEY_AFTER_LAST,
@@ -5330,11 +5349,12 @@ enum nl80211_feature_flags {
* by the %NL80211_SCAN_FLAG_MIN_PREQ_CONTENT flag.
* @NL80211_EXT_FEATURE_ENABLE_FTM_RESPONDER: Driver supports enabling fine
* timing measurement responder role.
- *
* @NL80211_EXT_FEATURE_CAN_REPLACE_PTK0: Driver/device confirm that they are
* able to rekey an in-use key correctly. Userspace must not rekey PTK keys
* if this flag is not set. Ignoring this can leak clear text packets and/or
* freeze the connection.
+ * @NL80211_EXT_FEATURE_EXT_KEY_ID: Driver supports "Extended Key ID for
+ * Individually Addressed Frames" from IEEE802.11-2016.
*
* @NL80211_EXT_FEATURE_AIRTIME_FAIRNESS: Driver supports getting airtime
* fairness for transmitted packets and has enabled airtime fairness
@@ -5384,6 +5404,7 @@ enum nl80211_ext_feature_index {
NL80211_EXT_FEATURE_ENABLE_FTM_RESPONDER,
NL80211_EXT_FEATURE_AIRTIME_FAIRNESS,
NL80211_EXT_FEATURE_AP_PMKSA_CACHING,
+ NL80211_EXT_FEATURE_EXT_KEY_ID,
/* add new features before the definition below */
NUM_NL80211_EXT_FEATURES,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index a3cc039b9f55..2a076b99737e 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -565,6 +565,7 @@ static const struct nla_policy nl80211_key_policy[NL80211_KEY_MAX + 1] = {
[NL80211_KEY_DEFAULT_MGMT] = { .type = NLA_FLAG },
[NL80211_KEY_TYPE] = NLA_POLICY_MAX(NLA_U32, NUM_NL80211_KEYTYPES - 1),
[NL80211_KEY_DEFAULT_TYPES] = { .type = NLA_NESTED },
+ [NL80211_KEY_INSTALL_MODE] = { .type = NLA_U8 },
};
/* policy for the key default flags */
@@ -970,6 +971,9 @@ static int nl80211_parse_key_new(struct genl_info *info, struct nlattr *key,
k->def_multi = kdt[NL80211_KEY_DEFAULT_TYPE_MULTICAST];
}
+ if (tb[NL80211_KEY_INSTALL_MODE])
+ k->p.install_mode = nla_get_u8(tb[NL80211_KEY_INSTALL_MODE]);
+
return 0;
}
@@ -3646,8 +3650,11 @@ static int nl80211_set_key(struct sk_buff *skb, struct genl_info *info)
if (key.idx < 0)
return -EINVAL;
- /* only support setting default key */
- if (!key.def && !key.defmgmt)
+ /* Only support setting default key and
+ * Extended Key ID action @NL80211_KEY_SWITCH_TX.
+ */
+ if (!key.def && !key.defmgmt &&
+ !(key.p.install_mode == NL80211_KEY_SWITCH_TX))
return -EINVAL;
wdev_lock(dev->ieee80211_ptr);
@@ -3671,7 +3678,7 @@ static int nl80211_set_key(struct sk_buff *skb, struct genl_info *info)
#ifdef CONFIG_CFG80211_WEXT
dev->ieee80211_ptr->wext.default_key = key.idx;
#endif
- } else {
+ } else if (key.defmgmt) {
if (key.def_uni || !key.def_multi) {
err = -EINVAL;
goto out;
@@ -3693,8 +3700,25 @@ static int nl80211_set_key(struct sk_buff *skb, struct genl_info *info)
#ifdef CONFIG_CFG80211_WEXT
dev->ieee80211_ptr->wext.default_mgmt_key = key.idx;
#endif
- }
+ } else if (key.p.install_mode == NL80211_KEY_SWITCH_TX &&
+ wiphy_ext_feature_isset(&rdev->wiphy,
+ NL80211_EXT_FEATURE_EXT_KEY_ID)) {
+ u8 *mac_addr = NULL;
+ if (info->attrs[NL80211_ATTR_MAC])
+ mac_addr = nla_data(info->attrs[NL80211_ATTR_MAC]);
+
+ if (!mac_addr || key.idx < 0 || key.idx > 1) {
+ err = -EINVAL;
+ goto out;
+ }
+
+ err = rdev_add_key(rdev, dev, key.idx,
+ NL80211_KEYTYPE_PAIRWISE,
+ mac_addr, &key.p);
+ } else {
+ err = -EINVAL;
+ }
out:
wdev_unlock(dev->ieee80211_ptr);
diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index 5cb48d135fab..4bf4e3774825 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -77,7 +77,8 @@ static inline int rdev_add_key(struct cfg80211_registered_device *rdev,
struct key_params *params)
{
int ret;
- trace_rdev_add_key(&rdev->wiphy, netdev, key_index, pairwise, mac_addr);
+ trace_rdev_add_key(&rdev->wiphy, netdev, key_index, pairwise,
+ mac_addr, params->install_mode);
ret = rdev->ops->add_key(&rdev->wiphy, netdev, key_index, pairwise,
mac_addr, params);
trace_rdev_return_int(&rdev->wiphy, ret);
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index 44b2ce1bb13a..b5c9e6729ff1 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -430,22 +430,43 @@ DECLARE_EVENT_CLASS(key_handle,
BOOL_TO_STR(__entry->pairwise), MAC_PR_ARG(mac_addr))
);
-DEFINE_EVENT(key_handle, rdev_add_key,
+DEFINE_EVENT(key_handle, rdev_get_key,
TP_PROTO(struct wiphy *wiphy, struct net_device *netdev, u8 key_index,
bool pairwise, const u8 *mac_addr),
TP_ARGS(wiphy, netdev, key_index, pairwise, mac_addr)
);
-DEFINE_EVENT(key_handle, rdev_get_key,
+DEFINE_EVENT(key_handle, rdev_del_key,
TP_PROTO(struct wiphy *wiphy, struct net_device *netdev, u8 key_index,
bool pairwise, const u8 *mac_addr),
TP_ARGS(wiphy, netdev, key_index, pairwise, mac_addr)
);
-DEFINE_EVENT(key_handle, rdev_del_key,
+TRACE_EVENT(rdev_add_key,
TP_PROTO(struct wiphy *wiphy, struct net_device *netdev, u8 key_index,
- bool pairwise, const u8 *mac_addr),
- TP_ARGS(wiphy, netdev, key_index, pairwise, mac_addr)
+ bool pairwise, const u8 *mac_addr, u8 install_mode),
+ TP_ARGS(wiphy, netdev, key_index, pairwise, mac_addr, install_mode),
+ TP_STRUCT__entry(
+ WIPHY_ENTRY
+ NETDEV_ENTRY
+ MAC_ENTRY(mac_addr)
+ __field(u8, key_index)
+ __field(bool, pairwise)
+ __field(u8, install_mode)
+ ),
+ TP_fast_assign(
+ WIPHY_ASSIGN;
+ NETDEV_ASSIGN;
+ MAC_ASSIGN(mac_addr, mac_addr);
+ __entry->key_index = key_index;
+ __entry->pairwise = pairwise;
+ __entry->install_mode = install_mode;
+ ),
+ TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", key_index: %u, "
+ "install_mode: %u, pairwise: %s, mac addr: " MAC_PR_FMT,
+ WIPHY_PR_ARG, NETDEV_PR_ARG, __entry->key_index,
+ __entry->install_mode, BOOL_TO_STR(__entry->pairwise),
+ MAC_PR_ARG(mac_addr))
);
TRACE_EVENT(rdev_set_default_key,
diff --git a/net/wireless/util.c b/net/wireless/util.c
index cd48cdd582c0..a3338e611190 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -236,14 +236,22 @@ int cfg80211_validate_key_settings(struct cfg80211_registered_device *rdev,
case WLAN_CIPHER_SUITE_CCMP_256:
case WLAN_CIPHER_SUITE_GCMP:
case WLAN_CIPHER_SUITE_GCMP_256:
- /* Disallow pairwise keys with non-zero index unless it's WEP
- * or a vendor specific cipher (because current deployments use
- * pairwise WEP keys with non-zero indices and for vendor
- * specific ciphers this should be validated in the driver or
- * hardware level - but 802.11i clearly specifies to use zero)
+ /* IEEE802.11-2016 allows only 0 and - when using Extended Key
+ * ID - 1 as index for pairwise keys.
+ * @NL80211_KEY_RX_ONLY is only allowed for pairwise keys when
+ * the driver supports Extended Key ID.
+ * @NL80211_KEY_SWITCH_TX must not be set when validating a key.
*/
- if (pairwise && key_idx)
+ if (params->install_mode == NL80211_KEY_RX_ONLY) {
+ if (!wiphy_ext_feature_isset(&rdev->wiphy,
+ NL80211_EXT_FEATURE_EXT_KEY_ID))
+ return -EINVAL;
+ else if (!pairwise || key_idx < 0 || key_idx > 1)
+ return -EINVAL;
+ } else if ((pairwise && key_idx) ||
+ params->install_mode == NL80211_KEY_SWITCH_TX) {
return -EINVAL;
+ }
break;
case WLAN_CIPHER_SUITE_AES_CMAC:
case WLAN_CIPHER_SUITE_BIP_CMAC_256:
--
2.20.1
When using Extended Key ID mac80211 drops @IEEE80211_TX_CTL_AMPDU for
the last packet which can be added to a A-MPDU in preparation.
Don't throw a warning and just handle the frame as if
@IEEE80211_TX_CTL_AMPDU would have been set.
Signed-off-by: Alexander Wetzel <[email protected]>
---
I cold not figure out so far how to make sure the card will not mix
A-MPDU frames. Looks like that is handled fully in the HW and I didn't
find any interface to influence it. (It even may work already, I have
some problems to capture A-MPDU frames with A-MPDU information intact
and the topic was pretty low on the list so far. After all it works...)
This patch is therefore basically just using aggregation when it's
enabled and ignores the key border signal from mac80211.
drivers/net/wireless/intel/iwlwifi/dvm/tx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/tx.c b/drivers/net/wireless/intel/iwlwifi/dvm/tx.c
index 4ff323a3a4e5..478f8e1c3e52 100644
--- a/drivers/net/wireless/intel/iwlwifi/dvm/tx.c
+++ b/drivers/net/wireless/intel/iwlwifi/dvm/tx.c
@@ -420,7 +420,7 @@ int iwlagn_tx_skb(struct iwl_priv *priv,
hdr->seq_ctrl |= cpu_to_le16(seq_number);
seq_number += 0x10;
- if (info->flags & IEEE80211_TX_CTL_AMPDU)
+ if (tid_data->agg.state == IWL_AGG_ON)
is_agg = true;
is_data_qos = true;
}
--
2.20.1
Driver is not supporting hardware encryption and therefore fully
compatible with Extended Key ID.
Signed-off-by: Alexander Wetzel <[email protected]>
---
drivers/net/wireless/mac80211_hwsim.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 87be2b18063a..306583316f5d 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -2799,6 +2799,7 @@ static int mac80211_hwsim_new_radio(struct genl_info *info,
ieee80211_hw_set(hw, SIGNAL_DBM);
ieee80211_hw_set(hw, SUPPORTS_PS);
ieee80211_hw_set(hw, TDLS_WIDER_BW);
+ ieee80211_hw_set(hw, EXT_KEY_ID_NATIVE);
if (rctbl)
ieee80211_hw_set(hw, SUPPORTS_RC_TABLE);
--
2.20.1
Update ath_key_config() call to work with Extended Key ID update for ath.
Signed-off-by: Alexander Wetzel <[email protected]>
---
This patch is only provided to not break compile for ath5k.
(ath5k is very likely also able to support Extended Key ID is
Compatibility mode.)
drivers/net/wireless/ath/ath5k/mac80211-ops.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath5k/mac80211-ops.c b/drivers/net/wireless/ath/ath5k/mac80211-ops.c
index 16e052d02c94..cbabb784decb 100644
--- a/drivers/net/wireless/ath/ath5k/mac80211-ops.c
+++ b/drivers/net/wireless/ath/ath5k/mac80211-ops.c
@@ -509,7 +509,7 @@ ath5k_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
switch (cmd) {
case SET_KEY:
- ret = ath_key_config(common, vif, sta, key);
+ ret = ath_key_config(common, vif, sta, key, true);
if (ret >= 0) {
key->hw_key_idx = ret;
/* push IV and Michael MIC generation to stack */
--
2.20.1
Add support for Extended Key ID as defined in IEEE 802.11-2016.
- Implement the nl80211 API for Extended Key ID
- Extend mac80211 API to allow drivers to support Extended Key ID
- Add handling for Rx-only keys (including tailroom_need_count)
- Select the decryption key based on the MPDU keyid
- Enforce cipher does not change when replacing a key.
Signed-off-by: Alexander Wetzel <[email protected]>
---
include/net/mac80211.h | 19 ++++-
net/mac80211/cfg.c | 38 ++++++++++
net/mac80211/debugfs.c | 1 +
net/mac80211/ieee80211_i.h | 2 +-
net/mac80211/key.c | 138 +++++++++++++++++++++++++++++--------
net/mac80211/key.h | 4 ++
net/mac80211/main.c | 5 ++
net/mac80211/rx.c | 74 ++++++++++----------
net/mac80211/sta_info.c | 9 +++
net/mac80211/sta_info.h | 2 +-
net/mac80211/tx.c | 13 +---
11 files changed, 227 insertions(+), 78 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index de866a7253c9..e16bc7623dc0 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1804,13 +1804,22 @@ struct ieee80211_cipher_scheme {
* enum set_key_cmd - key command
*
* Used with the set_key() callback in &struct ieee80211_ops, this
- * indicates whether a key is being removed or added.
+ * indicates which action has to be performed with the key.
*
- * @SET_KEY: a key is set
+ * @SET_KEY: a key is set and valid for Rx and Tx immediately
* @DISABLE_KEY: a key must be disabled
+ *
+ * Additional commands for drivers supporting Extended Key ID:
+ *
+ * @EXT_SET_KEY: a new key must be set but is only valid for decryption
+ * @EXT_KEY_RX_TX: a key installed with @EXT_SET_KEY is becoming the
+ * designated Rx/Tx key for the station
*/
enum set_key_cmd {
- SET_KEY, DISABLE_KEY,
+ SET_KEY,
+ DISABLE_KEY,
+ EXT_SET_KEY,
+ EXT_KEY_RX_TX,
};
/**
@@ -2219,6 +2228,9 @@ struct ieee80211_txq {
* @IEEE80211_HW_TX_STATUS_NO_AMPDU_LEN: Driver does not report accurate A-MPDU
* length in tx status information
*
+ * @IEEE80211_HW_EXT_KEY_ID_NATIVE: Driver and hardware are supporting Extended
+ * Key ID and can handle two unicast keys per station for Rx and Tx.
+ *
* @NUM_IEEE80211_HW_FLAGS: number of hardware flags, used for sizing arrays
*/
enum ieee80211_hw_flags {
@@ -2268,6 +2280,7 @@ enum ieee80211_hw_flags {
IEEE80211_HW_SUPPORTS_VHT_EXT_NSS_BW,
IEEE80211_HW_STA_MMPDU_TXQ,
IEEE80211_HW_TX_STATUS_NO_AMPDU_LEN,
+ IEEE80211_HW_EXT_KEY_ID_NATIVE,
/* keep last, obviously */
NUM_IEEE80211_HW_FLAGS
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index d65aa019ce85..a032da64eed2 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -351,6 +351,38 @@ static int ieee80211_set_noack_map(struct wiphy *wiphy,
return 0;
}
+static int ieee80211_set_tx_key(struct ieee80211_sub_if_data *sdata,
+ const u8 *mac_addr, u8 key_idx)
+{
+ struct ieee80211_local *local = sdata->local;
+ struct ieee80211_key *key;
+ struct sta_info *sta;
+ int ret;
+
+ if (!wiphy_ext_feature_isset(local->hw.wiphy,
+ NL80211_EXT_FEATURE_EXT_KEY_ID))
+ return -EINVAL;
+
+ sta = sta_info_get_bss(sdata, mac_addr);
+
+ if (!sta)
+ return -EINVAL;
+
+ if (sta->ptk_idx == key_idx)
+ return 0;
+
+ mutex_lock(&local->key_mtx);
+ key = key_mtx_dereference(local, sta->ptk[key_idx]);
+
+ if (key && key->flags & KEY_FLAG_RX_ONLY)
+ ret = ieee80211_key_activate_tx(key);
+ else
+ ret = -EINVAL;
+
+ mutex_unlock(&local->key_mtx);
+ return ret;
+}
+
static int ieee80211_add_key(struct wiphy *wiphy, struct net_device *dev,
u8 key_idx, bool pairwise, const u8 *mac_addr,
struct key_params *params)
@@ -365,6 +397,9 @@ static int ieee80211_add_key(struct wiphy *wiphy, struct net_device *dev,
if (!ieee80211_sdata_running(sdata))
return -ENETDOWN;
+ if (pairwise && params->install_mode == NL80211_KEY_SWITCH_TX)
+ return ieee80211_set_tx_key(sdata, mac_addr, key_idx);
+
/* reject WEP and TKIP keys if WEP failed to initialize */
switch (params->cipher) {
case WLAN_CIPHER_SUITE_WEP40:
@@ -396,6 +431,9 @@ static int ieee80211_add_key(struct wiphy *wiphy, struct net_device *dev,
if (pairwise)
key->conf.flags |= IEEE80211_KEY_FLAG_PAIRWISE;
+ if (params->install_mode == NL80211_KEY_RX_ONLY)
+ key->flags |= KEY_FLAG_RX_ONLY;
+
mutex_lock(&local->sta_mtx);
if (mac_addr) {
diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 343ad0a915e4..334a9883894f 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -219,6 +219,7 @@ static const char *hw_flag_names[] = {
FLAG(SUPPORTS_VHT_EXT_NSS_BW),
FLAG(STA_MMPDU_TXQ),
FLAG(TX_STATUS_NO_AMPDU_LEN),
+ FLAG(EXT_KEY_ID_NATIVE),
#undef FLAG
};
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 056b16bce3b0..cbc35a31adc5 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1263,7 +1263,7 @@ struct ieee80211_local {
/*
* Key mutex, protects sdata's key_list and sta_info's
- * key pointers (write access, they're RCU.)
+ * key pointers and @ptk_idx (write access, they're RCU.)
*/
struct mutex key_mtx;
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index b9f2bfc00263..d91503de1e1d 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -127,8 +127,13 @@ static void decrease_tailroom_need_count(struct ieee80211_sub_if_data *sdata,
static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
{
struct ieee80211_sub_if_data *sdata = key->sdata;
+ struct ieee80211_local *local = key->local;
struct sta_info *sta;
+ bool rx_only = key->flags & KEY_FLAG_RX_ONLY;
+ bool pairwise = key->conf.flags & IEEE80211_KEY_FLAG_PAIRWISE;
+ bool ext_native = ieee80211_hw_check(&local->hw, EXT_KEY_ID_NATIVE);
int ret = -EOPNOTSUPP;
+ int cmd;
might_sleep();
@@ -150,10 +155,10 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
return -EINVAL;
}
- if (!key->local->ops->set_key)
+ if (!local->ops->set_key)
goto out_unsupported;
- assert_key_lock(key->local);
+ assert_key_lock(local);
sta = key->sta;
@@ -161,8 +166,8 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
* If this is a per-STA GTK, check if it
* is supported; if not, return.
*/
- if (sta && !(key->conf.flags & IEEE80211_KEY_FLAG_PAIRWISE) &&
- !ieee80211_hw_check(&key->local->hw, SUPPORTS_PER_STA_GTK))
+ if (sta && !pairwise &&
+ !ieee80211_hw_check(&local->hw, SUPPORTS_PER_STA_GTK))
goto out_unsupported;
if (sta && !sta->uploaded)
@@ -173,19 +178,25 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
* The driver doesn't know anything about VLAN interfaces.
* Hence, don't send GTKs for VLAN interfaces to the driver.
*/
- if (!(key->conf.flags & IEEE80211_KEY_FLAG_PAIRWISE)) {
+ if (!pairwise) {
ret = 1;
goto out_unsupported;
}
}
- ret = drv_set_key(key->local, SET_KEY, sdata,
+ if (rx_only)
+ cmd = EXT_SET_KEY;
+ else
+ cmd = SET_KEY;
+
+ ret = drv_set_key(local, cmd, sdata,
sta ? &sta->sta : NULL, &key->conf);
if (!ret) {
key->flags |= KEY_FLAG_UPLOADED_TO_HARDWARE;
- if (!(key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
+ if (!(rx_only ||
+ key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
IEEE80211_KEY_FLAG_PUT_MIC_SPACE |
IEEE80211_KEY_FLAG_RESERVE_TAILROOM)))
decrease_tailroom_need_count(sdata, 1);
@@ -221,7 +232,7 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
/* all of these we can do in software - if driver can */
if (ret == 1)
return 0;
- if (ieee80211_hw_check(&key->local->hw, SW_CRYPTO_CONTROL))
+ if (ieee80211_hw_check(&local->hw, SW_CRYPTO_CONTROL))
return -EINVAL;
return 0;
default:
@@ -248,7 +259,8 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
sta = key->sta;
sdata = key->sdata;
- if (!(key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
+ if (!(key->flags & KEY_FLAG_RX_ONLY ||
+ key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
IEEE80211_KEY_FLAG_PUT_MIC_SPACE |
IEEE80211_KEY_FLAG_RESERVE_TAILROOM)))
increment_tailroom_need_count(sdata);
@@ -264,9 +276,55 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
sta ? sta->sta.addr : bcast_addr, ret);
}
+int ieee80211_key_activate_tx(struct ieee80211_key *key)
+{
+ struct ieee80211_sub_if_data *sdata = key->sdata;
+ struct sta_info *sta = key->sta;
+ struct ieee80211_local *local = key->local;
+ struct ieee80211_key *old;
+ int ret;
+
+ assert_key_lock(local);
+
+ key->flags &= ~KEY_FLAG_RX_ONLY;
+
+ if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) ||
+ key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
+ IEEE80211_KEY_FLAG_PUT_MIC_SPACE |
+ IEEE80211_KEY_FLAG_RESERVE_TAILROOM))
+ increment_tailroom_need_count(sdata);
+
+ if (key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
+ ret = drv_set_key(local, EXT_KEY_RX_TX, sdata,
+ &sta->sta, &key->conf);
+ if (ret) {
+ sdata_err(sdata,
+ "failed to activate key for Tx (%d, %pM)\n",
+ key->conf.keyidx, sta->sta.addr);
+ return ret;
+ }
+ }
+
+ old = key_mtx_dereference(local, sta->ptk[sta->ptk_idx]);
+ sta->ptk_idx = key->conf.keyidx;
+ ieee80211_check_fast_xmit(sta);
+
+ if (old) {
+ old->flags |= KEY_FLAG_RX_ONLY;
+
+ if (!(old->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) ||
+ old->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
+ IEEE80211_KEY_FLAG_PUT_MIC_SPACE |
+ IEEE80211_KEY_FLAG_RESERVE_TAILROOM))
+ decrease_tailroom_need_count(sdata, 1);
+ }
+
+ return 0;
+}
+
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;
@@ -283,16 +341,17 @@ 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) {
+ /* Unicast rekey without Extended Key ID needs special handling */
+ if (new_key && pairwise && sta &&
+ rcu_access_pointer(sta->ptk[sta->ptk_idx]) == old_key) {
local = old_key->local;
sdata = old_key->sdata;
- /* Stop TX till we are on the new key */
+ /* Stop Tx till we are on the new key */
old_key->flags |= KEY_FLAG_TAINTED;
ieee80211_clear_fast_xmit(sta);
- /* Aggregation sessions during rekey are complicated due to the
+ /* Aggregation sessions during rekey are complicated by the
* reorder buffer and retransmits. Side step that by blocking
* aggregation during rekey and tear down running sessions.
*/
@@ -400,10 +459,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 */
@@ -420,15 +475,19 @@ 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->flags & KEY_FLAG_RX_ONLY)) {
+ sta->ptk_idx = idx;
clear_sta_flag(sta, WLAN_STA_BLOCK_BA);
ieee80211_check_fast_xmit(sta);
}
} else {
rcu_assign_pointer(sta->gtk[idx], new);
}
- if (new)
+ /* Only needed when transition from no key -> key.
+ * Still triggers unnecessary when using Extended Key ID
+ * and installing the second key ID the first time.
+ */
+ if (new && !old)
ieee80211_check_fast_rx(sta);
} else {
defunikey = old &&
@@ -664,6 +723,9 @@ static void __ieee80211_key_destroy(struct ieee80211_key *key,
ieee80211_debugfs_key_remove(key);
+ if (key->flags & KEY_FLAG_RX_ONLY)
+ return;
+
if (delay_tailroom) {
/* see ieee80211_delayed_tailroom_dec */
sdata->crypto_tx_tailroom_pending_dec++;
@@ -744,16 +806,33 @@ int ieee80211_key_link(struct ieee80211_key *key,
* can cause warnings to appear.
*/
bool delay_tailroom = sdata->vif.type == NL80211_IFTYPE_STATION;
- int ret;
+ bool rx_only = key->flags & KEY_FLAG_RX_ONLY;
+ int ret = -EOPNOTSUPP;
mutex_lock(&sdata->local->key_mtx);
- if (sta && pairwise)
+ if (sta && pairwise) {
+ struct ieee80211_key *alt_key;
+
old_key = key_mtx_dereference(sdata->local, sta->ptk[idx]);
- else if (sta)
+ alt_key = key_mtx_dereference(sdata->local, sta->ptk[idx ^ 1]);
+
+ /* Don't allow pairwise keys to change cipher on rekey */
+ if (key &&
+ ((alt_key && alt_key->conf.cipher != key->conf.cipher) ||
+ (old_key && old_key->conf.cipher != key->conf.cipher)))
+ goto out;
+ } else if (sta) {
old_key = key_mtx_dereference(sdata->local, sta->gtk[idx]);
- else
+ } else {
old_key = key_mtx_dereference(sdata->local, sdata->keys[idx]);
+ }
+
+ /* Don't allow non-pairwise keys to change cipher on rekey */
+ if (!pairwise) {
+ if (key && old_key && old_key->conf.cipher != key->conf.cipher)
+ goto out;
+ }
/*
* Silently accept key re-installation without really installing the
@@ -769,7 +848,8 @@ int ieee80211_key_link(struct ieee80211_key *key,
key->sdata = sdata;
key->sta = sta;
- increment_tailroom_need_count(sdata);
+ if (!rx_only)
+ increment_tailroom_need_count(sdata);
ret = ieee80211_key_replace(sdata, sta, pairwise, old_key, key);
@@ -823,7 +903,8 @@ void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata)
}
list_for_each_entry(key, &sdata->key_list, list) {
- increment_tailroom_need_count(sdata);
+ if (!(key->flags & KEY_FLAG_RX_ONLY))
+ increment_tailroom_need_count(sdata);
ieee80211_key_enable_hw_accel(key);
}
@@ -1193,7 +1274,8 @@ void ieee80211_remove_key(struct ieee80211_key_conf *keyconf)
if (key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
- if (!(key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
+ if (!(key->flags & KEY_FLAG_RX_ONLY ||
+ key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
IEEE80211_KEY_FLAG_PUT_MIC_SPACE |
IEEE80211_KEY_FLAG_RESERVE_TAILROOM)))
increment_tailroom_need_count(key->sdata);
diff --git a/net/mac80211/key.h b/net/mac80211/key.h
index ebdb80b85dc3..1a3da999e0c4 100644
--- a/net/mac80211/key.h
+++ b/net/mac80211/key.h
@@ -18,6 +18,7 @@
#define NUM_DEFAULT_KEYS 4
#define NUM_DEFAULT_MGMT_KEYS 2
+#define INVALID_PTK_KEYIDX 2 /* Existing key slot never used by PTK keys */
struct ieee80211_local;
struct ieee80211_sub_if_data;
@@ -30,11 +31,13 @@ struct sta_info;
* in the hardware for TX crypto hardware acceleration.
* @KEY_FLAG_TAINTED: Key is tainted and packets should be dropped.
* @KEY_FLAG_CIPHER_SCHEME: This key is for a hardware cipher scheme
+ * @KEY_FLAG_RX_ONLY: Pairwise key only allowed to be used on Rx.
*/
enum ieee80211_internal_key_flags {
KEY_FLAG_UPLOADED_TO_HARDWARE = BIT(0),
KEY_FLAG_TAINTED = BIT(1),
KEY_FLAG_CIPHER_SCHEME = BIT(2),
+ KEY_FLAG_RX_ONLY = BIT(3),
};
enum ieee80211_internal_tkip_state {
@@ -146,6 +149,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_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 71005b6dfcd1..ea34544985f3 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -1051,6 +1051,11 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
}
}
+ /* mac80211 supports Extended Key ID when driver does */
+ if (ieee80211_hw_check(&local->hw, EXT_KEY_ID_NATIVE))
+ wiphy_ext_feature_set(local->hw.wiphy,
+ NL80211_EXT_FEATURE_EXT_KEY_ID);
+
/*
* Calculate scan IE length -- we need this to alloc
* memory and to subtract from the driver limit. It
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index bb4d71efb6fb..ce786311baf4 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -988,23 +988,43 @@ static int ieee80211_get_mmie_keyidx(struct sk_buff *skb)
return -1;
}
-static int ieee80211_get_cs_keyid(const struct ieee80211_cipher_scheme *cs,
- struct sk_buff *skb)
+static int ieee80211_get_keyid(struct sk_buff *skb,
+ const struct ieee80211_cipher_scheme *cs)
{
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
__le16 fc;
int hdrlen;
+ int minlen;
+ u8 key_idx_off;
+ u8 key_idx_shift;
u8 keyid;
fc = hdr->frame_control;
hdrlen = ieee80211_hdrlen(fc);
- if (skb->len < hdrlen + cs->hdr_len)
+ if (cs) {
+ minlen = hdrlen + cs->hdr_len;
+ key_idx_off = hdrlen + cs->key_idx_off;
+ key_idx_shift = cs->key_idx_shift;
+ } else {
+ /* WEP, TKIP, CCMP and GCMP have the key id at the same place */
+ minlen = hdrlen + IEEE80211_WEP_IV_LEN;
+ key_idx_off = hdrlen + 3;
+ key_idx_shift = 6;
+ }
+
+ if (unlikely(skb->len < minlen))
return -EINVAL;
- skb_copy_bits(skb, hdrlen + cs->key_idx_off, &keyid, 1);
- keyid &= cs->key_idx_mask;
- keyid >>= cs->key_idx_shift;
+ skb_copy_bits(skb, key_idx_off, &keyid, 1);
+
+ if (cs)
+ keyid &= cs->key_idx_mask;
+ keyid >>= key_idx_shift;
+
+ /* cs could use more than the usual two bits for the keyid */
+ if (unlikely(keyid > NUM_DEFAULT_KEYS))
+ return -EINVAL;
return keyid;
}
@@ -1835,9 +1855,9 @@ ieee80211_rx_h_decrypt(struct ieee80211_rx_data *rx)
struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
int keyidx;
- int hdrlen;
ieee80211_rx_result result = RX_DROP_UNUSABLE;
struct ieee80211_key *sta_ptk = NULL;
+ struct ieee80211_key *ptk_idx = NULL;
int mmie_keyidx = -1;
__le16 fc;
const struct ieee80211_cipher_scheme *cs = NULL;
@@ -1875,21 +1895,24 @@ ieee80211_rx_h_decrypt(struct ieee80211_rx_data *rx)
if (rx->sta) {
int keyid = rx->sta->ptk_idx;
+ sta_ptk = rcu_dereference(rx->sta->ptk[keyid]);
- if (ieee80211_has_protected(fc) && rx->sta->cipher_scheme) {
+ if (ieee80211_has_protected(fc)) {
cs = rx->sta->cipher_scheme;
- keyid = ieee80211_get_cs_keyid(cs, rx->skb);
+ keyid = ieee80211_get_keyid(rx->skb, cs);
+
if (unlikely(keyid < 0))
return RX_DROP_UNUSABLE;
+
+ ptk_idx = rcu_dereference(rx->sta->ptk[keyid]);
}
- sta_ptk = rcu_dereference(rx->sta->ptk[keyid]);
}
if (!ieee80211_has_protected(fc))
mmie_keyidx = ieee80211_get_mmie_keyidx(rx->skb);
if (!is_multicast_ether_addr(hdr->addr1) && sta_ptk) {
- rx->key = sta_ptk;
+ rx->key = ptk_idx ? ptk_idx : sta_ptk;
if ((status->flag & RX_FLAG_DECRYPTED) &&
(status->flag & RX_FLAG_IV_STRIPPED))
return RX_CONTINUE;
@@ -1949,8 +1972,6 @@ ieee80211_rx_h_decrypt(struct ieee80211_rx_data *rx)
}
return RX_CONTINUE;
} else {
- u8 keyid;
-
/*
* The device doesn't give us the IV so we won't be
* able to look up the key. That's ok though, we
@@ -1964,23 +1985,10 @@ ieee80211_rx_h_decrypt(struct ieee80211_rx_data *rx)
(status->flag & RX_FLAG_IV_STRIPPED))
return RX_CONTINUE;
- hdrlen = ieee80211_hdrlen(fc);
-
- if (cs) {
- keyidx = ieee80211_get_cs_keyid(cs, rx->skb);
+ keyidx = ieee80211_get_keyid(rx->skb, cs);
- if (unlikely(keyidx < 0))
- return RX_DROP_UNUSABLE;
- } else {
- if (rx->skb->len < 8 + hdrlen)
- return RX_DROP_UNUSABLE; /* TODO: count this? */
- /*
- * no need to call ieee80211_wep_get_keyidx,
- * it verifies a bunch of things we've done already
- */
- skb_copy_bits(rx->skb, hdrlen + 3, &keyid, 1);
- keyidx = keyid >> 6;
- }
+ if (unlikely(keyidx < 0))
+ return RX_DROP_UNUSABLE;
/* check per-station GTK first, if multicast packet */
if (is_multicast_ether_addr(hdr->addr1) && rx->sta)
@@ -4020,12 +4028,8 @@ void ieee80211_check_fast_rx(struct sta_info *sta)
case WLAN_CIPHER_SUITE_GCMP_256:
break;
default:
- /* we also don't want to deal with WEP or cipher scheme
- * since those require looking up the key idx in the
- * frame, rather than assuming the PTK is used
- * (we need to revisit this once we implement the real
- * PTK index, which is now valid in the spec, but we
- * haven't implemented that part yet)
+ /* We also don't want to deal with
+ * WEP or cipher scheme.
*/
goto clear_rcu;
}
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 11f058987a54..09c69955c6e3 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -347,6 +347,15 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
sta->sta.max_rx_aggregation_subframes =
local->hw.max_rx_aggregation_subframes;
+ /* Extended Key ID can install keys for keyid 0 and 1 as Rx only.
+ * Tx starts uses a key as soon as a key is installed in the slot
+ * ptk_idx references to. To avoid using the initial Rx key prematurely
+ * for Tx we initialize ptk_idx to a value never used, making sure the
+ * referenced key is always NULL till ptk_idx is set to a valid value.
+ */
+ BUILD_BUG_ON(ARRAY_SIZE(sta->ptk) <= INVALID_PTK_KEYIDX);
+ sta->ptk_idx = INVALID_PTK_KEYIDX;
+
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 71f7e4973329..304a7ea24757 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -449,7 +449,7 @@ struct ieee80211_sta_rx_stats {
* @local: pointer to the global information
* @sdata: virtual interface this station belongs to
* @ptk: peer keys negotiated with this station, if any
- * @ptk_idx: last installed peer key index
+ * @ptk_idx: peer key index to use for transmissions
* @gtk: group keys negotiated with this station, if any
* @rate_ctrl: rate control algorithm reference
* @rate_ctrl_lock: spinlock used to protect rate control data
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 8a49a74c0a37..111bd6c490a6 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -3000,23 +3000,15 @@ void ieee80211_check_fast_xmit(struct sta_info *sta)
switch (build.key->conf.cipher) {
case WLAN_CIPHER_SUITE_CCMP:
case WLAN_CIPHER_SUITE_CCMP_256:
- /* add fixed key ID */
- if (gen_iv) {
- (build.hdr + build.hdr_len)[3] =
- 0x20 | (build.key->conf.keyidx << 6);
+ if (gen_iv)
build.pn_offs = build.hdr_len;
- }
if (gen_iv || iv_spc)
build.hdr_len += IEEE80211_CCMP_HDR_LEN;
break;
case WLAN_CIPHER_SUITE_GCMP:
case WLAN_CIPHER_SUITE_GCMP_256:
- /* add fixed key ID */
- if (gen_iv) {
- (build.hdr + build.hdr_len)[3] =
- 0x20 | (build.key->conf.keyidx << 6);
+ if (gen_iv)
build.pn_offs = build.hdr_len;
- }
if (gen_iv || iv_spc)
build.hdr_len += IEEE80211_GCMP_HDR_LEN;
break;
@@ -3383,6 +3375,7 @@ static void ieee80211_xmit_fast_finish(struct ieee80211_sub_if_data *sdata,
pn = atomic64_inc_return(&key->conf.tx_pn);
crypto_hdr[0] = pn;
crypto_hdr[1] = pn >> 8;
+ crypto_hdr[3] = 0x20 | (key->conf.keyidx << 6);
crypto_hdr[4] = pn >> 16;
crypto_hdr[5] = pn >> 24;
crypto_hdr[6] = pn >> 32;
--
2.20.1
Implements %EXT_SET_KEY, %EXT_KEY_RX_TX and %EXT_DISABLE_KEY_RX and
enables EXT_KEY_ID_COMPAT.
Signed-off-by: Alexander Wetzel <[email protected]>
---
Like the generic ath patch to provide Extended Key ID support just the
minimum needed to get it working in AP mode and serve as an reference
how Compatibility Extended Key ID support looks like from a driver
perspective.
drivers/net/wireless/ath/ath9k/htc_drv_main.c | 2 +-
drivers/net/wireless/ath/ath9k/init.c | 1 +
drivers/net/wireless/ath/ath9k/main.c | 20 ++++++++++++++++---
3 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/htc_drv_main.c b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
index a82ad739ab80..2708572616f2 100644
--- a/drivers/net/wireless/ath/ath9k/htc_drv_main.c
+++ b/drivers/net/wireless/ath/ath9k/htc_drv_main.c
@@ -1446,7 +1446,7 @@ static int ath9k_htc_set_key(struct ieee80211_hw *hw,
switch (cmd) {
case SET_KEY:
- ret = ath_key_config(common, vif, sta, key);
+ ret = ath_key_config(common, vif, sta, key, true);
if (ret >= 0) {
key->hw_key_idx = ret;
/* push IV and Michael MIC generation to stack */
diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c
index c070a9e51ebf..ac1c6d59b954 100644
--- a/drivers/net/wireless/ath/ath9k/init.c
+++ b/drivers/net/wireless/ath/ath9k/init.c
@@ -929,6 +929,7 @@ static void ath9k_set_hw_capab(struct ath_softc *sc, struct ieee80211_hw *hw)
ieee80211_hw_set(hw, HOST_BROADCAST_PS_BUFFERING);
ieee80211_hw_set(hw, SUPPORT_FAST_XMIT);
ieee80211_hw_set(hw, SUPPORTS_CLONED_SKBS);
+ ieee80211_hw_set(hw, EXT_KEY_ID_COMPAT);
if (ath9k_ps_enable)
ieee80211_hw_set(hw, SUPPORTS_PS);
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index f23cb2f3d296..880687f09157 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -1518,7 +1518,7 @@ static int ath9k_sta_add(struct ieee80211_hw *hw,
vif->type != NL80211_IFTYPE_AP_VLAN)
return 0;
- key = ath_key_config(common, vif, sta, &ps_key);
+ key = ath_key_config(common, vif, sta, &ps_key, true);
if (key > 0) {
an->ps_key = key;
an->key_idx[0] = key;
@@ -1675,9 +1675,13 @@ static int ath9k_set_key(struct ieee80211_hw *hw,
struct ath_common *common = ath9k_hw_common(sc->sc_ah);
struct ath_node *an = NULL;
int ret = 0, i;
+ bool rx_accel = true;
- if (ath9k_modparam_nohwcrypt)
+ if (ath9k_modparam_nohwcrypt) {
+ if (cmd == EXT_DISABLE_KEY_RX || cmd == EXT_KEY_RX_TX)
+ return 0;
return -ENOSPC;
+ }
if ((vif->type == NL80211_IFTYPE_ADHOC ||
vif->type == NL80211_IFTYPE_MESH_POINT) &&
@@ -1701,12 +1705,15 @@ static int ath9k_set_key(struct ieee80211_hw *hw,
an = (struct ath_node *)sta->drv_priv;
switch (cmd) {
+ case EXT_SET_KEY:
+ rx_accel = false;
+ /* Fall trough */
case SET_KEY:
if (sta)
ath9k_del_ps_key(sc, vif, sta);
key->hw_key_idx = 0;
- ret = ath_key_config(common, vif, sta, key);
+ ret = ath_key_config(common, vif, sta, key, rx_accel);
if (ret >= 0) {
key->hw_key_idx = ret;
/* push IV and Michael MIC generation to stack */
@@ -1740,6 +1747,13 @@ static int ath9k_set_key(struct ieee80211_hw *hw,
}
key->hw_key_idx = 0;
break;
+ case EXT_DISABLE_KEY_RX:
+ rx_accel = false;
+ /* fall trough */
+ case EXT_KEY_RX_TX:
+ if (ath_hw_rx_crypt(common, key, sta, rx_accel))
+ ret = 0;
+ break;
default:
ret = -EINVAL;
}
--
2.20.1
This is not ready for merge and has known issues.
The patch is only for discussions to sort out how to handle it correctly!
Signed-off-by: Alexander Wetzel <[email protected]>
---
iwlwifi intel cards had two big surprises:
Assuming I did not make some stupid errors it looks like my old
"Intel Corporation Centrino Ultimate-N 6300 (rev 3e)" using ucode
9.221.4.1 build 25532 is perfectly fine with two keys uploaded to
harware and honoring the keyid in the MPDUs. For a card launched 2011
that's a pleasant surprise:-)
A much shorter test with a modern "Intel Corporation Wireless 8265 /
8175 (rev 78)" using ucode version 36.e91976c0.0 shows what seems to be
MPDUs decoded with the wrong key at each rekey and therefore a candidate
for the COMPAT support only..
So the bad news seems to be, that the modern card dropped the support.
It also seems to force us to add some per-card or per-firmware depending
check to decide which card can use the Native Extended Key ID support
and use the Compat mode for the rest.
Is there any way to find out which cards/firmware can be used with
Extended Key ID?
I also have tested patch for iwldvm using the Compat mode and I think
mvm cards will also work with that.
drivers/net/wireless/intel/iwlwifi/dvm/mac80211.c | 5 +++++
drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c | 5 +++++
2 files changed, 10 insertions(+)
diff --git a/drivers/net/wireless/intel/iwlwifi/dvm/mac80211.c b/drivers/net/wireless/intel/iwlwifi/dvm/mac80211.c
index 54b759cec8b3..0dd5c19ac412 100644
--- a/drivers/net/wireless/intel/iwlwifi/dvm/mac80211.c
+++ b/drivers/net/wireless/intel/iwlwifi/dvm/mac80211.c
@@ -111,6 +111,7 @@ int iwlagn_mac_setup_register(struct iwl_priv *priv,
ieee80211_hw_set(hw, SUPPORTS_DYNAMIC_PS);
ieee80211_hw_set(hw, SUPPORT_FAST_XMIT);
ieee80211_hw_set(hw, WANT_MONITOR_VIF);
+ ieee80211_hw_set(hw, EXT_KEY_ID_NATIVE);
if (priv->trans->max_skb_frags)
hw->netdev_features = NETIF_F_HIGHDMA | NETIF_F_SG;
@@ -676,6 +677,7 @@ static int iwlagn_mac_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
switch (cmd) {
+ case EXT_SET_KEY:
case SET_KEY:
if (is_default_wep_key) {
ret = iwl_set_default_wep_key(priv, vif_priv->ctx, key);
@@ -701,6 +703,9 @@ static int iwlagn_mac_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
IWL_DEBUG_MAC80211(priv, "disable hwcrypto key\n");
break;
+ case EXT_KEY_RX_TX:
+ ret = 0;
+ break;
default:
ret = -EINVAL;
}
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c b/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
index fc251cc47b7f..102367d2572f 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
@@ -442,6 +442,7 @@ int iwl_mvm_mac_setup_register(struct iwl_mvm *mvm)
ieee80211_hw_set(hw, STA_MMPDU_TXQ);
ieee80211_hw_set(hw, TX_AMSDU);
ieee80211_hw_set(hw, TX_FRAG_LIST);
+ ieee80211_hw_set(hw, EXT_KEY_ID_NATIVE);
if (iwl_mvm_has_tlc_offload(mvm)) {
ieee80211_hw_set(hw, TX_AMPDU_SETUP_IN_HW);
@@ -3357,6 +3358,7 @@ static int iwl_mvm_mac_set_key(struct ieee80211_hw *hw,
mutex_lock(&mvm->mutex);
switch (cmd) {
+ case EXT_SET_KEY:
case SET_KEY:
if ((vif->type == NL80211_IFTYPE_ADHOC ||
vif->type == NL80211_IFTYPE_AP) && !sta) {
@@ -3464,6 +3466,9 @@ static int iwl_mvm_mac_set_key(struct ieee80211_hw *hw,
IWL_DEBUG_MAC80211(mvm, "disable hwcrypto key\n");
ret = iwl_mvm_remove_sta_key(mvm, vif, sta, key);
break;
+ case EXT_KEY_RX_TX:
+ ret = 0;
+ break;
default:
ret = -EINVAL;
}
--
2.20.1
When using Extended Key ID mac80211 drops @IEEE80211_TX_CTL_AMPDU for
the last packet which can be added to a A-MPDU in preparation.
Finalize A-MPDU immediately and start a new aggregation.
Signed-off-by: Alexander Wetzel <[email protected]>
---
This is the (b)leading edge of my current attempt to figure out a
solution to the "must not aggregate keyid 0 and 1 frames together"
problem.
I've not even tested that patch properly, yet..
So it only shows how I think ath9k driver can use the A-MPDU border
signal to send out the A-MPDU frame currently under construction.
May be useful to figure out if we want to implement A-MPDU border signal
in mac80211, through.
drivers/net/wireless/ath/ath9k/xmit.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index a24265018cb2..84bbce6f212f 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -990,10 +990,8 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq,
* from a previous session or a failed attempt in the queue.
* Send them out as normal data frames
*/
- if (!tid->active)
+ if (!tid->active) {
tx_info->flags &= ~IEEE80211_TX_CTL_AMPDU;
-
- if (!(tx_info->flags & IEEE80211_TX_CTL_AMPDU)) {
bf->bf_state.bf_type = 0;
return bf;
}
@@ -1017,12 +1015,18 @@ ath_tx_get_tid_subframe(struct ath_softc *sc, struct ath_txq *txq,
break;
}
- if (tid->bar_index > ATH_BA_INDEX(tid->seq_start, seqno)) {
+ if (tid->bar_index > ATH_BA_INDEX(tid->seq_start, seqno) ||
+ !(tx_info->flags & IEEE80211_TX_CTL_AMPDU)) {
struct ath_tx_status ts = {};
struct list_head bf_head;
INIT_LIST_HEAD(&bf_head);
- list_add(&bf->list, &bf_head);
+ if (tx_info->flags & IEEE80211_TX_CTL_AMPDU &&
+ tid->bar_index <= ATH_BA_INDEX(tid->seq_start, seqno))
+ list_add(&bf->list, &bf_head);
+ else
+ ath_tx_addto_baw(sc, tid, bf);
+
ath_tx_update_baw(sc, tid, bf);
ath_tx_complete_buf(sc, bf, txq, &bf_head, NULL, &ts, 0);
continue;
--
2.20.1
Alexander Wetzel <[email protected]> writes:
> Extend the shared ath key cache code to support Extended Key ID.
>
> The key cache code has to accept unicast keys to use key idx 1 and allow
> drivers to enable/disable hardware Rx decryption for a key independent
> from Tx.
>
> Signed-off-by: Alexander Wetzel <[email protected]>
> ---
>
> I know this is the wrong audience to discuss ath drivers.
I think this is the right forum. Do note that somewhere in this patch
(in the cover letter) you mentioned "all ath drivers" but AFAICS this
patch only changes functionality for ath5k, ath9k and ath9k_htc. All the
rest like wil6210, ath6kl and ath10k are unaffected.
--
Kalle Valo
>
>> Extend the shared ath key cache code to support Extended Key ID.
>>
>> The key cache code has to accept unicast keys to use key idx 1 and allow
>> drivers to enable/disable hardware Rx decryption for a key independent
>> from Tx.
>>
>> Signed-off-by: Alexander Wetzel <[email protected]>
>> ---
>>
>> I know this is the wrong audience to discuss ath drivers.
>
> I think this is the right forum. Do note that somewhere in this patch
You are of course right. I mixed that up somehow.
We can of course also discuss the ath patches any time :-)
My initial plan was, to get the nl80211/mac80211 API finalized and then
get them reviewed together with another planned fix after some more
polishing.
At this stage they are just a POC and not ready for merge. They work
with ath9k in AP (vlan) mode and I believe managed mode should either
work or need some trivial fix only. (There even seems to be a chance
that managed mode could allow the usage of the NATIVE Extended Key ID
mode, but so far I could not tested that.)
> (in the cover letter) you mentioned "all ath drivers" but AFAICS this
> patch only changes functionality for ath5k, ath9k and ath9k_htc. All the
> rest like wil6210, ath6kl and ath10k are unaffected.
>
You are right, I should have used "shared ath key cache code" in the
Cover Letter, as in the patch itself. This is not (yet) an attempt to
implement Extended Key ID for anything else than ath9k AP mode. So any
driver not using ath_key_config() won't be affected at all.
Now I believe it's possible for all Atheros drivers but the ath10k to
get support. As long as a card can work with SW crypto we only need a
way to disable Rx HW crypto for a running key without impact for ongoing Tx.
But the initial results when trying my hand at ath10k are strongly
indicating the best we can hope there is SW encryption only with CT
firmware... or maybe a firmware update.
While the API itself is perfectly able to handle NATIVE mode the keyid
is not handled correctly. Installing a second key switches TX to the new
key and overwrites the keyid in the MPDU mac80211 prepared. (I could not
even get the card to properly make an RX/TX key to an TX only key, that
caused clear text packets when changing the key and it looks like that
SW crypto is only possible - with nonfree CT - when not using HW crypto
for TX at all. With those limitations I shelved any plans for ath10k.)
One of my next planned steps is now to either get another ath9k card or
get another driver working in AP mode to test ath9k also in managed
mode. Of course I also have to get sniffing working properly, all cards
tried so far have issues and it also looks like I have to update
wireshark for serious testing. So I guess driver support will still take
some time and efforts when we got the generic issues sorted out.
I can also try my hand at porting the other Atheros drives, but without
someone being able to confirm it works I'm not planning that at the moment.
Alexander
On Sun, 2019-02-10 at 22:06 +0100, Alexander Wetzel wrote:
> +/**
> + * enum nl80211_key_install_mode - Key install mode
> + *
> + * @NL80211_KEY_RX_TX: Key must be installed for Rx and Tx
> + * @NL80211_KEY_RX_ONLY: Allowed in combination with @NL80211_CMD_NEW_KEY:
> + * Unicast key has to be installed for Rx only.
> + * @NL80211_KEY_SWITCH_TX: Allowed in combination with @NL80211_CMD_SET_KEY:
> + * Switch Tx to a Rx only, referenced by sta mac and idx.
Don't you mean the other way around? Or, well, what you say is true for
the *other* keys?
> * by the %NL80211_SCAN_FLAG_MIN_PREQ_CONTENT flag.
> * @NL80211_EXT_FEATURE_ENABLE_FTM_RESPONDER: Driver supports enabling fine
> * timing measurement responder role.
> - *
no need to remove that :)
> - /* only support setting default key */
> - if (!key.def && !key.defmgmt)
> + /* Only support setting default key and
> + * Extended Key ID action @NL80211_KEY_SWITCH_TX.
> + */
you can remove the @, it's not a kernel-doc formatted comment
> - }
> + } else if (key.p.install_mode == NL80211_KEY_SWITCH_TX &&
> + wiphy_ext_feature_isset(&rdev->wiphy,
> + NL80211_EXT_FEATURE_EXT_KEY_ID)) {
> + u8 *mac_addr = NULL;
>
> + if (info->attrs[NL80211_ATTR_MAC])
> + mac_addr = nla_data(info->attrs[NL80211_ATTR_MAC]);
> +
> + if (!mac_addr || key.idx < 0 || key.idx > 1) {
> + err = -EINVAL;
> + goto out;
> + }
Really only 0 and 1 are allowed? Not 0-3?
> +++ b/net/wireless/util.c
> @@ -236,14 +236,22 @@ int cfg80211_validate_key_settings(struct cfg80211_registered_device *rdev,
> case WLAN_CIPHER_SUITE_CCMP_256:
> case WLAN_CIPHER_SUITE_GCMP:
> case WLAN_CIPHER_SUITE_GCMP_256:
> - /* Disallow pairwise keys with non-zero index unless it's WEP
> - * or a vendor specific cipher (because current deployments use
> - * pairwise WEP keys with non-zero indices and for vendor
> - * specific ciphers this should be validated in the driver or
> - * hardware level - but 802.11i clearly specifies to use zero)
> + /* IEEE802.11-2016 allows only 0 and - when using Extended Key
> + * ID - 1 as index for pairwise keys.
> + * @NL80211_KEY_RX_ONLY is only allowed for pairwise keys when
> + * the driver supports Extended Key ID.
> + * @NL80211_KEY_SWITCH_TX must not be set when validating a key.
> */
> - if (pairwise && key_idx)
> + if (params->install_mode == NL80211_KEY_RX_ONLY) {
> + if (!wiphy_ext_feature_isset(&rdev->wiphy,
> + NL80211_EXT_FEATURE_EXT_KEY_ID))
> + return -EINVAL;
> + else if (!pairwise || key_idx < 0 || key_idx > 1)
> + return -EINVAL;
same question here
johannes
On Sun, 2019-02-10 at 22:06 +0100, Alexander Wetzel wrote:
>
> - Enforce cipher does not change when replacing a key.
is that actually required somehow?
> + * @EXT_SET_KEY: a new key must be set but is only valid for decryption
> + * @EXT_KEY_RX_TX: a key installed with @EXT_SET_KEY is becoming the
> + * designated Rx/Tx key for the station
Not sure I like the EXT_SET_KEY. There's also no "designated Rx key", is
there? It's always selected by key ID.
How about SET_KEY_RXONLY and SET_KEY_TX or something like that?
> +static int ieee80211_set_tx_key(struct ieee80211_sub_if_data *sdata,
> + const u8 *mac_addr, u8 key_idx)
> +{
> + struct ieee80211_local *local = sdata->local;
> + struct ieee80211_key *key;
> + struct sta_info *sta;
> + int ret;
> +
> + if (!wiphy_ext_feature_isset(local->hw.wiphy,
> + NL80211_EXT_FEATURE_EXT_KEY_ID))
> + return -EINVAL;
You set this, wouldn't it make more sense to check EXT_KEY_ID_NATIVE?
Or maybe this is because of the next patch?
> + sta = sta_info_get_bss(sdata, mac_addr);
> +
> + if (!sta)
> + return -EINVAL;
> +
> + if (sta->ptk_idx == key_idx)
> + return 0;
> +
> + mutex_lock(&local->key_mtx);
> + key = key_mtx_dereference(local, sta->ptk[key_idx]);
> +
> + if (key && key->flags & KEY_FLAG_RX_ONLY)
do you even need the flag? Isn't it equivalent to checking
sta->ptk_idx != key->idx
or so?
Less data to maintain would be better.
> + bool ext_native = ieee80211_hw_check(&local->hw, EXT_KEY_ID_NATIVE);
you sort of only need this in the next patch, but I guess it doesn't
matter that much
> +int ieee80211_key_activate_tx(struct ieee80211_key *key)
> +{
> + struct ieee80211_sub_if_data *sdata = key->sdata;
> + struct sta_info *sta = key->sta;
> + struct ieee80211_local *local = key->local;
> + struct ieee80211_key *old;
> + int ret;
> +
> + assert_key_lock(local);
> +
> + key->flags &= ~KEY_FLAG_RX_ONLY;
> +
> + if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) ||
> + key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
> + IEEE80211_KEY_FLAG_PUT_MIC_SPACE |
> + IEEE80211_KEY_FLAG_RESERVE_TAILROOM))
> + increment_tailroom_need_count(sdata);
> +
> + if (key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
> + ret = drv_set_key(local, EXT_KEY_RX_TX, sdata,
> + &sta->sta, &key->conf);
> + if (ret) {
> + sdata_err(sdata,
> + "failed to activate key for Tx (%d, %pM)\n",
> + key->conf.keyidx, sta->sta.addr);
> + return ret;
You've already cleared the RX_ONLY flag, which gets you inconsistent
data now.
> + }
> + }
> +
> + old = key_mtx_dereference(local, sta->ptk[sta->ptk_idx]);
> + sta->ptk_idx = key->conf.keyidx;
but you set this only here.
> - /* Stop TX till we are on the new key */
> + /* Stop Tx till we are on the new key */
Uh, I had to read that three times ... please don't make changes like
that? :)
> old_key->flags |= KEY_FLAG_TAINTED;
> ieee80211_clear_fast_xmit(sta);
>
> - /* Aggregation sessions during rekey are complicated due to the
> + /* Aggregation sessions during rekey are complicated by the
similarly here, please don't make drive-by comment wording issues (also,
I'm not sure I agree - the old version just treats "complicated" as an
adjective, you treat it as a verb, but ultimately doesn't really matter?
> #define NUM_DEFAULT_KEYS 4
> #define NUM_DEFAULT_MGMT_KEYS 2
> +#define INVALID_PTK_KEYIDX 2 /* Existing key slot never used by PTK keys */
We could also use something obviously wrong like 0xff?
> +++ b/net/mac80211/tx.c
> @@ -3000,23 +3000,15 @@ void ieee80211_check_fast_xmit(struct sta_info *sta)
> switch (build.key->conf.cipher) {
> case WLAN_CIPHER_SUITE_CCMP:
> case WLAN_CIPHER_SUITE_CCMP_256:
> - /* add fixed key ID */
> - if (gen_iv) {
> - (build.hdr + build.hdr_len)[3] =
> - 0x20 | (build.key->conf.keyidx << 6);
> + if (gen_iv)
> build.pn_offs = build.hdr_len;
> - }
> if (gen_iv || iv_spc)
> build.hdr_len += IEEE80211_CCMP_HDR_LEN;
> break;
> case WLAN_CIPHER_SUITE_GCMP:
> case WLAN_CIPHER_SUITE_GCMP_256:
> - /* add fixed key ID */
> - if (gen_iv) {
> - (build.hdr + build.hdr_len)[3] =
> - 0x20 | (build.key->conf.keyidx << 6);
> + if (gen_iv)
> build.pn_offs = build.hdr_len;
> - }
> if (gen_iv || iv_spc)
> build.hdr_len += IEEE80211_GCMP_HDR_LEN;
> break;
> @@ -3383,6 +3375,7 @@ static void ieee80211_xmit_fast_finish(struct ieee80211_sub_if_data *sdata,
> pn = atomic64_inc_return(&key->conf.tx_pn);
> crypto_hdr[0] = pn;
> crypto_hdr[1] = pn >> 8;
> + crypto_hdr[3] = 0x20 | (key->conf.keyidx << 6);
> crypto_hdr[4] = pn >> 16;
> crypto_hdr[5] = pn >> 24;
> crypto_hdr[6] = pn >> 32;
This shouldn't be needed, you do update the fast TX cache when changing
the key?
johannes
> + if (!ext_native && key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
> + key->flags |= KEY_FLAG_RX_SW_CRYPTO;
> + /* Activate Rx crypto offload after max 10s when idle */
> + ieee80211_queue_delayed_work(&local->hw, &sta->ext_key_compat_wk,
> + round_jiffies_relative(HZ * 10));
> + }
Is there much point in this?
> + if (unlikely(rx->key->flags & KEY_FLAG_RX_SW_CRYPTO)) {
> + rx->key->flags &= ~KEY_FLAG_RX_SW_CRYPTO;
> + cancel_delayed_work(&rx->sta->ext_key_compat_wk);
> + ieee80211_queue_delayed_work(&rx->local->hw,
> + &rx->sta->ext_key_compat_wk, 0);
> + }
We'll almost certainly do it from here, so never exercise the other
path?
johannes
On Sun, 2019-02-10 at 22:06 +0100, Alexander Wetzel wrote:
>
> The driver patches are - with the exception of the hwsim patch -
> definitely not ready for merge and mostly here to illustrate how the
> different APIs can be used and to start some discussions how to handle HW
> specific challenges. Of course if someone wants to play with Extended Key
> ID they also should be useful... (I can provide updated mostly working
> hostapd/wpa_supplicant patches if someone is interested.
Of course :-)
Some tests for the hwsim tests there would also be nice, that's the
easiest way to see something working - if you have them.
johannes
> The intent of the wording was probably written without considering
> Extended Key IDs. At least it makes no sense for me to forbid mixing
> MPDUs using keyid 0 and 1 in one A-MPDU.
I think it may make sense - reprogramming the hardware engines may take
some time, and doing that in the middle of the A-MPDU may not be
feasible? You don't just have to load the key (that you need to do
anyway) but also extract the status? I dunno, I'm more handwaving, but
it doesn't make sense to add such a requirement when only one key index
can be used to start with.
> The code is assuming that the driver is not aggregating MPDUs more than
> 5s apart. We probably don't have wait nearly so long but I'm not sure
> what is the minimum time.
OTOH, if you have a lot of BE/VI/VO traffic BK might be starved even
longer than that, technically indefinitely.
> +static struct ieee80211_key debug_noinline
> +*ieee80211_select_sta_key(struct ieee80211_tx_data *tx)
> +{
> + struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
> + struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
> + struct sta_info *sta = tx->sta;
> + struct ieee80211_key *key;
> + struct ieee80211_key *next_key;
> +
> + key = rcu_dereference(tx->sta->ptk[tx->sta->ptk_idx]);
> +
> + if (likely(sta->ptk_idx_next == INVALID_PTK_KEYIDX))
> + return key;
> +
> + /* Only when using Extended Key ID the code below can be executed */
> +
> + if (!ieee80211_is_data_present(hdr->frame_control))
> + return key;
> +
> + if (sta->ptk_idx_next == sta->ptk_idx) {
> + /* First packet using new key with A-MPDU active*/
> + sta->ptk_idx_next = INVALID_PTK_KEYIDX;
> + ieee80211_check_fast_xmit(tx->sta);
I'm not convinced you can call this from this context? It looks safe
though, but it's really strange in a way.
> + info->flags &= ~IEEE80211_TX_CTL_AMPDU;
Like you say above, I don't think this really makes a lot of sense. If
we don't have any free bits I guess we should try to find some ...
johannes
On Sun, 2019-02-10 at 22:06 +0100, Alexander Wetzel wrote:
> This is not ready for merge and has known issues.
> The patch is only for discussions to sort out how to handle it correctly!
>
> Signed-off-by: Alexander Wetzel <[email protected]>
> ---
>
> iwlwifi intel cards had two big surprises:
>
> Assuming I did not make some stupid errors it looks like my old
> "Intel Corporation Centrino Ultimate-N 6300 (rev 3e)" using ucode
> 9.221.4.1 build 25532 is perfectly fine with two keys uploaded to
> harware and honoring the keyid in the MPDUs. For a card launched 2011
> that's a pleasant surprise:-)
:-)
> A much shorter test with a modern "Intel Corporation Wireless 8265 /
> 8175 (rev 78)" using ucode version 36.e91976c0.0 shows what seems to be
> MPDUs decoded with the wrong key at each rekey and therefore a candidate
> for the COMPAT support only..
> So the bad news seems to be, that the modern card dropped the support.
Probably just a firmware bug.
> It also seems to force us to add some per-card or per-firmware depending
> check to decide which card can use the Native Extended Key ID support
> and use the Compat mode for the rest.
> Is there any way to find out which cards/firmware can be used with
> Extended Key ID?
No, but if you have a good test case we can check out what the firmware
bug is and fix it. Perhaps not for all, but for the future at least.
Maybe we can still figure out where it was introduced and thus see where
it's good to use native mode.
> I also have tested patch for iwldvm using the Compat mode and I think
> mvm cards will also work with that.
No they don't, no firmware is available for that.
johannes
On Sun, 2019-02-10 at 22:06 +0100, Alexander Wetzel wrote:
> When using Extended Key ID mac80211 drops @IEEE80211_TX_CTL_AMPDU for
> the last packet which can be added to a A-MPDU in preparation.
>
> Don't throw a warning and just handle the frame as if
> @IEEE80211_TX_CTL_AMPDU would have been set.
>
> Signed-off-by: Alexander Wetzel <[email protected]>
> ---
>
> I cold not figure out so far how to make sure the card will not mix
> A-MPDU frames. Looks like that is handled fully in the HW and I didn't
> find any interface to influence it. (It even may work already, I have
> some problems to capture A-MPDU frames with A-MPDU information intact
> and the topic was pretty low on the list so far. After all it works...)
You can't really do that, as far as I can tell, unfortunately. So this
might be better in a "compat on steroids" mode?
johannes
>> +/**
>> + * enum nl80211_key_install_mode - Key install mode
>> + *
>> + * @NL80211_KEY_RX_TX: Key must be installed for Rx and Tx
>> + * @NL80211_KEY_RX_ONLY: Allowed in combination with @NL80211_CMD_NEW_KEY:
>> + * Unicast key has to be installed for Rx only.
>> + * @NL80211_KEY_SWITCH_TX: Allowed in combination with @NL80211_CMD_SET_KEY:
>> + * Switch Tx to a Rx only, referenced by sta mac and idx.
>
> Don't you mean the other way around? Or, well, what you say is true for
> the *other* keys?
I don't see the problem but I may simply be to set on my way to see
it... So I'll just give an outline what's required of the API and how
this implementation meets those. Hoping that answers your question or
allowing you to pinpoint our differences in understanding. (I've added a
more than really required, it may be useful for other discussions to
have that outlined in one piece, too.)
Extended Key ID has only two requirements for the kernel API:
1) Allow a key to be used for decryption first and switch it to a
"normal" Rx/Tx key with a second call
2) Allow pairwise keys to use keyids 0 and 1 instead only 0.
"Legacy" key installs are using NL80211_CMD_NEW_KEY to install a new key
and are allow to mark a key for WEP or management default usages via
NL80211_CMD_SET_KEY later.
With Extended Key ID we stick to a similar approach: NL80211_CMD_NEW_KEY
is called to install the new key and nl80211_key_install_mode allows to
select between a "legacy" or "extended" key install: NL80211_KEY_RX_TX
for "legacy" or NL80211_KEY_RX_ONLY for "extended".
NL80211_KEY_SWITCH_TX is only allowed with NL80211_CMD_SET_KEY and is
the only Extended Key ID action allowed for that function.
Any non-pairwise keys can only use NL80211_KEY_RX_TX which is of course
always the default and also allowed for "legacy" pairwise keys.
A Extended Key Install will:
1) call NL80211_CMD_NEW_KEY with all the usual parameters of a new key
install plus the flag NL80211_KEY_RX_ONLY set.
2) Once ready will call NL80211_CMD_SET_KEY with flag
NL80211_KEY_SWITCH_TX to activate a specific key.
>> * by the %NL80211_SCAN_FLAG_MIN_PREQ_CONTENT flag.
>> * @NL80211_EXT_FEATURE_ENABLE_FTM_RESPONDER: Driver supports enabling fine
>> * timing measurement responder role.
>> - *
>
> no need to remove that :)
ok, I'll remove all the drive-by comment "cleanups".
It (often) looks kind of untidy and not really worth separate patches
and I'm kind of responsible for (most) of them.. I see why you don't
want to see those fixed drive-by...
My understanding is now that we prefer to not change those and I'll
leave them alone.
>
>> - /* only support setting default key */
>> - if (!key.def && !key.defmgmt)
>> + /* Only support setting default key and
>> + * Extended Key ID action @NL80211_KEY_SWITCH_TX.
>> + */
>
> you can remove the @, it's not a kernel-doc formatted comment
>
>> - }
>> + } else if (key.p.install_mode == NL80211_KEY_SWITCH_TX &&
>> + wiphy_ext_feature_isset(&rdev->wiphy,
>> + NL80211_EXT_FEATURE_EXT_KEY_ID)) {
>> + u8 *mac_addr = NULL;
>>
>> + if (info->attrs[NL80211_ATTR_MAC])
>> + mac_addr = nla_data(info->attrs[NL80211_ATTR_MAC]);
>> +
>> + if (!mac_addr || key.idx < 0 || key.idx > 1) {
>> + err = -EINVAL;
>> + goto out;
>> + }
>
> Really only 0 and 1 are allowed? Not 0-3?
Yes. Extended Key ID only allows to use Key ID 1 on top of the
established ID 0. See "IEEE 802.11 - 2016 9.4.2.25.4 RSN capabilities":
Bit 13: Extended Key ID for Individually Addressed Frames. This subfield
is set to 1 to indicate that the STA supports Key ID values in the range
0 to 1 for a PTKSA and STKSA when the cipher suite is CCMP or GCMP. A
value of 0 indicates that the STA only supports Key ID 0 for a PTKSA and
STKSA.
Or also "12.7.6.4 4-way handshake message 2":
If the Extended Key ID for Individually Addressed Frames subfield of the
RSN Capabilities field is 1 for both the Authenticator and the
Supplicant, then the Authenticator assigns a new Key ID for the PTKSA in
the range 0 to 1 that is different from the Key ID assigned in the
previous handshake and uses the MLME-SETKEYS.request primitive to
install the new key to receive individually addressed MPDUs protected by
the PTK with the assigned Key ID.
>
>> +++ b/net/wireless/util.c
>> @@ -236,14 +236,22 @@ int cfg80211_validate_key_settings(struct cfg80211_registered_device *rdev,
>> case WLAN_CIPHER_SUITE_CCMP_256:
>> case WLAN_CIPHER_SUITE_GCMP:
>> case WLAN_CIPHER_SUITE_GCMP_256:
>> - /* Disallow pairwise keys with non-zero index unless it's WEP
>> - * or a vendor specific cipher (because current deployments use
>> - * pairwise WEP keys with non-zero indices and for vendor
>> - * specific ciphers this should be validated in the driver or
>> - * hardware level - but 802.11i clearly specifies to use zero)
>> + /* IEEE802.11-2016 allows only 0 and - when using Extended Key
>> + * ID - 1 as index for pairwise keys.
>> + * @NL80211_KEY_RX_ONLY is only allowed for pairwise keys when
>> + * the driver supports Extended Key ID.
>> + * @NL80211_KEY_SWITCH_TX must not be set when validating a key.
>> */
>> - if (pairwise && key_idx)
>> + if (params->install_mode == NL80211_KEY_RX_ONLY) {
>> + if (!wiphy_ext_feature_isset(&rdev->wiphy,
>> + NL80211_EXT_FEATURE_EXT_KEY_ID))
>> + return -EINVAL;
>> + else if (!pairwise || key_idx < 0 || key_idx > 1)
>> + return -EINVAL;
>
> same question here
same answer with one remark: The original code did only allow id 0 and
and I made sure only with install mode NL80211_KEY_RX_ONLY is allowed to
use non-zero IDs.
Alexander
>>
>> - Enforce cipher does not change when replacing a key.
>
> is that actually required somehow?
The code is silently assuming a rekey is using the same cipher. Someone
e.g. switching from WEP to CCMP with a rekey would pass all sanity
checks and allow to use the code in a way never intended or tested.
With the current handling the userspace e.g. should be able to install a
WEB key using keyid 3 and then rekey it with a CCMP key, claiming keyid
0 bit mac80211 will copy the keyid from the old and use keyid 3.
Something not possible otherwise.
That said I do not see how this could be exploited, I simply try to
enforce all assumptions to be on the safe side. (I did not dig deeper
into potential exploits after finding out how keyids are used during rekey.)
>> + * @EXT_SET_KEY: a new key must be set but is only valid for decryption
>> + * @EXT_KEY_RX_TX: a key installed with @EXT_SET_KEY is becoming the
>> + * designated Rx/Tx key for the station
>
> Not sure I like the EXT_SET_KEY. There's also no "designated Rx key", is
> there? It's always selected by key ID.
>
> How about SET_KEY_RXONLY and SET_KEY_TX or something like that?
>
First, you are of course right about the designated Rx key. I'll update
that.
Second, I spend quite some time finding good names for the calls and one
of the last tweaks to this patch was replacing SET_KEY_RX_ONLY to
EXT_SET_KEY...
So here the reasoning for why I named them as they are and why I prefer
the names used in the patch.
First, many drivers will handle SET_KEY and SET_KEY_RX_ONLY with the
same code and not differentiate between those at all. Using EXT_as a
prefix for the "normal" command is therefore a nice way to imply the
command can only be used with Extended Key ID and still link it to the
original command.
But more important for me was the clash between what the command spells
and what it does in the COMPAT mode: SET_KEY_RX_ONLY would then be used
to install a TX only key which never can be used by the card for Rx.
So I decided to rename it to EXT_SET_KEY, just indicating that this
command adds a new key to the card for Extended Key ID and drop the
confusing reference to Rx.
I also had a comparable problem with SET_KEY_TX:
Most cards will already have done what must be done to use a key for Tx
with the first command. Only ath10k (assuming it could support Extended
Key ID at all) would really switch Tx with this command.
All other drivers seem to lookup the hw key ID and use whatever key is
referenced there. In fact I think most NATIVE drivers won't have to do
anything here and just can return 0.
Now COMPAT drivers (normally) will normally need a new special command
to enable Rx crypto offload for the new key. So we either would have to
add a new command for COMPAT drivers or accept that SET_KEY_TX is used
for that, again putting quite some stain an the name.
Using EXT_SET_KEY instead just implies that we are adding a new key for
Extended Key IDs and it's our first contact with this key.
EXT_KEY_RX_TX is then the second contact and has to do whatever is
necessary to switch the added but not yet fully activated key to fully
activated. That COPMPAT drivers will enable Rx with it while native
drivers do nothing or really switch Tx with the command.
Long story short: Using SET_KEY_RXONLY and SET_KEY_TX is not wrong, but
I would rate them more confusing.
>> +static int ieee80211_set_tx_key(struct ieee80211_sub_if_data *sdata,
>> + const u8 *mac_addr, u8 key_idx)
>> +{
>> + struct ieee80211_local *local = sdata->local;
>> + struct ieee80211_key *key;
>> + struct sta_info *sta;
>> + int ret;
>> +
>> + if (!wiphy_ext_feature_isset(local->hw.wiphy,
>> + NL80211_EXT_FEATURE_EXT_KEY_ID))
>> + return -EINVAL;
>
> You set this, wouldn't it make more sense to check EXT_KEY_ID_NATIVE?
>
> Or maybe this is because of the next patch?
Exactly. Assuming we merge NATIVE and drop COMPAT that would move down
to the driver.
>
>> + sta = sta_info_get_bss(sdata, mac_addr);
>> +
>> + if (!sta)
>> + return -EINVAL;
>> +
>> + if (sta->ptk_idx == key_idx)
>> + return 0;
>> +
>> + mutex_lock(&local->key_mtx);
>> + key = key_mtx_dereference(local, sta->ptk[key_idx]);
>> +
>> + if (key && key->flags & KEY_FLAG_RX_ONLY)
>
> do you even need the flag? Isn't it equivalent to checking
> sta->ptk_idx != key->idx
> or so?
>
> Less data to maintain would be better.
I have to look at that again. It will change some assumptions for sure
but still could work out with some slight differences. I'll have to look
deeper into that since I remember two moments where I was sure needing
the flag. That may well be outdated, but at a first glance it would at
least open the door to first install two key in legacy mode and then
switch between them. (Which should be no problem, of course)
I'll follow up on that separately, but that may take some time. When it
works you'll get a new RFC.
>
>> + bool ext_native = ieee80211_hw_check(&local->hw, EXT_KEY_ID_NATIVE);
>
> you sort of only need this in the next patch, but I guess it doesn't
> matter that much
>
Ups, yes. Forgot that when I split it in two patches some weeks ago,
>> +int ieee80211_key_activate_tx(struct ieee80211_key *key)
>> +{
>> + struct ieee80211_sub_if_data *sdata = key->sdata;
>> + struct sta_info *sta = key->sta;
>> + struct ieee80211_local *local = key->local;
>> + struct ieee80211_key *old;
>> + int ret;
>> +
>> + assert_key_lock(local);
>> +
>> + key->flags &= ~KEY_FLAG_RX_ONLY;
>> +
>> + if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) ||
>> + key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
>> + IEEE80211_KEY_FLAG_PUT_MIC_SPACE |
>> + IEEE80211_KEY_FLAG_RESERVE_TAILROOM))
>> + increment_tailroom_need_count(sdata);
>> +
>> + if (key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
>> + ret = drv_set_key(local, EXT_KEY_RX_TX, sdata,
>> + &sta->sta, &key->conf);
>> + if (ret) {
>> + sdata_err(sdata,
>> + "failed to activate key for Tx (%d, %pM)\n",
>> + key->conf.keyidx, sta->sta.addr);
>> + return ret;
>
> You've already cleared the RX_ONLY flag, which gets you inconsistent
> data now.
>
I don't think so, it looks ok for me. But the delay_tailroom logic took
a surprisingly large chunk of time and I should explain how the updated
logic is intended to work. Maybe I've messed it up somehow and just do
not see it:
I decided to handle that exact, no shortcuts. Only keys which can be
used for Tx will be counted for delay tailroom. So when installing a Rx
only key tailroom_needed will never be increased.
Prior to enabling Tx with a key the code above drops the RX_ONLY flag
and then evaluates if we have to call increment_tailroom_need_count. The
key is then activated for Tx a few lines below, the old key is set to
RX_ONLY and - assuming the old key also increased the tailroom needed
counter - decrements it for the old key.
(And I'm not sure if I should get that working without a dedicated key
flag, but let's wait and see how that would look like and then discuss it.)
The obvious simplification would be to just skip both steps and neither
increment nor decrement tailroom needed. Problem with that is, that the
HW offload decides during key install if the driver can support HW
offload or not. So it could be that e.g. the old key did use HW
encryption but the new will not. Handling that would further complicated
by the fact that a key install also could failand then would have to
clean up that again.
So I just update tailroom_needed as soon as possible, allowing to abort
any time and not worry about all that.
>> + }
>> + }
>> +
>> + old = key_mtx_dereference(local, sta->ptk[sta->ptk_idx]);
>> + sta->ptk_idx = key->conf.keyidx;
>
> but you set this only here.
>
>> - /* Stop TX till we are on the new key */
>> + /* Stop Tx till we are on the new key */
>
> Uh, I had to read that three times ... please don't make changes like
> that? :)
Noted, will drop all of those changes.
>
>> old_key->flags |= KEY_FLAG_TAINTED;
>> ieee80211_clear_fast_xmit(sta);
>>
>> - /* Aggregation sessions during rekey are complicated due to the
>> + /* Aggregation sessions during rekey are complicated by the
>
> similarly here, please don't make drive-by comment wording issues (also,
> I'm not sure I agree - the old version just treats "complicated" as an
> adjective, you treat it as a verb, but ultimately doesn't really matter?
>
>> #define NUM_DEFAULT_KEYS 4
>> #define NUM_DEFAULT_MGMT_KEYS 2
>> +#define INVALID_PTK_KEYIDX 2 /* Existing key slot never used by PTK keys */
>
> We could also use something obviously wrong like 0xff?
No, not without some undesired modifications. We actually fetch the
referenced key and the key slot must exist and be NULL. We (mostly)
discussed that in the previous RFC, I just decided to use a define
instead the numeric value. (Mostly due the fact that A-MPDU also needs
an "invalid" ID and using the same looks like a good idea.
Here how we use this #define in sta_info.c
/* Extended Key ID can install keys for keyid 0 and 1 as Rx only.
* Tx starts uses a key as soon as a key is installed in the slot
* ptk_idx references to. To avoid using the initial Rx key prematurely
* for Tx we initialize ptk_idx to a value never used, making sure the
* referenced key is always NULL till ptk_idx is set to a valid value.
*/
BUILD_BUG_ON(ARRAY_SIZE(sta->ptk) <= INVALID_PTK_KEYIDX);
sta->ptk_idx = INVALID_PTK_KEYIDX;
sta->ptk_idx_next = INVALID_PTK_KEYIDX;
>
>> +++ b/net/mac80211/tx.c
>> @@ -3000,23 +3000,15 @@ void ieee80211_check_fast_xmit(struct sta_info *sta)
>> switch (build.key->conf.cipher) {
>> case WLAN_CIPHER_SUITE_CCMP:
>> case WLAN_CIPHER_SUITE_CCMP_256:
>> - /* add fixed key ID */
>> - if (gen_iv) {
>> - (build.hdr + build.hdr_len)[3] =
>> - 0x20 | (build.key->conf.keyidx << 6);
>> + if (gen_iv)
>> build.pn_offs = build.hdr_len;
>> - }
>> if (gen_iv || iv_spc)
>> build.hdr_len += IEEE80211_CCMP_HDR_LEN;
>> break;
>> case WLAN_CIPHER_SUITE_GCMP:
>> case WLAN_CIPHER_SUITE_GCMP_256:
>> - /* add fixed key ID */
>> - if (gen_iv) {
>> - (build.hdr + build.hdr_len)[3] =
>> - 0x20 | (build.key->conf.keyidx << 6);
>> + if (gen_iv)
>> build.pn_offs = build.hdr_len;
>> - }
>> if (gen_iv || iv_spc)
>> build.hdr_len += IEEE80211_GCMP_HDR_LEN;
>> break;
>> @@ -3383,6 +3375,7 @@ static void ieee80211_xmit_fast_finish(struct ieee80211_sub_if_data *sdata,
>> pn = atomic64_inc_return(&key->conf.tx_pn);
>> crypto_hdr[0] = pn;
>> crypto_hdr[1] = pn >> 8;
>> + crypto_hdr[3] = 0x20 | (key->conf.keyidx << 6);
>> crypto_hdr[4] = pn >> 16;
>> crypto_hdr[5] = pn >> 24;
>> crypto_hdr[6] = pn >> 32;
>
> This shouldn't be needed, you do update the fast TX cache when changing
> the key?
That's only right for the push path but can send out wrong packets when
the driver is using the pull path:
1) ieee80211_xmit_fast() will use fast_tx structure to fill in the
"cached" keyid and queue the packet. (let's say 0)
2) ieee80211_check_fast_xmit is called due to a rekey (and keyid change
from 0 -> 1)
3) ieee80211_tx_dequeue() will then dequeue the prepared skb from 1),
refresh the key information but keep keyid 0 in the skb and instruct the
driver to encrypt it for keyid 1 -> WRONG
4) The remote sta tries to decrypt the packet using the key 0, as
referenced by the keyid. Which will of course not work.
With Extended Key ID (and some debugging) I added a simple rule: When
you assign the pn you also set the matching keyid.
Alexander
>>>
>>> - Enforce cipher does not change when replacing a key.
>>
>> is that actually required somehow?
>
> The code is silently assuming a rekey is using the same cipher. Someone
> e.g. switching from WEP to CCMP with a rekey would pass all sanity
> checks and allow to use the code in a way never intended or tested.
> With the current handling the userspace e.g. should be able to install a
> WEB key using keyid 3 and then rekey it with a CCMP key, claiming keyid
> 0 bit mac80211 will copy the keyid from the old and use keyid 3.
> Something not possible otherwise.
>
> That said I do not see how this could be exploited, I simply try to
> enforce all assumptions to be on the safe side. (I did not dig deeper
> into potential exploits after finding out how keyids are used during
> rekey.)
>
>
>>> + * @EXT_SET_KEY: a new key must be set but is only valid for decryption
>>> + * @EXT_KEY_RX_TX: a key installed with @EXT_SET_KEY is becoming the
>>> + * designated Rx/Tx key for the station
>>
>> Not sure I like the EXT_SET_KEY. There's also no "designated Rx key", is
>> there? It's always selected by key ID.
>>
>> How about SET_KEY_RXONLY and SET_KEY_TX or something like that?
>>
>
> First, you are of course right about the designated Rx key. I'll update
> that.
>
> Second, I spend quite some time finding good names for the calls and one
> of the last tweaks to this patch was replacing SET_KEY_RX_ONLY to
> EXT_SET_KEY...
>
> So here the reasoning for why I named them as they are and why I prefer
> the names used in the patch.
> First, many drivers will handle SET_KEY and SET_KEY_RX_ONLY with the
> same code and not differentiate between those at all. Using EXT_as a
> prefix for the "normal" command is therefore a nice way to imply the
> command can only be used with Extended Key ID and still link it to the
> original command.
> But more important for me was the clash between what the command spells
> and what it does in the COMPAT mode: SET_KEY_RX_ONLY would then be used
> to install a TX only key which never can be used by the card for Rx.
> So I decided to rename it to EXT_SET_KEY, just indicating that this
> command adds a new key to the card for Extended Key ID and drop the
> confusing reference to Rx.
>
> I also had a comparable problem with SET_KEY_TX:
> Most cards will already have done what must be done to use a key for Tx
> with the first command. Only ath10k (assuming it could support Extended
> Key ID at all) would really switch Tx with this command.
> All other drivers seem to lookup the hw key ID and use whatever key is
> referenced there. In fact I think most NATIVE drivers won't have to do
> anything here and just can return 0.
> Now COMPAT drivers (normally) will normally need a new special command
> to enable Rx crypto offload for the new key. So we either would have to
> add a new command for COMPAT drivers or accept that SET_KEY_TX is used
> for that, again putting quite some stain an the name.
>
> Using EXT_SET_KEY instead just implies that we are adding a new key for
> Extended Key IDs and it's our first contact with this key.
> EXT_KEY_RX_TX is then the second contact and has to do whatever is
> necessary to switch the added but not yet fully activated key to fully
> activated. That COPMPAT drivers will enable Rx with it while native
> drivers do nothing or really switch Tx with the command.
>
> Long story short: Using SET_KEY_RXONLY and SET_KEY_TX is not wrong, but
> I would rate them more confusing.
>
>>> +static int ieee80211_set_tx_key(struct ieee80211_sub_if_data *sdata,
>>> + const u8 *mac_addr, u8 key_idx)
>>> +{
>>> + struct ieee80211_local *local = sdata->local;
>>> + struct ieee80211_key *key;
>>> + struct sta_info *sta;
>>> + int ret;
>>> +
>>> + if (!wiphy_ext_feature_isset(local->hw.wiphy,
>>> + NL80211_EXT_FEATURE_EXT_KEY_ID))
>>> + return -EINVAL;
>>
>> You set this, wouldn't it make more sense to check EXT_KEY_ID_NATIVE?
>>
>> Or maybe this is because of the next patch?
>
> Exactly. Assuming we merge NATIVE and drop COMPAT that would move down
> to the driver.
My first answer for that on makes no sense, so let me try again:
Exactly. With COMPAT we would have to check for EXT_KEY_ID_NATIVE and
EXT_KEY_ID_COMPAT. Since we have already done that and set
NL80211_EXT_FEATURE_EXT_KEY_ID I check that here.
Assuming we merge NATIVE and drop COMPAT it may make sense to keep the
check here and drop EXT_KEY_ID_NATIVE altogether.
The drivers than can set NL80211_EXT_FEATURE_EXT_KEY_ID directly.
>
>>
>>> + sta = sta_info_get_bss(sdata, mac_addr);
>>> +
>>> + if (!sta)
>>> + return -EINVAL;
>>> +
>>> + if (sta->ptk_idx == key_idx)
>>> + return 0;
>>> +
>>> + mutex_lock(&local->key_mtx);
>>> + key = key_mtx_dereference(local, sta->ptk[key_idx]);
>>> +
>>> + if (key && key->flags & KEY_FLAG_RX_ONLY)
>>
>> do you even need the flag? Isn't it equivalent to checking
>> sta->ptk_idx != key->idx
>> or so?
>>
>> Less data to maintain would be better.
>
> I have to look at that again. It will change some assumptions for sure
> but still could work out with some slight differences. I'll have to look
> deeper into that since I remember two moments where I was sure needing
> the flag. That may well be outdated, but at a first glance it would at
> least open the door to first install two key in legacy mode and then
> switch between them. (Which should be no problem, of course)
> I'll follow up on that separately, but that may take some time. When it
> works you'll get a new RFC.
>
>>
>>> + bool ext_native = ieee80211_hw_check(&local->hw,
>>> EXT_KEY_ID_NATIVE);
>>
>> you sort of only need this in the next patch, but I guess it doesn't
>> matter that much
>>
> Ups, yes. Forgot that when I split it in two patches some weeks ago,
>
>>> +int ieee80211_key_activate_tx(struct ieee80211_key *key)
>>> +{
>>> + struct ieee80211_sub_if_data *sdata = key->sdata;
>>> + struct sta_info *sta = key->sta;
>>> + struct ieee80211_local *local = key->local;
>>> + struct ieee80211_key *old;
>>> + int ret;
>>> +
>>> + assert_key_lock(local);
>>> +
>>> + key->flags &= ~KEY_FLAG_RX_ONLY;
>>> +
>>> + if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) ||
>>> + key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
>>> + IEEE80211_KEY_FLAG_PUT_MIC_SPACE |
>>> + IEEE80211_KEY_FLAG_RESERVE_TAILROOM))
>>> + increment_tailroom_need_count(sdata);
>>> +
>>> + if (key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
>>> + ret = drv_set_key(local, EXT_KEY_RX_TX, sdata,
>>> + &sta->sta, &key->conf);
>>> + if (ret) {
>>> + sdata_err(sdata,
>>> + "failed to activate key for Tx (%d, %pM)\n",
>>> + key->conf.keyidx, sta->sta.addr);
>>> + return ret;
>>
>> You've already cleared the RX_ONLY flag, which gets you inconsistent
>> data now.
>>
>
> I don't think so, it looks ok for me. But the delay_tailroom logic took
> a surprisingly large chunk of time and I should explain how the updated
> logic is intended to work. Maybe I've messed it up somehow and just do
> not see it:
>
> I decided to handle that exact, no shortcuts. Only keys which can be
> used for Tx will be counted for delay tailroom. So when installing a Rx
> only key tailroom_needed will never be increased.
> Prior to enabling Tx with a key the code above drops the RX_ONLY flag
> and then evaluates if we have to call increment_tailroom_need_count. The
> key is then activated for Tx a few lines below, the old key is set to
> RX_ONLY and - assuming the old key also increased the tailroom needed
> counter - decrements it for the old key.
> (And I'm not sure if I should get that working without a dedicated key
> flag, but let's wait and see how that would look like and then discuss it.)
>
> The obvious simplification would be to just skip both steps and neither
> increment nor decrement tailroom needed. Problem with that is, that the
> HW offload decides during key install if the driver can support HW
> offload or not. So it could be that e.g. the old key did use HW
> encryption but the new will not. Handling that would further complicated
> by the fact that a key install also could failand then would have to
> clean up that again.
> So I just update tailroom_needed as soon as possible, allowing to abort
> any time and not worry about all that.
>
>>> + }
>>> + }
>>> +
>>> + old = key_mtx_dereference(local, sta->ptk[sta->ptk_idx]);
>>> + sta->ptk_idx = key->conf.keyidx;
>>
>> but you set this only here.
>>
>>> - /* Stop TX till we are on the new key */
>>> + /* Stop Tx till we are on the new key */
>>
>> Uh, I had to read that three times ... please don't make changes like
>> that? :)
>
> Noted, will drop all of those changes.
>
>>
>>> old_key->flags |= KEY_FLAG_TAINTED;
>>> ieee80211_clear_fast_xmit(sta);
>>> - /* Aggregation sessions during rekey are complicated due to the
>>> + /* Aggregation sessions during rekey are complicated by the
>>
>> similarly here, please don't make drive-by comment wording issues (also,
>> I'm not sure I agree - the old version just treats "complicated" as an
>> adjective, you treat it as a verb, but ultimately doesn't really matter?
>>
>>> #define NUM_DEFAULT_KEYS 4
>>> #define NUM_DEFAULT_MGMT_KEYS 2
>>> +#define INVALID_PTK_KEYIDX 2 /* Existing key slot never used by PTK
>>> keys */
>>
>> We could also use something obviously wrong like 0xff?
>
> No, not without some undesired modifications. We actually fetch the
> referenced key and the key slot must exist and be NULL. We (mostly)
> discussed that in the previous RFC, I just decided to use a define
> instead the numeric value. (Mostly due the fact that A-MPDU also needs
> an "invalid" ID and using the same looks like a good idea.
>
> Here how we use this #define in sta_info.c
>
> /* Extended Key ID can install keys for keyid 0 and 1 as Rx only.
> * Tx starts uses a key as soon as a key is installed in the slot
> * ptk_idx references to. To avoid using the initial Rx key prematurely
> * for Tx we initialize ptk_idx to a value never used, making sure the
> * referenced key is always NULL till ptk_idx is set to a valid value.
> */
> BUILD_BUG_ON(ARRAY_SIZE(sta->ptk) <= INVALID_PTK_KEYIDX);
> sta->ptk_idx = INVALID_PTK_KEYIDX;
> sta->ptk_idx_next = INVALID_PTK_KEYIDX;
>
>>
>>> +++ b/net/mac80211/tx.c
>>> @@ -3000,23 +3000,15 @@ void ieee80211_check_fast_xmit(struct
>>> sta_info *sta)
>>> switch (build.key->conf.cipher) {
>>> case WLAN_CIPHER_SUITE_CCMP:
>>> case WLAN_CIPHER_SUITE_CCMP_256:
>>> - /* add fixed key ID */
>>> - if (gen_iv) {
>>> - (build.hdr + build.hdr_len)[3] =
>>> - 0x20 | (build.key->conf.keyidx << 6);
>>> + if (gen_iv)
>>> build.pn_offs = build.hdr_len;
>>> - }
>>> if (gen_iv || iv_spc)
>>> build.hdr_len += IEEE80211_CCMP_HDR_LEN;
>>> break;
>>> case WLAN_CIPHER_SUITE_GCMP:
>>> case WLAN_CIPHER_SUITE_GCMP_256:
>>> - /* add fixed key ID */
>>> - if (gen_iv) {
>>> - (build.hdr + build.hdr_len)[3] =
>>> - 0x20 | (build.key->conf.keyidx << 6);
>>> + if (gen_iv)
>>> build.pn_offs = build.hdr_len;
>>> - }
>>> if (gen_iv || iv_spc)
>>> build.hdr_len += IEEE80211_GCMP_HDR_LEN;
>>> break;
>>> @@ -3383,6 +3375,7 @@ static void ieee80211_xmit_fast_finish(struct
>>> ieee80211_sub_if_data *sdata,
>>> pn = atomic64_inc_return(&key->conf.tx_pn);
>>> crypto_hdr[0] = pn;
>>> crypto_hdr[1] = pn >> 8;
>>> + crypto_hdr[3] = 0x20 | (key->conf.keyidx << 6);
>>> crypto_hdr[4] = pn >> 16;
>>> crypto_hdr[5] = pn >> 24;
>>> crypto_hdr[6] = pn >> 32;
>>
>> This shouldn't be needed, you do update the fast TX cache when changing
>> the key?
>
> That's only right for the push path but can send out wrong packets when
> the driver is using the pull path:
>
> 1) ieee80211_xmit_fast() will use fast_tx structure to fill in the
> "cached" keyid and queue the packet. (let's say 0)
>
> 2) ieee80211_check_fast_xmit is called due to a rekey (and keyid change
> from 0 -> 1)
>
> 3) ieee80211_tx_dequeue() will then dequeue the prepared skb from 1),
> refresh the key information but keep keyid 0 in the skb and instruct the
> driver to encrypt it for keyid 1 -> WRONG
>
> 4) The remote sta tries to decrypt the packet using the key 0, as
> referenced by the keyid. Which will of course not work.
>
> With Extended Key ID (and some debugging) I added a simple rule: When
> you assign the pn you also set the matching keyid.
>
> Alexander
>
>> + if (!ext_native && key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
>> + key->flags |= KEY_FLAG_RX_SW_CRYPTO;
>> + /* Activate Rx crypto offload after max 10s when idle */
>> + ieee80211_queue_delayed_work(&local->hw, &sta->ext_key_compat_wk,
>> + round_jiffies_relative(HZ * 10));
>> + }
>
> Is there much point in this?
>
>> + if (unlikely(rx->key->flags & KEY_FLAG_RX_SW_CRYPTO)) {
>> + rx->key->flags &= ~KEY_FLAG_RX_SW_CRYPTO;
>> + cancel_delayed_work(&rx->sta->ext_key_compat_wk);
>> + ieee80211_queue_delayed_work(&rx->local->hw,
>> + &rx->sta->ext_key_compat_wk, 0);
>> + }
>
> We'll almost certainly do it from here, so never exercise the other
> path?
This is mostly to have a definite time we know the new key is used also
for RX. In probably 99.9% of all cases it will be triggered from the Rx
path.
Some special purpose devices may not send any packets for a long time
and trigger the fallback, as (wrong) firewall rules. (I've e.g. tested
it by dropping all outgoing packets on the remote sta.)
The idea was to be sure that a rekey intervall >10s prevents activating
Rx crypt when rekeying the next key. Which now sounds kind of thin...
So I'll remove the 10s fallback.
Am 15.02.19 um 12:50 schrieb Johannes Berg:
>
>> The intent of the wording was probably written without considering
>> Extended Key IDs. At least it makes no sense for me to forbid mixing
>> MPDUs using keyid 0 and 1 in one A-MPDU.
>
> I think it may make sense - reprogramming the hardware engines may take
> some time, and doing that in the middle of the A-MPDU may not be
> feasible? You don't just have to load the key (that you need to do
> anyway) but also extract the status? I dunno, I'm more handwaving, but
> it doesn't make sense to add such a requirement when only one key index
> can be used to start with.
>
I'm pretty new to all that and I know I have still huge gaps everywhere.
But the card must be able to process MPDUs using both KeyIDs at that
moment already. When receiving a A-MPDU it should not be very hard to
check each MPDU and process it accordingly. (TX should even be simpler:
The driver simply can decide if he want's to have a key border in the
A-MPDU or not and switch to key when it wants.)
Forbidding to mix unicast with both keyIDs is adding an ugly overhead to
an otherwise quite simple solution and adds complexity, at least for us
here.
So while I would like to understand the reason for that rule it's still
a rule and breaking it seems to be a bad idea.
>> The code is assuming that the driver is not aggregating MPDUs more than
>> 5s apart. We probably don't have wait nearly so long but I'm not sure
>> what is the minimum time.
>
> OTOH, if you have a lot of BE/VI/VO traffic BK might be starved even
> longer than that, technically indefinitely.
>
Hm, there is nothing preventing us to drop this "idle" switch as long as
we also drop the 10s Rx accel offload when idle fallback in the COMPAT
patch. (We have to drop the RX idle accel patch or get a small risk of
dropping packets in unlikely but possible scenarios. If that are eapol
packets things will become hairy, probably disconnecting the sta.)
It just feels strange to potentially still use the old key for one
packet more without time limit. It could be, that the first EAPOL packet
we send for the next rekey would still use the previous key, the second
eapol packet the current.
>> +static struct ieee80211_key debug_noinline
>> +*ieee80211_select_sta_key(struct ieee80211_tx_data *tx)
>> +{
>> + struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
>> + struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
>> + struct sta_info *sta = tx->sta;
>> + struct ieee80211_key *key;
>> + struct ieee80211_key *next_key;
>> +
>> + key = rcu_dereference(tx->sta->ptk[tx->sta->ptk_idx]);
>> +
>> + if (likely(sta->ptk_idx_next == INVALID_PTK_KEYIDX))
>> + return key;
>> +
>> + /* Only when using Extended Key ID the code below can be executed */
>> +
>> + if (!ieee80211_is_data_present(hdr->frame_control))
>> + return key;
>> +
>> + if (sta->ptk_idx_next == sta->ptk_idx) {
>> + /* First packet using new key with A-MPDU active*/
>> + sta->ptk_idx_next = INVALID_PTK_KEYIDX;
>> + ieee80211_check_fast_xmit(tx->sta);
>
> I'm not convinced you can call this from this context? It looks safe
> though, but it's really strange in a way.
>
Well, it's seems to work fine, no warnings or problems so far:-)
But I also had doubts and only after finding out that
ieee80211_check_fast_xmit() is already being called from the same
context I dared to use it, see ieee80211_tx_prepare().
>> + info->flags &= ~IEEE80211_TX_CTL_AMPDU;
>
> Like you say above, I don't think this really makes a lot of sense. If
> we don't have any free bits I guess we should try to find some ...
Well, all I can think of is quite invasive, so I hoped you would have a
better idea:
Move the flags out from CB and use the freed space for a pointer to the
new construct.
That will touch a ton of code and slow down things a bit.
The question here also would be, if we should use a struct able to
handle other extensions or just a long long for flags.
If it comes to that I would propose to merge the other patches up to
this one and then start looking into that...
Alexander
Am 15.02.19 um 12:10 schrieb Johannes Berg:
> On Sun, 2019-02-10 at 22:06 +0100, Alexander Wetzel wrote:
>>
>> The driver patches are - with the exception of the hwsim patch -
>> definitely not ready for merge and mostly here to illustrate how the
>> different APIs can be used and to start some discussions how to handle HW
>> specific challenges. Of course if someone wants to play with Extended Key
>> ID they also should be useful... (I can provide updated mostly working
>> hostapd/wpa_supplicant patches if someone is interested.
>
> Of course :-)
I've just upload my current hostapd patches here:
https://www.awhome.eu/index.php/s/FZx68eGMGbwf6EK
These are slightly updated versions of the patches I send to the hostapd
mailing list used in my tests. I'm using them since months in mixed
environments with and without Extennded Key ID support.
Once the nl80211 API stabilizes I'll plan to polish them, fill in the
known gaps and try to get them merged.
>
> Some tests for the hwsim tests there would also be nice, that's the
> easiest way to see something working - if you have them.
I did run the the existing PTK tests but proper tests for Extended Key
ID are the biggest open topic. I suspect proper tests will the most
complicated open task here.
The others are:
- No support for Extended Key ID in mesh mode
- Update to new kernel API (the one we extended for Extended KEY ID)
needs some more work.
- Make sure we do not install the keys twice when repeating either EAPOL
3 or 4 (I think one of them was ok, but I did not look at that for
ages... It's still working fine, mac80211 detects a duplicate key
install and just reports success.)
Alexander
On Sun, 2019-02-17 at 20:19 +0100, Alexander Wetzel wrote:
> I don't see the problem but I may simply be to set on my way to see
> it... So I'll just give an outline what's required of the API and how
> this implementation meets those. Hoping that answers your question or
> allowing you to pinpoint our differences in understanding. (I've added a
> more than really required, it may be useful for other discussions to
> have that outlined in one piece, too.)
:-)
> Extended Key ID has only two requirements for the kernel API:
> 1) Allow a key to be used for decryption first and switch it to a
> "normal" Rx/Tx key with a second call
>
> 2) Allow pairwise keys to use keyids 0 and 1 instead only 0.
Right.
> "Legacy" key installs are using NL80211_CMD_NEW_KEY to install a new key
> and are allow to mark a key for WEP or management default usages via
> NL80211_CMD_SET_KEY later.
Right.
> With Extended Key ID we stick to a similar approach: NL80211_CMD_NEW_KEY
> is called to install the new key and nl80211_key_install_mode allows to
> select between a "legacy" or "extended" key install: NL80211_KEY_RX_TX
> for "legacy" or NL80211_KEY_RX_ONLY for "extended".
Ah. That's where the "EXT" came from in the mac80211 names :-) FWIW, I'm
not sure that makes sense. Yes, we think of it as an "extension" or
being "extended" now while we work on it, but ultimately it'll be a
simple part of the API. But I digress.
> NL80211_KEY_SWITCH_TX is only allowed with NL80211_CMD_SET_KEY and is
> the only Extended Key ID action allowed for that function.
Yes, but what does it *do*? Your documentation said:
Switch Tx to a Rx only, referenced by sta mac and idx.
but that seems backwards to me based on the above requirement:
Allow a key to be used for decryption first and switch it to a
"normal" Rx/Tx key with a second call.
IOW, you said we need to have the ability to first install RX-only, and
then switch the key to RX & TX. I'd have called this "ENABLE_TX" or
something like that. Or perhaps SWITCH_(ON_)TX if you're so inclined,
but the documentation should then say
Switch key from RX-only to TX/RX, referenced by ...
no? That's what I meant by "the other way around".
> Any non-pairwise keys can only use NL80211_KEY_RX_TX which is of course
> always the default and also allowed for "legacy" pairwise keys.
Right.
> A Extended Key Install will:
> 1) call NL80211_CMD_NEW_KEY with all the usual parameters of a new key
> install plus the flag NL80211_KEY_RX_ONLY set.
So far makes sense.
> 2) Once ready will call NL80211_CMD_SET_KEY with flag
> NL80211_KEY_SWITCH_TX to activate a specific key.
Also makes sense, but the documentation doesn't.
I think we should rename these perhaps
NL80211_KEY_RX_TX - install key and enable RX/TX (default)
NL80211_KEY_RX_ONLY - install unicast key for RX only
NL80211_KEY_ENABLE_TX - enable TX for a previously installed
RX_ONLY key
I think?
About the ENABLE_TX vs. SWITCH_TX - we don't allow to switch TX *off*
again, only *on*, so I think "enable" makes more sense than "switch".
Anyway, my whole comment was only about the documentation text which
seemed backward or at least unclear to me.
> ok, I'll remove all the drive-by comment "cleanups".
> It (often) looks kind of untidy and not really worth separate patches
> and I'm kind of responsible for (most) of them.. I see why you don't
> want to see those fixed drive-by...
>
> My understanding is now that we prefer to not change those and I'll
> leave them alone.
I have no objection to even the most trivial cleanup patches going in
separately, and if you like multiple in a bigger "clean up this area"
patch, but here I think it detracts from the patch.
> Yes. Extended Key ID only allows to use Key ID 1 on top of the
> established ID 0. See "IEEE 802.11 - 2016 9.4.2.25.4 RSN capabilities":
>
> Bit 13: Extended Key ID for Individually Addressed Frames. This subfield
> is set to 1 to indicate that the STA supports Key ID values in the range
> 0 to 1 for a PTKSA and STKSA when the cipher suite is CCMP or GCMP. A
> value of 0 indicates that the STA only supports Key ID 0 for a PTKSA and
> STKSA.
OK :)
johannes
On Thu, 2019-02-21 at 20:47 +0100, Alexander Wetzel wrote:
> > > >
> > > > - Enforce cipher does not change when replacing a key.
> > >
> > > is that actually required somehow?
> >
> > The code is silently assuming a rekey is using the same cipher. Someone
> > e.g. switching from WEP to CCMP with a rekey would pass all sanity
> > checks and allow to use the code in a way never intended or tested.
> > With the current handling the userspace e.g. should be able to install a
> > WEB key using keyid 3 and then rekey it with a CCMP key, claiming keyid
> > 0 bit mac80211 will copy the keyid from the old and use keyid 3.
> > Something not possible otherwise.
> >
> > That said I do not see how this could be exploited, I simply try to
> > enforce all assumptions to be on the safe side. (I did not dig deeper
> > into potential exploits after finding out how keyids are used during
> > rekey.)
Ok. That's fair, but it'd be good to add that reasoning to the commit
log or even a comment where the validation happens.
> > Second, I spend quite some time finding good names for the calls and one
> > of the last tweaks to this patch was replacing SET_KEY_RX_ONLY to
> > EXT_SET_KEY...
:)
> > So here the reasoning for why I named them as they are and why I prefer
> > the names used in the patch.
> > First, many drivers will handle SET_KEY and SET_KEY_RX_ONLY with the
> > same code and not differentiate between those at all. Using EXT_as a
> > prefix for the "normal" command is therefore a nice way to imply the
> > command can only be used with Extended Key ID and still link it to the
> > original command.
> > But more important for me was the clash between what the command spells
> > and what it does in the COMPAT mode: SET_KEY_RX_ONLY would then be used
> > to install a TX only key which never can be used by the card for Rx.
> > So I decided to rename it to EXT_SET_KEY, just indicating that this
> > command adds a new key to the card for Extended Key ID and drop the
> > confusing reference to Rx.
Wait, what? In compat mode SET_KEY_RX_ONLY installs a TX-only key? Ah,
you mean before you changed this.
Why don't we split out compat mode then?
But I see where you're coming from with the EXT_ now. I need to think of
it less as an "extension" now, but as "extended key ID". I'm not really
entirely sure that makes sense - even what we think of as "extended key
ID" now might be the new normal soon? But then again the spec does the
same thing.
> > Long story short: Using SET_KEY_RXONLY and SET_KEY_TX is not wrong, but
> > I would rate them more confusing.
Fair enough.
> > > > +static int ieee80211_set_tx_key(struct ieee80211_sub_if_data *sdata,
> > > > + const u8 *mac_addr, u8 key_idx)
> > > > +{
> > > > + struct ieee80211_local *local = sdata->local;
> > > > + struct ieee80211_key *key;
> > > > + struct sta_info *sta;
> > > > + int ret;
> > > > +
> > > > + if (!wiphy_ext_feature_isset(local->hw.wiphy,
> > > > + NL80211_EXT_FEATURE_EXT_KEY_ID))
> > > > + return -EINVAL;
> > >
> > > You set this, wouldn't it make more sense to check EXT_KEY_ID_NATIVE?
> > >
> > > Or maybe this is because of the next patch?
> Exactly. With COMPAT we would have to check for EXT_KEY_ID_NATIVE and
> EXT_KEY_ID_COMPAT. Since we have already done that and set
> NL80211_EXT_FEATURE_EXT_KEY_ID I check that here.
> Assuming we merge NATIVE and drop COMPAT it may make sense to keep the
> check here and drop EXT_KEY_ID_NATIVE altogether.
> The drivers than can set NL80211_EXT_FEATURE_EXT_KEY_ID directly.
Yes, I think I get it. I don't think we need to drop compat, it seems
somewhat useful for some drivers - I think I mostly just got confused
here because compat support only comes later, and I wasn't really
thinking about it yet in the context of this patch.
> > I have to look at that again. It will change some assumptions for sure
> > but still could work out with some slight differences. I'll have to look
> > deeper into that since I remember two moments where I was sure needing
> > the flag. That may well be outdated, but at a first glance it would at
> > least open the door to first install two key in legacy mode and then
> > switch between them. (Which should be no problem, of course)
> > I'll follow up on that separately, but that may take some time. When it
> > works you'll get a new RFC.
No worries :-)
I don't really mind the flag, but much of the use I've seen now here
seemed equivalent, and then it doesn't seem necessary.
> > > > + key->flags &= ~KEY_FLAG_RX_ONLY;
[snip]
> > > > + if (key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
> > > > + ret = drv_set_key(local, EXT_KEY_RX_TX, sdata,
> > > > + &sta->sta, &key->conf);
> > > > + if (ret) {
> > > > + sdata_err(sdata,
> > > > + "failed to activate key for Tx (%d, %pM)\n",
> > > > + key->conf.keyidx, sta->sta.addr);
> > > > + return ret;
> > >
> > > You've already cleared the RX_ONLY flag, which gets you inconsistent
> > > data now.
> > >
> >
> > I don't think so, it looks ok for me. But the delay_tailroom logic took
> > a surprisingly large chunk of time and I should explain how the updated
> > logic is intended to work. Maybe I've messed it up somehow and just do
> > not see it:
My comment wasn't related to tailroom accounting at all. I've snipped
more code above to hopefully make it clearer.
If you fail to activate the key in the driver, then the key is not
actually enabled for TX, and thus you should not have cleared the
KEY_FLAG_RX_ONLY? You'll be returning this error all the way to
userspace, I assume.
(Maybe, btw, the driver should be allowed to return something like
"oops, I now want to use TX software crypto" like it can do for other
operations?)
> > > > #define NUM_DEFAULT_KEYS 4
> > > > #define NUM_DEFAULT_MGMT_KEYS 2
> > > > +#define INVALID_PTK_KEYIDX 2 /* Existing key slot never used by PTK
> > > > keys */
> > >
> > > We could also use something obviously wrong like 0xff?
> >
> > No, not without some undesired modifications. We actually fetch the
> > referenced key and the key slot must exist and be NULL. We (mostly)
> > discussed that in the previous RFC, I just decided to use a define
> > instead the numeric value. (Mostly due the fact that A-MPDU also needs
> > an "invalid" ID and using the same looks like a good idea.
Hm, yeah, I vaguely remember. OK.
> > That's only right for the push path but can send out wrong packets when
> > the driver is using the pull path:
> >
> > 1) ieee80211_xmit_fast() will use fast_tx structure to fill in the
> > "cached" keyid and queue the packet. (let's say 0)
> >
> > 2) ieee80211_check_fast_xmit is called due to a rekey (and keyid change
> > from 0 -> 1)
> >
> > 3) ieee80211_tx_dequeue() will then dequeue the prepared skb from 1),
> > refresh the key information but keep keyid 0 in the skb and instruct the
> > driver to encrypt it for keyid 1 -> WRONG
> >
> > 4) The remote sta tries to decrypt the packet using the key 0, as
> > referenced by the keyid. Which will of course not work.
> >
> > With Extended Key ID (and some debugging) I added a simple rule: When
> > you assign the pn you also set the matching keyid.
OK.
johannes
On Thu, 2019-02-21 at 22:20 +0100, Alexander Wetzel wrote:
> > I think it may make sense - reprogramming the hardware engines may take
> > some time, and doing that in the middle of the A-MPDU may not be
> > feasible? You don't just have to load the key (that you need to do
> > anyway) but also extract the status? I dunno, I'm more handwaving, but
> > it doesn't make sense to add such a requirement when only one key index
> > can be used to start with.
> >
>
> I'm pretty new to all that and I know I have still huge gaps everywhere.
And I'm just handwaving ;-)
But I know that for example we have A-MPDU spacing rules, ie. sometimes
padding must be inserted by the transmitter to give the receiver's HW or
FW enough time to program crypto engines. It thus stands to reason that
in order to minimise the spacing you'd want to keep the key material at
hand in the engine while processing the whole A-MPDU.
> But the card must be able to process MPDUs using both KeyIDs at that
> moment already. When receiving a A-MPDU it should not be very hard to
> check each MPDU and process it accordingly. (TX should even be simpler:
> The driver simply can decide if he want's to have a key border in the
> A-MPDU or not and switch to key when it wants.)
I tend to agree, but you never know in what surprising ways hardware
engines work :-)
> > > The code is assuming that the driver is not aggregating MPDUs more than
> > > 5s apart. We probably don't have wait nearly so long but I'm not sure
> > > what is the minimum time.
> >
> > OTOH, if you have a lot of BE/VI/VO traffic BK might be starved even
> > longer than that, technically indefinitely.
> >
>
> Hm, there is nothing preventing us to drop this "idle" switch as long as
> we also drop the 10s Rx accel offload when idle fallback in the COMPAT
> patch. (We have to drop the RX idle accel patch or get a small risk of
> dropping packets in unlikely but possible scenarios. If that are eapol
> packets things will become hairy, probably disconnecting the sta.)
>
> It just feels strange to potentially still use the old key for one
> packet more without time limit. It could be, that the first EAPOL packet
> we send for the next rekey would still use the previous key, the second
> eapol packet the current.
Not sure I understand this. If we have no TX going on at all, then
surely we can switch with the next packet, before we encrypt it?
And if we have a packet sitting on hardware queues forever, then we
can't do anything about it anyway?
> > > + if (sta->ptk_idx_next == sta->ptk_idx) {
> > > + /* First packet using new key with A-MPDU active*/
> > > + sta->ptk_idx_next = INVALID_PTK_KEYIDX;
> > > + ieee80211_check_fast_xmit(tx->sta);
> >
> > I'm not convinced you can call this from this context? It looks safe
> > though, but it's really strange in a way.
> >
>
> Well, it's seems to work fine, no warnings or problems so far:-)
>
> But I also had doubts and only after finding out that
> ieee80211_check_fast_xmit() is already being called from the same
> context I dared to use it, see ieee80211_tx_prepare().
OK :-)
I guess I misremembered then.
> > > + info->flags &= ~IEEE80211_TX_CTL_AMPDU;
> >
> > Like you say above, I don't think this really makes a lot of sense. If
> > we don't have any free bits I guess we should try to find some ...
>
> Well, all I can think of is quite invasive, so I hoped you would have a
> better idea:
I think we have free bits in enum mac80211_tx_control_flags, so that
should be workable? They won't be preserved until TX status, but that's
OK for this purpose, no?
johannes
On Thu, 2019-02-21 at 21:07 +0100, Alexander Wetzel wrote:
> >
> > > + if (!ext_native && key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
> > > + key->flags |= KEY_FLAG_RX_SW_CRYPTO;
> > > + /* Activate Rx crypto offload after max 10s when idle */
> > > + ieee80211_queue_delayed_work(&local->hw, &sta->ext_key_compat_wk,
> > > + round_jiffies_relative(HZ * 10));
> > > + }
> >
> > Is there much point in this?
> >
> > > + if (unlikely(rx->key->flags & KEY_FLAG_RX_SW_CRYPTO)) {
> > > + rx->key->flags &= ~KEY_FLAG_RX_SW_CRYPTO;
> > > + cancel_delayed_work(&rx->sta->ext_key_compat_wk);
> > > + ieee80211_queue_delayed_work(&rx->local->hw,
> > > + &rx->sta->ext_key_compat_wk, 0);
> > > + }
> >
> > We'll almost certainly do it from here, so never exercise the other
> > path?
>
> This is mostly to have a definite time we know the new key is used also
> for RX. In probably 99.9% of all cases it will be triggered from the Rx
> path.
> Some special purpose devices may not send any packets for a long time
> and trigger the fallback, as (wrong) firewall rules. (I've e.g. tested
> it by dropping all outgoing packets on the remote sta.)
>
> The idea was to be sure that a rekey intervall >10s prevents activating
> Rx crypt when rekeying the next key. Which now sounds kind of thin...
Not sure I even understand this?
You meant "Rx crypto offload"? I'm not really sure we _care_ that much?
Then again, an issue may be that some firmware may want (need) the keys
for RX so it can look at certain frames (action frames?) itself. So if
we never install the RX key and then only get an action frame that the
firmware should handle, we lose. Such firmware could not support COMPAT
mode then I guess, which may mean a bunch of iwlwifi devices shouldn't
use COMPAT mode.
johannes
On Fri, 2019-02-22 at 21:50 +0100, Alexander Wetzel wrote:
> > No, but if you have a good test case we can check out what the firmware
> > bug is and fix it. Perhaps not for all, but for the future at least.
> > Maybe we can still figure out where it was introduced and thus see where
> > it's good to use native mode.
>
> I'll verify if can reproduce the scrambled packets and will provide a
> capture if so. Assuming that confirms the initial finding I'll be able
> to reproduce that at will within minutes with access to a test system
> having a mvm card. (I have some plans which will improve access, but
> looks like that will take some time and efforts.)
>
> For now I handle that as low prio till we have generic Extended Key ID
> support merged
Agree
> and I've had some time to improve my test setup and
> hopefully have better access to a mvm card for testing.
If all that's lacking is a few NICs then I'm sure we/I can help out with
that :)
> > > I also have tested patch for iwldvm using the Compat mode and I think
> > > mvm cards will also work with that.
> >
> > No they don't, no firmware is available for that.
>
> So far I only looked at the dvm part of iwlwifi with only minutes spend
> on mvm to port the NATIVE solution from dvm.
:)
> Are you saying that mvm cards can't seamless switch a RX/TX key to TX
> only one? mvm seems to support SW crypto as needed and switching RX/TX
> to TX keys is the only other requirement for COMPAT mode.
No, sorry. I misread your statement. I thought you were saying "mvm
cards will also work with iwldvm" rather than "mvm cards will also work
with compat mode" :-)
I think they all should just be made to work in native mode, the
firmware basically supports this as you found, there must be some small
bugs.
johannes
On Fri, 2019-02-22 at 22:15 +0100, Alexander Wetzel wrote:
> I hoped that the HW would already enforce the key border.
I don't think so.
> But ok: The
> cards will still be able to use NATIVE mode. Only now the driver will
> have to request to disable Tx aggregation when finding a key border.
It doesn't really have to even completely disable it, it'd just have to
send and LQ_CMD with agg_params.agg_frame_cnt_limit = 1, temporarily,
until the relevant frames were transmitted (this is a global config, so
it'd have to track the key border on each TID, but that's not _too_
hard).
> Since Tx aggregation is disabled per default anyhow many users will not
> even see a difference.
Good point, I forgot all about that.
> (Off topic here, but I suspect the not reproducible problems leading to
> change the Tx aggregation default to off could also have been ptk0 rekey
> freezes. In that case we could now allow it again. But I've not found
> raw error reports and guess it's not worth the efforts/risks to change
> the default again.)
I wouldn't be so sure - PTK rekeying should still be a pretty uncommon
configuration. But I also don't recall the exact decision making going
into this.
johannes
Am 15.02.19 um 12:52 schrieb Johannes Berg:
> On Sun, 2019-02-10 at 22:06 +0100, Alexander Wetzel wrote:
>> This is not ready for merge and has known issues.
>> The patch is only for discussions to sort out how to handle it correctly!
>>
>> Signed-off-by: Alexander Wetzel <[email protected]>
>> ---
>>
>> iwlwifi intel cards had two big surprises:
>>
>> Assuming I did not make some stupid errors it looks like my old
>> "Intel Corporation Centrino Ultimate-N 6300 (rev 3e)" using ucode
>> 9.221.4.1 build 25532 is perfectly fine with two keys uploaded to
>> harware and honoring the keyid in the MPDUs. For a card launched 2011
>> that's a pleasant surprise:-)
>
> :-)
>
>> A much shorter test with a modern "Intel Corporation Wireless 8265 /
>> 8175 (rev 78)" using ucode version 36.e91976c0.0 shows what seems to be
>> MPDUs decoded with the wrong key at each rekey and therefore a candidate
>> for the COMPAT support only..
>> So the bad news seems to be, that the modern card dropped the support.
>
> Probably just a firmware bug.
>
>> It also seems to force us to add some per-card or per-firmware depending
>> check to decide which card can use the Native Extended Key ID support
>> and use the Compat mode for the rest.
>> Is there any way to find out which cards/firmware can be used with
>> Extended Key ID?
>
> No, but if you have a good test case we can check out what the firmware
> bug is and fix it. Perhaps not for all, but for the future at least.
> Maybe we can still figure out where it was introduced and thus see where
> it's good to use native mode.
I'll verify if can reproduce the scrambled packets and will provide a
capture if so. Assuming that confirms the initial finding I'll be able
to reproduce that at will within minutes with access to a test system
having a mvm card. (I have some plans which will improve access, but
looks like that will take some time and efforts.)
For now I handle that as low prio till we have generic Extended Key ID
support merged and I've had some time to improve my test setup and
hopefully have better access to a mvm card for testing.
>
>> I also have tested patch for iwldvm using the Compat mode and I think
>> mvm cards will also work with that.
>
> No they don't, no firmware is available for that.
So far I only looked at the dvm part of iwlwifi with only minutes spend
on mvm to port the NATIVE solution from dvm.
Are you saying that mvm cards can't seamless switch a RX/TX key to TX
only one? mvm seems to support SW crypto as needed and switching RX/TX
to TX keys is the only other requirement for COMPAT mode.
Alexander
> On Sun, 2019-02-10 at 22:06 +0100, Alexander Wetzel wrote:
>> When using Extended Key ID mac80211 drops @IEEE80211_TX_CTL_AMPDU for
>> the last packet which can be added to a A-MPDU in preparation.
>>
>> Don't throw a warning and just handle the frame as if
>> @IEEE80211_TX_CTL_AMPDU would have been set.
>>
>> Signed-off-by: Alexander Wetzel <[email protected]>
>> ---
>>
>> I cold not figure out so far how to make sure the card will not mix
>> A-MPDU frames. Looks like that is handled fully in the HW and I didn't
>> find any interface to influence it. (It even may work already, I have
>> some problems to capture A-MPDU frames with A-MPDU information intact
>> and the topic was pretty low on the list so far. After all it works...)
>
> You can't really do that, as far as I can tell, unfortunately. So this
> might be better in a "compat on steroids" mode?
I hoped that the HW would already enforce the key border. But ok: The
cards will still be able to use NATIVE mode. Only now the driver will
have to request to disable Tx aggregation when finding a key border.
Since Tx aggregation is disabled per default anyhow many users will not
even see a difference.
(Off topic here, but I suspect the not reproducible problems leading to
change the Tx aggregation default to off could also have been ptk0 rekey
freezes. In that case we could now allow it again. But I've not found
raw error reports and guess it's not worth the efforts/risks to change
the default again.)
Alexander
Am 22.02.19 um 09:30 schrieb Johannes Berg:
>> With Extended Key ID we stick to a similar approach: NL80211_CMD_NEW_KEY
>> is called to install the new key and nl80211_key_install_mode allows to
>> select between a "legacy" or "extended" key install: NL80211_KEY_RX_TX
>> for "legacy" or NL80211_KEY_RX_ONLY for "extended".
>
> Ah. That's where the "EXT" came from in the mac80211 names :-) FWIW, I'm
> not sure that makes sense. Yes, we think of it as an "extension" or
> being "extended" now while we work on it, but ultimately it'll be a
> simple part of the API. But I digress.
"EXT" is only intended to be the short form for "Extended Key ID for
Individually Addressed Frames" from IEEE 802.11 - 2016 and not as
"extension for nl80211/mac80211".
>> NL80211_KEY_SWITCH_TX is only allowed with NL80211_CMD_SET_KEY and is
>> the only Extended Key ID action allowed for that function.
>
> Yes, but what does it *do*? Your documentation said:
>
> Switch Tx to a Rx only, referenced by sta mac and idx.
>
> but that seems backwards to me based on the above requirement:
> Allow a key to be used for decryption first and switch it to a
> "normal" Rx/Tx key with a second call.
>
Ah, now I see your point!
I'm viewing a Rx only key as something different, a new "key group" like
the existing broadcast or unicast key groups. (Which btw. also why I
have a problems to not have a key marked as such.)
"Switch TX" was intended to bring across, that the Tx status would
switch from the "other" key to the referenced one. (That's also the
reason I did not want to use ENABLE_TX.)
Or in other words:
Make a potential existing RX/TX key to a RX only key and the current Rx
only key to the RX/Tx key. The key referenced by sta and mac must be
flagged Rx only or the command will have no effect/fail. (The last
requirement would vanish when we drop the Rx_Only key flag.)
> IOW, you said we need to have the ability to first install RX-only, and
> then switch the key to RX & TX. I'd have called this "ENABLE_TX" or
> something like that. Or perhaps SWITCH_(ON_)TX if you're so inclined,
> but the documentation should then say
>
> Switch key from RX-only to TX/RX, referenced by ...
>
> no? That's what I meant by "the other way around".
What do you think about:
Switch key referenced by referenced by sta mac and idx from RX-
only to RX/TX and make any other RX/TX key for the sta a RX-only
key.
>
>> Any non-pairwise keys can only use NL80211_KEY_RX_TX which is of course
>> always the default and also allowed for "legacy" pairwise keys.
>
> Right.
>
>> A Extended Key Install will:
>> 1) call NL80211_CMD_NEW_KEY with all the usual parameters of a new key
>> install plus the flag NL80211_KEY_RX_ONLY set.
>
> So far makes sense.
>
>> 2) Once ready will call NL80211_CMD_SET_KEY with flag
>> NL80211_KEY_SWITCH_TX to activate a specific key.
>
> Also makes sense, but the documentation doesn't.
>
> I think we should rename these perhaps
>
> NL80211_KEY_RX_TX - install key and enable RX/TX (default)
> NL80211_KEY_RX_ONLY - install unicast key for RX only
> NL80211_KEY_ENABLE_TX - enable TX for a previously installed
> RX_ONLY key
>
> I think?
>
> About the ENABLE_TX vs. SWITCH_TX - we don't allow to switch TX *off*
> again, only *on*, so I think "enable" makes more sense than "switch".
>
See above. NL80211_KEY_ENABLE_TX would also implicit disable Tx for the
"other" pairwise key (if present). Mac80211 e.g. evaluates delay
tailromm for both keys again and always reduces it for the Rx only key
when it was set and even sets the RX_only flag again.
That said I'm happy with any of these:
NL80211_KEY_ENABLE_TX
NL80211_KEY_SWITCH_TX
NL80211_KEY_MOVE_TX
After that discussion I'm now wondering if we maybe should change the
naming more fundamentally, similar to what I proposed for mac80211:
NL80211_KEY_SET - install key and enable RX/TX (default)
NL80211_KEY_EXT_SET add unicast key for Extended Key ID
NL80211_KEY_EXT_ENABLE - Set the key referenced by
sta mac the to the preferred TX key for sta
Here it would be nice to be able to use "NL80211_EXT_KEY_", but that
would break the naming schema quite badly. And using
NL80211_KEY_EXT_KEY_ looks also confusing...
Any preferences or other options? I think you have now all details about
that and whatever looks best for you is it now.
> Anyway, my whole comment was only about the documentation text which
> seemed backward or at least unclear to me.
>
I did not see that when coming from understanding the code to writing
comments/documentation. Chances are other readers will interpret that
like you did and not as intended... And the area is complex enough
without those misunderstandings.
>> ok, I'll remove all the drive-by comment "cleanups".
>> It (often) looks kind of untidy and not really worth separate patches
>> and I'm kind of responsible for (most) of them.. I see why you don't
>> want to see those fixed drive-by...
>>
>> My understanding is now that we prefer to not change those and I'll
>> leave them alone.
>
> I have no objection to even the most trivial cleanup patches going in
> separately, and if you like multiple in a bigger "clean up this area"
> patch, but here I think it detracts from the patch.
>
I understand.
Honestly I would feel silly to put most of the drive-by changes into a
dedicated patch and not worth the time for a review.
So I guess I'll only do separate patches if I really care about the
format/comment in future:-)
Alexander
>>>
>>>> + sta = sta_info_get_bss(sdata, mac_addr);
>>>> +
>>>> + if (!sta)
>>>> + return -EINVAL;
>>>> +
>>>> + if (sta->ptk_idx == key_idx)
>>>> + return 0;
>>>> +
>>>> + mutex_lock(&local->key_mtx);
>>>> + key = key_mtx_dereference(local, sta->ptk[key_idx]);
>>>> +
>>>> + if (key && key->flags & KEY_FLAG_RX_ONLY)
>>>
>>> do you even need the flag? Isn't it equivalent to checking
>>> sta->ptk_idx != key->idx
>>> or so?
>>>
>>> Less data to maintain would be better.
>>
>> I have to look at that again. It will change some assumptions for sure
>> but still could work out with some slight differences. I'll have to
>> look deeper into that since I remember two moments where I was sure
>> needing the flag. That may well be outdated, but at a first glance it
>> would at least open the door to first install two key in legacy mode
>> and then switch between them. (Which should be no problem, of course)
>> I'll follow up on that separately, but that may take some time. When
>> it works you'll get a new RFC.
I gave that a closer look today and tried to get rid of
KEY_FLAG_RX_ONLY. It looks like it could work, but only with a
(pointless looking) trade off:
The problem is the initial key install. Normal and Extended Key Installs
both call ieee80211_add_key() and the function has to "mark" either the
key or the sta to be using Extended Key ID so we later know if we have
to activate Tx for the key or not.
Without that ptk_idx will still be set to INVALID_PTK_KEYIDX in either
case and therefore useless. ieee80211_key_replace() will then have no
way to figure out if it must set ptk_idx and activate the key for TX or
not.
Both checking a key Flag (for RX only) or a station flag (for Extended
Key ID being used) would solve the problem but I do not see an good way
to do it without one or the other.
Now we could of course use some other existing variable like setting
ptk_idx to INVALID_PTK_KEYIDX+1 in ieee80211_add_key(). But it still
would be some kind of flag, only stored where nobody would expect it...
Storing the information with the key simplifies the delay tailroom
handling a bit but I think I can get that operational with a station
flag for Extended Key ID also. But for me that trade off looks pointless..
I can of course go ahead if you see some benefit or other way ahead...
But I would pull the plug on the plan to get rid of RX_Only Key flag
with that.
Alexander
>>>>> + key->flags &= ~KEY_FLAG_RX_ONLY;
> [snip]
>>>>> + if (key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
>>>>> + ret = drv_set_key(local, EXT_KEY_RX_TX, sdata,
>>>>> + &sta->sta, &key->conf);
>>>>> + if (ret) {
>>>>> + sdata_err(sdata,
>>>>> + "failed to activate key for Tx (%d, %pM)\n",
>>>>> + key->conf.keyidx, sta->sta.addr);
>>>>> + return ret;
>>>>
>>>> You've already cleared the RX_ONLY flag, which gets you inconsistent
>>>> data now.
>>>>
>>>
>>> I don't think so, it looks ok for me. But the delay_tailroom logic took
>>> a surprisingly large chunk of time and I should explain how the updated
>>> logic is intended to work. Maybe I've messed it up somehow and just do
>>> not see it:
>
> My comment wasn't related to tailroom accounting at all. I've snipped
> more code above to hopefully make it clearer.
>
> If you fail to activate the key in the driver, then the key is not
> actually enabled for TX, and thus you should not have cleared the
> KEY_FLAG_RX_ONLY? You'll be returning this error all the way to
> userspace, I assume.
>
> (Maybe, btw, the driver should be allowed to return something like
> "oops, I now want to use TX software crypto" like it can do for other
> operations?)
Yes, an error here will get passed through to the userspace.
I think I mentioned already that tailroom update really got tricky with
Rx only keys and I'll also add some comments to help understanding better:-)
Here a more verbose version to verify the logic:
An error here will abort the key install and at the end that will
disconnect the sta. But don't forget that the key at that moment was
already installed successfully to the driver as a Rx only key and in
most cases the driver is already "ready" to use Tx with that key.
Mac80211 is just not handing over any MPDUs for that key.
A "normal" key install would already have increased the tailroom needed
count at that time, we just skipped that since a Rx only key never can
have use for a tailroom and it's now time to update the count if needed
for Tx.
If that call to the driver here fails mac80211 may have a different
understanding of the key RX/TX status than the driver for cards similar
to ath10k we do not care about that. While we still may be able to
transport some packets if the driver still can handle them with the old
Tx key the userspace will have to tear down at least the encryption and
probably the complete association to start over. The connection is
basically already dead and we only care about our mac80211 housekeeping,
so in that case our delay tailroom needed count.
Allowing SW crypto fallback makes no sense, there is no way mac80211 can
rescue the situation: Only drivers like ath10k handling the key
selection with PN assignment themselves should need this call, all
others just have to return 0. So for any driver needing this call
mac80211 can't fix whatever went wrong in the driver.
Here the comments I've now add to the code for the next revision of the
pacth: (The code itself is unchanged)
+ key->flags &= ~KEY_FLAG_RX_ONLY;
+
+ /* Dropping the KEY_FLAG_RX_ONLY flag forces us to update tailroom,
+ * do that first.
+ */
+ if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) ||
+ key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
+ IEEE80211_KEY_FLAG_PUT_MIC_SPACE |
+ IEEE80211_KEY_FLAG_RESERVE_TAILROOM))
+ increment_tailroom_need_count(sdata);
+
+ if (key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
+ ret = drv_set_key(local, EXT_KEY_RX_TX, sdata,
+ &sta->sta, &key->conf);
+ if (ret) {
+ /* This is a not recoverable error, so we don't set
+ * KEY_FLAG_RX_ONLY again or fix the tailroom needed
+ * count here. By skipping both
ieee80211_remove_key()
+ * will do that for us.
+ */
+ sdata_err(sdata,
+ "failed to activate key for Tx (%d,
%pM)\n",
+ key->conf.keyidx, sta->sta.addr);
+ return ret;
+ }
+ }
Alexander
>>>> The code is assuming that the driver is not aggregating MPDUs more than
>>>> 5s apart. We probably don't have wait nearly so long but I'm not sure
>>>> what is the minimum time.
>>>
>>> OTOH, if you have a lot of BE/VI/VO traffic BK might be starved even
>>> longer than that, technically indefinitely.
>>>
>>
>> Hm, there is nothing preventing us to drop this "idle" switch as long as
>> we also drop the 10s Rx accel offload when idle fallback in the COMPAT
>> patch. (We have to drop the RX idle accel patch or get a small risk of
>> dropping packets in unlikely but possible scenarios. If that are eapol
>> packets things will become hairy, probably disconnecting the sta.)
>>
>> It just feels strange to potentially still use the old key for one
>> packet more without time limit. It could be, that the first EAPOL packet
>> we send for the next rekey would still use the previous key, the second
>> eapol packet the current.
>
> Not sure I understand this. If we have no TX going on at all, then
> surely we can switch with the next packet, before we encrypt it?
Well, yes. But how can we figure out we are indeed idle?
(Chances are there is something and I missed it, I've not even look at
many subsystems in mac80211 and probably even don't know some of them
exist...)
The driver/card must have given up on waiting for more frames to
aggregate and send out whatever it had in the queue.
You just told me above that a card may keep a partially assembled A-MPDU
in the buffer for >5s if we have higher priority traffic... That was my
idea to just handle it time-based with an insane but still ok long time.
Checking if all higher priority TIDs are idle sounds complicated and we
still must know when the driver/card gives up and send out the bufferd
frames even if the A-MPDU is not full. That does not make the idea to
handle it time-based any simpler...
I'm not even sure it makes much sense to send out packets with 5s delay
at all, tcp will already have queued retransmitts and for udp the frames
are either also considered to be lost by the application or we don't
care about them any more.
So yes, we can switch Tx to the new key immediately when we are sure the
driver/card has no A-MPDU "in aggregation" at that time. I guess we can
add a call the driver, asking it to tell us if a TID is idle, which also
sounds complicated. So I guess I just would send the "key border" signal
even when idle and accept that this can in corner cases use different
keys for even and odd EAPOL frames.
>>>> + info->flags &= ~IEEE80211_TX_CTL_AMPDU;
>>>
>>> Like you say above, I don't think this really makes a lot of sense. If
>>> we don't have any free bits I guess we should try to find some ...
>>
>> Well, all I can think of is quite invasive, so I hoped you would have a
>> better idea:
>
> I think we have free bits in enum mac80211_tx_control_flags, so that
> should be workable? They won't be preserved until TX status, but that's
> OK for this purpose, no?
Wonderful idea:-)
I'll rewrite drop you a new patch revision with all the other changes we
discussed (or are still discussing).
If nothing new pops up - either in our discussions or when rewriting the
key border signal - I would drop RFC from the next patch series all the
POC patches after this one so we can focus on the merge relevant defects.
Alexander
> On Thu, 2019-02-21 at 21:07 +0100, Alexander Wetzel wrote:
>>>
>>>> + if (!ext_native && key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
>>>> + key->flags |= KEY_FLAG_RX_SW_CRYPTO;
>>>> + /* Activate Rx crypto offload after max 10s when idle */
>>>> + ieee80211_queue_delayed_work(&local->hw, &sta->ext_key_compat_wk,
>>>> + round_jiffies_relative(HZ * 10));
>>>> + }
>>>
>>> Is there much point in this?
>>>
>>>> + if (unlikely(rx->key->flags & KEY_FLAG_RX_SW_CRYPTO)) {
>>>> + rx->key->flags &= ~KEY_FLAG_RX_SW_CRYPTO;
>>>> + cancel_delayed_work(&rx->sta->ext_key_compat_wk);
>>>> + ieee80211_queue_delayed_work(&rx->local->hw,
>>>> + &rx->sta->ext_key_compat_wk, 0);
>>>> + }
>>>
>>> We'll almost certainly do it from here, so never exercise the other
>>> path?
>>
>> This is mostly to have a definite time we know the new key is used also
>> for RX. In probably 99.9% of all cases it will be triggered from the Rx
>> path.
>> Some special purpose devices may not send any packets for a long time
>> and trigger the fallback, as (wrong) firewall rules. (I've e.g. tested
>> it by dropping all outgoing packets on the remote sta.)
>>
>> The idea was to be sure that a rekey intervall >10s prevents activating
>> Rx crypt when rekeying the next key. Which now sounds kind of thin...
>
> Not sure I even understand this?
>
> You meant "Rx crypto offload"? I'm not really sure we _care_ that much?
>
> Then again, an issue may be that some firmware may want (need) the keys
> for RX so it can look at certain frames (action frames?) itself. So if
> we never install the RX key and then only get an action frame that the
> firmware should handle, we lose. Such firmware could not support COMPAT
> mode then I guess, which may mean a bunch of iwlwifi devices shouldn't
> use COMPAT mode.
We still seem to have a problem understanding each other here. That
sounds like something we have to sort out with the RFC patch series to
avoid bad surprises later... It's probably nothing, but here we go:-)
First, COMPAT drivers must not have any key installed for Rx offload in
Hw. Any active Rx key will be changed to a Tx only key with mac80211
taking over the Rx decryption in software.
That is bad, weak CPU devices asked to take over decryption will be not
able to handle the same load as with HW. (My test router can't and with
Sw crypto I'm around 20MBit/s slower. The router even become
unresponsive on a shell when stressing it in a quick test some weeks
ago, with a single client uploading.)
So we try to handle as few packets as possible with SW crypto and as
soon as we see a packet encrypted with the new key activate Rx crypto
with the new key again. Any packets send with the old key after that are
very likely mangled by the Rx crypto offload and lost and part of the
price we have to pay for being able to use extended Key ID with cards
never designed for it and at least with traffic between ath9k/iwldvm not
an issue at all.
But it's of course all that is irrelevant when we are no packets to
decrypt and the code sample above starting the discussion. That is
handling a very rare corner case I reproduced by dropping all outgoing
packets with iptables on the remote station. EAPOL packets of course
don't care about iptable rules and the key could still be rekeyed, but
EAPOL frames were the only traffic possible and since the handshake
still use the outgoing key the receiving station never got a packet
encrypted with the new key, preventing it to activate Rx offload. So 10s
after the rekey the "fallback" mechanism kicked in.
Not installing the Rx key after 10s as a fallback has no real downsides,
it only may break the assumption of someone debugging a problem that at
10s after we installed the key to mac80211 the Hw crypto will be active
again.
Without the fallback it's just something like extremely likely and may
be not true in unusual setups or for problems affecting the Rx path.
Long story short, if you don't like the fallback activating mechanism
for no Rx packets seen we can simply drop it.
As you said it's a rare case and not handling it only chances what you
assume from what you know for sure during debugging of a problem.
Management Frame crypto is not affected by COMPAT mode workarounds or
even by Extended Key ID at all, or I have a big flaw in my understanding:
Managment frames (802.11w) and for my understanding therefore also
action frames do not use the keys PTK keys (IDs 0+1) but 4+5 and never
touch the QoS MPDUs Extended Key ID is all about.
But after figuring that much out I ignored it...
Alexander
>> and I've had some time to improve my test setup and
>> hopefully have better access to a mvm card for testing.
>
> If all that's lacking is a few NICs then I'm sure we/I can help out with
> that :)
>
Thanks, but as you suspected the NICs are the smallest problem...
I've simply delayed setting up a good lab far too long. So prior to
shifting the focus to drivers I have to remedy that somehow.
Not really relevant here, but if you want to know what I plan for the
future here my thoughts on it. If not you better skip that section:-)
Current plan is, to get one (ideally two, depending on price) mini PCs
(amd64) with m.2 slots, good antennas and one or two matching Intel AC
9260 cards.
Additionally I want to get rid of the whitelist in my development
notebook by flashing coreboot, opening the path to install a minipci to
m.2 adapter mount a second/third Intel 9260 card.
The core boot flash rollout is imminent, just do not want to risk the
system by soldering wires to the mainboard till the generic support is
sorted out. If that goes wrong I could brick my main system and lose
access to my dvm card, but no risk no fun:-)
With that setup and using AC-9260 in all devices I'll have near-perfect
setup to test and observe AC-9260 and can simply swap out cards to test
and and maybe also get working after (hopefully) getting AC-9260
Extended Key ID "certified" as reference implementation.
With three stations I can then try my hand at the still open mesh
support for Extended Key ID, but that should wpa_supplicant only.
That said the initial heavy lifting is done. I'm still amazed I get that
off the ground and working end2end with comparable ease. While there is
still much work left the structures are all in place and it's now mainly
closing the gaps.
I currently plan stick to the project a bit langer getting at least some
intel cards and ath9k working, finalize the already existing Extended
Key ID and PTK0 rekey patches and also update wireshark again.
I suspect I'll need at least one year for that in the best case,
potentially much longer.
I already have three bugs/issues flagged for analysis. Nothing really
relevant for the generic support but all three itching enough I'll plan
to look into them:
1) Wrong time stamps with at least iwlwifi when using monitor in
parallel to the normal connection for outgoing MPDUs (probably mac80211
issue, )
2) Probably: Wpa_supplicant scan list is flushed too often, preventing
fast reconnects
3) Wireshark can't handle Extended Key IDs decoding (and decoding should
get at least some other generic improvements
Chances are I'll be busy with coreboot for some time and then
finding/setting up some mini PC(s). That's nothing I've had any need for
so far and suitable systems with M.2 slots with at least two external
antennas are hard to find. (So I may end up mounting external antennas
to the system myself.)
To continue I primarily need another Extended Key ID AP, ideally able to
support NATIVE and COMPAT mode. Having a second device (or maybe just
second M.2 card in the same one?) for sniffing nice.
Finding a really good sniffer able to also capture A-MPDU frames
including control frames would be awesome. All the so-called good
sniffing USB sticks I got my hands on crap even for sniffing normal
frames compared to my built-in dvm card nobody seems to suggest for
sniffing...
Full-speed sniffing to observe a rekey during load is hardly a common
use case and I suspect usb interface alone an indication the card won't
be able to handle it well. And some things like the cleartext packet
leaks only becomes visible at higher speeds. (Which btw also need a
follow up. My crypt skills are not the best, but with my current
understanding I'm quite sure a retransmit of a previously encrypted
frame in the clear text gives away the TK key for all other frames
encrypted with the same TK.)
So plenty of work and reason to get a better test environment set up:-)
I hope the newer mvm cards will be as good as my old mvm card and when
using the same card in the AP, sta and sniffer able to get basically
every frame as long as the sniffer has the best antennas.
If someone here has a suggestion what works for I would appreciate some
pointers here:-) Don't want sink too much money at it but if it's
something around 300€ or so I could simply start with one and decide
later if I really need a second one. Non-amd64 systems would also be ok,
but it must at least work with vanilla kernels and something like
Debian. I'm wasted enough time of porting my patches for each test and
keep them in sync.
If someone is willing to donate a suitable test system I would not say
no, but I'll plan to finish that with private funds also.
I probably should work on the new AP, but then I always wanted to test
coreboot and finding out my notebook is now supported is too alluring to
resist;-)
>>>> I also have tested patch for iwldvm using the Compat mode and I think
>>>> mvm cards will also work with that.
>>>
>>> No they don't, no firmware is available for that.
>>
>> So far I only looked at the dvm part of iwlwifi with only minutes spend
>> on mvm to port the NATIVE solution from dvm.
>
> :)
>
>> Are you saying that mvm cards can't seamless switch a RX/TX key to TX
>> only one? mvm seems to support SW crypto as needed and switching RX/TX
>> to TX keys is the only other requirement for COMPAT mode.
>
> No, sorry. I misread your statement. I thought you were saying "mvm
> cards will also work with iwldvm" rather than "mvm cards will also work
> with compat mode" :-)
>
> I think they all should just be made to work in native mode, the
> firmware basically supports this as you found, there must be some small
> bugs.
Agree. And with your statements that it already should work and the
option to get ucode updates I like our chances:-) With some luck it
could even work and I made some error the first time. I'll give that a
second look with what I have at hand soon. But after bombing you with
mails for what feels like most of the weekend I'll postpone that for now:-)
As mentioned above I'm currently aiming for two or three Intel AC-9260
cards for the next development round. It's seems to be the most modern
card and the price difference between the cards is irrelevant compared
to both efforts and costs to get the cards working in two or three
devices. If you thing another card would be better for development I'll
just use that one instead...
Alexander
Alexander
>>> So here the reasoning for why I named them as they are and why I prefer
>>> the names used in the patch.
>>> First, many drivers will handle SET_KEY and SET_KEY_RX_ONLY with the
>>> same code and not differentiate between those at all. Using EXT_as a
>>> prefix for the "normal" command is therefore a nice way to imply the
>>> command can only be used with Extended Key ID and still link it to the
>>> original command.
>>> But more important for me was the clash between what the command spells
>>> and what it does in the COMPAT mode: SET_KEY_RX_ONLY would then be used
>>> to install a TX only key which never can be used by the card for Rx.
>>> So I decided to rename it to EXT_SET_KEY, just indicating that this
>>> command adds a new key to the card for Extended Key ID and drop the
>>> confusing reference to Rx.
> Wait, what? In compat mode SET_KEY_RX_ONLY installs a TX-only key? Ah,
> you mean before you changed this.
>
> Why don't we split out compat mode then?
>
> But I see where you're coming from with the EXT_ now. I need to think of
> it less as an "extension" now, but as "extended key ID". I'm not really
> entirely sure that makes sense - even what we think of as "extended key
> ID" now might be the new normal soon? But then again the spec does the
> same thing.
>
>>> Long story short: Using SET_KEY_RXONLY and SET_KEY_TX is not wrong, but
>>> I would rate them more confusing.
> Fair enough.
I've had second (or more like tenths..) thoughts about this API.
If you like any of the solutions below more than the others I'll use
that in the next patch, of course...
In a nutshell, the point of EXT_SET_KEY is to install a key, prepare it
for Tx but NOT use it for Tx. The driver can use it for Rx (NATIVE) or
just do nothing with the (in COMPAT mode) Tx only key till mac80211
starts using it for Tx later.
1) Assuming we keep the RX_ONLY key flag we could just drop EXT_SET_KEY
and use the "normal" SET_KEY with the flag moved to ieee80211_key_conf.
We really just want to install the key to the driver here, only drivers
like ath10k need a way to figure out it must not be used for Tx.
2) We also can drop the Rx only flag/extra command and simply use
SET_KEY. ath10k will then have to honor the keyID mac80211 prepared when
it want to support Extended Key ID. (ath10k seems to need a firmware
update either way, so that could be acceptable.)
That said I've now decided to simply name the calls in the next patch:
SET_KEY
-> no RX only flag!
SET_KEY_RX
-> SET_KEY, only key must not be used for Tx
DISABLE_KEY,
-> unchanged
ACTIVATE_KEY
-> so far named EXT_KEY_RX_TX
DISABLE_KEY_RX
-> so far named EXT_DISABLE_KEY_RX
We can then continue the discussion with a new patch and the other fixes
in it when needed.
To really clean that up we would have to change the existing names to
something like ADD_KEY, REMOVE_KEY. We could then name the new calls
ADD_KEY_RX, ACTIVATE_KEY and DISABLE_KEY_RX. But that seems to be overkill.
Alexander
Alexander
On Sun, 2019-02-24 at 14:04 +0100, Alexander Wetzel wrote:
> Finding a really good sniffer able to also capture A-MPDU frames
> including control frames would be awesome.
I think the AC-9260 you have should be a decent sniffer. The (yet
unreleased) follow-up hardware is even better, but this one is fine.
Just remember to load with amsdu_size set to the appropriate (maximum)
A-MSDU size you want to capture.
> I probably should work on the new AP, but then I always wanted to test
> coreboot and finding out my notebook is now supported is too alluring to
> resist;-)
:-)
> > I think they all should just be made to work in native mode, the
> > firmware basically supports this as you found, there must be some small
> > bugs.
>
> Agree. And with your statements that it already should work and the
> option to get ucode updates I like our chances:-) With some luck it
> could even work and I made some error the first time. I'll give that a
> second look with what I have at hand soon. But after bombing you with
> mails for what feels like most of the weekend I'll postpone that for now:-)
I was looking at the firmware now and ... well, I want to really test
this to understand what's going wrong, because it really *looks* like
even the recent ones should be supported natively, at least as far as
I've looked now.
> As mentioned above I'm currently aiming for two or three Intel AC-9260
> cards for the next development round. It's seems to be the most modern
> card and the price difference between the cards is irrelevant compared
> to both efforts and costs to get the cards working in two or three
> devices. If you thing another card would be better for development I'll
> just use that one instead...
AC-9260 should be fine, as far as Intel is concerned. Also make for good
sniffers, in my experience, we use them all the time for that.
johannes
Am 08.04.19 um 22:10 schrieb Johannes Berg:
> On Sun, 2019-02-24 at 14:04 +0100, Alexander Wetzel wrote:
>
>> Finding a really good sniffer able to also capture A-MPDU frames
>> including control frames would be awesome.
>
> I think the AC-9260 you have should be a decent sniffer. The (yet
> unreleased) follow-up hardware is even better, but this one is fine.
>
> Just remember to load with amsdu_size set to the appropriate (maximum)
> A-MSDU size you want to capture.
I did not do that so far... Thanks for that tip!
>
>> I probably should work on the new AP, but then I always wanted to test
>> coreboot and finding out my notebook is now supported is too alluring to
>> resist;-)
>
> :-)
>
I've got a new test system in the meantime and have it up and running as
a simple test AP with Extended Key ID. I suspect I'll should tune the
antennas (and probably also the card) based on the first test spins, but
it's good enough for the moment.
>>> I think they all should just be made to work in native mode, the
>>> firmware basically supports this as you found, there must be some small
>>> bugs.
>>
>> Agree. And with your statements that it already should work and the
>> option to get ucode updates I like our chances:-) With some luck it
>> could even work and I made some error the first time. I'll give that a
>> second look with what I have at hand soon. But after bombing you with
>> mails for what feels like most of the weekend I'll postpone that for now:-)
>
> I was looking at the firmware now and ... well, I want to really test
> this to understand what's going wrong, because it really *looks* like
> even the recent ones should be supported natively, at least as far as
> I've looked now.
>
my new test AP came with a Intel AC-3168, which seems to use only one
antenna, potentially also explaining my fist impression that it's a
worse card for sniffing than my old Ultimate-N 6300. But it looks like
it's acting exactly the same in my other iwlmvm test. So I actually
started to run some tests and started writing a mail. It's so far
inconclusive and I want to verify the packets on the air next. Something
is off here and I want to look at that again from scratch, including an
external sniffer.
That said here what I've got so far and some fresh captures.
It looks like my (new) Wireless-AC 3168NGW (firmware 29.1044073957.0)
does not have the new key ready for Rx when needed. I have a roughly 5 -
15 ms long window where the card scrambles received packets using the
new key. (Note: I can't replicate that at the moment. May be wrong!)
I first suspected the card "cleared" the new key for usage a bit too
soon and tried to verify that by waiting a bit after installing a key to
the HW. But it looks like it's not so simple...
I've added a 40 ms delay in the mvm driver after the call to
iwl_mvm_set_sta_key() and it first looked like that improved the
situation. So I moved the sleep to iwl_trans_send_cmd() behind
send_cmd() when not being in CMD_ASYNC but I can't see any any
differences any longer. At the moment (with the new test setup) I always
get one corrupted frame when downloading from the AP. Always the first
frame using the new key...
As for the test procedure: I just add a monitor interface in parallel to
the "normal" interface on the AP. With HW encryption enabled we should
only get cleartext packets and don't have to worry about encrypted
packets in our capture at all:
iw phy phy0 interface add mon0 type monitor
ip link set up dev mon0
And start a capture in the interface:
tcpdump -pi mon0 -s0 -w /tmp/AP.cap
I've just uploaded some captures for you to
https://www.awhome.eu/index.php/s/AJJXBLsZmzHdxpX also. I've enabled
swcrypto on the client for the first two and enabled HW crypto on the
client again for the third and forth.
AP-40ms.pcap.gz
delay hack as outlined above on the AP
AP-no-delay.cap.gz
no hack (just some useless printks)
AP-no-delay-client-HW-crypt.cap.gz
same as above, only cleint using HW crypto
AP-upload-no-delay-HW-crypt.cap.gz
same as previous, only uploading instead of downloading.
(and too many broken packets on receive, indicating a bad
reception/sniffer card)
In all captures I have a normal (1s) ping running to the AP from the
cleint and start a download from an internal server after a while.
You can e.g. find the "corrupted" looking frames with the wireshark filter
"(wlan.fc.type_subtype == 0x0028) && !(llc.dsap == 0xaa)"
Each capture here only has exactly one, the very first packet using the
new key.
I'll plane to look deeper here, but that is as far as I got so far.
When you look at the captures keep in mind that both the client and the
AP also have the two not merged patches applied. But I do not see how
that makes a difference here.
>> As mentioned above I'm currently aiming for two or three Intel AC-9260
>> cards for the next development round. It's seems to be the most modern
>> card and the price difference between the cards is irrelevant compared
>> to both efforts and costs to get the cards working in two or three
>> devices. If you thing another card would be better for development I'll
>> just use that one instead...
>
> AC-9260 should be fine, as far as Intel is concerned. Also make for good
> sniffers, in my experience, we use them all the time for that.
Guess I will have to get one for my AP soon at least:-)
Alexander
On Wed, 2019-04-10 at 22:46 +0200, Alexander Wetzel wrote:
> my new test AP came with a Intel AC-3168, which seems to use only one
> antenna, potentially also explaining my fist impression that it's a
> worse card for sniffing than my old Ultimate-N 6300.
Right, that's worse in some way.
> It looks like my (new) Wireless-AC 3168NGW (firmware 29.1044073957.0)
> does not have the new key ready for Rx when needed. I have a roughly 5 -
> 15 ms long window where the card scrambles received packets using the
> new key. (Note: I can't replicate that at the moment. May be wrong!)
> I first suspected the card "cleared" the new key for usage a bit too
> soon and tried to verify that by waiting a bit after installing a key to
> the HW. But it looks like it's not so simple...
>
> I've added a 40 ms delay in the mvm driver after the call to
> iwl_mvm_set_sta_key() and it first looked like that improved the
> situation. So I moved the sleep to iwl_trans_send_cmd() behind
> send_cmd() when not being in CMD_ASYNC but I can't see any any
> differences any longer. At the moment (with the new test setup) I always
> get one corrupted frame when downloading from the AP. Always the first
> frame using the new key...
Ok, that's interesting. Definitely something I'd want to reproduce here
locally and see where exactly we select the wrong key.
FWIW, no timing changes should be needed, once the firmware responds
(and we wait for that when installing a key) everything will be in
place.
> As for the test procedure: I just add a monitor interface in parallel to
> the "normal" interface on the AP. With HW encryption enabled we should
> only get cleartext packets and don't have to worry about encrypted
> packets in our capture at all:
> iw phy phy0 interface add mon0 type monitor
> ip link set up dev mon0
> And start a capture in the interface:
> tcpdump -pi mon0 -s0 -w /tmp/AP.cap
Right.
> I've just uploaded some captures for you to
> https://www.awhome.eu/index.php/s/AJJXBLsZmzHdxpX also. I've enabled
> swcrypto on the client for the first two and enabled HW crypto on the
> client again for the third and forth.
>
> AP-40ms.pcap.gz
> delay hack as outlined above on the AP
> AP-no-delay.cap.gz
> no hack (just some useless printks)
> AP-no-delay-client-HW-crypt.cap.gz
> same as above, only cleint using HW crypto
> AP-upload-no-delay-HW-crypt.cap.gz
> same as previous, only uploading instead of downloading.
> (and too many broken packets on receive, indicating a bad
> reception/sniffer card)
>
> In all captures I have a normal (1s) ping running to the AP from the
> cleint and start a download from an internal server after a while.
>
> You can e.g. find the "corrupted" looking frames with the wireshark filter
> "(wlan.fc.type_subtype == 0x0028) && !(llc.dsap == 0xaa)"
>
> Each capture here only has exactly one, the very first packet using the
> new key.
I'll take a look, but a trace-cmd recording would be more interesting
than the monitor interface, as it also tells us when what key was
installed etc.
If you have some time, you can find how to record that here (under the
"Tracing" section):
https://wireless.wiki.kernel.org/en/users/drivers/iwlwifi/debugging
Please do review the privacy notes there at the end of the page if you
do this, you should probably send the files to me/us directly rather
than post publicly. They do contain the keys of your (test) network to
some extent - at least of course the PTK(s)/GTK(s), but usually also the
KEK/KCK.
> When you look at the captures keep in mind that both the client and the
> AP also have the two not merged patches applied. But I do not see how
> that makes a difference here.
Agree.
johannes
> AP-no-delay-client-HW-crypt.cap.gz
> same as above, only cleint using HW crypto
> You can e.g. find the "corrupted" looking frames with the wireshark filter
> "(wlan.fc.type_subtype == 0x0028) && !(llc.dsap == 0xaa)"
For everyone else - make sure to set "ignore protected bit - with IV" in
wireshark.
> Each capture here only has exactly one, the very first packet using the
> new key.
Yeah. Note that this packet is really quickly after the EAPOL 4/4
(1.5ms) so perhaps the key wasn't installed yet or something? Hmm, then
again, it should be before we send the confirmation?
But I don't know. The tracing would definitely tell me what's going on.
Btw, if you do record such tracing, it helps if you compile
wpa_supplicant with CONFIG_DEBUG_LINUX_TRACING and start it with the -T
argument to record all of its debugging into the trace file as well.
johannes
> I'll take a look, but a trace-cmd recording would be more interesting
> than the monitor interface, as it also tells us when what key was
> installed etc.
I've just uploaded better captures also including traces to the same
location. Everything relevant for this mail is in the folder
iwlwifi-debug here:
https://www.awhome.eu/index.php/s/AJJXBLsZmzHdxpX
The old traces have been moved to top dir "old", if you still are
interested in them, but I think the new ones should be better.
1) Both AP and the STA are now running wt-2019-04-10 *without* the not
merged patches from the Extended Key ID series. (I've only enabled
Ext_Key_ID for mvm and dvm on top and added some printk's. The patches
for that are also available via the link above but really boring...
2) the files with the same prefix (in the format HH:MM) show the same
communication.
The AP files are of course from the AP, STA from the client station and
Sniff from my Netgear 7800 used as a OTA sniffer. With the AP literally
sitting on top of it.
The "Sniff" files can be decoded with the WPA Password "ExtKeyTestAP".
My "Hack" patch to wireshark to allow it to decode keyID 1 is also
uploaded. (The hack just allows decoding of keyID 1 at the price of no
longer decoding frames encrypted with gtk keyid 1.)
3) the AP is using a AC 3168NGW and the STA a AC 8265. The two devices
are roughly 2m apart with a direct line of sight on a channel with next
to no other traffic on it. (You see what's there in the Sniff captures,
these are all totally unfiltered.)
4) I also tried running a trace on the STA. But I could not reproduce
the issue when a trace was running on the STA. (That's probably
coincidence, through, I just gave it three tries and the first without a
trace on the STA worked.)
5) The files in the "broken" folder are showing the issue and the ones
in "working" (seem) to be ok at a first glance.
I can run more tests and also try catch the problem with a STA trace,
too, if you want. But that may not be necessary:
It looks like we only have a problem when we get frames with the "new"
keyID and then again some with the "old" keyID. Of course we also could
have multiple problems, too... But in that case I would say let's first
look at this one.
The problematic frames are encrypted with the correct "old" key,
according to wireshark.
But on the STA they are scrambled and therefor probably have been
decrypted with the - in this case - wrong new key.
And as it happens there is also a really good looking first suspect why
this may have happened:
According to the STA captures the broken frames came in one A-MPDU which
started with keyID 1 and then "appended" the older keyID 0 frames at the
end. (The OTA sniffer seems to be miss the A-MPDU details, see the STA
capture...)
Now it really looks like the mvm cards are trusting the standard and
decode all MPDUs within one A-MPDU with the same key while at the same
time allow mixing different keyIDs in a-MPDU.
The so far mostly theoretical question how far mac80211 should support
the driver (e.g. key ID border signal) or if we want to let the
drivers/card handle that would become much more pressing...
Ideally the card/firmware would be able to detect that and start a new
A-MPDU. But for my understanding the driver could also set A-MPDU to
only one frame, forcing the firmware to flush all A-MPDUs under
construction and then set it back to the normal value. (Or that is how
I've planned to test that in that way with your tips in the past and
what's documented of the mvm/dvm firmware API.)
> Please do review the privacy notes there at the end of the page if you
> do this, you should probably send the files to me/us directly rather
> than post publicly. They do contain the keys of your (test) network to
> some extent - at least of course the PTK(s)/GTK(s), but usually also the
> KEK/KCK.
Thanks for the reminder.
I'll definitely will remember the option, I was not aware that there was
a more private way:-)
So far I can accept the risk - as I understand it - or even some short
term intruders in my test network.
For my understanding the previous captures (without the PSK in the
mail) only allows brute forcing the PSK due to the 4-way handshakes.
Providing a valid handshake allows anyone to crack the PSK, without the
challenge to capture it himself.
But risk of providing eapol frames of your network changed last year,
the handshake is no longer required to crack the password. Anyone able
to talk to the AP can get equivalent information by a simple query. So I
assume sharing captures with the eapol handshakes is not impacting the
security of my (test) Wlan in any relevant way. So I'm only giving
everyone a small peak in my normally encrypted wlan, correct?
Here a link to the vulnerability discovered last year:
https://blog.avira.com/cracking-your-wpa2-wi-fi-password-just-became-easier/
But I've changed the PSK already for my test Wlan again from the ones
used in the provided captures. The "real" Wlan is using EAP-TLS, the one
I had the ptk rekey freezes some years ago:-)
Alexander
On Sun, 2019-04-14 at 18:12 +0200, Alexander Wetzel wrote:
> > I'll take a look, but a trace-cmd recording would be more interesting
> > than the monitor interface, as it also tells us when what key was
> > installed etc.
>
> I've just uploaded better captures also including traces to the same
> location. Everything relevant for this mail is in the folder
> iwlwifi-debug here:
> https://www.awhome.eu/index.php/s/AJJXBLsZmzHdxpX
Great, thanks.
> It looks like we only have a problem when we get frames with the "new"
> keyID and then again some with the "old" keyID. Of course we also could
> have multiple problems, too... But in that case I would say let's first
> look at this one.
This part kinda surprises me.
> The problematic frames are encrypted with the correct "old" key,
> according to wireshark.
>
> But on the STA they are scrambled and therefor probably have been
> decrypted with the - in this case - wrong new key.
>
> And as it happens there is also a really good looking first suspect why
> this may have happened:
> According to the STA captures the broken frames came in one A-MPDU which
> started with keyID 1 and then "appended" the older keyID 0 frames at the
> end. (The OTA sniffer seems to be miss the A-MPDU details, see the STA
> capture...)
This doesn't surprise me at all.
> Now it really looks like the mvm cards are trusting the standard and
> decode all MPDUs within one A-MPDU with the same key while at the same
> time allow mixing different keyIDs in a-MPDU.
Yes, I'm pretty sure the firmware will only be able to look at the whole
A-MPDU and thus must make a decision based on the first subframe
regarding the key.
> The so far mostly theoretical question how far mac80211 should support
> the driver (e.g. key ID border signal) or if we want to let the
> drivers/card handle that would become much more pressing...
Yeah, good point. I guess this really would have to be on the
transmitter though.
> Ideally the card/firmware would be able to detect that and start a new
> A-MPDU.
For similar reasons, I don't think it can do this even on TX.
> But for my understanding the driver could also set A-MPDU to
> only one frame, forcing the firmware to flush all A-MPDUs under
> construction and then set it back to the normal value. (Or that is how
> I've planned to test that in that way with your tips in the past and
> what's documented of the mvm/dvm firmware API.)
It's probably getting more complicated with newer versions of the API,
but yes, I guess it should be doable to suspend aggregation.
> Thanks for the reminder.
> I'll definitely will remember the option, I was not aware that there was
> a more private way:-)
Just wanted to point it out. I'd agree the risk is minimal since you use
a separate test network anyway.
johannes
Am 15.04.19 um 10:44 schrieb Johannes Berg:
>> But for my understanding the driver could also set A-MPDU to
>> only one frame, forcing the firmware to flush all A-MPDUs under
>> construction and then set it back to the normal value. (Or that is how
>> I've planned to test that in that way with your tips in the past and
>> what's documented of the mvm/dvm firmware API.)
> It's probably getting more complicated with newer versions of the API,
> but yes, I guess it should be doable to suspend aggregation.
Honestly I don't like my own patch for the KeyId border signal, too
complex and not intuitive at all. But I wanted to have some code for
discussion and then got carried away a bit in what may have been not the
right direction.
Could we not simply ask (at least) the drivers supporting Extended Key
IDs to implement a special for the remote STA invisible A-MPDU "stop" mode?
In this new mode each A-MPDU would simply be build out of a single MPDU.
We then could keep Block-Ack active and we don't have to tell the remote
STA anything about that decision. After all a A-MPDU with only one MPDU
is allowed...
We then could tell the driver to switch to this mode when installing the
new key and out of it again when we have activated the new key for Tx.
That should be perfectly fine to run only in the control path and
comparable simple to set up, too.
That also sounds like something all drivers supporting A-MPDU should be
able to pull off somehow. (But then I've never even looked at more than
ath9k and iwlwifi so far for A-MPDU, and at those not close, yet.)
Do you see any problems with that solution and/or a better idea?
Also would you prefer we first sort out the A-MPDU issue prior I adding
test cases to the hostapd/wpa_supplicant or the other way round?
Looks like I'll have some time at Easter to (start) looking into one or
the other.
I'm currently trending to the A-MPDU issue, as it's the more fundamental
of the two immediate topics. (And the captures are pretty clear, this
explains so far every corrupted packet captured.)
Regards,
Alexander
On Mon, 2019-04-15 at 22:09 +0200, Alexander Wetzel wrote:
> Honestly I don't like my own patch for the KeyId border signal, too
> complex and not intuitive at all. But I wanted to have some code for
> discussion and then got carried away a bit in what may have been not the
> right direction.
:-)
> Could we not simply ask (at least) the drivers supporting Extended Key
> IDs to implement a special for the remote STA invisible A-MPDU "stop" mode?
> In this new mode each A-MPDU would simply be build out of a single MPDU.
> We then could keep Block-Ack active and we don't have to tell the remote
> STA anything about that decision. After all a A-MPDU with only one MPDU
> is allowed...
> We then could tell the driver to switch to this mode when installing the
> new key and out of it again when we have activated the new key for Tx.
> That should be perfectly fine to run only in the control path and
> comparable simple to set up, too.
Sounds doable, but I'm still debating if we even should give them a
signal - they have all the information in a sense, and do we have a good
idea of when we can go out of this mode again?
(FWIW, I'd probably call it "suspend" and "resume" or so)
> That also sounds like something all drivers supporting A-MPDU should be
> able to pull off somehow. (But then I've never even looked at more than
> ath9k and iwlwifi so far for A-MPDU, and at those not close, yet.)
> Do you see any problems with that solution and/or a better idea?
It ought to be possible, or if not then the device just can't support
extended key ID?
> Also would you prefer we first sort out the A-MPDU issue prior I adding
> test cases to the hostapd/wpa_supplicant or the other way round?
I think adding the code to hostapd/wpa_s is fine - right now we're
obviously OK since we have no drivers using extended key ID that use
hardware crypto, and if we have software crypto there's no problem
either way, even if A-MPDUs are built (which is probably not the case
for any such drivers anyway.)
In a sense I'd prefer getting the necessary bits and pieces elsewhere in
the stack upstream first since that's a prerequisite for anyone
else being able to easily work on this, and that's something we need for
drivers to pick it up.
johannes
>> Could we not simply ask (at least) the drivers supporting Extended Key
>> IDs to implement a special for the remote STA invisible A-MPDU "stop" mode?
>> In this new mode each A-MPDU would simply be build out of a single MPDU.
>> We then could keep Block-Ack active and we don't have to tell the remote
>> STA anything about that decision. After all a A-MPDU with only one MPDU
>> is allowed...
>> We then could tell the driver to switch to this mode when installing the
>> new key and out of it again when we have activated the new key for Tx.
>> That should be perfectly fine to run only in the control path and
>> comparable simple to set up, too.
>
> Sounds doable, but I'm still debating if we even should give them a
> signal - they have all the information in a sense, and do we have a good
> idea of when we can go out of this mode again?
Handling that on the Tx path would be tricky, but I you are right:
Drivers should be able to handle that as it is on the control path, too.
They can enable the mode when a key with IEEE80211_KEY_FLAG_NO_AUTO_TX
set and quiet it again as soon as they get a MPDU using the new KeyID.
Since switching back to normal doesn't have to be done immediately a
asyc call from Tx path or even a worker should do the job just fine.
Btw:
This also means we'll have to update the merged mac80211 Extended Key ID
support: We can only enable it for cards without HW crypto when they do
not set AMPDU_AGGREGATION. With the updated userspace these cards will
start using Extended Key ID with the already merged patches.
Drivers which should be able to use Extended Key ID with the two merged
patches seem to be:
admtek/adm8211.c
ath/ar5523/ar5523.c
broadcom/b43legacy/main.c
broadcom/brcm80211/brcmsmac/mac80211_if.c
mac80211_hwsim.c
marvell/libertas_tf/main.c
ralink/rt2x00/rt2400pci.c
ralink/rt2x00/rt2500pci.c
realtek/rtl818x/rtl8180/dev.c
realtek/rtl818x/rtl8187/dev.c
zydas/zd1211rw/zd_mac.c
Of those only hwsim and brcmsmac seems to support AMPDU and only
brcmsmac relly needs the fix to not lose some packets when rekeying.
>> That also sounds like something all drivers supporting A-MPDU should be
>> able to pull off somehow. (But then I've never even looked at more than
>> ath9k and iwlwifi so far for A-MPDU, and at those not close, yet.)
>> Do you see any problems with that solution and/or a better idea?
>
> It ought to be possible, or if not then the device just can't support
> extended key ID?
Agree. Or drivers can decide to deny A-MPDU setup when the key has been
installed with IEEE80211_KEY_FLAG_NO_AUTO_TX when they want.
>> Also would you prefer we first sort out the A-MPDU issue prior I adding
>> test cases to the hostapd/wpa_supplicant or the other way round?
>
> I think adding the code to hostapd/wpa_s is fine - right now we're
> obviously OK since we have no drivers using extended key ID that use
> hardware crypto, and if we have software crypto there's no problem
> either way, even if A-MPDUs are built (which is probably not the case
> for any such drivers anyway.)
>
> In a sense I'd prefer getting the necessary bits and pieces elsewhere in
> the stack upstream first since that's a prerequisite for anyone
> else being able to easily work on this, and that's something we need for
> drivers to pick it up.
Ok:-)
I assume we still have to wait till the API is in mainline (probably
5.2) to ask hostapd/wpa_supplicant to merge the patches?
At the moment I'm planning to get the patches merge ready and posted as
RFC till 5.2 is out. I'm also planing to keep Extended Key ID for Mesh
networks till later, so we can focus on AP/STA.
Alexander
On Tue, 2019-04-16 at 20:28 +0200, Alexander Wetzel wrote:
> They can enable the mode when a key with IEEE80211_KEY_FLAG_NO_AUTO_TX
> set
This I agree with.
> and quiet it again as soon as they get a MPDU using the new KeyID.
This isn't true, afaict. You need to be sure that no MPDUs remain using
the old key ID, not just that the new key ID showed up.
> Since switching back to normal doesn't have to be done immediately a
> asyc call from Tx path or even a worker should do the job just fine.
Sure.
> Btw:
> This also means we'll have to update the merged mac80211 Extended Key ID
> support: We can only enable it for cards without HW crypto when they do
> not set AMPDU_AGGREGATION. With the updated userspace these cards will
> start using Extended Key ID with the already merged patches.
I was going to say this is fine, but no, of course not ... we shouldn't
use different key id in the same A-MPDU.
That said, I'd be very surprised if there are any such drivers, except
in corner cases (like loading some drivers like ath9k or iwlwifi with
swcrypto=1 or so)
> Of those only hwsim and brcmsmac seems to support AMPDU and only
> brcmsmac relly needs the fix to not lose some packets when rekeying.
I can't believe that brcmsmac has no HW crypto support?
Anyway, a patch - even if it serves mostly as documentation - would be
most welcome.
> I assume we still have to wait till the API is in mainline (probably
> 5.2) to ask hostapd/wpa_supplicant to merge the patches?
No, mac80211-next is (usually?) good enough.
johannes
Am 16.04.19 um 21:11 schrieb Johannes Berg:
> On Tue, 2019-04-16 at 20:28 +0200, Alexander Wetzel wrote:
>
>> They can enable the mode when a key with IEEE80211_KEY_FLAG_NO_AUTO_TX
>> set
>
> This I agree with.
>
>> and quiet it again as soon as they get a MPDU using the new KeyID.
>
> This isn't true, afaict. You need to be sure that no MPDUs remain using
> the old key ID, not just that the new key ID showed up.
>
Hm, you are right. There is no grantee that after the first frame with
the new keyID all frames after that will also use the new keyID when
using different priority classes, I assume..
But each TID should have a clean cut over, shouldn't it?
But then that would only help us with cards able to control the A-MPDU
size per TID...
Short of sending a dummy MPDU to each TID from mac80211 as key border
and the driver waiting for all of them I can't think of anything right
now. I really hope we can find something better...
>> Since switching back to normal doesn't have to be done immediately a
>> asyc call from Tx path or even a worker should do the job just fine.
>
> Sure.
>
>> Btw:
>> This also means we'll have to update the merged mac80211 Extended Key ID
>> support: We can only enable it for cards without HW crypto when they do
>> not set AMPDU_AGGREGATION. With the updated userspace these cards will
>> start using Extended Key ID with the already merged patches.
>
> I was going to say this is fine, but no, of course not ... we shouldn't
> use different key id in the same A-MPDU.
>
> That said, I'd be very surprised if there are any such drivers, except
> in corner cases (like loading some drivers like ath9k or iwlwifi with
> swcrypto=1 or so)
>
These should be no problem. The drivers still implement set_key and
mac80211 will not enable Extended Key ID for them. Enabling Extended Key
ID with SW crypto for drivers implementing set_key is only possible by
patching either mac80211 or the driver.
Btw:
That was the main use case I added the mac80211 module parameter
ieee80211_extended_key_id for. But looks like that was of too little use
and cut out.
>> Of those only hwsim and brcmsmac seems to support AMPDU and only
>> brcmsmac relly needs the fix to not lose some packets when rekeying.
>
> I can't believe that brcmsmac has no HW crypto support?
I've just greped the drivers, brcmsmac has ampdu_action in its
implementation of struct ieee80211_ops but no set_key.
So it really looks like SW crypto only to me...
>
> Anyway, a patch - even if it serves mostly as documentation - would be
> most welcome.
I'll prepare something. Looks really trivial to fix that.
>> I assume we still have to wait till the API is in mainline (probably
>> 5.2) to ask hostapd/wpa_supplicant to merge the patches?
>
> No, mac80211-next is (usually?) good enough.
Good to know:-)
Alexander