[PATCH 1/2] Revert "mac80211: support NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211_MAC_ADDRS"

[PATCH 1/2] Revert "mac80211: support NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211_MAC_ADDRS"

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

This reverts commit 9b125c27998719288e4dcf2faf54511039526692.

As Jouni points out, there's really no need for this, since the
RSN pre-authentication frames are normal data frames, not port
control frames (locally).

Fixes: 9b125c279987 ("mac80211: support NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211_MAC_ADDRS")
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/main.c | 2 --
 net/mac80211/tx.c   | 2 +-
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index cae3a34d3503..944e86da5c65 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -589,8 +589,6 @@ 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);
-	wiphy_ext_feature_set(wiphy,
-			      NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211_MAC_ADDRS);
 
 	if (!ops->hw_scan) {
 		wiphy->features |= NL80211_FEATURE_LOW_PRIORITY_SCAN |
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 8dd93072f6e6..2645a39cce88 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -5315,7 +5315,7 @@ int ieee80211_tx_control_port(struct wiphy *wiphy, struct net_device *dev,
 
 	ehdr = skb_push(skb, sizeof(struct ethhdr));
 	memcpy(ehdr->h_dest, dest, ETH_ALEN);
-	memcpy(ehdr->h_source, src, ETH_ALEN);
+	memcpy(ehdr->h_source, sdata->vif.addr, ETH_ALEN);
 	ehdr->h_proto = proto;
 
 	skb->dev = dev;
-- 
2.24.1

[PATCH 2/2] Revert "nl80211: add src and dst addr attributes for control port tx/rx"

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

This reverts commit 8c3ed7aa2b9ef666195b789e9b02e28383243fa8.

As Jouni points out, there's really no need for this, since the
RSN pre-authentication frames are normal data frames, not port
control frames (locally).

We can still revert this now since it hasn't actually gone beyond
-next.

Fixes: 8c3ed7aa2b9e ("nl80211: add src and dst addr attributes for control port tx/rx")
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/cfg80211.h       |  3 +--
 include/uapi/linux/nl80211.h | 16 +---------------
 net/mac80211/ieee80211_i.h   |  3 +--
 net/mac80211/tx.c            |  3 +--
 net/wireless/nl80211.c       | 18 +++---------------
 net/wireless/rdev-ops.h      |  8 ++++----
 net/wireless/trace.h         | 27 ++++++++++-----------------
 7 files changed, 21 insertions(+), 57 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 7ea23caa7301..089ffdd6dba5 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -3974,8 +3974,7 @@ struct cfg80211_ops {
 	int	(*tx_control_port)(struct wiphy *wiphy,
 				   struct net_device *dev,
 				   const u8 *buf, size_t len,
-				   const u8 *dest, const u8 *src,
-				   const __be16 proto,
+				   const u8 *dest, const __be16 proto,
 				   const bool noencrypt);
 
 	int	(*get_ftm_responder_stats)(struct wiphy *wiphy,
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 158bccb4a47b..350ab86fe20e 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1039,14 +1039,11 @@
  *	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. An optional %NL80211_ATTR_SRC_MAC can be used to send
- *	pre-auth frames to STAs on behalf of other APs.
+ *	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_ATTR_DST_MAC can be used to forward pre-auth frames in
- *	userspace while using AP mode.
  *
  * @NL80211_CMD_RELOAD_REGDB: Request that the regdb firmware file is reloaded.
  *
@@ -2412,9 +2409,6 @@ enum nl80211_commands {
  *	%NL80211_ATTR_AKM_SUITES are default capabilities if AKM suites not
  *	advertised for a specific interface type.
  *
- * @NL80211_ATTR_SRC_MAC: MAC address used in control port over nl80211 transmit
- * @NL80211_ATTR_DST_MAC: MAC address used in control port over nl80211 receive
- *
  * @NUM_NL80211_ATTR: total number of nl80211_attrs available
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -2883,9 +2877,6 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_IFTYPE_AKM_SUITES,
 
-	NL80211_ATTR_SRC_MAC,
-	NL80211_ATTR_DST_MAC,
-
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
@@ -5548,10 +5539,6 @@ enum nl80211_feature_flags {
  *	feature, which prevents bufferbloat by using the expected transmission
  *	time to limit the amount of data buffered in the hardware.
  *
- * @NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211_MAC_ADDRS: The driver
- *	can use src and dst MAC addresses with control port over nl80211 rx
- *	and tx operations.
- *
  * @NUM_NL80211_EXT_FEATURES: number of extended features.
  * @MAX_NL80211_EXT_FEATURES: highest extended feature index.
  */
@@ -5599,7 +5586,6 @@ enum nl80211_ext_feature_index {
 	NL80211_EXT_FEATURE_SAE_OFFLOAD,
 	NL80211_EXT_FEATURE_VLAN_OFFLOAD,
 	NL80211_EXT_FEATURE_AQL,
-	NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211_MAC_ADDRS,
 
 	/* add new features before the definition below */
 	NUM_NL80211_EXT_FEATURES,
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index da9eaa9ee37e..8a49d78ad7c9 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1792,8 +1792,7 @@ void ieee80211_check_fast_xmit_iface(struct ieee80211_sub_if_data *sdata);
 void ieee80211_clear_fast_xmit(struct sta_info *sta);
 int ieee80211_tx_control_port(struct wiphy *wiphy, struct net_device *dev,
 			      const u8 *buf, size_t len,
-			      const u8 *dest, const u8 *src, __be16 proto,
-			      bool unencrypted);
+			      const u8 *dest, __be16 proto, bool unencrypted);
 int ieee80211_probe_mesh_link(struct wiphy *wiphy, struct net_device *dev,
 			      const u8 *buf, size_t len);
 
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 2645a39cce88..cddaacaa31a3 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -5283,8 +5283,7 @@ void __ieee80211_tx_skb_tid_band(struct ieee80211_sub_if_data *sdata,
 
 int ieee80211_tx_control_port(struct wiphy *wiphy, struct net_device *dev,
 			      const u8 *buf, size_t len,
-			      const u8 *dest, const u8 *src, __be16 proto,
-			      bool unencrypted)
+			      const u8 *dest, __be16 proto, bool unencrypted)
 {
 	struct ieee80211_sub_if_data *sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 	struct ieee80211_local *local = sdata->local;
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index f0112dabe21e..d795552f97b2 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -634,8 +634,6 @@ const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
 	[NL80211_ATTR_HE_OBSS_PD] = NLA_POLICY_NESTED(he_obss_pd_policy),
 	[NL80211_ATTR_VLAN_ID] = NLA_POLICY_RANGE(NLA_U16, 1, VLAN_N_VID - 2),
 	[NL80211_ATTR_HE_BSS_COLOR] = NLA_POLICY_NESTED(he_bss_color_policy),
-	[NL80211_ATTR_SRC_MAC] = NLA_POLICY_ETH_ADDR,
-	[NL80211_ATTR_DST_MAC] = NLA_POLICY_ETH_ADDR,
 };
 
 /* policy for the key attributes */
@@ -13698,7 +13696,6 @@ static int nl80211_tx_control_port(struct sk_buff *skb, struct genl_info *info)
 	const u8 *buf;
 	size_t len;
 	u8 *dest;
-	u8 src[ETH_ALEN];
 	u16 proto;
 	bool noencrypt;
 	int err;
@@ -13736,13 +13733,6 @@ static int nl80211_tx_control_port(struct sk_buff *skb, struct genl_info *info)
 		goto out;
 	}
 
-	/* copy src address under wdev_lock, as we may copy wdev_address */
-	if (info->attrs[NL80211_ATTR_SRC_MAC])
-		ether_addr_copy(src,
-				nla_data(info->attrs[NL80211_ATTR_SRC_MAC]));
-	else
-		ether_addr_copy(src, wdev_address(wdev));
-
 	wdev_unlock(wdev);
 
 	buf = nla_data(info->attrs[NL80211_ATTR_FRAME]);
@@ -13753,7 +13743,7 @@ static int nl80211_tx_control_port(struct sk_buff *skb, struct genl_info *info)
 		nla_get_flag(info->attrs[NL80211_ATTR_CONTROL_PORT_NO_ENCRYPT]);
 
 	return rdev_tx_control_port(rdev, dev, buf, len,
-				    dest, src, cpu_to_be16(proto), noencrypt);
+				    dest, cpu_to_be16(proto), noencrypt);
 
  out:
 	wdev_unlock(wdev);
@@ -16010,8 +16000,7 @@ static int __nl80211_rx_control_port(struct net_device *dev,
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 	struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy);
 	struct ethhdr *ehdr = eth_hdr(skb);
-	const u8 *daddr = ehdr->h_dest;
-	const u8 *saddr = ehdr->h_source;
+	const u8 *addr = ehdr->h_source;
 	u16 proto = be16_to_cpu(skb->protocol);
 	struct sk_buff *msg;
 	void *hdr;
@@ -16036,8 +16025,7 @@ static int __nl80211_rx_control_port(struct net_device *dev,
 	    nla_put_u32(msg, NL80211_ATTR_IFINDEX, dev->ifindex) ||
 	    nla_put_u64_64bit(msg, NL80211_ATTR_WDEV, wdev_id(wdev),
 			      NL80211_ATTR_PAD) ||
-	    nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, saddr) ||
-	    nla_put(msg, NL80211_ATTR_DST_MAC, ETH_ALEN, daddr) ||
+	    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)))
diff --git a/net/wireless/rdev-ops.h b/net/wireless/rdev-ops.h
index 5ea34c1b60fe..e0d34f796d0b 100644
--- a/net/wireless/rdev-ops.h
+++ b/net/wireless/rdev-ops.h
@@ -734,14 +734,14 @@ static inline int rdev_mgmt_tx(struct cfg80211_registered_device *rdev,
 static inline int rdev_tx_control_port(struct cfg80211_registered_device *rdev,
 				       struct net_device *dev,
 				       const void *buf, size_t len,
-				       const u8 *dest, const u8 *src,
-				       __be16 proto, const bool noencrypt)
+				       const u8 *dest, __be16 proto,
+				       const bool noencrypt)
 {
 	int ret;
 	trace_rdev_tx_control_port(&rdev->wiphy, dev, buf, len,
-				   dest, src, proto, noencrypt);
+				   dest, proto, noencrypt);
 	ret = rdev->ops->tx_control_port(&rdev->wiphy, dev, buf, len,
-					 dest, src, proto, noencrypt);
+					 dest, proto, noencrypt);
 	trace_rdev_return_int(&rdev->wiphy, ret);
 	return ret;
 }
