[RFC PATCH v2 0/2] Extended Key ID support for linux

[RFC PATCH v2 0/2] Extended Key ID support for linux

[Alexander Wetzel]

IEEE 802.11-2012 added support for Extended Key ID, allowing pairwise
keys to also use keyID 1 and moving group keys to IDs 2 and 3.

Support for Extended Key ID is basically completed and confirmed working
with both hwsim and "on the air" with ath9k/iwldvm using software
encryption and those patches here.

(The corresponding patch for wpa_supplicanat/hostapd need some more
work, but that's mostly cleanup and support for STKSAs.)

Prior to propose this patch for merging I would like to get Extended
Key ID working with HW encryption for at least some devices, but after
experimenting with ath9k and to a lesser extend with ath10k it's now
clear that this will be an per-driver effort and it may well turn out to
be impossible without firmware updates.

So I've decided to continue working on the HW support for now but also
ask you for feedback for what I got so far. 
Any feedback is welcome and I especially like to learn what you think of
the API extensions and what has to be changed to get it merged.

RFC patch history:
v2:
Correct tested version without null pointer bug

Alexander Wetzel (2):
  nl80211/cfg80211: Add support for Extended Key ID
  mac80211: Add support for Extended Key ID

 include/net/cfg80211.h       |  2 ++
 include/net/mac80211.h       |  6 +++++
 include/uapi/linux/nl80211.h | 41 ++++++++++++++++++++++++++---
 net/mac80211/cfg.c           | 30 ++++++++++++++++++++-
 net/mac80211/debugfs_sta.c   |  1 +
 net/mac80211/key.c           | 46 +++++++++++++++++++++++++-------
 net/mac80211/key.h           |  1 +
 net/mac80211/main.c          |  2 ++
 net/mac80211/sta_info.c      |  1 +
 net/mac80211/sta_info.h      |  1 +
 net/wireless/nl80211.c       | 51 ++++++++++++++++++++++++++++++++----
 net/wireless/rdev-ops.h      |  3 ++-
 net/wireless/trace.h         | 31 ++++++++++++++++++----
 net/wireless/util.c          |  9 ++++---
 14 files changed, 197 insertions(+), 28 deletions(-)

-- 
2.19.1

[RFC PATCH v2 1/2] nl80211/cfg80211: Add support for Extended Key ID

[Alexander Wetzel]

Extend cfg80211 and nl80211 to allow pairwise keys to be installed for
RX only, allowing to switching over TX independently, as required by
IEEE-802.11-2016 to support "Extended Key ID for Individually Addressed
Frames"

PTK and STK keys are now also allowed to use Key ID 1.

Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>
---
 include/net/cfg80211.h       |  2 ++
 include/uapi/linux/nl80211.h | 41 ++++++++++++++++++++++++++---
 net/wireless/nl80211.c       | 51 ++++++++++++++++++++++++++++++++----
 net/wireless/rdev-ops.h      |  3 ++-
 net/wireless/trace.h         | 31 ++++++++++++++++++----
 net/wireless/util.c          |  9 ++++---
 6 files changed, 119 insertions(+), 18 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 1fa41b7a1be3..0d59340563e0 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.
+ * @flag: One flag from &enum key_params_flag
  */
 struct key_params {
 	const u8 *key;
@@ -492,6 +493,7 @@ struct key_params {
 	int key_len;
 	int seq_len;
 	u32 cipher;
+	enum key_params_flag flag;
 };
 
 /**
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 6d610bae30a9..d6c7fa72f433 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -2254,6 +2254,14 @@ enum nl80211_commands {
  * @NL80211_ATTR_FTM_RESPONDER_STATS: Nested attribute with FTM responder
  *	statistics, see &enum nl80211_ftm_responder_stats.
  *
+ * @NL80211_ATTR_KEY_RXONLY: Flag attribute to request RX key install only for
+ *      a pairwise key. Only supported for keyid's 0 and 1 when driver is
+ *      supporting Extended Key ID.
+ *
+ * @NL80211_ATTR_KEY_SETTX: Flag attribute to switch TX to a specified keyid.
+ *      Only supported for keyid's 0 and 1 when driver is supporting Extended
+ *      Key ID.
+ *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2699,6 +2707,9 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_FTM_RESPONDER_STATS,
 
+	NL80211_ATTR_KEY_RXONLY,
+	NL80211_ATTR_KEY_SETTX,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
@@ -4056,6 +4067,22 @@ enum nl80211_channel_type {
 	NL80211_CHAN_HT40PLUS
 };
 
+/**
+ * enum key_params_flag - additional key flag for drivers
+ *
+ * Actions other than @NL80211_KEY_DEFAULT_RX_TX are only required from drivers
+ * supporting Extended Key ID for pairwise keys using keyid 0 or 1.
+ *
+ * @NL80211_KEY_DEFAULT_RX_TX: key can be immediately used for Rx and Tx
+ * @NL80211_KEY_RX_ONLY: key must be installed for Rx only
+ * @NL80211_KEY_SET_TX: switch Tx for sta to specified keyid
+ */
+enum key_params_flag {
+	NL80211_KEY_DEFAULT_RX_TX,
+	NL80211_KEY_RX_ONLY,
+	NL80211_KEY_SET_TX
+};
+
 /**
  * enum nl80211_chan_width - channel width definitions
  *
@@ -4299,6 +4326,10 @@ 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_RXONLY: Flag attribute to request RX key install only for
+ *      a pairwise key.
+ * @NL80211_KEY_SETTX: Flag attribute to switch TX to a selected key.
+ *
  * @__NL80211_KEY_AFTER_LAST: internal
  * @NL80211_KEY_MAX: highest key attribute
  */
@@ -4312,6 +4343,8 @@ enum nl80211_key_attributes {
 	NL80211_KEY_DEFAULT_MGMT,
 	NL80211_KEY_TYPE,
 	NL80211_KEY_DEFAULT_TYPES,
+	NL80211_KEY_RXONLY,
+	NL80211_KEY_SETTX,
 
 	/* keep last */
 	__NL80211_KEY_AFTER_LAST,
@@ -5250,13 +5283,14 @@ enum nl80211_feature_flags {
  * @NL80211_EXT_FEATURE_SCAN_MIN_PREQ_CONTENT: Driver/device can omit all data
  *	except for supported rates from the probe request content if requested
  *	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_ENABLE_FTM_RESPONDER: Driver supports enabling fine
+ *	timing measurement responder role.
+ * @NL80211_EXT_FEATURE_EXT_KEY_ID: Driver supports "Extended Key ID for
+ *      Individually Addressed Frames" from IEEE802.11-2016.
  *
  * @NUM_NL80211_EXT_FEATURES: number of extended features.
  * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
@@ -5297,6 +5331,7 @@ enum nl80211_ext_feature_index {
 	NL80211_EXT_FEATURE_SCAN_MIN_PREQ_CONTENT,
 	NL80211_EXT_FEATURE_CAN_REPLACE_PTK0,
 	NL80211_EXT_FEATURE_ENABLE_FTM_RESPONDER,
+	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 744b5851bbf9..51ba4c9bffc5 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -275,6 +275,8 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
 	[NL80211_ATTR_KEY_SEQ] = { .type = NLA_BINARY, .len = 16 },
 	[NL80211_ATTR_KEY_TYPE] =
 		NLA_POLICY_MAX(NLA_U32, NUM_NL80211_KEYTYPES),
+	[NL80211_ATTR_KEY_RXONLY] = { .type = NLA_FLAG },
+	[NL80211_ATTR_KEY_SETTX] = { .type = NLA_FLAG },
 
 	[NL80211_ATTR_BEACON_INTERVAL] = { .type = NLA_U32 },
 	[NL80211_ATTR_DTIM_PERIOD] = { .type = NLA_U32 },
@@ -509,6 +511,8 @@ 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_RXONLY] = { .type = NLA_FLAG },
+	[NL80211_KEY_SETTX] = { .type = NLA_FLAG },
 };
 
 /* policy for the key default flags */
@@ -860,6 +864,7 @@ struct key_parse {
 	int type;
 	bool def, defmgmt;
 	bool def_uni, def_multi;
+	bool rx_only, set_tx;
 };
 
 static int nl80211_parse_key_new(struct genl_info *info, struct nlattr *key,
@@ -881,6 +886,16 @@ static int nl80211_parse_key_new(struct genl_info *info, struct nlattr *key,
 	if (k->defmgmt)
 		k->def_multi = true;
 
+	k->rx_only = !!tb[NL80211_KEY_RXONLY];
+	k->set_tx = !!tb[NL80211_KEY_SETTX];
+
+	if (k->rx_only && k->set_tx)
+		return -EINVAL;
+	else if (k->rx_only)
+		k->p.flag = NL80211_KEY_RX_ONLY;
+	else if (k->set_tx)
+		k->p.flag = NL80211_KEY_SET_TX;
+
 	if (tb[NL80211_KEY_IDX])
 		k->idx = nla_get_u8(tb[NL80211_KEY_IDX]);
 
@@ -945,6 +960,16 @@ static int nl80211_parse_key_old(struct genl_info *info, struct key_parse *k)
 	if (k->defmgmt)
 		k->def_multi = true;
 
+	k->rx_only = !!info->attrs[NL80211_ATTR_KEY_RXONLY];
+	k->set_tx = !!info->attrs[NL80211_ATTR_KEY_SETTX];
+
+	if (k->rx_only && k->set_tx)
+		return -EINVAL;
+	else if (k->rx_only)
+		k->p.flag = NL80211_KEY_RX_ONLY;
+	else if (k->set_tx)
+		k->p.flag = NL80211_KEY_SET_TX;
+
 	if (info->attrs[NL80211_ATTR_KEY_TYPE])
 		k->type = nla_get_u32(info->attrs[NL80211_ATTR_KEY_TYPE]);
 
@@ -3471,6 +3496,7 @@ static int nl80211_set_key(struct sk_buff *skb, struct genl_info *info)
 	struct key_parse key;
 	int err;
 	struct net_device *dev = info->user_ptr[1];
+	u8 *mac_addr = NULL;
 
 	err = nl80211_parse_key(info, &key);
 	if (err)
@@ -3479,8 +3505,9 @@ 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_SET_TX */
+	if (!key.def && !key.defmgmt && !key.set_tx)
 		return -EINVAL;
 
 	wdev_lock(dev->ieee80211_ptr);
@@ -3504,7 +3531,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;
@@ -3526,8 +3553,22 @@ 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 (wiphy_ext_feature_isset(&rdev->wiphy,
+					   NL80211_EXT_FEATURE_EXT_KEY_ID)) {
+		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 = -EOPNOTSUPP;
+			goto out;
+		}
+
+		err = rdev_add_key(rdev, dev, key.idx,
+				   NL80211_KEYTYPE_PAIRWISE,
+				   mac_addr, &key.p);
+	} else {
+		err = -EOPNOTSUPP;
+	}
  out:
 	wdev_unlock(dev->ieee80211_ptr);
 
@@ -3546,7 +3587,7 @@ static int nl80211_new_key(struct sk_buff *skb, struct genl_info *info)
 	if (err)
 		return err;
 
-	if (!key.p.key)
+	if (!key.p.key || key.set_tx)
 		return -EINVAL;
 
 	if (info->attrs[NL80211_ATTR_MAC])
diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index 51380b5c32f2..bc2f6f26b729 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->flag);
 	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 c6a9446b4e6b..903e4cda930c 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -412,22 +412,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 flag),
+	TP_ARGS(wiphy, netdev, key_index, pairwise, mac_addr, flag),
+	TP_STRUCT__entry(
+		WIPHY_ENTRY
+		NETDEV_ENTRY
+		MAC_ENTRY(mac_addr)
+		__field(u8, key_index)
+		__field(bool, pairwise)
+		__field(u8, flag)
+	),
+	TP_fast_assign(
+		WIPHY_ASSIGN;
+		NETDEV_ASSIGN;
+		MAC_ASSIGN(mac_addr, mac_addr);
+		__entry->key_index = key_index;
+		__entry->pairwise = pairwise;
+		__entry->flag = flag;
+	),
+	TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", key_index: %u, flag: %u, "
+		  "pairwise: %s, mac addr: " MAC_PR_FMT,
+		  WIPHY_PR_ARG, NETDEV_PR_ARG, __entry->key_index,
+		  __entry->flag, 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 ef14d80ca03e..3f2d116b8557 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -236,13 +236,14 @@ 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
+		/* IEEE802.11-2016 allows only 0 and 1 for pairwise keys.
+		 * Disallow pairwise keys with index above 1 unless it's WEP
 		 * or a vendor specific cipher (because current deployments use
-		 * pairwise WEP keys with non-zero indices and for vendor
+		 * pairwise WEP keys with higher indices and for vendor
 		 * specific ciphers this should be validated in the driver or
-		 * hardware level - but 802.11i clearly specifies to use zero)
+		 * hardware level.
 		 */
-		if (pairwise && key_idx)
+		if (pairwise && key_idx > 1)
 			return -EINVAL;
 		break;
 	case WLAN_CIPHER_SUITE_AES_CMAC:
-- 
2.19.1

Re: [RFC PATCH v2 1/2] nl80211/cfg80211: Add support for Extended Key ID

[Johannes Berg]

On Sun, 2018-11-11 at 12:02 +0100, Alexander Wetzel wrote:
> Extend cfg80211 and nl80211 to allow pairwise keys to be installed for
> RX only, allowing to switching over TX independently, as required by
> IEEE-802.11-2016 to support "Extended Key ID for Individually Addressed
> Frames"
> 
> PTK and STK keys are now also allowed to use Key ID 1.
> 
> Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>
> ---
>  include/net/cfg80211.h       |  2 ++
>  include/uapi/linux/nl80211.h | 41 ++++++++++++++++++++++++++---
>  net/wireless/nl80211.c       | 51 ++++++++++++++++++++++++++++++++----
>  net/wireless/rdev-ops.h      |  3 ++-
>  net/wireless/trace.h         | 31 ++++++++++++++++++----
>  net/wireless/util.c          |  9 ++++---
>  6 files changed, 119 insertions(+), 18 deletions(-)
> 
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 1fa41b7a1be3..0d59340563e0 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.
> + * @flag: One flag from &enum key_params_flag

That should be nl80211_key_params_flag.

But if only one flag can be set, then maybe this should instead be

enum nl80211_key_install_mode install_mode;

or so?
> +/**
> + * enum key_params_flag - additional key flag for drivers
> + *
> + * Actions other than @NL80211_KEY_DEFAULT_RX_TX are only required from drivers
> + * supporting Extended Key ID for pairwise keys using keyid 0 or 1.
> + *
> + * @NL80211_KEY_DEFAULT_RX_TX: key can be immediately used for Rx and Tx
> + * @NL80211_KEY_RX_ONLY: key must be installed for Rx only
> + * @NL80211_KEY_SET_TX: switch Tx for sta to specified keyid
> + */
> +enum key_params_flag {
> +	NL80211_KEY_DEFAULT_RX_TX,
> +	NL80211_KEY_RX_ONLY,
> +	NL80211_KEY_SET_TX
> +};

Clearly those aren't flags anyway.

I guess RX_ONLY and SET_TX are mostly needed AP-side?


> + * @NL80211_ATTR_KEY_RXONLY: Flag attribute to request RX key install only for
> + *      a pairwise key. Only supported for keyid's 0 and 1 when driver is
> + *      supporting Extended Key ID.
> + *
> + * @NL80211_ATTR_KEY_SETTX: Flag attribute to switch TX to a specified keyid.
> + *      Only supported for keyid's 0 and 1 when driver is supporting Extended
> + *      Key ID.

Ok, so you have these as separate netlink flags, but then you really
shouldn't also have the "install mode" in nl80211.h, that's not related
to userspace API then.

We might discuss instead having an NL80211_ATTR_INSTALL_MODE attribute,
and that takes the values from the enum, and then you do need the enum -
but if you don't need the enum then don't define it in nl80211.h but
keep it kernel-internal in cfg80211.h (and name it appropriately).

> @@ -4312,6 +4343,8 @@ enum nl80211_key_attributes {
>  	NL80211_KEY_DEFAULT_MGMT,
>  	NL80211_KEY_TYPE,
>  	NL80211_KEY_DEFAULT_TYPES,
> +	NL80211_KEY_RXONLY,
> +	NL80211_KEY_SETTX,

Indeed if you have this, you don't need the corresponding top-level NL80211_ATTR_*?

We went through a few iterations with the API, so a lot of this is
backward compatibility stuff, you should update the latest version only.
I believe it's this one.

> - * @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_ENABLE_FTM_RESPONDER: Driver supports enabling fine
> + *	timing measurement responder role.

What's going on here?

> @@ -945,6 +960,16 @@ static int nl80211_parse_key_old(struct genl_info *info, struct key_parse *k)
>  	if (k->defmgmt)

Yeah, just don't change parse_key_old, whoever wants to use this stuff
should upgrade to the new API. wpa_s has both IIRC, but of course the
old-side is ignored on newer kernels (and the new on older) so the older
stuff never needs new features.

>  		k->def_multi = true;
>  
> +	k->rx_only = !!info->attrs[NL80211_ATTR_KEY_RXONLY];
> +	k->set_tx = !!info->attrs[NL80211_ATTR_KEY_SETTX];

shouldn't this already be install_mode?

> +	/* only support setting default key and
> +	 * Extended Key ID action NL80211_KEY_SET_TX */
> +	if (!key.def && !key.defmgmt && !key.set_tx)
>  		return -EINVAL;

You need to check if extended key ID is supported

> -	}
> +	} else if (wiphy_ext_feature_isset(&rdev->wiphy,
> +					   NL80211_EXT_FEATURE_EXT_KEY_ID)) {
> +		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 = -EOPNOTSUPP;
> +			goto out;
> +		}
> +
> +		err = rdev_add_key(rdev, dev, key.idx,
> +				   NL80211_KEYTYPE_PAIRWISE,
> +				   mac_addr, &key.p);

I think you should _parse_ all the new stuff, and then reject it, when
extended key ID support isn't there?

Though not sure I'm parsing this code correctly right now.

> +++ b/net/wireless/util.c
> @@ -236,13 +236,14 @@ 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
> +		/* IEEE802.11-2016 allows only 0 and 1 for pairwise keys.
> +		 * Disallow pairwise keys with index above 1 unless it's WEP
>  		 * or a vendor specific cipher (because current deployments use
> -		 * pairwise WEP keys with non-zero indices and for vendor
> +		 * pairwise WEP keys with higher indices and for vendor
>  		 * specific ciphers this should be validated in the driver or
> -		 * hardware level - but 802.11i clearly specifies to use zero)
> +		 * hardware level.
>  		 */
> -		if (pairwise && key_idx)
> +		if (pairwise && key_idx > 1)
>  			return -EINVAL;
>  		break;

Again, only if driver support is advertised, and the comment should
probably reference the feature bit from the spec.

johannes

Re: [RFC PATCH v2 1/2] nl80211/cfg80211: Add support for Extended Key ID

[Alexander Wetzel]

> On Sun, 2018-11-11 at 12:02 +0100, Alexander Wetzel wrote:
>> Extend cfg80211 and nl80211 to allow pairwise keys to be installed for
>> RX only, allowing to switching over TX independently, as required by
>> IEEE-802.11-2016 to support "Extended Key ID for Individually Addressed
>> Frames"
>>
>> PTK and STK keys are now also allowed to use Key ID 1.
>>
>> Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>
>> ---
>>   include/net/cfg80211.h       |  2 ++
>>   include/uapi/linux/nl80211.h | 41 ++++++++++++++++++++++++++---
>>   net/wireless/nl80211.c       | 51 ++++++++++++++++++++++++++++++++----
>>   net/wireless/rdev-ops.h      |  3 ++-
>>   net/wireless/trace.h         | 31 ++++++++++++++++++----
>>   net/wireless/util.c          |  9 ++++---
>>   6 files changed, 119 insertions(+), 18 deletions(-)
>>
>> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
>> index 1fa41b7a1be3..0d59340563e0 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.
>> + * @flag: One flag from &enum key_params_flag
> 
> That should be nl80211_key_params_flag.
> 
> But if only one flag can be set, then maybe this should instead be
> 
> enum nl80211_key_install_mode install_mode;
> 
> or so?

It started out as a flag and I switched to enum later without updating 
it. I'll chnage that to nl80211_key_install_mode, much much better...

>> +/**
>> + * enum key_params_flag - additional key flag for drivers
>> + *
>> + * Actions other than @NL80211_KEY_DEFAULT_RX_TX are only required from drivers
>> + * supporting Extended Key ID for pairwise keys using keyid 0 or 1.
>> + *
>> + * @NL80211_KEY_DEFAULT_RX_TX: key can be immediately used for Rx and Tx
>> + * @NL80211_KEY_RX_ONLY: key must be installed for Rx only
>> + * @NL80211_KEY_SET_TX: switch Tx for sta to specified keyid
>> + */
>> +enum key_params_flag {
>> +	NL80211_KEY_DEFAULT_RX_TX,
>> +	NL80211_KEY_RX_ONLY,
>> +	NL80211_KEY_SET_TX
>> +};
> 
> Clearly those aren't flags anyway.
> 
> I guess RX_ONLY and SET_TX are mostly needed AP-side?
> 
> 
No, all stations using Extended Key ID will always use RX_ONLY and 
SET_TX for pairwise key installs. The AP will install the Key Rx only 
prior to sending eapol #3, the sta prior to sending eapol #4.

>> + * @NL80211_ATTR_KEY_RXONLY: Flag attribute to request RX key install only for
>> + *      a pairwise key. Only supported for keyid's 0 and 1 when driver is
>> + *      supporting Extended Key ID.
>> + *
>> + * @NL80211_ATTR_KEY_SETTX: Flag attribute to switch TX to a specified keyid.
>> + *      Only supported for keyid's 0 and 1 when driver is supporting Extended
>> + *      Key ID.
> 
> Ok, so you have these as separate netlink flags, but then you really
> shouldn't also have the "install mode" in nl80211.h, that's not related
> to userspace API then.
> 
> We might discuss instead having an NL80211_ATTR_INSTALL_MODE attribute,
> and that takes the values from the enum, and then you do need the enum -
> but if you don't need the enum then don't define it in nl80211.h but
> keep it kernel-internal in cfg80211.h (and name it appropriately).
> 
I may have mixed up mac80211 and nl80211 api and/or the correct include 
files..

The idea was, to have NL80211_ATTR_KEY_RXONLY, NL80211_ATTR_KEY_SETTX 
with the new API duplicates of NL80211_KEY_RXONLY and NL80211_KEY_SETTX 
for communication with the userspace and the enums ones in 
key_params_flag for communication with the drivers.

But unifying that with NL80211_ATTR_INSTALL_MODE attribute sounds like a 
good idea. I'll try to sanitize that.

>> @@ -4312,6 +4343,8 @@ enum nl80211_key_attributes {
>>   	NL80211_KEY_DEFAULT_MGMT,
>>   	NL80211_KEY_TYPE,
>>   	NL80211_KEY_DEFAULT_TYPES,
>> +	NL80211_KEY_RXONLY,
>> +	NL80211_KEY_SETTX,
> 
> Indeed if you have this, you don't need the corresponding top-level NL80211_ATTR_*?
> 
> We went through a few iterations with the API, so a lot of this is
> backward compatibility stuff, you should update the latest version only.
> I believe it's this one.
> 
I did not think of that when writing the code, but came a bit late to 
the same conclusion... I'll drop the old API support.

>> - * @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_ENABLE_FTM_RESPONDER: Driver supports enabling fine
>> + *	timing measurement responder role.
> 
> What's going on here?
>

The PTK0 rekey patch added a new line in front of the description. The 
next author did not notice that and added the description for 
NL80211_EXT_FEATURE_ENABLE_FTM_RESPONDER at the wrong place, probably 
assuming it was the end of the list. I've noticed that and fixed the 
documentation order and the misleading empty line.
I'll break that out as a separate cleanup patch if nobody beats me to it:-)

>> @@ -945,6 +960,16 @@ static int nl80211_parse_key_old(struct genl_info *info, struct key_parse *k)
>>   	if (k->defmgmt)
> 
> Yeah, just don't change parse_key_old, whoever wants to use this stuff
> should upgrade to the new API. wpa_s has both IIRC, but of course the
> old-side is ignored on newer kernels (and the new on older) so the older
> stuff never needs new features.
> 
ok.

>>   		k->def_multi = true;
>>   
>> +	k->rx_only = !!info->attrs[NL80211_ATTR_KEY_RXONLY];
>> +	k->set_tx = !!info->attrs[NL80211_ATTR_KEY_SETTX];
> 
> shouldn't this already be install_mode?
> 
Looks like switching to NL80211_ATTR_INSTALL_MODE will throw out that, 
but with the code from this patch that's an interim layer for checks and 
mapping it. So I'm not sure I understand that comment.

>> +	/* only support setting default key and
>> +	 * Extended Key ID action NL80211_KEY_SET_TX */
>> +	if (!key.def && !key.defmgmt && !key.set_tx)
>>   		return -EINVAL;
> 
> You need to check if extended key ID is supported

Yes. I have added checks in cfg80211_validate_key_settings current 
development version already, making sure only valid combinations can be 
called and reach this section.
>> -	}
>> +	} else if (wiphy_ext_feature_isset(&rdev->wiphy,
>> +					   NL80211_EXT_FEATURE_EXT_KEY_ID)) {
>> +		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 = -EOPNOTSUPP;
>> +			goto out;
>> +		}
>> +
>> +		err = rdev_add_key(rdev, dev, key.idx,
>> +				   NL80211_KEYTYPE_PAIRWISE,
>> +				   mac_addr, &key.p);
> 
> I think you should _parse_ all the new stuff, and then reject it, when
> extended key ID support isn't there?
> 
> Though not sure I'm parsing this code correctly right now.
> 
NL80211_CMD_SET_KEY is normally only used to set default keys for wep or 
managment keys. That changes here.

