diff mbox series

nl80211: use the process_bss_event for the nl_connect handler

Message ID 20210121154037.32654-3-andrei.otcheretianski@intel.com
State Changes Requested
Headers show
Series nl80211: use the process_bss_event for the nl_connect handler | expand

Commit Message

Andrei Otcheretianski Jan. 21, 2021, 3:40 p.m. UTC 
From: Avraham Stern <avraham.stern@intel.com>

The nl_connect is initialized with the process_bss_event() handler.
However, it is used several times with the default valid handler.
As a result, if a message that is only valid for the
process_bss_event() is received while the default handler is used, it
will be dropped.

This has been observed in a case where during the 4 way handshake, a
beacon is received on the AP side, which triggers a beacon update,
just before receiving the next EAPOL.
When send_and_recv_msgs_owner() is called for sending the
NL80211_CMD_SET_BEACON command, the NL80211_CMD_CONTROL_PORT_FRAME
event is already pending.
As a result, it is received with the default handler, which drops it.
Since the EAPOL frame is dropped, the connection attempt fails.

Fix it by using the process_bss_event() handler when the nl_connect
handler is used.

Signed-off-by: Avraham Stern <avraham.stern@intel.com>
---
 src/drivers/driver_nl80211.c      | 75 +++++++++++++++----------------
 src/drivers/driver_nl80211.h      |  2 +-
 src/drivers/driver_nl80211_scan.c |  2 +-
 3 files changed, 38 insertions(+), 41 deletions(-)

Comments

Jouni Malinen Feb. 6, 2021, 9:35 a.m. UTC | #1 
On Thu, Jan 21, 2021 at 05:40:34PM +0200, Andrei Otcheretianski wrote:
> Fix it by using the process_bss_event() handler when the nl_connect
> handler is used.

In general, that sounds fine, but there is one detail that is missed
here in the implementation:

