diff mbox series

[v2,2/4] wifi: mac80211: Add support for randomizing auth and deauth frames TA

Message ID 20220919121155.3069765-2-quic_vjakkam@quicinc.com
State New
Headers show
Series None | expand

Commit Message

Veerendranath Jakkam Sept. 19, 2022, 12:11 p.m. UTC 
Configure randomized transmit address of the authentication frames to
the driver when advertise NL80211_EXT_FEATURE_AUTH_AND_DEAUTH_RANDOM_TA
to enable receive behavior to accept frames with the same random MAC
address as RA also those frames need to be acknowledged similarly to the
frames sent to the local permanent MAC address. Disable the random MAC
address configuration when deauthentication frames with same random MAC
address received or transmitted.

Signed-off-by: Veerendranath Jakkam <quic_vjakkam@quicinc.com>
---
 include/net/mac80211.h    |  9 ++++++++
 net/mac80211/driver-ops.h | 10 +++++++++
 net/mac80211/main.c       |  5 +++++
 net/mac80211/rx.c         | 45 ++++++++++++++++++++++++++++++++++-----
 net/mac80211/trace.h      | 19 +++++++++++++++++
 net/mac80211/tx.c         | 27 +++++++++++++++++++++++
 6 files changed, 110 insertions(+), 5 deletions(-)

Comments

Johannes Berg Dec. 1, 2022, 1:32 p.m. UTC | #1 
Hi,

So I was going to apply patches 1 and 4 (with a leak fix to 4, please
check before you resend), but have some issues with this one, and that
raises a design question ... +Jouni because of that.