In this API draft NL80211_CMD_NEW_KEY is only used when installing a 
Extended Key ID key RX only. The switch to TX is added to 
NL80211_CMD_SET_KEY. The code has some sanity checks and then tells the 
driver to switch the key to tx.

But that has been changed quite a bit, the procedure in this patch 
turned out to be not so good and even had a locking issue, so it has 
changed a bit. I guess we should shelf that till I get the new variant 
working and then look at it again.


>> +++ b/net/wireless/util.c
>> @@ -236,13 +236,14 @@ 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
>> +		/* IEEE802.11-2016 allows only 0 and 1 for pairwise keys.
>> +		 * Disallow pairwise keys with index above 1 unless it's WEP
>>   		 * or a vendor specific cipher (because current deployments use
>> -		 * pairwise WEP keys with non-zero indices and for vendor
>> +		 * pairwise WEP keys with higher indices and for vendor
>>   		 * specific ciphers this should be validated in the driver or
>> -		 * hardware level - but 802.11i clearly specifies to use zero)
>> +		 * hardware level.
>>   		 */
>> -		if (pairwise && key_idx)
>> +		if (pairwise && key_idx > 1)
>>   			return -EINVAL;
>>   		break;
> 
> Again, only if driver support is advertised, and the comment should
> probably reference the feature bit from the spec.