diff --git a/net/wireless/trace.h b/net/wireless/trace.h
index b6b60e3aea41..3ef1679b0e66 100644
--- a/net/wireless/trace.h
+++ b/net/wireless/trace.h
@@ -1928,31 +1928,27 @@ TRACE_EVENT(rdev_mgmt_tx,
 
 TRACE_EVENT(rdev_tx_control_port,
 	TP_PROTO(struct wiphy *wiphy, struct net_device *netdev,
-		 const u8 *buf, size_t len,
-		 const u8 *dest, const u8 *src, __be16 proto,
+		 const u8 *buf, size_t len, const u8 *dest, __be16 proto,
 		 bool unencrypted),
-	TP_ARGS(wiphy, netdev, buf, len, dest, src, proto, unencrypted),
+	TP_ARGS(wiphy, netdev, buf, len, dest, proto, unencrypted),
 	TP_STRUCT__entry(
 		WIPHY_ENTRY
 		NETDEV_ENTRY
 		MAC_ENTRY(dest)
-		MAC_ENTRY(src)
-		__field(u16, proto)
+		__field(__be16, proto)
 		__field(bool, unencrypted)
 	),
 	TP_fast_assign(
 		WIPHY_ASSIGN;
 		NETDEV_ASSIGN;
 		MAC_ASSIGN(dest, dest);
-		MAC_ASSIGN(src, src);
-		__entry->proto = be16_to_cpu(proto);
+		__entry->proto = proto;
 		__entry->unencrypted = unencrypted;
 	),
-	TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", dest: " MAC_PR_FMT
-		  ", src: " MAC_PR_FMT ", proto: 0x%x, unencrypted: %s",
-		  WIPHY_PR_ARG, NETDEV_PR_ARG,
-		  MAC_PR_ARG(dest), MAC_PR_ARG(src),
-		  __entry->proto,
+	TP_printk(WIPHY_PR_FMT ", " NETDEV_PR_FMT ", " MAC_PR_FMT ","
+		  " proto: 0x%x, unencrypted: %s",
+		  WIPHY_PR_ARG, NETDEV_PR_ARG, MAC_PR_ARG(dest),
+		  be16_to_cpu(__entry->proto),
 		  BOOL_TO_STR(__entry->unencrypted))
 );
 
@@ -2844,7 +2840,6 @@ TRACE_EVENT(cfg80211_rx_control_port,
 	TP_STRUCT__entry(
 		NETDEV_ENTRY
 		__field(int, len)
-		MAC_ENTRY(to)
 		MAC_ENTRY(from)
 		__field(u16, proto)
 		__field(bool, unencrypted)
@@ -2852,14 +2847,12 @@ TRACE_EVENT(cfg80211_rx_control_port,
 	TP_fast_assign(
 		NETDEV_ASSIGN;
 		__entry->len = skb->len;
-		MAC_ASSIGN(to, eth_hdr(skb)->h_dest);
 		MAC_ASSIGN(from, eth_hdr(skb)->h_source);
 		__entry->proto = be16_to_cpu(skb->protocol);
 		__entry->unencrypted = unencrypted;
 	),
-	TP_printk(NETDEV_PR_FMT ", len=%d, dest: " MAC_PR_FMT
-		  ", src: " MAC_PR_FMT ", proto: 0x%x, unencrypted: %s",
-		  NETDEV_PR_ARG, __entry->len, MAC_PR_ARG(to), MAC_PR_ARG(from),
+	TP_printk(NETDEV_PR_FMT ", len=%d, " MAC_PR_FMT ", proto: 0x%x, unencrypted: %s",
+		  NETDEV_PR_ARG, __entry->len, MAC_PR_ARG(from),
 		  __entry->proto, BOOL_TO_STR(__entry->unencrypted))
 );
 
-- 
2.24.1

Re: [PATCH 1/2] Revert "mac80211: support NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211_MAC_ADDRS"

[Denis Kenzior]

Hi Johannes,

On 2/24/20 3:19 AM, Johannes Berg wrote:
> As Jouni points out, there's really no need for this, since the
> RSN pre-authentication frames are normal data frames, not port
> control frames (locally).

Using control port for pre-auth frame TX/RX allows userspace to skip 
setting up a raw socket and associated bpf filters.  This greatly 
simplifies the rx/tx path and uses less resources as well.

So to me this patch set seemed like a good idea...  We (iwd) don't have 
plans to support pre-auth in AP mode in the near future, so this revert 
doesn't really affect us.  I do wonder what is the actual concern to 
warrant a revert?

Regards,
-Denis

Re: [PATCH 1/2] Revert "mac80211: support NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211_MAC_ADDRS"

[Johannes Berg]

On Mon, 2020-02-24 at 10:56 -0600, Denis Kenzior wrote:

> So to me this patch set seemed like a good idea...  We (iwd) don't have 
> plans to support pre-auth in AP mode in the near future, so this revert 
> doesn't really affect us.  I do wonder what is the actual concern to 
> warrant a revert?

These are two entirely different things, preauth is simply real data as
far as the local system is concerned. It's not related to controlled
port operation at all, which this nl80211 API is about.

FWIW, you may have seen Markus's patch to remove preauth from the RX as
well, this won't work as is, but I'm still a bit on the fence as to
whether I'll force you into the right model or not (i.e. clear the
existing capability bit in mac80211, and introduce a new one that
doesn't report preauth over nl80211). For RX, however, the difference
isn't really that much of a big deal, so maybe just make it optional.

johannes

Re: [PATCH 1/2] Revert "mac80211: support NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211_MAC_ADDRS"

[Denis Kenzior]

Hi Johannes,

On 2/24/20 12:26 PM, Johannes Berg wrote:
> On Mon, 2020-02-24 at 10:56 -0600, Denis Kenzior wrote:
> 
>> So to me this patch set seemed like a good idea...  We (iwd) don't have
>> plans to support pre-auth in AP mode in the near future, so this revert
>> doesn't really affect us.  I do wonder what is the actual concern to
>> warrant a revert?
> 
> These are two entirely different things, preauth is simply real data as
> far as the local system is concerned. It's not related to controlled
> port operation at all, which this nl80211 API is about.

I can understand this argument, but from what I remember, one of the 
goals of the control port API was to make this legacy 'special data 
packet' processing unnecessary for userspace.  In other words userspace 
wouldn't need to establish raw sockets.  Hence my question, what is the 
actual concern here?

> 
> FWIW, you may have seen Markus's patch to remove preauth from the RX as
> well, this won't work as is, but I'm still a bit on the fence as to
> whether I'll force you into the right model or not (i.e. clear the
> existing capability bit in mac80211, and introduce a new one that
> doesn't report preauth over nl80211). For RX, however, the difference
> isn't really that much of a big deal, so maybe just make it optional.

We're actually quite happy with the current model.  So I'd like to keep 
things as they are.

Regards,
-Denis

Re: [PATCH 1/2] Revert "mac80211: support NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211_MAC_ADDRS"

[Johannes Berg]

On Mon, 2020-02-24 at 12:26 -0600, Denis Kenzior wrote:

> > These are two entirely different things, preauth is simply real data as
> > far as the local system is concerned. It's not related to controlled
> > port operation at all, which this nl80211 API is about.
> 
> I can understand this argument, but from what I remember, one of the 
> goals of the control port API was to make this legacy 'special data 
> packet' processing unnecessary for userspace.  In other words userspace 
> wouldn't need to establish raw sockets.  Hence my question, what is the 
> actual concern here?

That's a question of how you define "special data packet processing"...
You're defining it purely in terms of the mechanics of how you handle
them, but that's not really the point.

Preauth frames are _not_ special. They're entirely regular data packets
as far as wifi is concerned.

What _is_ special is EAPOL packets, because you may need to control
their encryption, know if they were ACKed, etc.

> > FWIW, you may have seen Markus's patch to remove preauth from the RX as
> > well, this won't work as is, but I'm still a bit on the fence as to
> > whether I'll force you into the right model or not (i.e. clear the
> > existing capability bit in mac80211, and introduce a new one that
> > doesn't report preauth over nl80211). For RX, however, the difference
> > isn't really that much of a big deal, so maybe just make it optional.
> 
> We're actually quite happy with the current model.  So I'd like to keep 
> things as they are.

I concede that on RX, there isn't actually really a _problem_, although
it's really strange to transport what really is just data frames (from a
wifi POV) over a control path ...

Depending on how much complexity it adds in the kernel (I've not tried
to fix that today) I guess we can keep this. I'd _prefer_ not to, but I
guess I cannot convince you that the preauth model is wrong, and -
mostly as a sign of how much you've worn me down - I'll probably just
keep it.

johannes

Re: [PATCH 1/2] Revert "mac80211: support NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211_MAC_ADDRS"

[Denis Kenzior]

Hi Johannes,

> That's a question of how you define "special data packet processing"...
> You're defining it purely in terms of the mechanics of how you handle
> them, but that's not really the point.

Why isn't it the point?  These are the only data packets userspace 
management daemon(s) actually care about and has to setup raw sockets + 
bpf filters for every interface it manages.  The current control port 
makes all of that unnecessary.

So from a holistic point of view, taking kernel + userspace into 
account, what is wrong with letting control port transport preauth 
frames if that saves a bunch of resources (and possibly wakeups if the 
bpf is setup badly) on the system?

Also, the question is what changed your mind?  I asked you specifically 
if preauth should be included in the control port API and you thought it 
was a good idea at the time?

> 
> Preauth frames are _not_ special. They're entirely regular data packets
> as far as wifi is concerned.

Sure.  I already conceded this point if this wasn't clear earlier.

Regards,
-Denis

Re: [PATCH 1/2] Revert "mac80211: support NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211_MAC_ADDRS"

[Johannes Berg]

On Mon, 2020-02-24 at 12:57 -0600, Denis Kenzior wrote:
> 
> Why isn't it the point?  These are the only data packets userspace 
> management daemon(s) actually care about and has to setup raw sockets + 
> bpf filters for every interface it manages.  The current control port 
> makes all of that unnecessary.

Sure.

> So from a holistic point of view, taking kernel + userspace into 
> account, what is wrong with letting control port transport preauth 
> frames if that saves a bunch of resources (and possibly wakeups if the 
> bpf is setup badly) on the system?

If you paint it that way, it doesn't seem like there's anything wrong
with it, does it?

But not sure that's the right color - you could apply that precise same
argument to, say, transporting DHCP packets over the same path? I think
you'd agree that doesn't make it right?

Just because preauth is a wifi related protocol doesn't mean we should
treat it in a wifi control path.

> Also, the question is what changed your mind?  I asked you specifically 
> if preauth should be included in the control port API and you thought it 
> was a good idea at the time?

I don't really remember that, but perhaps? Mostly I guess the discussion
on the hostap list now, I suppose.

johannes

Re: [PATCH 1/2] Revert "mac80211: support NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211_MAC_ADDRS"

[Denis Kenzior]

Hi Johannes,

>> So from a holistic point of view, taking kernel + userspace into
>> account, what is wrong with letting control port transport preauth
>> frames if that saves a bunch of resources (and possibly wakeups if the
>> bpf is setup badly) on the system?
> 
> If you paint it that way, it doesn't seem like there's anything wrong
> with it, does it?
> 
> But not sure that's the right color - you could apply that precise same
> argument to, say, transporting DHCP packets over the same path? I think
> you'd agree that doesn't make it right?

But I'm not, and I don't think the argument for DHCP is anywhere as 
strong.  DHCP is typically handled outside of the wifi management daemon 
(though it can be done inside) and is usually something that is very 
much separate from WiFi details regardless of where it lives.

> 
> Just because preauth is a wifi related protocol doesn't mean we should
> treat it in a wifi control path.

But it seems like the benefits outweigh the drawbacks?  At least we have 
been super happy with how control port works for us.  If you take the 
pre-auth path away, I'm really not sure there's any point in (at least 
for us) keeping support for the control port path.

> 
>> Also, the question is what changed your mind?  I asked you specifically
>> if preauth should be included in the control port API and you thought it
>> was a good idea at the time?
> 
> I don't really remember that, but perhaps? Mostly I guess the discussion
> on the hostap list now, I suppose.

I'm not subscribed there.  A synopsis of the discussion (or 
linux-wireless CC) would have been nice.

Regards,
-Denis

Re: [PATCH 1/2] Revert "mac80211: support NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211_MAC_ADDRS"

[Jouni Malinen]

On Mon, Feb 24, 2020 at 01:35:51PM -0600, Denis Kenzior wrote:
> But it seems like the benefits outweigh the drawbacks?  At least we have
> been super happy with how control port works for us.  If you take the
> pre-auth path away, I'm really not sure there's any point in (at least for
> us) keeping support for the control port path.

Do you use the control port for RX only or both TX and RX? The RX side
is mostly harmless _if_ something filters unprotected RSN
pre-authentication frames that are received between the association and
the completion of 4-way handshake. That something would either need to
be the specific user space application using the interface or
potentially mac80211 with some special rules that are different between
EAPOL and RSN pre-authentication ethertypes.

For TX, the control port path will likely result in more problematic
issues. I'd expect drivers to use higher priority and/or higher
reliability for delivering the frames. That is justifiable for EAPOL
frames, but unnecessary for RSN pre-authentication frames. Being able to
bypass the port authorization control would be undesired from security
view point.

The key point for me here is the concept of authorized/unauthorized port
for normal Data frames based on the IEEE 802.1X standard. Only the
frames critical for the authentication service (establishing protected
link with the current access point) are allowed to be transmitted and
received while the port used for normal data is unauthorized. For IEEE
802.11, only the EAPOL frames are such Data frames that are needed
before the port can be authorized. RSN pre-authentication frames are
used to establish a new security association with a different access
point once the port with the current AP is authorized. As such, RSN
pre-authentication frames do not need to go through any special path
from the protocol view point and in fact, it would be incorrect to allow
them to be transmitted or received before the main port has been
authorized.
 
The IEEE 802.11 standard describes this with "communication of all
non-IEEE-802.1X MSDUs sent or received" being authorized/not-authorized.
MSDU is a reference to Data frames and "non-IEEE-802.1X" in this context
to any ethertype other than the one defined in IEEE 802.1X (EAPOL).

As a more specific example, the EAPOL frames are expected to be
transmitted unencrypted during the initial 4-way handshake (and with
some old IEEE 802.1X/WEP designs and some WPA(v1) implementation, even
during rekeying). On the other hand, RSN pre-authentication frames are
never supposed to go out unencrypted over the air (i.e., they must not
be sent or received before the encryption key has been established for
the link). The IEEE 802.11 standard describes this with "A STA shall not
use preauthentication except when pairwise keys are employed" and "As
preauthentication frames do not use the IEEE 802.1X EAPOL EtherType
field, the AP with which the STA is currently associated need not apply
any special handling. The AP and the MAC in the STA shall handle these
frames in the same way as other frames with arbitrary EtherType field
values that require distribution via the DS."

I understand that there is a different view point for this from the
kernel--user space interface side and it may indeed look more
convenient to use the same path for both EAPOL and RSN
pre-authentication frames from that view point. If that mechanism is
used, it needs to be understood that the rules for EAPOL and RSN
pre-authentication frames are different, though, and it is not clear
where that difference is going to be enforced if the same interface path
is used.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH 1/2] Revert "mac80211: support NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211_MAC_ADDRS"

[Denis Kenzior]

Hi Jouni,

On 2/25/20 5:00 AM, Jouni Malinen wrote:
> On Mon, Feb 24, 2020 at 01:35:51PM -0600, Denis Kenzior wrote:
>> But it seems like the benefits outweigh the drawbacks?  At least we have
>> been super happy with how control port works for us.  If you take the
>> pre-auth path away, I'm really not sure there's any point in (at least for
>> us) keeping support for the control port path.
> 
> Do you use the control port for RX only or both TX and RX? The RX side

Both.  For reasons already discussed.

> is mostly harmless _if_ something filters unprotected RSN
> pre-authentication frames that are received between the association and
> the completion of 4-way handshake. That something would either need to
> be the specific user space application using the interface or
> potentially mac80211 with some special rules that are different between
> EAPOL and RSN pre-authentication ethertypes.

For mac80211, pre-authentication frames are already filtered, or at 
least that is the intent.  See ieee80211_frame_allowed().  Only 
control_port_protocol packets are allowed through if the station is not 
yet authorized.  Under normal circumstances that would only be EAPoL 
packets (or esoteric protocols like WAPI).  Pre-auth frames would be 
filtered.

Furthermore, only the userspace daemon that initiated the association 
would get to see the packets flowing via NL80211_CMD_CONTROL_PORT_FRAME 
(by providing SOCKET_OWNER attribute).  In iwd, we would drop any 
pre-auth packets without an initiated session on the floor.  And we 
don't initiate pre-auth sessions until we are fully 
associated/authenticated/authorized.

Last I checked, mac80211 is the only driver that supports this control 
port mechanism.  If other drivers obtain this support, then perhaps 
stricter checking of the packets flowing via cfg80211_rx_control_port() 
would be a good idea.  However, I'm not sure how much can be done here 
due to nl80211 being stateless.

> 
> For TX, the control port path will likely result in more problematic
> issues. I'd expect drivers to use higher priority and/or higher
> reliability for delivering the frames. That is justifiable for EAPOL

Fair enough, but the driver is notified of the protocol being sent in 
tx_control_port().  So it can easily choose not to prioritize pre-auth 
packets.

> frames, but unnecessary for RSN pre-authentication frames. Being able to
> bypass the port authorization control would be undesired from security
> view point.

TX_CONTROL_PORT does require administrative access for the caller.  The 
userspace management daemon is already trusted to do a lot of things by 
the kernel (including cleaning up things inside the kernel itself in 
certain cases).  If the userspace daemon cannot be trusted to send 
control port frames at the appropriate time, then the 'undesired from 
security view point' argument would apply quite broadly across the 
entire nl80211 API.  In fact, even the 
NL80211_ATTR_CONTROL_PORT_ETHERTYPE is implicitly trusted to be provided 
correctly by userspace.

 From a practical perspective, if you're worried about this, then 
perhaps stricter checking in nl80211_tx_control_port() is warranted. 
But that really implies peeking into the frames being sent...

Another related issue is that NL80211_CMD_TX_CONTROL_PORT (amongst 
others) doesn't actually check whether the command is being called by 
the $SOCKET_OWNER process for the target netdev.  Realistically, only 
the $SOCKET_OWNER process has the necessary secrets to generate packets 
that go via this path.  I've submitted some RFC patches to lock this 
down, but they were rejected.

> 
> The key point for me here is the concept of authorized/unauthorized port
> for normal Data frames based on the IEEE 802.1X standard. Only the
> frames critical for the authentication service (establishing protected
> link with the current access point) are allowed to be transmitted and
> received while the port used for normal data is unauthorized. For IEEE
> 802.11, only the EAPOL frames are such Data frames that are needed
> before the port can be authorized. RSN pre-authentication frames are
> used to establish a new security association with a different access
> point once the port with the current AP is authorized. As such, RSN
> pre-authentication frames do not need to go through any special path
> from the protocol view point and in fact, it would be incorrect to allow
> them to be transmitted or received before the main port has been
> authorized.
>   
> The IEEE 802.11 standard describes this with "communication of all
> non-IEEE-802.1X MSDUs sent or received" being authorized/not-authorized.
> MSDU is a reference to Data frames and "non-IEEE-802.1X" in this context
> to any ethertype other than the one defined in IEEE 802.1X (EAPOL).
> 
> As a more specific example, the EAPOL frames are expected to be
> transmitted unencrypted during the initial 4-way handshake (and with
> some old IEEE 802.1X/WEP designs and some WPA(v1) implementation, even
> during rekeying). On the other hand, RSN pre-authentication frames are
> never supposed to go out unencrypted over the air (i.e., they must not
> be sent or received before the encryption key has been established for
> the link). The IEEE 802.11 standard describes this with "A STA shall not
> use preauthentication except when pairwise keys are employed" and "As
> preauthentication frames do not use the IEEE 802.1X EAPOL EtherType
> field, the AP with which the STA is currently associated need not apply
> any special handling. The AP and the MAC in the STA shall handle these
> frames in the same way as other frames with arbitrary EtherType field
> values that require distribution via the DS."

No disagreement with anything you said here...

> 
> I understand that there is a different view point for this from the
> kernel--user space interface side and it may indeed look more
> convenient to use the same path for both EAPOL and RSN
> pre-authentication frames from that view point. If that mechanism is
> used, it needs to be understood that the rules for EAPOL and RSN
> pre-authentication frames are different, though, and it is not clear
> where that difference is going to be enforced if the same interface path
> is used.
> 

I understand the purity argument you're making here, and I don't 
disagree with it.  Perhaps the naming of the commands, with CONTROL_PORT 
inside them, is unfortunate, but it seemed like a good idea at the time. 
  It is also unfortunate that pre-authentication was designed the way it 
was.  It is turning out to be somewhat of an anomaly that we 
nevertheless have to deal with.

I do indeed view this through a different lens.  Only the wifi 
management daemon can realistically generate any frames that flow 
through the control port, including pre-auth frames.  So I think it 
makes very good sense to provide an optimized path for the userspace 
daemon to send/receive these frames and not force it to create sockets 
and use up resources needlessly.

I also don't mind if RX forwarding of pre-auth frames via the control 
port is made optional.  What I would not want to happen is for this 
capability to be removed completely.

Regards,
-Denis

Re: [PATCH 1/2] Revert "mac80211: support NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211_MAC_ADDRS"

[Markus Theil]

On 25.02.20 18:06, Denis Kenzior wrote:
> Hi Jouni,
>
> On 2/25/20 5:00 AM, Jouni Malinen wrote:
>> On Mon, Feb 24, 2020 at 01:35:51PM -0600, Denis Kenzior wrote:
>>> But it seems like the benefits outweigh the drawbacks?  At least we
>>> have
>>> been super happy with how control port works for us.  If you take the
>>> pre-auth path away, I'm really not sure there's any point in (at
>>> least for
>>> us) keeping support for the control port path.
>>
>> Do you use the control port for RX only or both TX and RX? The RX side
>
> Both.  For reasons already discussed.
>
>> is mostly harmless _if_ something filters unprotected RSN
>> pre-authentication frames that are received between the association and
>> the completion of 4-way handshake. That something would either need to
>> be the specific user space application using the interface or
>> potentially mac80211 with some special rules that are different between
>> EAPOL and RSN pre-authentication ethertypes.
>
> For mac80211, pre-authentication frames are already filtered, or at
> least that is the intent.  See ieee80211_frame_allowed().  Only
> control_port_protocol packets are allowed through if the station is
> not yet authorized.  Under normal circumstances that would only be
> EAPoL packets (or esoteric protocols like WAPI).  Pre-auth frames
> would be filtered.
>
> Furthermore, only the userspace daemon that initiated the association
> would get to see the packets flowing via
> NL80211_CMD_CONTROL_PORT_FRAME (by providing SOCKET_OWNER attribute). 
> In iwd, we would drop any pre-auth packets without an initiated
> session on the floor.  And we don't initiate pre-auth sessions until
> we are fully associated/authenticated/authorized.
>
> Last I checked, mac80211 is the only driver that supports this control
> port mechanism.  If other drivers obtain this support, then perhaps
> stricter checking of the packets flowing via
> cfg80211_rx_control_port() would be a good idea.  However, I'm not
> sure how much can be done here due to nl80211 being stateless.
>
>>
>> For TX, the control port path will likely result in more problematic
>> issues. I'd expect drivers to use higher priority and/or higher
>> reliability for delivering the frames. That is justifiable for EAPOL
>
> Fair enough, but the driver is notified of the protocol being sent in
> tx_control_port().  So it can easily choose not to prioritize pre-auth
> packets.
>
>> frames, but unnecessary for RSN pre-authentication frames. Being able to
>> bypass the port authorization control would be undesired from security
>> view point.
>
> TX_CONTROL_PORT does require administrative access for the caller. 
> The userspace management daemon is already trusted to do a lot of
> things by the kernel (including cleaning up things inside the kernel
> itself in certain cases).  If the userspace daemon cannot be trusted
> to send control port frames at the appropriate time, then the
> 'undesired from security view point' argument would apply quite
> broadly across the entire nl80211 API.  In fact, even the
> NL80211_ATTR_CONTROL_PORT_ETHERTYPE is implicitly trusted to be
> provided correctly by userspace.
>
> From a practical perspective, if you're worried about this, then
> perhaps stricter checking in nl80211_tx_control_port() is warranted.
> But that really implies peeking into the frames being sent...
>
> Another related issue is that NL80211_CMD_TX_CONTROL_PORT (amongst
> others) doesn't actually check whether the command is being called by
> the $SOCKET_OWNER process for the target netdev.  Realistically, only
> the $SOCKET_OWNER process has the necessary secrets to generate
> packets that go via this path.  I've submitted some RFC patches to
> lock this down, but they were rejected.
>
>>
>> The key point for me here is the concept of authorized/unauthorized port
>> for normal Data frames based on the IEEE 802.1X standard. Only the
>> frames critical for the authentication service (establishing protected
>> link with the current access point) are allowed to be transmitted and
>> received while the port used for normal data is unauthorized. For IEEE
>> 802.11, only the EAPOL frames are such Data frames that are needed
>> before the port can be authorized. RSN pre-authentication frames are
>> used to establish a new security association with a different access
>> point once the port with the current AP is authorized. As such, RSN
>> pre-authentication frames do not need to go through any special path
>> from the protocol view point and in fact, it would be incorrect to allow
>> them to be transmitted or received before the main port has been
>> authorized.
>>   The IEEE 802.11 standard describes this with "communication of all
>> non-IEEE-802.1X MSDUs sent or received" being authorized/not-authorized.
>> MSDU is a reference to Data frames and "non-IEEE-802.1X" in this context
>> to any ethertype other than the one defined in IEEE 802.1X (EAPOL).
>>
>> As a more specific example, the EAPOL frames are expected to be
>> transmitted unencrypted during the initial 4-way handshake (and with
>> some old IEEE 802.1X/WEP designs and some WPA(v1) implementation, even
>> during rekeying). On the other hand, RSN pre-authentication frames are
>> never supposed to go out unencrypted over the air (i.e., they must not
>> be sent or received before the encryption key has been established for
>> the link). The IEEE 802.11 standard describes this with "A STA shall not
>> use preauthentication except when pairwise keys are employed" and "As
>> preauthentication frames do not use the IEEE 802.1X EAPOL EtherType
>> field, the AP with which the STA is currently associated need not apply
>> any special handling. The AP and the MAC in the STA shall handle these
>> frames in the same way as other frames with arbitrary EtherType field
>> values that require distribution via the DS."
>
> No disagreement with anything you said here...
>
>>
>> I understand that there is a different view point for this from the
>> kernel--user space interface side and it may indeed look more
>> convenient to use the same path for both EAPOL and RSN
>> pre-authentication frames from that view point. If that mechanism is
>> used, it needs to be understood that the rules for EAPOL and RSN
>> pre-authentication frames are different, though, and it is not clear
>> where that difference is going to be enforced if the same interface path
>> is used.
>>
>
> I understand the purity argument you're making here, and I don't
> disagree with it.  Perhaps the naming of the commands, with
> CONTROL_PORT inside them, is unfortunate, but it seemed like a good
> idea at the time.  It is also unfortunate that pre-authentication was
> designed the way it was.  It is turning out to be somewhat of an
> anomaly that we nevertheless have to deal with.
>
> I do indeed view this through a different lens.  Only the wifi
> management daemon can realistically generate any frames that flow
> through the control port, including pre-auth frames.  So I think it
> makes very good sense to provide an optimized path for the userspace
> daemon to send/receive these frames and not force it to create sockets
> and use up resources needlessly.
>
> I also don't mind if RX forwarding of pre-auth frames via the control
> port is made optional.  What I would not want to happen is for this
> capability to be removed completely. 
Is there any consensus on how to move on in this discussion? In my
opinion a pragmatic way would be to make rx forwarding of pre-auth
frames optional with a flag which can be set whenever
NL80211_ATTR_CONTROL_PORT_OVER_NL80211 is included, like Denis has
already proposed. We could call this flag for example
NL80211_ATTR_CONTROL_PORT_NO_PREAUTH.

Re: [PATCH 1/2] Revert "mac80211: support NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211_MAC_ADDRS"

[Johannes Berg]

On Wed, 2020-03-04 at 17:24 +0100, Markus Theil wrote:

> Is there any consensus on how to move on in this discussion? In my
> opinion a pragmatic way would be to make rx forwarding of pre-auth
> frames optional with a flag which can be set whenever
> NL80211_ATTR_CONTROL_PORT_OVER_NL80211 is included, like Denis has
> already proposed. We could call this flag for example
> NL80211_ATTR_CONTROL_PORT_NO_PREAUTH.

Yeah, that seems right.

Do you want to make the patch?

johannes

Re: [PATCH 1/2] Revert "mac80211: support NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211_MAC_ADDRS"

[Markus Theil]

On 11.03.20 10:31, Johannes Berg wrote:
> On Wed, 2020-03-04 at 17:24 +0100, Markus Theil wrote:
>
>> Is there any consensus on how to move on in this discussion? In my
>> opinion a pragmatic way would be to make rx forwarding of pre-auth
>> frames optional with a flag which can be set whenever
>> NL80211_ATTR_CONTROL_PORT_OVER_NL80211 is included, like Denis has
>> already proposed. We could call this flag for example
>> NL80211_ATTR_CONTROL_PORT_NO_PREAUTH.
> Yeah, that seems right.
>
> Do you want to make the patch?
Yes, I have already some code lying around for it, but did not have time
to test them.
> johannes
>