> +++ b/include/net/mac80211.h
> @@ -1832,6 +1832,7 @@ struct ieee80211_vif_cfg {
>   * @offload_flags: 802.3 -> 802.11 enapsulation offload flags, see
>   *	&enum ieee80211_offload_flags.
>   * @mbssid_tx_vif: Pointer to the transmitting interface if MBSSID is enabled.
> + * @random_addr: random address in use for this interface.


That could be in vif_cfg, no?

> 
> @@ -4164,6 +4167,11 @@ struct ieee80211_prep_tx_info {
>   *	Note that a sta can also be inserted or removed with valid links,
>   *	i.e. passed to @sta_add/@sta_state with sta->valid_links not zero.
>   *	In fact, cannot change from having valid_links and not having them.
> + * @config_random_mac: Configure random MAC address to send acknowledgment when
> + *	RA of the received frame matches with configured random MAC address.
> + *	Also, clear random MAC address configuration if zero MAC address set.
> + *	Driver must register callback for this when advertise
> + *	%NL80211_EXT_FEATURE_AUTH_AND_DEAUTH_RANDOM_TA.

And then maybe this isn't necessary, and we can use a new BSS change
flag instead?

Either way, you didn't document the context requirements here.

> +++ b/net/mac80211/rx.c
> @@ -4211,6 +4211,21 @@ static bool ieee80211_accept_frame(struct ieee80211_rx_data *rx)
>  	u8 *bssid = ieee80211_get_bssid(hdr, skb->len, sdata->vif.type);
>  	bool multicast = is_multicast_ether_addr(hdr->addr1) ||
>  			 ieee80211_is_s1g_beacon(hdr->frame_control);
> +	bool skip_addr1_check = false;
> +
> +	if ((ieee80211_is_auth(hdr->frame_control) ||
> +	     ieee80211_is_deauth(hdr->frame_control)) &&
> +	    wiphy_ext_feature_isset(
> +		   sdata->local->hw.wiphy,
> +		   NL80211_EXT_FEATURE_AUTH_AND_DEAUTH_RANDOM_TA) &&

No need for the support check - if not supported then vif.random_addr on
the next line cannot be set properly?

> +	    ether_addr_equal(sdata->vif.random_addr, hdr->addr1)) {
> +		skip_addr1_check = true;
> +		if (ieee80211_is_deauth(hdr->frame_control)) {
> +			eth_zero_addr(sdata->vif.random_addr);
> +			drv_config_random_mac(sdata->local,
> +					      rx->sdata->vif.random_addr);
> +		}
> +	}

maybe some unlikely() would be appropriate, e.g. around even only the
first is_auth||is_deauth since those are already unlikely and the rest
of the checks can be done separately?

I also _really_ don't like the call to drv_config_random_mac() in the
middle of this flow here - coming from the driver back into the driver
is always a pain.

This might be a reason *not* to do the vif config and all that, but you
should document that more clearly (the design decision process in the
commit message, and the actual context requirements in the kernel-doc.)

That said, aren't many drivers going to anyway punt this to a worker,
maybe we should here and then we can put it together with other vif
config?

I'm also asking myself when this is going to get cancelled if the frame
is *not* a deauth? For something like PASN you'd expect one response for
every TX, so maybe that's OK and we can cancel when you stop waiting?

OTOH, maybe this really shouldn't be called "random TA" but rather
"temporary address", and userspace picks in both mgmt-TX and remain-on-
channel?

As it is, until you deauth, you keep the random address alive in the
driver, afaict?

> @@ -4657,6 +4676,21 @@ static bool ieee80211_invoke_fast_rx(struct ieee80211_rx_data *rx,
>  	} addrs __aligned(2);
>  	struct link_sta_info *link_sta;
>  	struct ieee80211_sta_rx_stats *stats;
> +	bool skip_addr1_check = false;
> +
> +	if ((ieee80211_is_auth(hdr->frame_control) ||
> +	     ieee80211_is_deauth(hdr->frame_control)) &&
> +	    wiphy_ext_feature_isset(
> +		   rx->sdata->local->hw.wiphy,
> +		   NL80211_EXT_FEATURE_AUTH_AND_DEAUTH_RANDOM_TA) &&
> +	    ether_addr_equal(rx->sdata->vif.random_addr, hdr->addr1)) {
> +		skip_addr1_check = true;
> +		if (ieee80211_is_deauth(hdr->frame_control)) {
> +			eth_zero_addr(rx->sdata->vif.random_addr);
> +			drv_config_random_mac(rx->sdata->local,
> +					      rx->sdata->vif.random_addr);
> +		}
> +	}

Um, no, there's really no point in changing this function??? How did
that happen?

> +++ b/net/mac80211/tx.c
> @@ -2048,6 +2048,7 @@ void ieee80211_xmit(struct ieee80211_sub_if_data *sdata,
>  	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
>  	int headroom;
>  	enum ieee80211_encrypt encrypt;
> +	bool our_addr = true;
>  
>  	if (info->flags & IEEE80211_TX_INTFL_DONT_ENCRYPT)
>  		encrypt = ENCRYPT_NO;
> @@ -2071,6 +2072,32 @@ void ieee80211_xmit(struct ieee80211_sub_if_data *sdata,
>  	hdr = (struct ieee80211_hdr *) skb->data;
>  	info->control.vif = &sdata->vif;
>  
> +	switch (sdata->vif.type) {

This doesn't seem like an appropriate place for this logic - why not do
that closer to userspace? We go through this for *every* mac80211 TX,
but we only *need* it for userspace TX - i.e. somewhere mgmt_tx or so?


Anyway, I still think there's a design issue here in how you handle the
lifetime of this temporary address, and would think that it might make
more sense to address that within the context of remain-on-channel and
the response waiting time offchannel period. Remain-on-channel might not
even be needed for the purpose here since you'd get a response and then
give up waiting for a response? But it might be needed in some retry
cases perhaps, I don't know.

johannes
Veerendranath Jakkam Jan. 8, 2023, 7:11 a.m. UTC | #2 
On 12/1/2022 7:02 PM, Johannes Berg wrote:

>
> Anyway, I still think there's a design issue here in how you handle the
> lifetime of this temporary address, and would think that it might make
> more sense to address that within the context of remain-on-channel and
> the response waiting time offchannel period. Remain-on-channel might not
> even be needed for the purpose here since you'd get a response and then
> give up waiting for a response? But it might be needed in some retry
> cases perhaps, I don't know.
>
> johannes


Thanks Johannes for reviewing the patches. The suggested approach to 
keep the allowed random/temporary address till the response waiting time 
off-channel period  looks more cleaner and better. I will send new patch 
series with the suggested changes.


Also, we are considering to drop authentication offload changes patch 
(4th patch) from this series. We think it would be good to align with 
mac80211 and use MLD addresses in the frames during authentication 
offload case also. I will send a separate to patch for supporting 
authentication offload with MLD addressed frames.

--

veeru
diff mbox series

Patch

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index ac2bad57933f..662b5db84fe3 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1832,6 +1832,7 @@  struct ieee80211_vif_cfg {
  * @offload_flags: 802.3 -> 802.11 enapsulation offload flags, see
  *	&enum ieee80211_offload_flags.
  * @mbssid_tx_vif: Pointer to the transmitting interface if MBSSID is enabled.
+ * @random_addr: random address in use for this interface.
  */
 struct ieee80211_vif {
 	enum nl80211_iftype type;
@@ -1861,6 +1862,8 @@  struct ieee80211_vif {
 
 	struct ieee80211_vif *mbssid_tx_vif;
 
+	u8 random_addr[ETH_ALEN] __aligned(2);
+
 	/* must be last */
 	u8 drv_priv[] __aligned(sizeof(void *));
 };
@@ -4164,6 +4167,11 @@  struct ieee80211_prep_tx_info {
  *	Note that a sta can also be inserted or removed with valid links,
  *	i.e. passed to @sta_add/@sta_state with sta->valid_links not zero.
  *	In fact, cannot change from having valid_links and not having them.
+ * @config_random_mac: Configure random MAC address to send acknowledgment when
+ *	RA of the received frame matches with configured random MAC address.
+ *	Also, clear random MAC address configuration if zero MAC address set.
+ *	Driver must register callback for this when advertise
+ *	%NL80211_EXT_FEATURE_AUTH_AND_DEAUTH_RANDOM_TA.
  */
 struct ieee80211_ops {
 	void (*tx)(struct ieee80211_hw *hw,
@@ -4519,6 +4527,7 @@  struct ieee80211_ops {
 				struct ieee80211_vif *vif,
 				struct ieee80211_sta *sta,
 				u16 old_links, u16 new_links);
+	void (*config_random_mac)(struct ieee80211_hw *hw, const u8 *mac_addr);
 };
 
 /**
diff --git a/net/mac80211/driver-ops.h b/net/mac80211/driver-ops.h
index 81e40b0a3b16..ea2cd8048638 100644
--- a/net/mac80211/driver-ops.h
+++ b/net/mac80211/driver-ops.h
@@ -1479,4 +1479,14 @@  int drv_change_sta_links(struct ieee80211_local *local,
 			 struct ieee80211_sta *sta,
 			 u16 old_links, u16 new_links);
 
+static inline void drv_config_random_mac(struct ieee80211_local *local,
+					 const u8 *mac_addr)
+{
+	if (local->ops->config_random_mac) {
+		trace_drv_config_random_mac(local, mac_addr);
+		local->ops->config_random_mac(&local->hw, mac_addr);
+		trace_drv_return_void(local);
+	}
+}
+
 #endif /* __MAC80211_DRIVER_OPS */
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 46f3eddc2388..707b38865227 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -953,6 +953,11 @@  int ieee80211_register_hw(struct ieee80211_hw *hw)
 	     !local->ops->tdls_recv_channel_switch))
 		return -EOPNOTSUPP;
 
+	if ((hw->wiphy->features &
+	     NL80211_EXT_FEATURE_AUTH_AND_DEAUTH_RANDOM_TA) &&
+	    !local->ops->config_random_mac)
+		return -EOPNOTSUPP;
+
 	if (WARN_ON(ieee80211_hw_check(hw, SUPPORTS_TX_FRAG) &&
 		    !local->ops->set_frag_threshold))
 		return -EINVAL;
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index a57811372027..e76653795c1c 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -4211,6 +4211,21 @@  static bool ieee80211_accept_frame(struct ieee80211_rx_data *rx)
 	u8 *bssid = ieee80211_get_bssid(hdr, skb->len, sdata->vif.type);
 	bool multicast = is_multicast_ether_addr(hdr->addr1) ||
 			 ieee80211_is_s1g_beacon(hdr->frame_control);
+	bool skip_addr1_check = false;
+
+	if ((ieee80211_is_auth(hdr->frame_control) ||
+	     ieee80211_is_deauth(hdr->frame_control)) &&
+	    wiphy_ext_feature_isset(
+		   sdata->local->hw.wiphy,
+		   NL80211_EXT_FEATURE_AUTH_AND_DEAUTH_RANDOM_TA) &&
+	    ether_addr_equal(sdata->vif.random_addr, hdr->addr1)) {
+		skip_addr1_check = true;
+		if (ieee80211_is_deauth(hdr->frame_control)) {
+			eth_zero_addr(sdata->vif.random_addr);
+			drv_config_random_mac(sdata->local,
+					      rx->sdata->vif.random_addr);
+		}
+	}
 
 	switch (sdata->vif.type) {
 	case NL80211_IFTYPE_STATION:
@@ -4220,6 +4235,8 @@  static bool ieee80211_accept_frame(struct ieee80211_rx_data *rx)
 			return false;
 		if (multicast)
 			return true;
+		if (skip_addr1_check)
+			return true;
 		return ieee80211_is_our_addr(sdata, hdr->addr1, &rx->link_id);
 	case NL80211_IFTYPE_ADHOC:
 		if (!bssid)
@@ -4232,7 +4249,7 @@  static bool ieee80211_accept_frame(struct ieee80211_rx_data *rx)
 			return true;
 		if (!ieee80211_bssid_match(bssid, sdata->u.ibss.bssid))
 			return false;
-		if (!multicast &&
+		if (!multicast && !skip_addr1_check &&
 		    !ether_addr_equal(sdata->vif.addr, hdr->addr1))
 			return false;
 		if (!rx->sta) {
@@ -4252,7 +4269,7 @@  static bool ieee80211_accept_frame(struct ieee80211_rx_data *rx)
 			return false;
 		if (!is_broadcast_ether_addr(bssid))
 			return false;
-		if (!multicast &&
+		if (!multicast && !skip_addr1_check &&
 		    !ether_addr_equal(sdata->dev->dev_addr, hdr->addr1))
 			return false;
 		if (!rx->sta) {
@@ -4270,10 +4287,12 @@  static bool ieee80211_accept_frame(struct ieee80211_rx_data *rx)
 			return false;
 		if (multicast)
 			return true;
+		if (skip_addr1_check)
+			return true;
 		return ether_addr_equal(sdata->vif.addr, hdr->addr1);
 	case NL80211_IFTYPE_AP_VLAN:
 	case NL80211_IFTYPE_AP:
-		if (!bssid)
+		if (!bssid && !skip_addr1_check)
 			return ieee80211_is_our_addr(sdata, hdr->addr1,
 						     &rx->link_id);
 
@@ -4285,7 +4304,7 @@  static bool ieee80211_accept_frame(struct ieee80211_rx_data *rx)
 			 * and location updates. Note that mac80211
 			 * itself never looks at these frames.
 			 */
-			if (!multicast &&
+			if (!multicast && !skip_addr1_check &&
 			    !ieee80211_is_our_addr(sdata, hdr->addr1,
 						   &rx->link_id))
 				return false;
@@ -4657,6 +4676,21 @@  static bool ieee80211_invoke_fast_rx(struct ieee80211_rx_data *rx,
 	} addrs __aligned(2);
 	struct link_sta_info *link_sta;
 	struct ieee80211_sta_rx_stats *stats;
+	bool skip_addr1_check = false;
+
+	if ((ieee80211_is_auth(hdr->frame_control) ||
+	     ieee80211_is_deauth(hdr->frame_control)) &&
+	    wiphy_ext_feature_isset(
+		   rx->sdata->local->hw.wiphy,
+		   NL80211_EXT_FEATURE_AUTH_AND_DEAUTH_RANDOM_TA) &&
+	    ether_addr_equal(rx->sdata->vif.random_addr, hdr->addr1)) {
+		skip_addr1_check = true;
+		if (ieee80211_is_deauth(hdr->frame_control)) {
+			eth_zero_addr(rx->sdata->vif.random_addr);
+			drv_config_random_mac(rx->sdata->local,
+					      rx->sdata->vif.random_addr);
+		}
+	}
 
 	/* for parallel-rx, we need to have DUP_VALIDATED, otherwise we write
 	 * to a common data structure; drivers can implement that per queue
@@ -4690,7 +4724,8 @@  static bool ieee80211_invoke_fast_rx(struct ieee80211_rx_data *rx,
 	 * punting here will make it go through the full checks in
 	 * ieee80211_accept_frame().
 	 */
-	if (!ether_addr_equal(fast_rx->vif_addr, hdr->addr1))
+	if (!skip_addr1_check &&
+	    !ether_addr_equal(fast_rx->vif_addr, hdr->addr1))
 		return false;
 
 	if ((hdr->frame_control & cpu_to_le16(IEEE80211_FCTL_FROMDS |
diff --git a/net/mac80211/trace.h b/net/mac80211/trace.h
index 9f4377566c42..4996fd5a6887 100644
--- a/net/mac80211/trace.h
+++ b/net/mac80211/trace.h
@@ -3026,6 +3026,25 @@  TRACE_EVENT(stop_queue,
 	)
 );
 
+TRACE_EVENT(drv_config_random_mac,
+	TP_PROTO(struct ieee80211_local *local,
+		 const u8 *mac_addr),
+
+	TP_ARGS(local, mac_addr),
+
+	TP_STRUCT__entry(
+		LOCAL_ENTRY
+		__array(char, mac_addr, ETH_ALEN)
+	),
+
+	TP_fast_assign(
+		LOCAL_ASSIGN;
+		memcpy(__entry->mac_addr, mac_addr, ETH_ALEN);
+	),
+
+	TP_printk(LOCAL_PR_FMT ", addr:%pM", LOCAL_PR_ARG, __entry->mac_addr)
+);
+
 #endif /* !__MAC80211_DRIVER_TRACE || TRACE_HEADER_MULTI_READ */
 
 #undef TRACE_INCLUDE_PATH
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 24c0a1706b92..76e2457050cb 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -2048,6 +2048,7 @@  void ieee80211_xmit(struct ieee80211_sub_if_data *sdata,
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
 	int headroom;
 	enum ieee80211_encrypt encrypt;
+	bool our_addr = true;
 
 	if (info->flags & IEEE80211_TX_INTFL_DONT_ENCRYPT)
 		encrypt = ENCRYPT_NO;
@@ -2071,6 +2072,32 @@  void ieee80211_xmit(struct ieee80211_sub_if_data *sdata,
 	hdr = (struct ieee80211_hdr *) skb->data;
 	info->control.vif = &sdata->vif;
 
+	switch (sdata->vif.type) {
+	case NL80211_IFTYPE_OCB:
+		if (!ether_addr_equal(sdata->dev->dev_addr, hdr->addr2))
+			our_addr = false;
+		break;
+	default:
+		if (!ieee80211_is_our_addr(sdata, hdr->addr2, NULL))
+			our_addr = false;
+		break;
+	}
+
+	if (!our_addr &&
+	    wiphy_ext_feature_isset(
+		   local->hw.wiphy,
+		   NL80211_EXT_FEATURE_AUTH_AND_DEAUTH_RANDOM_TA)) {
+		if (ieee80211_is_auth(hdr->frame_control)) {
+			drv_config_random_mac(local, hdr->addr2);
+			ether_addr_copy(sdata->vif.random_addr, hdr->addr2);
+		} else if (ieee80211_is_deauth(hdr->frame_control) &&
+			   ether_addr_equal(sdata->vif.random_addr,
+					    hdr->addr2)) {
+			eth_zero_addr(sdata->vif.random_addr);
+			drv_config_random_mac(local, sdata->vif.random_addr);
+		}
+	}
+
 	if (ieee80211_vif_is_mesh(&sdata->vif)) {
 		if (ieee80211_is_data(hdr->frame_control) &&
 		    is_unicast_ether_addr(hdr->addr1)) {