That is where I added most of the sanity checks in the meantime.
But what feature bit from the spec are you referring to? The RSN 
Capability one?


Alexander

Re: [RFC PATCH v2 1/2] nl80211/cfg80211: Add support for Extended Key ID

[Johannes Berg]

On Wed, 2018-12-05 at 21:54 +0100, Alexander Wetzel wrote:
> 
> It started out as a flag and I switched to enum later without updating 
> it. I'll chnage that to nl80211_key_install_mode, much much better...

> No, all stations using Extended Key ID will always use RX_ONLY and 
> SET_TX for pairwise key installs. The AP will install the Key Rx only 
> prior to sending eapol #3, the sta prior to sending eapol #4.

Actually ... let's see all the operations at nl80211 level.

We have NEW_KEY and SET_KEY, right? SET_KEY basically already says to
use a given key for TX from now on, IIRC?

So realistically, don't we only need

NEW_KEY (RX-only)

as a new variant of NEW_KEY?

> The PTK0 rekey patch added a new line in front of the description. The 
> next author did not notice that and added the description for 
> NL80211_EXT_FEATURE_ENABLE_FTM_RESPONDER at the wrong place, probably 
> assuming it was the end of the list. I've noticed that and fixed the 
> documentation order and the misleading empty line.
> I'll break that out as a separate cleanup patch if nobody beats me to it:-)