> +send_and_recv_msgs_connect_handle(struct wpa_driver_nl80211_data *drv,
> +				  struct nl_msg *msg, struct i802_bss *bss)
> +{
> +	struct nl_sock *nl_connect = get_connect_handle(bss);
> +
> +	if (nl_connect)
> +		return send_and_recv_msgs_owner(drv, msg, nl_connect, 1,
> +						process_bss_event, bss, NULL,
> +						NULL);

> @@ -6196,8 +6199,7 @@ skip_auth_type:
> -	ret = send_and_recv_msgs_owner(drv, msg, nl_connect, 1, NULL,
> -				       (void *) -1, NULL, NULL);
> +	ret = send_and_recv_msgs_connect_handle(drv, msg, bss);

This would lose that special valid_handler = NULL, valid_data = (void *)
-1 combination that is needed at the end of send_and_recv() to be able
to use nl80211_nlmsg_clear(msg) to get any private material like keys
explicitly cleared from freed heap memory. See commit bbd89bfca0b4i
("nl80211: Clear nlmsg payload with keys before freeing") for more
details.

That special case needs to be covered here. Since it may be inconvenient
to cover this without adding new arguments to all send_and_recv
functions, it may be worth considering whether that conditional
nl80211_nlmsg_clear() call at the end of send_and_recv() should simply
be made unconditional.. It would burn some more resources clearing
memory unnecessarily for most messages, but that's unlikely to be much
of and issue in practice.
Andrei Otcheretianski Feb. 10, 2021, 12:58 p.m. UTC | #2 
> > @@ -6196,8 +6199,7 @@ skip_auth_type:
> > -	ret = send_and_recv_msgs_owner(drv, msg, nl_connect, 1, NULL,
> > -				       (void *) -1, NULL, NULL);
> > +	ret = send_and_recv_msgs_connect_handle(drv, msg, bss);
> 
> This would lose that special valid_handler = NULL, valid_data = (void *)
> -1 combination that is needed at the end of send_and_recv() to be able to
> use nl80211_nlmsg_clear(msg) to get any private material like keys explicitly
> cleared from freed heap memory. See commit bbd89bfca0b4i
> ("nl80211: Clear nlmsg payload with keys before freeing") for more details.
> 
> That special case needs to be covered here. Since it may be inconvenient to
> cover this without adding new arguments to all send_and_recv functions, it
> may be worth considering whether that conditional
> nl80211_nlmsg_clear() call at the end of send_and_recv() should simply be
> made unconditional.. It would burn some more resources clearing memory
> unnecessarily for most messages, but that's unlikely to be much of and issue
> in practice.

This is a good point that I missed in the review. I will submit a fixed version.
Thank you for reviewing.
Andrei

> 
> --
> Jouni Malinen                                            PGP id EFC895FA
diff mbox series

Patch

diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
index 13ecd11d3f..3a78380172 100644
--- a/src/drivers/driver_nl80211.c
+++ b/src/drivers/driver_nl80211.c
@@ -530,6 +530,21 @@  static int send_and_recv_msgs_owner(struct wpa_driver_nl80211_data *drv,
 }
 
 
+static int
+send_and_recv_msgs_connect_handle(struct wpa_driver_nl80211_data *drv,
+				  struct nl_msg *msg, struct i802_bss *bss)
+{
+	struct nl_sock *nl_connect = get_connect_handle(bss);
+
+	if (nl_connect)
+		return send_and_recv_msgs_owner(drv, msg, nl_connect, 1,
+						process_bss_event, bss, NULL,
+						NULL);
+	else
+		return send_and_recv_msgs(drv, msg, NULL, NULL, NULL, NULL);
+}
+
+
 struct nl_sock * get_connect_handle(struct i802_bss *bss)
 {
 	if ((bss->drv->capa.flags2 & WPA_DRIVER_FLAGS2_CONTROL_PORT_RX) ||
@@ -3515,10 +3530,11 @@  static int nl80211_set_conn_keys(struct wpa_driver_associate_params *params,
 int wpa_driver_nl80211_mlme(struct wpa_driver_nl80211_data *drv,
 			    const u8 *addr, int cmd, u16 reason_code,
 			    int local_state_change,
-			    struct nl_sock *nl_connect)
+			    struct i802_bss *bss)
 {
 	int ret;
 	struct nl_msg *msg;
+	struct nl_sock *nl_connect = get_connect_handle(bss);
 
 	if (!(msg = nl80211_drv_msg(drv, 0, cmd)) ||
 	    nla_put_u16(msg, NL80211_ATTR_REASON_CODE, reason_code) ||
@@ -3530,8 +3546,8 @@  int wpa_driver_nl80211_mlme(struct wpa_driver_nl80211_data *drv,
 	}
 
 	if (nl_connect)
-		ret = send_and_recv(drv->global, nl_connect, msg, NULL, NULL,
-				    NULL, NULL);
+		ret = send_and_recv(drv->global, nl_connect, msg,
+				    process_bss_event, bss, NULL, NULL);
 	else
 		ret = send_and_recv_msgs(drv, msg, NULL, NULL, NULL, NULL);
 	if (ret) {
@@ -3545,7 +3561,7 @@  int wpa_driver_nl80211_mlme(struct wpa_driver_nl80211_data *drv,
 
 static int wpa_driver_nl80211_disconnect(struct wpa_driver_nl80211_data *drv,
 					 u16 reason_code,
-					 struct nl_sock *nl_connect)
+					 struct i802_bss *bss)
 {
 	int ret;
 	int drv_associated = drv->associated;
@@ -3554,7 +3570,7 @@  static int wpa_driver_nl80211_disconnect(struct wpa_driver_nl80211_data *drv,
 	nl80211_mark_disconnected(drv);
 	/* Disconnect command doesn't need BSSID - it uses cached value */
 	ret = wpa_driver_nl80211_mlme(drv, NULL, NL80211_CMD_DISCONNECT,
-				      reason_code, 0, nl_connect);
+				      reason_code, 0, bss);
 	/*
 	 * For locally generated disconnect, supplicant already generates a
 	 * DEAUTH event, so ignore the event from NL80211.
@@ -3577,14 +3593,13 @@  static int wpa_driver_nl80211_deauthenticate(struct i802_bss *bss,
 		return nl80211_leave_ibss(drv, 1);
 	}
 	if (!(drv->capa.flags & WPA_DRIVER_FLAGS_SME)) {
-		return wpa_driver_nl80211_disconnect(drv, reason_code,
-						     get_connect_handle(bss));
+		return wpa_driver_nl80211_disconnect(drv, reason_code, bss);
 	}
 	wpa_printf(MSG_DEBUG, "%s(addr=" MACSTR " reason_code=%d)",
 		   __func__, MAC2STR(addr), reason_code);
 	nl80211_mark_disconnected(drv);
 	ret = wpa_driver_nl80211_mlme(drv, addr, NL80211_CMD_DEAUTHENTICATE,
-				      reason_code, 0, get_connect_handle(bss));
+				      reason_code, 0, bss);
 	/*
 	 * For locally generated deauthenticate, supplicant already generates a
 	 * DEAUTH event, so ignore the event from NL80211.
@@ -4604,15 +4619,7 @@  static int wpa_driver_nl80211_set_ap(void *priv,
 	}
 #endif /* CONFIG_IEEE80211AX */
 
-#ifdef CONFIG_SAE
-	if (((params->key_mgmt_suites & WPA_KEY_MGMT_SAE) ||
-	     (params->key_mgmt_suites & WPA_KEY_MGMT_FT_SAE)) &&
-	    nl80211_put_sae_pwe(msg, params->sae_pwe) < 0)
-		goto fail;
-#endif /* CONFIG_SAE */
-
-	ret = send_and_recv_msgs_owner(drv, msg, get_connect_handle(bss), 1,
-				       NULL, NULL, NULL, NULL);
+	ret = send_and_recv_msgs_connect_handle(drv, msg, bss);
 	if (ret) {
 		wpa_printf(MSG_DEBUG, "nl80211: Beacon set failed: %d (%s)",
 			   ret, strerror(-ret));
@@ -5652,9 +5659,7 @@  static int nl80211_leave_ibss(struct wpa_driver_nl80211_data *drv,
 	int ret;
 
 	msg = nl80211_drv_msg(drv, 0, NL80211_CMD_LEAVE_IBSS);
-	ret = send_and_recv_msgs_owner(drv, msg,
-				       get_connect_handle(drv->first_bss), 1,
-				       NULL, NULL, NULL, NULL);
+	ret = send_and_recv_msgs_connect_handle(drv, msg, drv->first_bss);
 	if (ret) {
 		wpa_printf(MSG_DEBUG, "nl80211: Leave IBSS failed: ret=%d "
 			   "(%s)", ret, strerror(-ret));
@@ -5786,9 +5791,7 @@  retry:
 	if (ret < 0)
 		goto fail;
 
-	ret = send_and_recv_msgs_owner(drv, msg,
-				       get_connect_handle(drv->first_bss), 1,
-				       NULL, NULL, NULL, NULL);
+	ret = send_and_recv_msgs_connect_handle(drv, msg, drv->first_bss);
 	msg = NULL;
 	if (ret) {
 		wpa_printf(MSG_DEBUG, "nl80211: Join IBSS failed: ret=%d (%s)",
@@ -6124,7 +6127,7 @@  static int nl80211_connect_common(struct wpa_driver_nl80211_data *drv,
 static int wpa_driver_nl80211_try_connect(
 	struct wpa_driver_nl80211_data *drv,
 	struct wpa_driver_associate_params *params,
-	struct nl_sock *nl_connect)
+	struct i802_bss *bss)
 {
 	struct nl_msg *msg;
 	enum nl80211_auth_type type;
@@ -6196,8 +6199,7 @@  skip_auth_type:
 	if (ret)
 		goto fail;
 
-	ret = send_and_recv_msgs_owner(drv, msg, nl_connect, 1, NULL,
-				       (void *) -1, NULL, NULL);
+	ret = send_and_recv_msgs_connect_handle(drv, msg, bss);
 	msg = NULL;
 	if (ret) {
 		wpa_printf(MSG_DEBUG, "nl80211: MLME connect failed: ret=%d "
@@ -6221,7 +6223,7 @@  fail:
 static int wpa_driver_nl80211_connect(
 	struct wpa_driver_nl80211_data *drv,
 	struct wpa_driver_associate_params *params,
-	struct nl_sock *nl_connect)
+	struct i802_bss *bss)
 {
 	int ret;
 
@@ -6231,7 +6233,7 @@  static int wpa_driver_nl80211_connect(
 	else
 		os_memset(drv->auth_attempt_bssid, 0, ETH_ALEN);
 
-	ret = wpa_driver_nl80211_try_connect(drv, params, nl_connect);
+	ret = wpa_driver_nl80211_try_connect(drv, params, bss);
 	if (ret == -EALREADY) {
 		/*
 		 * cfg80211 does not currently accept new connections if
@@ -6242,9 +6244,9 @@  static int wpa_driver_nl80211_connect(
 			   "disconnecting before reassociation "
 			   "attempt");
 		if (wpa_driver_nl80211_disconnect(
-			    drv, WLAN_REASON_PREV_AUTH_NOT_VALID, nl_connect))
+			    drv, WLAN_REASON_PREV_AUTH_NOT_VALID, bss))
 			return -1;
-		ret = wpa_driver_nl80211_try_connect(drv, params, nl_connect);
+		ret = wpa_driver_nl80211_try_connect(drv, params, bss);
 	}
 	return ret;
 }
@@ -6278,8 +6280,7 @@  static int wpa_driver_nl80211_associate(
 		else
 			bss->use_nl_connect = 0;
 
-		return wpa_driver_nl80211_connect(drv, params,
-						  get_connect_handle(bss));
+		return wpa_driver_nl80211_connect(drv, params, bss);
 	}
 
 	nl80211_mark_disconnected(drv);
@@ -6314,9 +6315,7 @@  static int wpa_driver_nl80211_associate(
 			goto fail;
 	}
 
-	ret = send_and_recv_msgs_owner(drv, msg,
-				       get_connect_handle(drv->first_bss), 1,
-				       NULL, NULL, NULL, NULL);
+	ret = send_and_recv_msgs_connect_handle(drv, msg, drv->first_bss);
 	msg = NULL;
 	if (ret) {
 		wpa_dbg(drv->ctx, MSG_DEBUG,
@@ -10299,8 +10298,7 @@  static int nl80211_join_mesh(struct i802_bss *bss,
 	if (nl80211_put_mesh_config(msg, &params->conf) < 0)
 		goto fail;
 
-	ret = send_and_recv_msgs_owner(drv, msg, get_connect_handle(bss), 1,
-				       NULL, NULL, NULL, NULL);
+	ret = send_and_recv_msgs_connect_handle(drv, msg, bss);
 	msg = NULL;
 	if (ret) {
 		wpa_printf(MSG_DEBUG, "nl80211: mesh join failed: ret=%d (%s)",
@@ -10357,8 +10355,7 @@  static int wpa_driver_nl80211_leave_mesh(void *priv)
 
 	wpa_printf(MSG_DEBUG, "nl80211: mesh leave (ifindex=%d)", drv->ifindex);
 	msg = nl80211_drv_msg(drv, 0, NL80211_CMD_LEAVE_MESH);
-	ret = send_and_recv_msgs_owner(drv, msg, get_connect_handle(bss), 0,
-				       NULL, NULL, NULL, NULL);
+	ret = send_and_recv_msgs_connect_handle(drv, msg, bss);
 	if (ret) {
 		wpa_printf(MSG_DEBUG, "nl80211: mesh leave failed: ret=%d (%s)",
 			   ret, strerror(-ret));
diff --git a/src/drivers/driver_nl80211.h b/src/drivers/driver_nl80211.h
index 7b9be1f3a9..924064fd76 100644
--- a/src/drivers/driver_nl80211.h
+++ b/src/drivers/driver_nl80211.h
@@ -270,7 +270,7 @@  int wpa_driver_nl80211_set_mode(struct i802_bss *bss,
 int wpa_driver_nl80211_mlme(struct wpa_driver_nl80211_data *drv,
 			    const u8 *addr, int cmd, u16 reason_code,
 			    int local_state_change,
-			    struct nl_sock *nl_connect);
+			    struct i802_bss *bss);
 
 int nl80211_create_monitor_interface(struct wpa_driver_nl80211_data *drv);
 void nl80211_remove_monitor_interface(struct wpa_driver_nl80211_data *drv);
diff --git a/src/drivers/driver_nl80211_scan.c b/src/drivers/driver_nl80211_scan.c
index 233175d19b..1316084805 100644
--- a/src/drivers/driver_nl80211_scan.c
+++ b/src/drivers/driver_nl80211_scan.c
@@ -870,7 +870,7 @@  static void clear_state_mismatch(struct wpa_driver_nl80211_data *drv,
 		wpa_driver_nl80211_mlme(drv, addr,
 					NL80211_CMD_DEAUTHENTICATE,
 					WLAN_REASON_PREV_AUTH_NOT_VALID, 1,
-					get_connect_handle(drv->first_bss));
+					drv->first_bss);
 	}
 }