[RFC 0/4] EAPoL over NL80211

[RFC 0/4] EAPoL over NL80211

[Denis Kenzior]

This patchset adds support for running 802.11 authentication mechanisms (e.g.
802.1X, 4-Way Handshake, etc) over NL80211 instead of putting them onto the
network device.  This has the advantage of fixing several long-standing race
conditions that result from userspace operating on multiple transports in order
to manage a 802.11 connection (e.g. NL80211 and wireless netdev, wlan0, etc)

For example, userspace would sometimes see 4-Way handshake packets before
NL80211 signaled that the connection has been established.  Leading to ugly
hacks or having the STA wait for retransmissions from the AP.

To make this possible this patchset introduces a new NL80211 command and several
new attributes.  A userspace that is capable of processing EAPoL packets over
NL80211 includes a new NL80211_ATTR_CONTROL_PORT_OVER_NL80211 attribute in its
NL80211_CMD_ASSOCIATE or NL80211_CMD_CONNECT requests being sent to the kernel.
The previously added NL80211_ATTR_SOCKET_OWNER attribute must also be included.
The latter is used by the kernel to send NL80211_CMD_CONTROL_PORT_FRAME
notifications back to userspace via a netlink unicast.  If the
NL80211_ATTR_CONTROL_PORT_OVER_NL80211 attribute is not specified, then legacy
behavior is kept and control port packets continue to flow over the network
interface.

If control port over nl80211 transport is requested, then control port packets
are intercepted just prior to being handed to the network device and sent over
netlink via the NL80211_CMD_CONTROL_PORT_FRAME notification.  NL80211_ATTR_CONTROL_PORT_ETHERTYPE and NL80211_ATTR_MAC are included to specify the control port
frame protocol and source address respectively.  If the control port frame was
received unencrypted then NL80211_ATTR_CONTROL_PORT_NO_ENCRYPT flag is also
included.  NL80211_ATTR_FRAME attribute contains the raw control port frame with
all transport layer headers stripped (e.g. this would be the raw EAPoL frame).

Userspace can reply to control port frames either via legacy methods (by sending
frames to the network device) or via NL80211_CMD_CONTROL_PORT_FRAME request.
Userspace would included NL80211_ATTR_FRAME with the raw control port frame.

Open Questions
==============

1. It is not clear whether all drivers provide information as to whether the
control port frame was sent encrypted or not.  Do they?

2. It has been previously suggested that CMD_FRAME infrastructure is used to
accomplish control port over nl80211 transport.  However, it did not seem to be
a good fit as the relevant code paths assume that only management frames are
to be sent via this mechanism.  Thoughts?

3. I'm not sure that the TX implementation is the right way to go.  It has been
suggested before that userspace should be able to specify whether a given
control port frame should be encrypted or not.  The current implementation would
still try to honor the global CONTROL_PORT_NO_ENCRYPT flag but I didn't find a
good way to specify this for individual frames.  Ideas?

4. It would be nice to extend this functionaly for Pre-Authentiation frames
(protocol 0x88c7) as well so that userspace does not have to deal with these
using a different transport.

The proposed patchset has been tested in a mac80211_hwsim based environment with
hostapd and iwd.

Denis Kenzior (4):
  nl80211: Add CONTROL_PORT_OVER_NL80211 attribute
  nl80211: Add CMD_CONTROL_PORT_FRAME API
  mac80211: Send control port frames over nl80211
  nl80211: Implement TX of control port frames

 include/net/cfg80211.h       |  18 ++++++
 include/uapi/linux/nl80211.h |  29 ++++++++-
 net/mac80211/cfg.c           |   2 +
 net/mac80211/ieee80211_i.h   |   1 +
 net/mac80211/mlme.c          |   2 +
 net/mac80211/rx.c            |  32 +++++++++-
 net/wireless/nl80211.c       | 139 ++++++++++++++++++++++++++++++++++++++++++-
 net/wireless/trace.h         |  15 +++++
 8 files changed, 233 insertions(+), 5 deletions(-)

-- 
2.13.5

[RFC 1/4] nl80211: Add CONTROL_PORT_OVER_NL80211 attribute

[Denis Kenzior]