Oh. OK. It also doesn't really matter ;-)

> > >   		k->def_multi = true;
> > >   
> > > +	k->rx_only = !!info->attrs[NL80211_ATTR_KEY_RXONLY];
> > > +	k->set_tx = !!info->attrs[NL80211_ATTR_KEY_SETTX];
> > 
> > shouldn't this already be install_mode?
> > 
> 
> Looks like switching to NL80211_ATTR_INSTALL_MODE will throw out that, 
> but with the code from this patch that's an interim layer for checks and 
> mapping it. So I'm not sure I understand that comment.

Well, me neither. Sounds almost like I got ahead of myself.

> > You need to check if extended key ID is supported
> 
> Yes. I have added checks in cfg80211_validate_key_settings current 
> development version already, making sure only valid combinations can be 
> called and reach this section.

Ok, great.

> NL80211_CMD_SET_KEY is normally only used to set default keys for wep or 
> managment keys. That changes here.

Right.

> In this API draft NL80211_CMD_NEW_KEY is only used when installing a 
> Extended Key ID key RX only. The switch to TX is added to 
> NL80211_CMD_SET_KEY. The code has some sanity checks and then tells the 
> driver to switch the key to tx.

Makes sense.

> But that has been changed quite a bit, the procedure in this patch 
> turned out to be not so good and even had a locking issue, so it has 
> changed a bit. I guess we should shelf that till I get the new variant 
> working and then look at it again.

Fair enough.

> > > +++ b/net/wireless/util.c
> > > @@ -236,13 +236,14 @@ 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
> > > +		/* IEEE802.11-2016 allows only 0 and 1 for pairwise keys.
> > > +		 * Disallow pairwise keys with index above 1 unless it's WEP
> > >   		 * or a vendor specific cipher (because current deployments use
> > > -		 * pairwise WEP keys with non-zero indices and for vendor
> > > +		 * pairwise WEP keys with higher indices and for vendor
> > >   		 * specific ciphers this should be validated in the driver or
> > > -		 * hardware level - but 802.11i clearly specifies to use zero)
> > > +		 * hardware level.
> > >   		 */
> > > -		if (pairwise && key_idx)
> > > +		if (pairwise && key_idx > 1)
> > >   			return -EINVAL;
> > >   		break;
> > 
> > Again, only if driver support is advertised, and the comment should
> > probably reference the feature bit from the spec.
> 
> That is where I added most of the sanity checks in the meantime.
> But what feature bit from the spec are you referring to? The RSN 
> Capability one?

Well, I wasn't thinking that precisely. I just thought it should mention
that it now says "IEEE802.11-2016 allows only 0 and 1 for pairwise keys"
but doesn't clarify that this is only for stations that actually want to
support it, so it could be read as being always that way.

johannes

Re: [RFC PATCH v2 1/2] nl80211/cfg80211: Add support for Extended Key ID

[Alexander Wetzel]

> On Wed, 2018-12-05 at 21:54 +0100, Alexander Wetzel wrote:
>>
>> It started out as a flag and I switched to enum later without updating
>> it. I'll chnage that to nl80211_key_install_mode, much much better...
> 
>> No, all stations using Extended Key ID will always use RX_ONLY and
>> SET_TX for pairwise key installs. The AP will install the Key Rx only
>> prior to sending eapol #3, the sta prior to sending eapol #4.
> 
> Actually ... let's see all the operations at nl80211 level.
> 
> We have NEW_KEY and SET_KEY, right? SET_KEY basically already says to
> use a given key for TX from now on, IIRC?
> 
> So realistically, don't we only need
> 
> NEW_KEY (RX-only)
> 
> as a new variant of NEW_KEY?
> 

Yes, that should indeed work. I'll try that and see how it plays out.

>> The PTK0 rekey patch added a new line in front of the description. The
>> next author did not notice that and added the description for
>> NL80211_EXT_FEATURE_ENABLE_FTM_RESPONDER at the wrong place, probably
>> assuming it was the end of the list. I've noticed that and fixed the
>> documentation order and the misleading empty line.
>> I'll break that out as a separate cleanup patch if nobody beats me to it:-)
> 
> Oh. OK. It also doesn't really matter ;-)
> 
>>>>    		k->def_multi = true;
>>>>    
>>>> +	k->rx_only = !!info->attrs[NL80211_ATTR_KEY_RXONLY];
>>>> +	k->set_tx = !!info->attrs[NL80211_ATTR_KEY_SETTX];
>>>
>>> shouldn't this already be install_mode?
>>>
>>
>> Looks like switching to NL80211_ATTR_INSTALL_MODE will throw out that,
>> but with the code from this patch that's an interim layer for checks and
>> mapping it. So I'm not sure I understand that comment.
> 
> Well, me neither. Sounds almost like I got ahead of myself.
> 
>>> You need to check if extended key ID is supported
>>
>> Yes. I have added checks in cfg80211_validate_key_settings current
>> development version already, making sure only valid combinations can be
>> called and reach this section.
> 
> Ok, great.
> 
>> NL80211_CMD_SET_KEY is normally only used to set default keys for wep or
>> managment keys. That changes here.
> 
> Right.
> 
>> In this API draft NL80211_CMD_NEW_KEY is only used when installing a
>> Extended Key ID key RX only. The switch to TX is added to
>> NL80211_CMD_SET_KEY. The code has some sanity checks and then tells the
>> driver to switch the key to tx.
> 
> Makes sense.
> 
>> But that has been changed quite a bit, the procedure in this patch
>> turned out to be not so good and even had a locking issue, so it has
>> changed a bit. I guess we should shelf that till I get the new variant
>> working and then look at it again.
> 
> Fair enough.
> 
>>>> +++ b/net/wireless/util.c
>>>> @@ -236,13 +236,14 @@ 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
>>>> +		/* IEEE802.11-2016 allows only 0 and 1 for pairwise keys.
>>>> +		 * Disallow pairwise keys with index above 1 unless it's WEP
>>>>    		 * or a vendor specific cipher (because current deployments use
>>>> -		 * pairwise WEP keys with non-zero indices and for vendor
>>>> +		 * pairwise WEP keys with higher indices and for vendor
>>>>    		 * specific ciphers this should be validated in the driver or
>>>> -		 * hardware level - but 802.11i clearly specifies to use zero)
>>>> +		 * hardware level.
>>>>    		 */
>>>> -		if (pairwise && key_idx)
>>>> +		if (pairwise && key_idx > 1)
>>>>    			return -EINVAL;
>>>>    		break;
>>>
>>> Again, only if driver support is advertised, and the comment should
>>> probably reference the feature bit from the spec.
>>
>> That is where I added most of the sanity checks in the meantime.
>> But what feature bit from the spec are you referring to? The RSN
>> Capability one?
> 
> Well, I wasn't thinking that precisely. I just thought it should mention
> that it now says "IEEE802.11-2016 allows only 0 and 1 for pairwise keys"
> but doesn't clarify that this is only for stations that actually want to
> support it, so it could be read as being always that way.
> 
Makes sense. I'll reword that a bit.

[RFC PATCH v2 2/2] mac80211: Add support for Extended Key ID

[Alexander Wetzel]

Allow drivers using mac80211 to support Extended Key IDs.

Signed-off-by: Alexander Wetzel <alexander@wetzel-home.de>
---
 include/net/mac80211.h     |  6 +++++
 net/mac80211/cfg.c         | 30 ++++++++++++++++++++++++-
 net/mac80211/debugfs_sta.c |  1 +
 net/mac80211/key.c         | 46 ++++++++++++++++++++++++++++++--------
 net/mac80211/key.h         |  1 +
 net/mac80211/main.c        |  2 ++
 net/mac80211/sta_info.c    |  1 +
 net/mac80211/sta_info.h    |  1 +
 8 files changed, 78 insertions(+), 10 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 71985e95d2d9..fb53e7c84c01 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1643,6 +1643,10 @@ struct wireless_dev *ieee80211_vif_to_wdev(struct ieee80211_vif *vif);
  * @IEEE80211_KEY_FLAG_PUT_MIC_SPACE: This flag should be set by the driver for
  *	a TKIP key if it only requires MIC space. Do not set together with
  *	@IEEE80211_KEY_FLAG_GENERATE_MMIC on the same key.
+ * @IEEE80211_KEY_FLAG_RX_ONLY: Set by mac80211 to indicate that the key
+ *      must not be used for TX (yet).
+ * @IEEE80211_KEY_FLAG_SET_TX: Set by mac80211 to indicate that a previously
+ *      installed key with IEEE80211_KEY_FLAG_RX_ONLY should take over TX also.
  */
 enum ieee80211_key_flags {
 	IEEE80211_KEY_FLAG_GENERATE_IV_MGMT	= BIT(0),
@@ -1654,6 +1658,8 @@ enum ieee80211_key_flags {
 	IEEE80211_KEY_FLAG_RX_MGMT		= BIT(6),
 	IEEE80211_KEY_FLAG_RESERVE_TAILROOM	= BIT(7),
 	IEEE80211_KEY_FLAG_PUT_MIC_SPACE	= BIT(8),
+	IEEE80211_KEY_FLAG_RX_ONLY		= BIT(9),
+	IEEE80211_KEY_FLAG_SET_TX		= BIT(10),
 };
 
 /**
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 51622333d460..c0af820bc557 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -365,6 +365,25 @@ static int ieee80211_add_key(struct wiphy *wiphy, struct net_device *dev,
 	if (!ieee80211_sdata_running(sdata))
 		return -ENETDOWN;
 
+	if (pairwise && params->flag == NL80211_KEY_SET_TX) {
+		mutex_lock(&local->sta_mtx);
+		sta = sta_info_get_bss(sdata, mac_addr);
+
+		if (!sta ||
+		   !(key = rcu_dereference(sta->ptk[key_idx])) ||
+		   !(key->conf.flags | IEEE80211_KEY_FLAG_RX_ONLY)) {
+			err = -ENOENT;
+		} else {
+			key->conf.flags &= ~IEEE80211_KEY_FLAG_RX_ONLY;
+			key->conf.flags |= IEEE80211_KEY_FLAG_SET_TX;
+			err = ieee80211_key_hw_activate_tx(key);
+			if (!err)
+				sta->ptk_idx = key_idx;
+		}
+		mutex_unlock(&local->sta_mtx);
+		return err;
+	}
+
 	/* reject WEP and TKIP keys if WEP failed to initialize */
 	switch (params->cipher) {
 	case WLAN_CIPHER_SUITE_WEP40:
@@ -451,9 +470,18 @@ static int ieee80211_add_key(struct wiphy *wiphy, struct net_device *dev,
 		break;
 	}
 
-	if (sta)
+	if (sta) {
 		sta->cipher_scheme = cs;
 
+		/* Flag STA correctly when using Extended Key ID */
+		if (pairwise && params->flag == NL80211_KEY_RX_ONLY &&
+		    !test_sta_flag(sta, WLAN_STA_EXT_KEY_ID))
+			set_sta_flag(sta, WLAN_STA_EXT_KEY_ID);
+	}
+
+	if (params->flag == NL80211_KEY_RX_ONLY)
+		key->conf.flags |= IEEE80211_KEY_FLAG_RX_ONLY;
+
 	err = ieee80211_key_link(key, sdata, sta);
 
  out_unlock:
diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
index af5185a836e5..d63dca26d504 100644
--- a/net/mac80211/debugfs_sta.c
+++ b/net/mac80211/debugfs_sta.c
@@ -81,6 +81,7 @@ static const char * const sta_flag_names[] = {
 	FLAG(MPSP_OWNER),
 	FLAG(MPSP_RECIPIENT),
 	FLAG(PS_DELIVER),
+	FLAG(EXT_KEY_ID),
 #undef FLAG
 };
 
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index 4700718e010f..007f90a1f948 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -124,6 +124,36 @@ static void decrease_tailroom_need_count(struct ieee80211_sub_if_data *sdata,
 	sdata->crypto_tx_tailroom_needed_cnt -= delta;
 }
 
+int ieee80211_key_hw_activate_tx(struct ieee80211_key *key)
+{
+	struct ieee80211_sub_if_data *sdata = key->sdata;
+	struct sta_info *sta = key->sta;
+	int ret;
+
+	assert_key_lock(key->local);
+
+	if (!sta)
+		return -EOPNOTSUPP;
+
+	if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE))
+		/* No need to inform driver, report success */
+		return 0;
+
+	/* Inform driver that key is no longer RX only */
+	ret = drv_set_key(key->local, SET_KEY, sdata,
+			  &sta->sta, &key->conf);
+	if (ret) {
+		if (ret == 1)
+			/* Software crypto, report success */
+			return 0;
+		sdata_err(sdata,
+			  "failed to activate key for TX (%d, %pM) in hardware (%d)\n",
+			  key->conf.keyidx,
+			  sta->sta.addr, ret);
+	}
+	return ret;
+}
+
 static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
 {
 	struct ieee80211_sub_if_data *sdata = key->sdata;
@@ -261,7 +291,7 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
 
 static int ieee80211_hw_key_replace(struct ieee80211_key *old_key,
 				    struct ieee80211_key *new_key,
-				    bool ptk0rekey)
+				    bool pairwise)
 {
 	struct ieee80211_sub_if_data *sdata;
 	struct ieee80211_local *local;
@@ -278,8 +308,9 @@ static int ieee80211_hw_key_replace(struct ieee80211_key *old_key,
 	assert_key_lock(old_key->local);
 	sta = old_key->sta;
 
-	/* PTK only using key ID 0 needs special handling on rekey */
-	if (new_key && sta && ptk0rekey) {
+	/* PTK rekey without Extended Key ID needs special handling */
+	if (new_key && pairwise && sta &&
+	    !test_sta_flag(sta, WLAN_STA_EXT_KEY_ID)) {
 		local = old_key->local;
 		sdata = old_key->sdata;
 
@@ -395,10 +426,6 @@ static int ieee80211_key_replace(struct ieee80211_sub_if_data *sdata,
 
 	if (old) {
 		idx = old->conf.keyidx;
-		/* TODO: proper implement and test "Extended Key ID for
-		 * Individually Addressed Frames" from IEEE 802.11-2016.
-		 * Till then always assume only key ID 0 is used for
-		 * pairwise keys.*/
 		ret = ieee80211_hw_key_replace(old, new, pairwise);
 	} else {
 		/* new must be provided in case old is not */
@@ -415,8 +442,9 @@ static int ieee80211_key_replace(struct ieee80211_sub_if_data *sdata,
 	if (sta) {
 		if (pairwise) {
 			rcu_assign_pointer(sta->ptk[idx], new);
-			sta->ptk_idx = idx;
-			if (new) {
+			if (new &&
+			    !(new->conf.flags & IEEE80211_KEY_FLAG_RX_ONLY)) {
+				sta->ptk_idx = idx;
 				clear_sta_flag(sta, WLAN_STA_BLOCK_BA);
 				ieee80211_check_fast_xmit(sta);
 			}
diff --git a/net/mac80211/key.h b/net/mac80211/key.h
index ebdb80b85dc3..e0612a0b3adc 100644
--- a/net/mac80211/key.h
+++ b/net/mac80211/key.h
@@ -146,6 +146,7 @@ ieee80211_key_alloc(u32 cipher, int idx, size_t key_len,
 int ieee80211_key_link(struct ieee80211_key *key,
 		       struct ieee80211_sub_if_data *sdata,
 		       struct sta_info *sta);
+int ieee80211_key_hw_activate_tx(struct ieee80211_key *key);
 void ieee80211_key_free(struct ieee80211_key *key, bool delay_tailroom);
 void ieee80211_key_free_unused(struct ieee80211_key *key);
 void ieee80211_set_default_key(struct ieee80211_sub_if_data *sdata, int idx,
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 83e71e6b2ebe..8a8ca813494a 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -574,6 +574,8 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
 	wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_FILS_STA);
 	wiphy_ext_feature_set(wiphy,
 			      NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211);
+	/* !! DEVELOPMENT ONLY, must normally be set by driver !! */
+	wiphy_ext_feature_set(wiphy, NL80211_EXT_FEATURE_EXT_KEY_ID);
 
 	if (!ops->hw_scan) {
 		wiphy->features |= NL80211_FEATURE_LOW_PRIORITY_SCAN |
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index fb8c2252ac0e..2d83d8e13769 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -350,6 +350,7 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
 	sta->sta.max_rx_aggregation_subframes =
 		local->hw.max_rx_aggregation_subframes;
 
+	sta->ptk_idx = NUM_DEFAULT_KEYS - 1;
 	sta->local = local;
 	sta->sdata = sdata;
 	sta->rx_stats.last_rx = jiffies;
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 9a04327d71d1..7787e773a350 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -101,6 +101,7 @@ enum ieee80211_sta_info_flags {
 	WLAN_STA_MPSP_OWNER,
 	WLAN_STA_MPSP_RECIPIENT,
 	WLAN_STA_PS_DELIVER,
+	WLAN_STA_EXT_KEY_ID,
 
 	NUM_WLAN_STA_FLAGS,
 };
-- 
2.19.1

Re: [RFC PATCH v2 2/2] mac80211: Add support for Extended Key ID

[Johannes Berg]

> + * @IEEE80211_KEY_FLAG_RX_ONLY: Set by mac80211 to indicate that the key
> + *      must not be used for TX (yet).

I'm not sure that's relevant, since you have one key pointer for TX?

> + * @IEEE80211_KEY_FLAG_SET_TX: Set by mac80211 to indicate that a previously
> + *      installed key with IEEE80211_KEY_FLAG_RX_ONLY should take over TX also.

That also doesn't seem relevant ...

Oh, all of this is for HW offloads?

I _think_ I would prefer to have new key ops instead. Now you'd have 

SET_KEY / <empty flags>
SET_KEY / RX_ONLY
SET_KEY / SET_TX

but I think maybe

SET_KEY
SET_KEY_RX_ONLY
KEY_ENABLE_TX

would make more sense?

> +	if (pairwise && params->flag == NL80211_KEY_SET_TX) {
> +		mutex_lock(&local->sta_mtx);
> +		sta = sta_info_get_bss(sdata, mac_addr);
> +
> +		if (!sta ||
> +		   !(key = rcu_dereference(sta->ptk[key_idx])) ||

indentation here is off by one

> +		   !(key->conf.flags | IEEE80211_KEY_FLAG_RX_ONLY)) {

that makes no sense, should be & I guess

> -	/* PTK only using key ID 0 needs special handling on rekey */
> -	if (new_key && sta && ptk0rekey) {
> +	/* PTK rekey without Extended Key ID needs special handling */
> +	if (new_key && pairwise && sta &&
> +	    !test_sta_flag(sta, WLAN_STA_EXT_KEY_ID)) {
>  		local = old_key->local;
>  		sdata = old_key->sdata;

This seems wrong, even if you have ext key ID support and everything,
but you do 0 -> 0 rekeying, then you still need all the special handling
(in fact also then if you go 1->1!). So it seems you'd instead want to
see if you're going from a TX key to a TX key with the same key ID, and
then you don't need this flag at all.

> +++ b/net/mac80211/sta_info.c
> @@ -350,6 +350,7 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
>  	sta->sta.max_rx_aggregation_subframes =
>  		local->hw.max_rx_aggregation_subframes;
>  
> +	sta->ptk_idx = NUM_DEFAULT_KEYS - 1;

That makes no sense? Why should it be 3? That's invalid anyway?

johannes

Re: [RFC PATCH v2 2/2] mac80211: Add support for Extended Key ID

[Alexander Wetzel]

>> + * @IEEE80211_KEY_FLAG_RX_ONLY: Set by mac80211 to indicate that the key
>> + *      must not be used for TX (yet).
> 
> I'm not sure that's relevant, since you have one key pointer for TX?
> 
>> + * @IEEE80211_KEY_FLAG_SET_TX: Set by mac80211 to indicate that a previously
>> + *      installed key with IEEE80211_KEY_FLAG_RX_ONLY should take over TX also.
> 
> That also doesn't seem relevant ...
> 
> Oh, all of this is for HW offloads?
> 
> I _think_ I would prefer to have new key ops instead. Now you'd have
> 
> SET_KEY / <empty flags>
> SET_KEY / RX_ONLY
> SET_KEY / SET_TX
> 
> but I think maybe
> 
> SET_KEY
> SET_KEY_RX_ONLY
> KEY_ENABLE_TX
> 
> would make more sense?

Fine for me and should make it more understandable. So I'll try that.

> 
>> +	if (pairwise && params->flag == NL80211_KEY_SET_TX) {
>> +		mutex_lock(&local->sta_mtx);
>> +		sta = sta_info_get_bss(sdata, mac_addr);
>> +
>> +		if (!sta ||
>> +		   !(key = rcu_dereference(sta->ptk[key_idx])) ||
> 
> indentation here is off by one
> 
Thanks.

>> +		   !(key->conf.flags | IEEE80211_KEY_FLAG_RX_ONLY)) {
> 
> that makes no sense, should be & I guess
>
yes, I think that was one of the bugs I fixed the last weeks:-)


>> -	/* PTK only using key ID 0 needs special handling on rekey */
>> -	if (new_key && sta && ptk0rekey) {
>> +	/* PTK rekey without Extended Key ID needs special handling */
>> +	if (new_key && pairwise && sta &&
>> +	    !test_sta_flag(sta, WLAN_STA_EXT_KEY_ID)) {
>>   		local = old_key->local;
>>   		sdata = old_key->sdata;
> 
> This seems wrong, even if you have ext key ID support and everything,
> but you do 0 -> 0 rekeying, then you still need all the special handling
> (in fact also then if you go 1->1!). So it seems you'd instead want to
> see if you're going from a TX key to a TX key with the same key ID, and
> then you don't need this flag at all.
> 
The intention for Extended Key ID is, to have a comparable short time 
frame where both key IDs can be used. When replacing e.g. key ID 0 again 
it should be idle for a long time. I guess if someone starts re-keying 
in 1s intervals it may become an issue, but then anyone re-keying that 
often can't be helped...

With Extended Key IDs it's impossible to directly switch from a TX key 
with one key ID to another one with the same id.

1) Association
2) key ID 0 installed RX only
3) key Id 0 set_tx
4) rekey timeout passes
5) key ID 1 installed RX only
6) key ID 1 set_tx (also making key ID 0 RX only)
7) rekey timeout passes
8) key ID 0 replaced with new RX only key
9) key ID 0 set_tx
10) rekey timeout passes
...

So nobody will use the key being replaced, we don't have to protect 
against PN poisoning. And when a driver supports Extended Key ID we 
don't care about if the driver is able to rekey PTK0 correctly.


>> +++ b/net/mac80211/sta_info.c
>> @@ -350,6 +350,7 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
>>   	sta->sta.max_rx_aggregation_subframes =
>>   		local->hw.max_rx_aggregation_subframes;
>>   
>> +	sta->ptk_idx = NUM_DEFAULT_KEYS - 1;
> 
> That makes no sense? Why should it be 3? That's invalid anyway?

Yes, that's the whole reason for that change:-) Setting it to 2 would 
also be fine, as long as it's not 0 or 1.

ieee80211_tx_h_select_key starts encrypting packets as soon as 
sta->ptk[tx->sta->ptk_idx] is not null.

So installing the first key as RX only will also activate the key for 
TX. the AP will therefore encrypt EAPOL #3 of the initial connect...

To avoid expensive run time checks I simply switched the default setting 
to make sure sta->ptk[tx->sta->ptk_idx] will be NULL at the initial key 
install.

Alexander

Re: [RFC PATCH v2 2/2] mac80211: Add support for Extended Key ID

[Johannes Berg]

> > > -	/* PTK only using key ID 0 needs special handling on rekey */
> > > -	if (new_key && sta && ptk0rekey) {
> > > +	/* PTK rekey without Extended Key ID needs special handling */
> > > +	if (new_key && pairwise && sta &&
> > > +	    !test_sta_flag(sta, WLAN_STA_EXT_KEY_ID)) {
> > >   		local = old_key->local;
> > >   		sdata = old_key->sdata;
> > 
> > This seems wrong, even if you have ext key ID support and everything,
> > but you do 0 -> 0 rekeying, then you still need all the special handling
> > (in fact also then if you go 1->1!). So it seems you'd instead want to
> > see if you're going from a TX key to a TX key with the same key ID, and
> > then you don't need this flag at all.
> > 
> 
> The intention for Extended Key ID is, to have a comparable short time 
> frame where both key IDs can be used. When replacing e.g. key ID 0 again 
> it should be idle for a long time. I guess if someone starts re-keying 
> in 1s intervals it may become an issue, but then anyone re-keying that 
> often can't be helped...

Sure. But ... not sure how that's related?

> With Extended Key IDs it's impossible to directly switch from a TX key 
> with one key ID to another one with the same id.
> 
> 1) Association
> 2) key ID 0 installed RX only
> 3) key Id 0 set_tx
> 4) rekey timeout passes
> 5) key ID 1 installed RX only
> 6) key ID 1 set_tx (also making key ID 0 RX only)
> 7) rekey timeout passes
> 8) key ID 0 replaced with new RX only key
> 9) key ID 0 set_tx
> 10) rekey timeout passes
> ...
> 
> So nobody will use the key being replaced, we don't have to protect 
> against PN poisoning.

Exactly.

> And when a driver supports Extended Key ID we 
> don't care about if the driver is able to rekey PTK0 correctly.

Strictly speaking, that's false, since you don't know if wpa_s actually
used it, and the peer STA allowed it.

It's also not what you implemented, you implemented checking if
NL80211_KEY_RX_ONLY was ever used.

However, what I'm trying to say is that I'm not sure this makes sense?

It seems to me it would be safer, and easier (no station flag), to just
check

 if ("we're replacing the current TX key")

and trigger the workarounds in that case. No?

Yes, parts of the issue also manifest themselves on the RX side, but if
you're not replacing the current key then you were using extended key ID
support?

> > > +++ b/net/mac80211/sta_info.c
> > > @@ -350,6 +350,7 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
> > >   	sta->sta.max_rx_aggregation_subframes =
> > >   		local->hw.max_rx_aggregation_subframes;
> > >   
> > > +	sta->ptk_idx = NUM_DEFAULT_KEYS - 1;
> > 
> > That makes no sense? Why should it be 3? That's invalid anyway?
> 
> Yes, that's the whole reason for that change:-) Setting it to 2 would 
> also be fine, as long as it's not 0 or 1.

Hmm, ok. So that probably wants a big comment saying that it relies on
key idx 2/3 being invalid. I'm not sure I like the NUM_DEFAULT_KEYS-1,
better perhaps to do something like

	/* comment saying why */
	BUILD_BUG_ON(ARRAY_SIZE(sta->ptks) > 2);
	sta->ptk_idx = 2;

or so?

> ieee80211_tx_h_select_key starts encrypting packets as soon as 
> sta->ptk[tx->sta->ptk_idx] is not null.

Right, so I guess this makes sense.

johannes

Re: [RFC PATCH v2 2/2] mac80211: Add support for Extended Key ID

[Alexander Wetzel]

> 
>>>> -	/* PTK only using key ID 0 needs special handling on rekey */
>>>> -	if (new_key && sta && ptk0rekey) {
>>>> +	/* PTK rekey without Extended Key ID needs special handling */
>>>> +	if (new_key && pairwise && sta &&
>>>> +	    !test_sta_flag(sta, WLAN_STA_EXT_KEY_ID)) {
>>>>    		local = old_key->local;
>>>>    		sdata = old_key->sdata;
>>>
>>> This seems wrong, even if you have ext key ID support and everything,
>>> but you do 0 -> 0 rekeying, then you still need all the special handling
>>> (in fact also then if you go 1->1!). So it seems you'd instead want to
>>> see if you're going from a TX key to a TX key with the same key ID, and
>>> then you don't need this flag at all.
>>>
>>
>> The intention for Extended Key ID is, to have a comparable short time
>> frame where both key IDs can be used. When replacing e.g. key ID 0 again
>> it should be idle for a long time. I guess if someone starts re-keying
>> in 1s intervals it may become an issue, but then anyone re-keying that
>> often can't be helped...
> 
> Sure. But ... not sure how that's related?
> 
>> With Extended Key IDs it's impossible to directly switch from a TX key
>> with one key ID to another one with the same id.
>>
>> 1) Association
>> 2) key ID 0 installed RX only
>> 3) key Id 0 set_tx
>> 4) rekey timeout passes
>> 5) key ID 1 installed RX only
>> 6) key ID 1 set_tx (also making key ID 0 RX only)
>> 7) rekey timeout passes
>> 8) key ID 0 replaced with new RX only key
>> 9) key ID 0 set_tx
>> 10) rekey timeout passes
>> ...
>>
>> So nobody will use the key being replaced, we don't have to protect
>> against PN poisoning.
> 
> Exactly.
> 
>> And when a driver supports Extended Key ID we
>> don't care about if the driver is able to rekey PTK0 correctly.
> 
> Strictly speaking, that's false, since you don't know if wpa_s actually
> used it, and the peer STA allowed it.
> 
> It's also not what you implemented, you implemented checking if
> NL80211_KEY_RX_ONLY was ever used.
> 
> However, what I'm trying to say is that I'm not sure this makes sense?
> 
> It seems to me it would be safer, and easier (no station flag), to just
> check
> 
>   if ("we're replacing the current TX key")
> 
> and trigger the workarounds in that case. No?
> 
> Yes, parts of the issue also manifest themselves on the RX side, but if
> you're not replacing the current key then you were using extended key ID
> support?
> 
Ah, now I get it:-)
Will try that out also.