Signed-off-by: Denis Kenzior <denkenz@gmail.com>
---
 include/net/cfg80211.h       |  3 +++
 include/uapi/linux/nl80211.h | 14 +++++++++++++-
 net/mac80211/cfg.c           |  2 ++
 net/mac80211/ieee80211_i.h   |  1 +
 net/mac80211/mlme.c          |  2 ++
 net/wireless/nl80211.c       | 10 ++++++++++
 6 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 3a4a1a903a4d..0aa1c866a73b 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -646,6 +646,8 @@ struct survey_info {
  *	allowed through even on unauthorized ports
  * @control_port_no_encrypt: TRUE to prevent encryption of control port
  *	protocol frames.
+ * @control_port_over_nl80211: TRUE if userspace expects to exchange control
+ *	port frames over NL80211 instead of the network interface.
  * @wep_keys: static WEP keys, if not NULL points to an array of
  *	CFG80211_MAX_WEP_KEYS WEP keys
  * @wep_tx_key: key index (0..3) of the default TX static WEP key
@@ -661,6 +663,7 @@ struct cfg80211_crypto_settings {
 	bool control_port;
 	__be16 control_port_ethertype;
 	bool control_port_no_encrypt;
+	bool control_port_over_nl80211;
 	struct key_params *wep_keys;
 	int wep_tx_key;
 	const u8 *psk;
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index c587a61c32bf..8855b7eaf92c 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -542,7 +542,8 @@
  *	IEs in %NL80211_ATTR_IE, %NL80211_ATTR_AUTH_TYPE, %NL80211_ATTR_USE_MFP,
  *	%NL80211_ATTR_MAC, %NL80211_ATTR_WIPHY_FREQ, %NL80211_ATTR_CONTROL_PORT,
  *	%NL80211_ATTR_CONTROL_PORT_ETHERTYPE,
- *	%NL80211_ATTR_CONTROL_PORT_NO_ENCRYPT, %NL80211_ATTR_MAC_HINT, and
+ *	%NL80211_ATTR_CONTROL_PORT_NO_ENCRYPT,
+ *	%NL80211_ATTR_CONTROL_PORT_OVER_NL80211, %NL80211_ATTR_MAC_HINT, and
  *	%NL80211_ATTR_WIPHY_FREQ_HINT.
  *	If included, %NL80211_ATTR_MAC and %NL80211_ATTR_WIPHY_FREQ are
  *	restrictions on BSS selection, i.e., they effectively prevent roaming
@@ -1445,6 +1446,15 @@ enum nl80211_commands {
  * @NL80211_ATTR_CONTROL_PORT_NO_ENCRYPT: When included along with
  *	%NL80211_ATTR_CONTROL_PORT_ETHERTYPE, indicates that the custom
  *	ethertype frames used for key negotiation must not be encrypted.
+ * @NL80211_ATTR_CONTROL_PORT_OVER_NL80211: A flag indicating whether control
+ *	port frames (e.g. of type given in %NL80211_ATTR_CONTROL_PORT_ETHERTYPE)
+ *	will be sent directly to the network interface or sent via the NL80211
+ *	socket.  If this attribute is missing, then legacy behavior of sending
+ *	control port frames directly to the network interface is used.  If the
+ *	flag is included, then control port frames are sent over NL80211 instead
+ *	using %CMD_CONTROL_PORT_FRAME.  If control port routing over NL80211 is
+ *	to be used then userspace must also use the %NL80211_ATTR_SOCKET_OWNER
+ *	flag.
  *
  * @NL80211_ATTR_TESTDATA: Testmode data blob, passed through to the driver.
  *	We recommend using nested, driver-specific attributes within this.
@@ -2579,6 +2589,8 @@ enum nl80211_attrs {
 	NL80211_ATTR_PMKR0_NAME,
 	NL80211_ATTR_PORT_AUTHORIZED,
 
+	NL80211_ATTR_CONTROL_PORT_OVER_NL80211,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index b77ee342b5f8..b6f37ae5e3be 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -925,6 +925,7 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
 	 */
 	sdata->control_port_protocol = params->crypto.control_port_ethertype;
 	sdata->control_port_no_encrypt = params->crypto.control_port_no_encrypt;
+	sdata->control_port_over_nl80211 = false;
 	sdata->encrypt_headroom = ieee80211_cs_headroom(sdata->local,
 							&params->crypto,
 							sdata->vif.type);
@@ -934,6 +935,7 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
 			params->crypto.control_port_ethertype;
 		vlan->control_port_no_encrypt =
 			params->crypto.control_port_no_encrypt;
+		vlan->control_port_over_nl80211 = false;
 		vlan->encrypt_headroom =
 			ieee80211_cs_headroom(sdata->local,
 					      &params->crypto,
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 26900025de2f..6f91aea6a4cb 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -899,6 +899,7 @@ struct ieee80211_sub_if_data {
 	u16 sequence_number;
 	__be16 control_port_protocol;
 	bool control_port_no_encrypt;
+	bool control_port_over_nl80211;
 	int encrypt_headroom;
 
 	atomic_t num_tx_queued;
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 39b660b9a908..fc71a906939b 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -4830,6 +4830,8 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata,
 
 	sdata->control_port_protocol = req->crypto.control_port_ethertype;
 	sdata->control_port_no_encrypt = req->crypto.control_port_no_encrypt;
+	sdata->control_port_over_nl80211 =
+					req->crypto.control_port_over_nl80211;
 	sdata->encrypt_headroom = ieee80211_cs_headroom(local, &req->crypto,
 							sdata->vif.type);
 
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index b3f8970c3a47..f5b4008f40da 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -286,6 +286,7 @@ static const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
 	[NL80211_ATTR_CONTROL_PORT] = { .type = NLA_FLAG },
 	[NL80211_ATTR_CONTROL_PORT_ETHERTYPE] = { .type = NLA_U16 },
 	[NL80211_ATTR_CONTROL_PORT_NO_ENCRYPT] = { .type = NLA_FLAG },
+	[NL80211_ATTR_CONTROL_PORT_OVER_NL80211] = { .type = NLA_FLAG },
 	[NL80211_ATTR_PRIVACY] = { .type = NLA_FLAG },
 	[NL80211_ATTR_CIPHER_SUITE_GROUP] = { .type = NLA_U32 },
 	[NL80211_ATTR_WPA_VERSIONS] = { .type = NLA_U32 },
@@ -8209,6 +8210,15 @@ static int nl80211_crypto_settings(struct cfg80211_registered_device *rdev,
 	} else
 		settings->control_port_ethertype = cpu_to_be16(ETH_P_PAE);
 
+	if (info->attrs[NL80211_ATTR_CONTROL_PORT_OVER_NL80211]) {
+		if (!info->attrs[NL80211_ATTR_SOCKET_OWNER])
+			return -EINVAL;
+
+		settings->control_port_over_nl80211 = true;
+	} else {
+		settings->control_port_over_nl80211 = false;
+	}
+
 	if (info->attrs[NL80211_ATTR_CIPHER_SUITES_PAIRWISE]) {
 		void *data;
 		int len, i;
-- 
2.13.5

[RFC 2/4] nl80211: Add CMD_CONTROL_PORT_FRAME API

[Denis Kenzior]

This commit also adds cfg80211_rx_control_port function.  This is used
to generate a CMD_CONTROL_PORT_FRAME event out to userspace.  The
conn_owner_nlportid is used as the unicast destination.  This means that
userspace must specify NL80211_ATTR_SOCKET_OWNER flag if control port
over nl80211 routing is requested in NL80211_CMD_CONNECT or
NL80211_CMD_ASSOCIATE

Signed-off-by: Denis Kenzior <denkenz@gmail.com>
---
 include/net/cfg80211.h       | 15 +++++++++++
 include/uapi/linux/nl80211.h | 15 +++++++++++
 net/wireless/nl80211.c       | 62 ++++++++++++++++++++++++++++++++++++++++++++
 net/wireless/trace.h         | 15 +++++++++++
 4 files changed, 107 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 0aa1c866a73b..cc480e252367 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5626,6 +5626,21 @@ void cfg80211_mgmt_tx_status(struct wireless_dev *wdev, u64 cookie,
 
 
 /**
+ * cfg80211_rx_control_port - inform userspace about a received control port
+ * frame, e.g. EAPoL.  This is used if userspace has specified it wants to
+ * receive control port frames over NL80211.
+ * @dev: The device the frame matched to
+ * @buf: control port frame
+ * @len: length of the frame data
+ * @unencrypted: Whether the frame was received unencrypted
+ *
+ * Return: %true if the frame was passed to userspace
+ */
+bool cfg80211_rx_control_port(struct wireless_dev *wdev,
+			      const u8 *buf, size_t len,
+			      const u8 *addr, u16 proto, bool unencrypted);
+
+/**
  * cfg80211_cqm_rssi_notify - connection quality monitoring rssi event
  * @dev: network device
  * @rssi_event: the triggered RSSI event
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 8855b7eaf92c..b902614e876e 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -991,6 +991,17 @@
  *	&NL80211_CMD_CONNECT or &NL80211_CMD_ROAM. If the 4 way handshake failed
  *	&NL80211_CMD_DISCONNECT should be indicated instead.
  *
+ * @NL80211_CMD_CONTROL_PORT_FRAME: Control Port (e.g. PAE) frame TX request
+ *	and RX notification.  This command is used both as a request to transmit
+ *	a control port frame and as a notification that a control port frame
+ *	has been received. %NL80211_ATTR_FRAME is used to specify the
+ *	frame contents.  The frame is the raw EAPoL data, without ethernet or
+ *	802.11 headers.
+ *	When used as an event indication %NL80211_ATTR_CONTROL_PORT_ETHERTYPE,
+ *	%NL80211_ATTR_CONTROL_PORT_NO_ENCRYPT and %NL80211_ATTR_MAC are added
+ *	indicating the protocol type of the received frame; whether the frame
+ *	was received unencrypted and the MAC address of the peer respectively.
+ *
  * @NL80211_CMD_RELOAD_REGDB: Request that the regdb firmware file is reloaded.
  *
  * @NL80211_CMD_MAX: highest used command number
@@ -1199,6 +1210,8 @@ enum nl80211_commands {
 
 	NL80211_CMD_RELOAD_REGDB,
 
+	NL80211_CMD_CONTROL_PORT_FRAME,
+
 	/* add new commands above here */
 
 	/* used to define NL80211_CMD_MAX below */
@@ -1446,6 +1459,8 @@ enum nl80211_commands {
  * @NL80211_ATTR_CONTROL_PORT_NO_ENCRYPT: When included along with
  *	%NL80211_ATTR_CONTROL_PORT_ETHERTYPE, indicates that the custom
  *	ethertype frames used for key negotiation must not be encrypted.
+ *	When included in %NL80211_CMD_CONTROL_PORT_FRAME it means that the
+ *	control port frame was received unencrypted.
  * @NL80211_ATTR_CONTROL_PORT_OVER_NL80211: A flag indicating whether control
  *	port frames (e.g. of type given in %NL80211_ATTR_CONTROL_PORT_ETHERTYPE)
  *	will be sent directly to the network interface or sent via the NL80211
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index f5b4008f40da..220fe5bc57fd 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -14490,6 +14490,68 @@ void cfg80211_mgmt_tx_status(struct wireless_dev *wdev, u64 cookie,
 }
 EXPORT_SYMBOL(cfg80211_mgmt_tx_status);
 
+static int __nl80211_control_port(struct cfg80211_registered_device *rdev,
+				  struct wireless_dev *wdev,
+				  const u8 *buf, size_t len,
+				  const u8 *addr, u16 proto,
+				  bool unencrypted, gfp_t gfp)
+{
+	struct net_device *netdev = wdev->netdev;
+	struct sk_buff *msg;
+	void *hdr;
+	u32 nlportid = READ_ONCE(wdev->conn_owner_nlportid);
+
+	if (!nlportid)
+		return -ENOENT;
+
+	msg = nlmsg_new(100 + len, gfp);
+	if (!msg)
+		return -ENOMEM;
+
+	hdr = nl80211hdr_put(msg, 0, 0, 0, NL80211_CMD_CONTROL_PORT_FRAME);
+	if (!hdr) {
+		nlmsg_free(msg);
+		return -ENOMEM;
+	}
+
+	if (nla_put_u32(msg, NL80211_ATTR_WIPHY, rdev->wiphy_idx) ||
+	    (netdev && nla_put_u32(msg, NL80211_ATTR_IFINDEX,
+					netdev->ifindex)) ||
+	    nla_put_u64_64bit(msg, NL80211_ATTR_WDEV, wdev_id(wdev),
+			      NL80211_ATTR_PAD) ||
+	    nla_put(msg, NL80211_ATTR_FRAME, len, buf) ||
+	    nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, addr) ||
+	    nla_put_u16(msg, NL80211_ATTR_CONTROL_PORT_ETHERTYPE, proto) ||
+	    (unencrypted && nla_put_flag(msg,
+					 NL80211_ATTR_CONTROL_PORT_NO_ENCRYPT)))
+		goto nla_put_failure;
+
+	genlmsg_end(msg, hdr);
+
+	return genlmsg_unicast(wiphy_net(&rdev->wiphy), msg, nlportid);
+
+ nla_put_failure:
+	genlmsg_cancel(msg, hdr);
+	nlmsg_free(msg);
+	return -ENOBUFS;
+}
+
+bool cfg80211_rx_control_port(struct wireless_dev *wdev,
+			      const u8 *buf, size_t len,
+			      const u8 *addr, u16 proto, bool unencrypted)
+{
+	struct wiphy *wiphy = wdev->wiphy;
+	struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
+	bool ret;
+
+	trace_cfg80211_rx_control_port(wdev, unencrypted);
+	ret = __nl80211_control_port(rdev, wdev, buf, len, addr, proto,
+				     unencrypted, GFP_ATOMIC);
+	trace_cfg80211_return_bool(ret);
+	return ret;
+}
+EXPORT_SYMBOL(cfg80211_rx_control_port);
+
 static struct sk_buff *cfg80211_prepare_cqm(struct net_device *dev,
 					    const char *mac, gfp_t gfp)
 {
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index bcfedd39e7a3..1703a0fa315b 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -2577,6 +2577,21 @@ TRACE_EVENT(cfg80211_mgmt_tx_status,
 		  WDEV_PR_ARG, __entry->cookie, BOOL_TO_STR(__entry->ack))
 );
 
+TRACE_EVENT(cfg80211_rx_control_port,
+	TP_PROTO(struct wireless_dev *wdev, bool unencrypted),
+	TP_ARGS(wdev, unencrypted),
+	TP_STRUCT__entry(
+		WDEV_ENTRY
+		__field(bool, unencrypted)
+	),
+	TP_fast_assign(
+		WDEV_ASSIGN;
+		__entry->unencrypted = unencrypted;
+	),
+	TP_printk(WDEV_PR_FMT", unencrypted: %s",
+		  WDEV_PR_ARG, BOOL_TO_STR(__entry->unencrypted))
+);
+
 TRACE_EVENT(cfg80211_cqm_rssi_notify,
 	TP_PROTO(struct net_device *netdev,
 		 enum nl80211_cqm_rssi_threshold_event rssi_event,
-- 
2.13.5

[RFC 3/4] mac80211: Send control port frames over nl80211

[Denis Kenzior]

If userspace requested control port frames to go over 80211, then do so.
The control packets are intercepted just prior to delivery of the packet
to the underlying network device.

Signed-off-by: Denis Kenzior <denkenz@gmail.com>
---
 net/mac80211/rx.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index b3cff69bfd66..13e786fe62a6 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -2326,16 +2326,42 @@ ieee80211_deliver_skb(struct ieee80211_rx_data *rx)
 	}
 #endif
 
-	if (skb) {
+	if (!skb)
+		goto try_xmit;
+
+	skb->protocol = eth_type_trans(skb, dev);
+	memset(skb->cb, 0, sizeof(skb->cb));
+
+	if (unlikely(skb->protocol == sdata->control_port_protocol &&
+		     sdata->control_port_over_nl80211)) {
+		struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
+		bool noencrypt;
+
+		ehdr = eth_hdr(skb);
+
+		if (status->flag & RX_FLAG_DECRYPTED)
+			noencrypt = false;
+		else
+			noencrypt = true;
+
+		if (!cfg80211_rx_control_port(&sdata->wdev,
+					      skb->data,
+					      skb->len,
+					      ehdr->h_source,
+					      be16_to_cpu(skb->protocol),
+					      noencrypt)) {
+			dev_kfree_skb(skb);
+			skb = NULL;
+		}
+	} else {
 		/* deliver to local stack */
-		skb->protocol = eth_type_trans(skb, dev);
-		memset(skb->cb, 0, sizeof(skb->cb));
 		if (rx->napi)
 			napi_gro_receive(rx->napi, skb);
 		else
 			netif_receive_skb(skb);
 	}
 
+try_xmit:
 	if (xmit_skb) {
 		/*
 		 * Send to wireless media and increase priority by 256 to
-- 
2.13.5

[RFC 4/4] nl80211: Implement TX of control port frames

[Denis Kenzior]

This commit implements the TX side of NL80211_CMD_CONTROL_PORT_FRAME.
Userspace provides the raw EAPoL frame using NL80211_ATTR_FRAME.  A
skbuf is built and then injected onto the netdev of the wireless device.
The CONTROL_PORT_ETHERTYPE_NO_ENCRYPT will still in theory be honored by
the underlying TX path code.

Signed-off-by: Denis Kenzior <denkenz@gmail.com>
---
 net/wireless/nl80211.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 66 insertions(+), 1 deletion(-)

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 220fe5bc57fd..d6191579f044 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -12464,6 +12464,64 @@ static int nl80211_del_pmk(struct sk_buff *skb, struct genl_info *info)
 	return ret;
 }
 
+static int nl80211_tx_control_port(struct sk_buff *skb, struct genl_info *info)
+{
+	struct wireless_dev *wdev = info->user_ptr[1];
+	const u8 *buf;
+	u8 *dest;
+	size_t len;
+	struct ethhdr *ehdr;
+	int err;
+
+	if (!info->attrs[NL80211_ATTR_FRAME])
+		return -EINVAL;
+
+	wdev_lock(wdev);
+
+	switch (wdev->iftype) {
+	case NL80211_IFTYPE_STATION:
+		if (wdev->current_bss)
+			break;
+		err = -ENOTCONN;
+		goto out;
+	default:
+		err = -EOPNOTSUPP;
+		goto out;
+	}
+
+	buf = nla_data(info->attrs[NL80211_ATTR_FRAME]);
+	len = nla_len(info->attrs[NL80211_ATTR_FRAME]);
+
+	skb = dev_alloc_skb(sizeof(struct ethhdr) + len);
+	if (!skb) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	skb_reserve(skb, sizeof(struct ethhdr));
+
+	dest = skb_put(skb, len);
+	memcpy(dest, buf, len);
+
+	ehdr = skb_push(skb, sizeof(struct ethhdr));
+	memcpy(ehdr->h_dest, wdev->current_bss->pub.bssid, ETH_ALEN);
+	memcpy(ehdr->h_source, wdev_address(wdev), ETH_ALEN);
+	ehdr->h_proto = cpu_to_be16(ETH_P_PAE); // TODO: How to get ethertype?
+
+	wdev_unlock(wdev);
+
+	skb->dev = wdev->netdev;
+	skb->protocol = htons(ETH_P_802_3);
+	skb_reset_network_header(skb);
+	skb_reset_mac_header(skb);
+	dev_queue_xmit(skb);
+	return 0;
+
+ out:
+	wdev_unlock(wdev);
+	return err;
+}
+
 #define NL80211_FLAG_NEED_WIPHY		0x01
 #define NL80211_FLAG_NEED_NETDEV	0x02
 #define NL80211_FLAG_NEED_RTNL		0x04
@@ -13359,7 +13417,14 @@ static const struct genl_ops nl80211_ops[] = {
 		.internal_flags = NL80211_FLAG_NEED_NETDEV_UP |
 				  NL80211_FLAG_NEED_RTNL,
 	},
-
+	{
+		.cmd = NL80211_CMD_CONTROL_PORT_FRAME,
+		.doit = nl80211_tx_control_port,
+		.policy = nl80211_policy,
+		.flags = GENL_UNS_ADMIN_PERM,
+		.internal_flags = NL80211_FLAG_NEED_WDEV_UP |
+				  NL80211_FLAG_NEED_RTNL,
+	},
 };
 
 static struct genl_family nl80211_fam __ro_after_init = {
-- 
2.13.5

Re: [RFC 4/4] nl80211: Implement TX of control port frames

[Johannes Berg]

On Thu, 2017-12-28 at 11:58 -0600, Denis Kenzior wrote:
> This commit implements the TX side of NL80211_CMD_CONTROL_PORT_FRAME.
> Userspace provides the raw EAPoL frame using NL80211_ATTR_FRAME.  A
> skbuf is built and then injected onto the netdev of the wireless device.
> The CONTROL_PORT_ETHERTYPE_NO_ENCRYPT will still in theory be honored by

You mean CONTROL_PORT_NO_ENCRYPT :-)

> the underlying TX path code.

I think this isn't good enough.

There are cases where CONTROL_PORT_ETHERTYPE_NO_ENCRYPT should be
unset, but specific frames still shouldn't be encrypted.

So I think for this particular path it would be better to deprecate
CONTROL_PORT_ETHERTYPE_NO_ENCRYPT entirely, and have a separate per-
frame flag.

That also means that we can't really implement it fully in cfg80211,
but have to provide some functionality for the driver to do things to
be able to honour such flags.

Perhaps a new operation, where we pass a pre-built SKB and control
flags?

johannes

Re: [RFC 4/4] nl80211: Implement TX of control port frames

[Denis Kenzior]

Hi Johannes,

On 01/02/2018 07:30 AM, Johannes Berg wrote:
> On Thu, 2017-12-28 at 11:58 -0600, Denis Kenzior wrote:
>> This commit implements the TX side of NL80211_CMD_CONTROL_PORT_FRAME.
>> Userspace provides the raw EAPoL frame using NL80211_ATTR_FRAME.  A
>> skbuf is built and then injected onto the netdev of the wireless device.
>> The CONTROL_PORT_ETHERTYPE_NO_ENCRYPT will still in theory be honored by
> 
> You mean CONTROL_PORT_NO_ENCRYPT :-)

Right

> 
>> the underlying TX path code.
> 
> I think this isn't good enough.
> 

That was my thinking as well

> There are cases where CONTROL_PORT_ETHERTYPE_NO_ENCRYPT should be
> unset, but specific frames still shouldn't be encrypted.
> 
> So I think for this particular path it would be better to deprecate
> CONTROL_PORT_ETHERTYPE_NO_ENCRYPT entirely, and have a separate per-
> frame flag.
> 
> That also means that we can't really implement it fully in cfg80211,
> but have to provide some functionality for the driver to do things to
> be able to honour such flags.

Here's another thought I had while poking around.  Given the above I 
don't want to pursue it too seriously unless you think it might work:

We already have the IEEE80211_TX_INTFL_DONT_ENCRYPT flag on the skb and 
some drivers seem to honor this.  At least that seems to be the intent 
as the CONTROL_PORT_NO_ENCRYPT flag ends up being translated to this 
somewhere in net/mac80211/tx.c.  Are the drivers supposed to honor that 
flag?

If so, can we do something like what ieee80211_process_sa_query_req in 
net/mac80211/rx.c or ieee80211_tdls_prep_mgmt_packet in 
net/mac80211/tdls.c do?  E.g. use ieee80211_tx_skb or 
__ieee80211_subif_start_xmit or similar to inject the skb with the 
DONT_ENCRYPT flag?

> 
> Perhaps a new operation, where we pass a pre-built SKB and control
> flags?
> 

This would likely mean that legacy behavior would still have to be 
supported for quite some time (forever?) for drivers that don't get 
around to implementing this, which would be unfortunate.

Regards,
-Denis

Re: [RFC 4/4] nl80211: Implement TX of control port frames

[Johannes Berg]

On Tue, 2018-01-02 at 12:22 -0600, Denis Kenzior wrote:

> > There are cases where CONTROL_PORT_ETHERTYPE_NO_ENCRYPT should be
> > unset, but specific frames still shouldn't be encrypted.
> > 
> > So I think for this particular path it would be better to deprecate
> > CONTROL_PORT_ETHERTYPE_NO_ENCRYPT entirely, and have a separate per-
> > frame flag.
> > 
> > That also means that we can't really implement it fully in cfg80211,
> > but have to provide some functionality for the driver to do things to
> > be able to honour such flags.
> 
> Here's another thought I had while poking around.  Given the above I 
> don't want to pursue it too seriously unless you think it might work:
> 
> We already have the IEEE80211_TX_INTFL_DONT_ENCRYPT flag on the skb and 
> some drivers seem to honor this.  At least that seems to be the intent 
> as the CONTROL_PORT_NO_ENCRYPT flag ends up being translated to this 
> somewhere in net/mac80211/tx.c.  Are the drivers supposed to honor that 
> flag?

Drivers don't have a choice, and don't need to check the flag. What
happens internally in mac80211 is that either txinfo->control.key is
assigned, or not, and drivers make encryption decisions based on that.

> If so, can we do something like what ieee80211_process_sa_query_req in 
> net/mac80211/rx.c or ieee80211_tdls_prep_mgmt_packet in 
> net/mac80211/tdls.c do?  E.g. use ieee80211_tx_skb or 
> __ieee80211_subif_start_xmit or similar to inject the skb with the 
> DONT_ENCRYPT flag?

Yes, this will work - but like I said, it requires more control over
the SKB than cfg80211 has today, and than you can get through the
regular netdev xmit.

> > Perhaps a new operation, where we pass a pre-built SKB and control
> > flags?

> 
> This would likely mean that legacy behavior would still have to be 
> supported for quite some time (forever?) for drivers that don't get 
> around to implementing this, which would be unfortunate.

What do you mean by "legacy behaviour"? *Drivers* don't really need to
do anything one way or the other, and mac80211 is the only thing
implementing control port encrypt/ethertype right now, I suspect.

johannes

Re: [RFC 4/4] nl80211: Implement TX of control port frames

[Denis Kenzior]

Hi Johannes,

<snip>

>> If so, can we do something like what ieee80211_process_sa_query_req in
>> net/mac80211/rx.c or ieee80211_tdls_prep_mgmt_packet in
>> net/mac80211/tdls.c do?  E.g. use ieee80211_tx_skb or
>> __ieee80211_subif_start_xmit or similar to inject the skb with the
>> DONT_ENCRYPT flag?
> 
> Yes, this will work - but like I said, it requires more control over
> the SKB than cfg80211 has today, and than you can get through the
> regular netdev xmit.

So, I'm a bit lost here.  You're saying this will work but then it 
won't.  Are you saying this would only work for mac80211 based devices 
(e.g. ones that implement ieee80211_ops) but not ones that implement 
cfg80211_ops?  I presume because those provide their own net_device_ops?

> 
>>> Perhaps a new operation, where we pass a pre-built SKB and control
>>> flags?
> 
>>
>> This would likely mean that legacy behavior would still have to be
>> supported for quite some time (forever?) for drivers that don't get
>> around to implementing this, which would be unfortunate.
> 
> What do you mean by "legacy behaviour"? *Drivers* don't really need to
> do anything one way or the other, and mac80211 is the only thing
> implementing control port encrypt/ethertype right now, I suspect.
> 

When I say legacy I mean 'over netdev', e.g. not over nl80211.

Okay, so what you're saying is that the current CONTROL_PORT_* stuff 
doesn't work on anything besides mac80211 since drivers can completely 
ignore stuff inside cfg80211_connect_params.  And there's no way for 
userspace to know this ahead of time?

So essentially we'd need a new operation in cfg80211_ops that would 
accept the control port frame data and some control flags.  Do we want 
to pass in an skb with all the 802.11 headers set or a 802.3 formatted 
skb (since that is what other data frames look like initially on the 
netdev).

Regards,
-Denis

Re: [RFC 4/4] nl80211: Implement TX of control port frames

[Arend Van Spriel]

On Wed, Jan 3, 2018 at 6:17 PM, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Johannes,
>
> <snip>
>
>>> If so, can we do something like what ieee80211_process_sa_query_req in
>>> net/mac80211/rx.c or ieee80211_tdls_prep_mgmt_packet in
>>> net/mac80211/tdls.c do?  E.g. use ieee80211_tx_skb or
>>> __ieee80211_subif_start_xmit or similar to inject the skb with the
>>> DONT_ENCRYPT flag?
>>
>>
>> Yes, this will work - but like I said, it requires more control over
>> the SKB than cfg80211 has today, and than you can get through the
>> regular netdev xmit.
>
>
> So, I'm a bit lost here.  You're saying this will work but then it won't.
> Are you saying this would only work for mac80211 based devices (e.g. ones
> that implement ieee80211_ops) but not ones that implement cfg80211_ops?  I
> presume because those provide their own net_device_ops?

That is my assumption as well.

>>
>>>> Perhaps a new operation, where we pass a pre-built SKB and control
>>>> flags?
>>
>>
>>>
>>> This would likely mean that legacy behavior would still have to be
>>> supported for quite some time (forever?) for drivers that don't get
>>> around to implementing this, which would be unfortunate.
>>
>>
>> What do you mean by "legacy behaviour"? *Drivers* don't really need to
>> do anything one way or the other, and mac80211 is the only thing
>> implementing control port encrypt/ethertype right now, I suspect.
>>
>
> When I say legacy I mean 'over netdev', e.g. not over nl80211.
>
> Okay, so what you're saying is that the current CONTROL_PORT_* stuff doesn't
> work on anything besides mac80211 since drivers can completely ignore stuff
> inside cfg80211_connect_params.  And there's no way for userspace to know
> this ahead of time?
>
> So essentially we'd need a new operation in cfg80211_ops that would accept
> the control port frame data and some control flags.  Do we want to pass in
> an skb with all the 802.11 headers set or a 802.3 formatted skb (since that
> is what other data frames look like initially on the netdev).

Our firmware expects EAPOL stuff to come in as 802.3 packet, which
probably applies to the other full-mac devices as well. Is there any
reason for passing 802.11 packets.

Regards,
Arend

Re: [RFC 4/4] nl80211: Implement TX of control port frames

[Denis Kenzior]

Hi Arend,

>> So essentially we'd need a new operation in cfg80211_ops that would accept
>> the control port frame data and some control flags.  Do we want to pass in
>> an skb with all the 802.11 headers set or a 802.3 formatted skb (since that
>> is what other data frames look like initially on the netdev).
> 
> Our firmware expects EAPOL stuff to come in as 802.3 packet, which
> probably applies to the other full-mac devices as well. Is there any
> reason for passing 802.11 packets.
> 

I think it was suggested earlier somewhere in this thread to use 802.11 
header similar to how mgmt_tx does things.  To me it makes more sense to 
use 802.3 headers so I thought I would double check.

Regards,
-Denis

Re: [RFC 4/4] nl80211: Implement TX of control port frames

[Johannes Berg]

Hi,

> > > If so, can we do something like what ieee80211_process_sa_query_req in
> > > net/mac80211/rx.c or ieee80211_tdls_prep_mgmt_packet in
> > > net/mac80211/tdls.c do?  E.g. use ieee80211_tx_skb or
> > > __ieee80211_subif_start_xmit or similar to inject the skb with the
> > > DONT_ENCRYPT flag?
> > 
> > Yes, this will work - but like I said, it requires more control over
> > the SKB than cfg80211 has today, and than you can get through the
> > regular netdev xmit.
> 
> So, I'm a bit lost here.  You're saying this will work but then it 
> won't.  Are you saying this would only work for mac80211 based devices 
> (e.g. ones that implement ieee80211_ops) but not ones that implement 
> cfg80211_ops?  I presume because those provide their own net_device_ops?

Heh, yeah, badly phrased. It'll work to do it like process_sa_query or
similar code, for mac80211 drivers. What will *not* work is actually
building and submitting the SKB in cfg80211 though, since you can't
even set the DONT_ENCRYPT flag from there.

> > > This would likely mean that legacy behavior would still have to be
> > > supported for quite some time (forever?) for drivers that don't get
> > > around to implementing this, which would be unfortunate.
> > 
> > What do you mean by "legacy behaviour"? *Drivers* don't really need to
> > do anything one way or the other, and mac80211 is the only thing
> > implementing control port encrypt/ethertype right now, I suspect.
> > 
> 
> When I say legacy I mean 'over netdev', e.g. not over nl80211.

We need to support that behaviour forever anyway, for existing versions
of userspace software.

> Okay, so what you're saying is that the current CONTROL_PORT_* stuff 
> doesn't work on anything besides mac80211 since drivers can completely 
> ignore stuff inside cfg80211_connect_params.  And there's no way for 
> userspace to know this ahead of time?

Yes, NL80211_ATTR_CONTROL_PORT_ETHERTYPE is an attribute set in the
wiphy info - see WIPHY_FLAG_CONTROL_PORT_PROTOCOL in the code.

> So essentially we'd need a new operation in cfg80211_ops that would 
> accept the control port frame data and some control flags.  Do we want 
> to pass in an skb with all the 802.11 headers set or a 802.3 formatted 
> skb (since that is what other data frames look like initially on the 
> netdev).

I think 802.3 format would be better - matches better for full-MAC
devices that might do all header conversion in the device, and getting
the BSSID might even be difficult from cfg80211, etc.

johannes

Re: [RFC 0/4] EAPoL over NL80211

[Arend van Spriel]

On 12/28/2017 6:58 PM, Denis Kenzior wrote:
> This patchset adds support for running 802.11 authentication mechanisms (e.g.
> 802.1X, 4-Way Handshake, etc) over NL80211 instead of putting them onto the
> network device.  This has the advantage of fixing several long-standing race
> conditions that result from userspace operating on multiple transports in order
> to manage a 802.11 connection (e.g. NL80211 and wireless netdev, wlan0, etc)
>
> For example, userspace would sometimes see 4-Way handshake packets before
> NL80211 signaled that the connection has been established.  Leading to ugly
> hacks or having the STA wait for retransmissions from the AP.
>
> To make this possible this patchset introduces a new NL80211 command and several
> new attributes.  A userspace that is capable of processing EAPoL packets over
> NL80211 includes a new NL80211_ATTR_CONTROL_PORT_OVER_NL80211 attribute in its
> NL80211_CMD_ASSOCIATE or NL80211_CMD_CONNECT requests being sent to the kernel.
> The previously added NL80211_ATTR_SOCKET_OWNER attribute must also be included.

Does it make sense to require a combination of attributes. It is always 
a bit awkward so prefer to avoid it. Could we implicitly make the 
netlink unicast for notifications when 
NL80211_ATTR_CONTROL_PORT_OVER_NL80211 is provided by user-space.

> The latter is used by the kernel to send NL80211_CMD_CONTROL_PORT_FRAME
> notifications back to userspace via a netlink unicast.  If the
> NL80211_ATTR_CONTROL_PORT_OVER_NL80211 attribute is not specified, then legacy
> behavior is kept and control port packets continue to flow over the network
> interface.

[...]

> Open Questions
> ==============
>
> 1. It is not clear whether all drivers provide information as to whether the
> control port frame was sent encrypted or not.  Do they?

You mean in some tx status info. I think brcmfmac does not have that 
info available in the driver, but would need to verify.

> 2. It has been previously suggested that CMD_FRAME infrastructure is used to
> accomplish control port over nl80211 transport.  However, it did not seem to be
> a good fit as the relevant code paths assume that only management frames are
> to be sent via this mechanism.  Thoughts?

What are the issues coming from that assumption? Does it assume 802.11 
header is present? What else?

Regards,
Arend

Re: [RFC 0/4] EAPoL over NL80211

[Denis Kenzior]

Hi Arend,

<snip>

>> To make this possible this patchset introduces a new NL80211 command 
>> and several
>> new attributes.  A userspace that is capable of processing EAPoL 
>> packets over
>> NL80211 includes a new NL80211_ATTR_CONTROL_PORT_OVER_NL80211 
>> attribute in its
>> NL80211_CMD_ASSOCIATE or NL80211_CMD_CONNECT requests being sent to 
>> the kernel.
>> The previously added NL80211_ATTR_SOCKET_OWNER attribute must also be 
>> included.
> 
> Does it make sense to require a combination of attributes. It is always 
> a bit awkward so prefer to avoid it. Could we implicitly make the 
> netlink unicast for notifications when 
> NL80211_ATTR_CONTROL_PORT_OVER_NL80211 is provided by user-space.
> 

Agreed, requiring both attributes is less than ideal, but I tried to 
make the initial RFC as minimal as possible.  It also helped that iwd 
uses SOCKET_OWNER by default.  What can be done is to always set 
conn_owner_nlportid and introduce another flag that would indicate 
whether 'connection tear-down on application exit' was requested.

However, my opinion is that the current SOCKET_OWNER behavior should 
just be made default, especially for control port over nl80211 
connections, even if SOCKET_OWNER was not requested.  Once the 
controlling application dies, there's no hope of salvaging the 
connection, perform rekeys, etc.

<snip>

>> 2. It has been previously suggested that CMD_FRAME infrastructure is 
>> used to
>> accomplish control port over nl80211 transport.  However, it did not 
>> seem to be
>> a good fit as the relevant code paths assume that only management 
>> frames are
>> to be sent via this mechanism.  Thoughts?
> 
> What are the issues coming from that assumption? Does it assume 802.11 
> header is present? What else?
> 

Correct.  There's also quite a bit of logic to figure out whether the 
frame is being sent offchannel or not; whether offchannel capability is 
present in the driver, etc.  This can be ignored for control port 
frames, but makes the code path complicated.

The biggest issue was that each driver defines a set of management 
frames it can accept via this mechanism.  The set is structured using 
management frame type as an identifier and the code checks this set 
prior to accepting the frame to be sent via CMD_FRAME.  Since control 
port frames are data frames it would probably require quite a bit of 
surgery in the core mac80211/wireless code and the driver code to make 
it work.

Another issue is that cfg80211_mgmt_tx_params doesn't have a 'don't 
encrypt' setting.  So that part would need to be added as well.

Regards,
-Denis

Re: [RFC 0/4] EAPoL over NL80211

[Arend van Spriel]

On 12/29/2017 7:29 PM, Denis Kenzior wrote:
> Hi Arend,
>
> <snip>
>
>>> To make this possible this patchset introduces a new NL80211 command
>>> and several
>>> new attributes.  A userspace that is capable of processing EAPoL
>>> packets over
>>> NL80211 includes a new NL80211_ATTR_CONTROL_PORT_OVER_NL80211
>>> attribute in its
>>> NL80211_CMD_ASSOCIATE or NL80211_CMD_CONNECT requests being sent to
>>> the kernel.
>>> The previously added NL80211_ATTR_SOCKET_OWNER attribute must also be
>>> included.
>>
>> Does it make sense to require a combination of attributes. It is
>> always a bit awkward so prefer to avoid it. Could we implicitly make
>> the netlink unicast for notifications when
>> NL80211_ATTR_CONTROL_PORT_OVER_NL80211 is provided by user-space.
>>
>
> Agreed, requiring both attributes is less than ideal, but I tried to
> make the initial RFC as minimal as possible.  It also helped that iwd
> uses SOCKET_OWNER by default.  What can be done is to always set
> conn_owner_nlportid and introduce another flag that would indicate
> whether 'connection tear-down on application exit' was requested.
>
> However, my opinion is that the current SOCKET_OWNER behavior should
> just be made default, especially for control port over nl80211
> connections, even if SOCKET_OWNER was not requested.  Once the
> controlling application dies, there's no hope of salvaging the
> connection, perform rekeys, etc.

If you mean that all notifications need to be unicast I tend to 
disagree. It would kill the multicast functionality. If you just mean 
for NL80211_CMD_ASSOCIATE or NL80211_CMD_CONNECT it makes sense for 
secure connections, but what about unencrypted connections.

> <snip>
>
>>> 2. It has been previously suggested that CMD_FRAME infrastructure is
>>> used to
>>> accomplish control port over nl80211 transport.  However, it did not
>>> seem to be
>>> a good fit as the relevant code paths assume that only management
>>> frames are
>>> to be sent via this mechanism.  Thoughts?
>>
>> What are the issues coming from that assumption? Does it assume 802.11
>> header is present? What else?
>>
>
> Correct.  There's also quite a bit of logic to figure out whether the
> frame is being sent offchannel or not; whether offchannel capability is
> present in the driver, etc.  This can be ignored for control port
> frames, but makes the code path complicated.

It seems to boil down to a single question "offchannel or not" so I 
suppose that bit of logic could be isolated.

> The biggest issue was that each driver defines a set of management
> frames it can accept via this mechanism.  The set is structured using
> management frame type as an identifier and the code checks this set
> prior to accepting the frame to be sent via CMD_FRAME.  Since control
> port frames are data frames it would probably require quite a bit of
> surgery in the core mac80211/wireless code and the driver code to make
> it work.

Yes. It assumes management frame type and as such subtypes are stored in 
struct wiphy::mgmt_stypes. Together these are part of the frame control 
field in 802.11 header. So I suppose you could add struct 
wiphy::data_stypes, but for "eapol over nl80211" you may want to add 
ethernet protocol in the mix. I am not sure if we need subtype 
granularity for data frametypes as I think the 802.11 stack, ie. 
mac80211 or some fullmac firmware, decides the subtype further down.

> Another issue is that cfg80211_mgmt_tx_params doesn't have a 'don't
> encrypt' setting.  So that part would need to be added as well.

True. Looking at the above I would stick with the separate primitive 
although the name might be a bit more generic so it can be used for the 
pre-auth protocol as well.

Anyway, I will review the individual patches keeping this in mind. 
Thanks for the clarifications.

Regards,
Arend

Re: [RFC 0/4] EAPoL over NL80211

[Johannes Berg]

On Fri, 2017-12-29 at 12:29 -0600, Denis Kenzior wrote:

> Agreed, requiring both attributes is less than ideal, but I tried to 
> make the initial RFC as minimal as possible.  It also helped that iwd 
> uses SOCKET_OWNER by default.  What can be done is to always set 
> conn_owner_nlportid and introduce another flag that would indicate 
> whether 'connection tear-down on application exit' was requested.
> 
> However, my opinion is that the current SOCKET_OWNER behavior should 
> just be made default, especially for control port over nl80211 
> connections, even if SOCKET_OWNER was not requested.  Once the 
> controlling application dies, there's no hope of salvaging the 
> connection, perform rekeys, etc.

I think we should keep both attributes; it's better to be explicit that
both are needed than to set socket-owner automatically.

> The biggest issue was that each driver defines a set of management 
> frames it can accept via this mechanism.

I'm not sure this is "the biggest issue", but I tend to agree with
keeping them separate.

johannes

Re: [RFC 0/4] EAPoL over NL80211

[Arend Van Spriel]

On Tue, Jan 2, 2018 at 2:27 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2017-12-29 at 12:29 -0600, Denis Kenzior wrote:
>
>> Agreed, requiring both attributes is less than ideal, but I tried to
>> make the initial RFC as minimal as possible.  It also helped that iwd
>> uses SOCKET_OWNER by default.  What can be done is to always set
>> conn_owner_nlportid and introduce another flag that would indicate
>> whether 'connection tear-down on application exit' was requested.
>>
>> However, my opinion is that the current SOCKET_OWNER behavior should
>> just be made default, especially for control port over nl80211
>> connections, even if SOCKET_OWNER was not requested.  Once the
>> controlling application dies, there's no hope of salvaging the
>> connection, perform rekeys, etc.
>
> I think we should keep both attributes; it's better to be explicit that
> both are needed than to set socket-owner automatically.

As I see it the problem is that SOCKET_OWNER behavior is actually two
behaviors wrapped in one, ie. 1) make notifications unicast, and 2)
tear-down on socket disconnect. So what is the reason for requiring
this attribute. In the cover letter you mention it is for unicast
notifications, but above you seem to put more weight in the tear-down
behavior. From earlier discussion I recall that multicast netlink
messages may not be received. Is that the reason for opting for
unicast here. If so, I would suggest to mention that in the commit
message.

>> The biggest issue was that each driver defines a set of management
>> frames it can accept via this mechanism.
>
> I'm not sure this is "the biggest issue", but I tend to agree with
> keeping them separate.

Yes. Seems like dealing with mgmt and data adds quite a bit of
complexity and/or redesign.

Just another note. In the cover letter it is mentioned that
eapol-over-nl80211 solves some long standing races. Now the example
mentioned is indeed one for which wpa_supplicant temporarily stores M1
or at least that is what I suspect you are referring to. Another
issue, for which we have a hack in brcmfmac, is a race between
installing the keys through nl80211 and M4 being passed through the
netdev which can result in M4 being sent out encrypted. Not sure if
your discussion about DONT_CRYPT is about that.

Gr. AvS

Re: [RFC 0/4] EAPoL over NL80211

[Denis Kenzior]

Hi Arend,

On 01/03/2018 02:24 PM, Arend Van Spriel wrote:
> On Tue, Jan 2, 2018 at 2:27 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
>> On Fri, 2017-12-29 at 12:29 -0600, Denis Kenzior wrote:
>>
>>> Agreed, requiring both attributes is less than ideal, but I tried to
>>> make the initial RFC as minimal as possible.  It also helped that iwd
>>> uses SOCKET_OWNER by default.  What can be done is to always set
>>> conn_owner_nlportid and introduce another flag that would indicate
>>> whether 'connection tear-down on application exit' was requested.
>>>
>>> However, my opinion is that the current SOCKET_OWNER behavior should
>>> just be made default, especially for control port over nl80211
>>> connections, even if SOCKET_OWNER was not requested.  Once the
>>> controlling application dies, there's no hope of salvaging the
>>> connection, perform rekeys, etc.
>>
>> I think we should keep both attributes; it's better to be explicit that
>> both are needed than to set socket-owner automatically.
> 
> As I see it the problem is that SOCKET_OWNER behavior is actually two
> behaviors wrapped in one, ie. 1) make notifications unicast, and 2)
> tear-down on socket disconnect. So what is the reason for requiring
> this attribute. In the cover letter you mention it is for unicast
> notifications, but above you seem to put more weight in the tear-down
> behavior. From earlier discussion I recall that multicast netlink
> messages may not be received. Is that the reason for opting for
> unicast here. If so, I would suggest to mention that in the commit
> message.
>

There are several ways I could have implemented EAPoL over NL80211 
notifications:

1. Multicast it to the wide world
2. Setup a registration framework ala CMD_REGISTER_FRAME or 
CMD_UNEXPECTED_FRAME
3. Assume frames should be unicast to the process that initiated CMD_CONNECT

I see no good reason why anyone would want option 1.  Option 2 seemed 
pointless.  Only the supplicant daemon (e.g. iwd or wpa_s) would really 
want to process these anyway.  Is this the reasoning you want me to 
include in the cover letter?

The simplest way of accomplishing 3 was to reuse the conn_owner_nlportid 
member that the kernel stores in the case that SOCKET_OWNER attribute is 
used.  iwd always uses this by default and quite frankly in the case of 
EAPoL over NL80211 it doesn't make sense to not use it, at least to me. 
Without SOCKET_OWNER, if the supplicant daemon dies you have an 
un-managed, dangling connection.

>>> The biggest issue was that each driver defines a set of management
>>> frames it can accept via this mechanism.
>>
>> I'm not sure this is "the biggest issue", but I tend to agree with
>> keeping them separate.
> 
> Yes. Seems like dealing with mgmt and data adds quite a bit of
> complexity and/or redesign.
> 
> Just another note. In the cover letter it is mentioned that
> eapol-over-nl80211 solves some long standing races. Now the example
> mentioned is indeed one for which wpa_supplicant temporarily stores M1

yes, this race has to be taken care of by the supplicant.  We have 
similar workaround in iwd which I would love to get rid of.

> or at least that is what I suspect you are referring to. Another
> issue, for which we have a hack in brcmfmac, is a race between
> installing the keys through nl80211 and M4 being passed through the
> netdev which can result in M4 being sent out encrypted. Not sure if
> your discussion about DONT_CRYPT is about that.

This is really a kernel issue being bled out into userspace.  Many 
drivers seem to suffer from this race.  I think Ben had a thread some 
time in June about this as well.  But yes, I believe the main impetus to 
add a DONT_ENCRYPT flag for EAPoL frames sent over NL80211 is to fix 
this along with supporting weird protocols that explicitly want to be 
sent unencrypted.

Regards,
-Denis