>>>> +++ b/net/mac80211/sta_info.c
>>>> @@ -350,6 +350,7 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
>>>>    	sta->sta.max_rx_aggregation_subframes =
>>>>    		local->hw.max_rx_aggregation_subframes;
>>>>    
>>>> +	sta->ptk_idx = NUM_DEFAULT_KEYS - 1;
>>>
>>> That makes no sense? Why should it be 3? That's invalid anyway?
>>
>> Yes, that's the whole reason for that change:-) Setting it to 2 would
>> also be fine, as long as it's not 0 or 1.
> 
> Hmm, ok. So that probably wants a big comment saying that it relies on
> key idx 2/3 being invalid. I'm not sure I like the NUM_DEFAULT_KEYS-1,
> better perhaps to do something like
> 
> 	/* comment saying why */
> 	BUILD_BUG_ON(ARRAY_SIZE(sta->ptks) > 2);
> 	sta->ptk_idx = 2;
> 
> or so?
> 
>> ieee80211_tx_h_select_key starts encrypting packets as soon as
>> sta->ptk[tx->sta->ptk_idx] is not null.
> 
> Right, so I guess this makes sense.
> 
> johannes
>Thank you very much for all the helpful tips and suggestions!

Alexander

Re: [RFC PATCH v2 0/2] Extended Key ID support for linux

[Johannes Berg]

Hi,

Sorry for the delay.

On Sun, 2018-11-11 at 12:02 +0100, Alexander Wetzel wrote:
> IEEE 802.11-2012 added support for Extended Key ID, allowing pairwise
> keys to also use keyID 1 and moving group keys to IDs 2 and 3.

Where do you read this? I've always been under the impression that
individually and group addressed frames use key IDs from different
"namespaces", so to speak, where PTK/STK can use 0 (0 or 1 with
"Extended Key ID" support) and GTK can use 0-3.

In fact, the per-frame pseudocode in 802.11-2016 12.9.2.6 clearly
states:

if MPDU has individual RA then
	lookup pairwise key using Key ID from MPDU
else
	lookup group key using Key ID from MPDU
endif

If it weren't different namespaces, you'd not have to differentiate
here.

> Support for Extended Key ID is basically completed and confirmed working
> with both hwsim and "on the air" with ath9k/iwldvm using software
> encryption and those patches here.

:)

> Prior to propose this patch for merging I would like to get Extended
> Key ID working with HW encryption for at least some devices, but after
> experimenting with ath9k and to a lesser extend with ath10k it's now
> clear that this will be an per-driver effort and it may well turn out to
> be impossible without firmware updates.

Indeed. I think there might be some support with iwlwifi firmware, at
least newer versions? I can check later.

> So I've decided to continue working on the HW support for now but also
> ask you for feedback for what I got so far. 

Sounds good.

> Any feedback is welcome and I especially like to learn what you think of
> the API extensions and what has to be changed to get it merged.

I'll look over the individual patches.

johannes

Re: [RFC PATCH v2 0/2] Extended Key ID support for linux

[Alexander Wetzel]

> Hi,
> 
> Sorry for the delay.
> No problem. That's hardly urgent:-)

> On Sun, 2018-11-11 at 12:02 +0100, Alexander Wetzel wrote:
>> IEEE 802.11-2012 added support for Extended Key ID, allowing pairwise
>> keys to also use keyID 1 and moving group keys to IDs 2 and 3.
> 
> Where do you read this? I've always been under the impression that
> individually and group addressed frames use key IDs from different
> "namespaces", so to speak, where PTK/STK can use 0 (0 or 1 with
> "Extended Key ID" support) and GTK can use 0-3.
> 
> In fact, the per-frame pseudocode in 802.11-2016 12.9.2.6 clearly
> states:
> 
> if MPDU has individual RA then
> 	lookup pairwise key using Key ID from MPDU
> else
> 	lookup group key using Key ID from MPDU
> endif
> 
> If it weren't different namespaces, you'd not have to differentiate
> here.
> 

I was indeed struggling to understand what the intend of the standard is 
here. I may well be wrong, but the note in "12.6.1.1.10 Mesh GTKSA" 
tipped the scales to assume keyIDs are within one namespace only.

"Since Key ID 0 is reserved for individually addressed frame 
transmission, there are at most three available Key IDs (only two if 
extended Key IDs for individually addressed frames are in use), and the 
different MGTKs would contend for the single remaining Key ID upon 
rollover."

I got the impression Extended Key IDs were  added without updating all 
sections which should get updates. But the pattern is suspect, even the 
igtk numbers fit into the pattern:

  PTK 0 & 1
  GTK 1 & 2 & 3
iGTK 4 & 5

That may well be utterly wrong... Any idea how can we sort that out?

>> Support for Extended Key ID is basically completed and confirmed working
>> with both hwsim and "on the air" with ath9k/iwldvm using software
>> encryption and those patches here.
> 
> :)
> 
>> Prior to propose this patch for merging I would like to get Extended
>> Key ID working with HW encryption for at least some devices, but after
>> experimenting with ath9k and to a lesser extend with ath10k it's now
>> clear that this will be an per-driver effort and it may well turn out to
>> be impossible without firmware updates.
> 
> Indeed. I think there might be some support with iwlwifi firmware, at
> least newer versions? I can check later.
>
>> So I've decided to continue working on the HW support for now but also
>> ask you for feedback for what I got so far.
> 
> Sounds good.
> 

I think I've solved the HW support issue, it looks like we'll be able to 
support Extended Key IDs with minimal changes to the drivers in a 
compatibility mode. It's basically working with iwldvm and ath9k but 
needs some more work.

Alexander

Re: [RFC PATCH v2 0/2] Extended Key ID support for linux

[Jouni Malinen]

On Wed, Dec 05, 2018 at 08:06:33PM +0100, Alexander Wetzel wrote:
> >On Sun, 2018-11-11 at 12:02 +0100, Alexander Wetzel wrote:
> >>IEEE 802.11-2012 added support for Extended Key ID, allowing pairwise
> >>keys to also use keyID 1 and moving group keys to IDs 2 and 3.
> >
> >Where do you read this? I've always been under the impression that
> >individually and group addressed frames use key IDs from different
> >"namespaces", so to speak, where PTK/STK can use 0 (0 or 1 with
> >"Extended Key ID" support) and GTK can use 0-3.

There is no such requirement to change group key IDs in the IEEE 802.11
standard when taking Extended Key ID mechanism into use.

> >In fact, the per-frame pseudocode in 802.11-2016 12.9.2.6 clearly
> >states:
> >
> >if MPDU has individual RA then
> >	lookup pairwise key using Key ID from MPDU
> >else
> >	lookup group key using Key ID from MPDU
> >endif
> >
> >If it weren't different namespaces, you'd not have to differentiate
> >here.
> >
> 
> I was indeed struggling to understand what the intend of the standard is
> here. I may well be wrong, but the note in "12.6.1.1.10 Mesh GTKSA" tipped
> the scales to assume keyIDs are within one namespace only.
> 
> "Since Key ID 0 is reserved for individually addressed frame transmission,
> there are at most three available Key IDs (only two if extended Key IDs for
> individually addressed frames are in use), and the different MGTKs would
> contend for the single remaining Key ID upon rollover."

Please note that this is is an informative note and not normative part
of the standard. IMHO, that note is not accurate and it looks likely
that it was added without full understanding on how the keys are used in
the standard..

> I got the impression Extended Key IDs were  added without updating all
> sections which should get updates. But the pattern is suspect, even the igtk
> numbers fit into the pattern:
> 
>  PTK 0 & 1
>  GTK 1 & 2 & 3
> iGTK 4 & 5

An AP is allowed to do this, but there is no requirement for doing so.
The pairwise key (TK, not PTK) is required to use Key ID 0 unless the
optional Extended Key ID for Individually Addressed Frames capability is
negotiated (and 0 or 1 if that capability is negotiated). Group keys
(GTK) are allowed to use Key IDs 0..4. IGTKs are allowed to use Key ID
values 4 and 5.

There is a long history behind this and some de facto constraints due to
that history and possible implementation constraints. However, as far as
the protocol itself is concerned, there would be no real need for having
IGTK use 4..5; it could have as well been 0..1 or 1..2 or whatever
combination the AP would like to use.

These three cases have completely independent namespaces for Key IDs as
far as RSN is concerned with one exception: "Use group cipher suite"
that was added as an option for some AP implementation that did not
support individual key mapping. That special case would end up using GTK
for both group-addressed and individually-addressed frames. That said,
I'm not aware of there having ever been an actually deployed device with
this constraint and even if there were, this mode is highly discouraged
and should not be used for anything today. Anyway, this exception and
similar implementation constraints are likely behind the expectations of
TK and GTK having to use different Key ID values.

As far as the kernel changes are concerned, cfg80211 and mac80211 should
support everything that's allowed by the standard, i.e., use of Key IDs
0..3 for GTK. It is up to the user space implementation on the AP side
(e.g., hostapd) to select which Key IDs are actually taken into use.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [RFC PATCH v2 0/2] Extended Key ID support for linux

[Alexander Wetzel]

>> I got the impression Extended Key IDs were  added without updating all
>> sections which should get updates. But the pattern is suspect, even the igtk
>> numbers fit into the pattern:
>>
>>   PTK 0 & 1
>>   GTK 1 & 2 & 3
>> iGTK 4 & 5
> 
> An AP is allowed to do this, but there is no requirement for doing so.
> The pairwise key (TK, not PTK) is required to use Key ID 0 unless the > optional Extended Key ID for Individually Addressed Frames capability is
> negotiated (and 0 or 1 if that capability is negotiated). Group keys
> (GTK) are allowed to use Key IDs 0..4. IGTKs are allowed to use Key ID
> values 4 and 5.
> 
> There is a long history behind this and some de facto constraints due to
> that history and possible implementation constraints. However, as far as
> the protocol itself is concerned, there would be no real need for having
> IGTK use 4..5; it could have as well been 0..1 or 1..2 or whatever
> combination the AP would like to use.
> 
> These three cases have completely independent namespaces for Key IDs as
> far as RSN is concerned with one exception: "Use group cipher suite"
> that was added as an option for some AP implementation that did not
> support individual key mapping. That special case would end up using GTK
> for both group-addressed and individually-addressed frames. That said,
> I'm not aware of there having ever been an actually deployed device with
> this constraint and even if there were, this mode is highly discouraged
> and should not be used for anything today. Anyway, this exception and
> similar implementation constraints are likely behind the expectations of
> TK and GTK having to use different Key ID values.

Thanks for the clarifications!
If there really are drivers using "Use group cipher suite" it does not 
sound like they would be able to support Extended Key ID with APs using 
key ID 1+2 for GTKs. But sounds not likely anyone would need that...

My experimentation with hw accel and Extended Key ID for existing 
drivers so far are also indicating that it will work using the keyid 1 
for both PTK and GTK, so this should be a trivial change in hostapd only.

> 
> As far as the kernel changes are concerned, cfg80211 and mac80211 should
> support everything that's allowed by the standard, i.e., use of Key IDs
> 0..3 for GTK. It is up to the user space implementation on the AP side
> (e.g., hostapd) to select which Key IDs are actually taken into use.
> 

I'm pretty sure that is already the case, but so far I only tested it 
with GTK shifted to 2+3. I'll make sure to test the next revision with 
GTK using 1+2 also. I'll test that once I get that working again from 
end2end. The patch here is getting a bit dated and makes no sense to 
invest time for that win it.

Alexander