[PATCH 1/8] ft: separate ft_sm from ft_process_ies

[PATCH 1/8] ft: separate ft_sm from ft_process_ies

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 2352 bytes --]

To prepare for some refactoring remove the ft_sm dependency
from ft_process_ies and instead only make it depend on the
handshake_state object.
---
 src/ft.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/src/ft.c b/src/ft.c
index ba9c3d2c..2c58787d 100644
--- a/src/ft.c
+++ b/src/ft.c
@@ -329,13 +329,13 @@ error:
 	return -EINVAL;
 }
 
-static int ft_process_ies(struct ft_sm *ft, const uint8_t *ies, size_t ies_len)
+static int ft_process_ies(struct handshake_state *hs, const uint8_t *ies,
+			size_t ies_len)
 {
 	struct ie_tlv_iter iter;
 	const uint8_t *rsne = NULL;
 	const uint8_t *mde = NULL;
 	const uint8_t *fte = NULL;
-	struct handshake_state *hs = ft->hs;
 	uint32_t kck_len = handshake_state_get_kck_len(hs);
 	bool is_rsn;
 
@@ -469,7 +469,7 @@ static int ft_process_ies(struct ft_sm *ft, const uint8_t *ies, size_t ies_len)
 	} else if (fte)
 		goto ft_error;
 
-	return ft_tx_reassociate(ft);
+	return 0;
 
 ft_error:
 	return -EBADMSG;
@@ -482,6 +482,7 @@ static int ft_rx_action(struct auth_proto *ap, const uint8_t *frame,
 	uint16_t status_code = MMPDU_STATUS_CODE_UNSPECIFIED;
 	const uint8_t *ies = NULL;
 	size_t ies_len;
+	int ret;
 
 	if (!ft_parse_action_resp_frame(frame, frame_len, ft->hs->spa,
 						ft->hs->aa, &status_code,
@@ -492,7 +493,11 @@ static int ft_rx_action(struct auth_proto *ap, const uint8_t *frame,
 	if (status_code != 0)
 		goto auth_error;
 
-	return ft_process_ies(ft, ies, ies_len);
+	ret = ft_process_ies(ft->hs, ies, ies_len);
+	if (ret < 0)
+		goto auth_error;
+
+	return ft_tx_reassociate(ft);
 
 auth_error:
 	return (int)status_code;
@@ -505,6 +510,7 @@ static int ft_rx_authenticate(struct auth_proto *ap, const uint8_t *frame,
 	uint16_t status_code = MMPDU_STATUS_CODE_UNSPECIFIED;
 	const uint8_t *ies = NULL;
 	size_t ies_len;
+	int ret;
 
 	/*
 	 * Parse the Authentication Response and validate the contents
@@ -520,7 +526,11 @@ static int ft_rx_authenticate(struct auth_proto *ap, const uint8_t *frame,
 	if (status_code != 0)
 		goto auth_error;
 
-	return ft_process_ies(ft, ies, ies_len);
+	ret = ft_process_ies(ft->hs, ies, ies_len);
+	if (ret < 0)
+		goto auth_error;
+
+	return ft_tx_reassociate(ft);
 
 auth_error:
 	return (int)status_code;
-- 
2.26.2

[PATCH 2/8] ft: expose ft_build_authenticate_ies

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 3808 bytes --]

The building of the (FT)Authenticate frame will need
to be shared between ft and netdev once FT-over-Air
is refactored.

The building was refactored to work off the callers
buffer rather than internal stack buffers. An argument
'new_snonce' was included as FT-over-DS will generate
a new snonce for the initial action frame, hence the
handshakes snonce cannot be used.
---
 src/ft.c | 62 +++++++++++++++++++++++++++++++-------------------------
 src/ft.h |  4 ++++
 2 files changed, 38 insertions(+), 28 deletions(-)

diff --git a/src/ft.c b/src/ft.c
index 2c58787d..08e6cdeb 100644
--- a/src/ft.c
+++ b/src/ft.c
@@ -679,19 +679,16 @@ static void ft_sm_free(struct auth_proto *ap)
 	l_free(ft);
 }
 
-static bool ft_start(struct auth_proto *ap)
+bool ft_build_authenticate_ies(struct handshake_state *hs,
+				const uint8_t *new_snonce, uint8_t *buf,
+				size_t *len)
 {
-	struct ft_sm *ft = l_container_of(ap, struct ft_sm, ap);
-	struct handshake_state *hs = ft->hs;
 	uint32_t kck_len = handshake_state_get_kck_len(hs);
 	bool is_rsn = hs->supplicant_ie != NULL;
-	uint8_t mde[5];
-	struct iovec iov[3];
-	size_t iov_elems = 0;
+	uint8_t *ptr = buf;
 
 	if (is_rsn) {
 		struct ie_rsn_info rsn_info;
-		uint8_t *rsne;
 
 		/*
 		 * Rebuild the RSNE to include the PMKR0Name and append
@@ -712,26 +709,18 @@ static bool ft_start(struct auth_proto *ap)
 		rsn_info.num_pmkids = 1;
 		rsn_info.pmkids = hs->pmk_r0_name;
 
-		rsne = alloca(256);
-		ie_build_rsne(&rsn_info, rsne);
-
-		iov[iov_elems].iov_base = rsne;
-		iov[iov_elems].iov_len = rsne[1] + 2;
-		iov_elems += 1;
+		ie_build_rsne(&rsn_info, ptr);
+		ptr += ptr[1] + 2;
 	}
 
 	/* The MDE advertised by the BSS must be passed verbatim */
-	mde[0] = IE_TYPE_MOBILITY_DOMAIN;
-	mde[1] = 3;
-	memcpy(mde + 2, hs->mde + 2, 3);
-
-	iov[iov_elems].iov_base = mde;
-	iov[iov_elems].iov_len = 5;
-	iov_elems += 1;
+	ptr[0] = IE_TYPE_MOBILITY_DOMAIN;
+	ptr[1] = 3;
+	memcpy(ptr + 2, hs->mde + 2, 3);
+	ptr += 5;
 
 	if (is_rsn) {
 		struct ie_ft_info ft_info;
-		uint8_t *fte;
 
 		/*
 		 * 12.8.2: "If present, the FTE shall be set as follows:
@@ -748,17 +737,34 @@ static bool ft_start(struct auth_proto *ap)
 		memcpy(ft_info.r0khid, hs->r0khid, hs->r0khid_len);
 		ft_info.r0khid_len = hs->r0khid_len;
 
-		memcpy(ft_info.snonce, hs->snonce, 32);
+		memcpy(ft_info.snonce, new_snonce, 32);
 
-		fte = alloca(256);
-		ie_build_fast_bss_transition(&ft_info, kck_len, fte);
+		ie_build_fast_bss_transition(&ft_info, kck_len, ptr);
 
-		iov[iov_elems].iov_base = fte;
-		iov[iov_elems].iov_len = fte[1] + 2;
-		iov_elems += 1;
+		ptr += ptr[1] + 2;
 	}
 
-	ft->tx_auth(iov, iov_elems, ft->user_data);
+	if (len)
+		*len = ptr - buf;
+
+	return true;
+}
+
+static bool ft_start(struct auth_proto *ap)
+{
+	struct ft_sm *ft = l_container_of(ap, struct ft_sm, ap);
+	struct handshake_state *hs = ft->hs;
+	struct iovec iov;
+	uint8_t buf[512];
+	size_t len;
+
+	if (!ft_build_authenticate_ies(hs, hs->snonce, buf, &len))
+		return false;
+
+	iov.iov_base = buf;
+	iov.iov_len = len;
+
+	ft->tx_auth(&iov, 1, ft->user_data);
 
 	return true;
 }
diff --git a/src/ft.h b/src/ft.h
index 6f6a7fd5..f24b3b5e 100644
--- a/src/ft.h
+++ b/src/ft.h
@@ -25,6 +25,10 @@ typedef void (*ft_tx_authenticate_func_t)(struct iovec *iov, size_t iov_len,
 typedef void (*ft_tx_associate_func_t)(struct iovec *ie_iov, size_t iov_len,
 					void *user_data);
 
+bool ft_build_authenticate_ies(struct handshake_state *hs,
+				const uint8_t *new_snonce, uint8_t *buf,
+				size_t *len);
+
 struct auth_proto *ft_over_air_sm_new(struct handshake_state *hs,
 				ft_tx_authenticate_func_t tx_auth,
 				ft_tx_associate_func_t tx_assoc,
-- 
2.26.2

Re: [PATCH 2/8] ft: expose ft_build_authenticate_ies

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1385 bytes --]

Hi James,

On 4/15/21 5:45 PM, James Prestwood wrote:
> The building of the (FT)Authenticate frame will need

FT IEs for Action or Authenticate frame, right?

> to be shared between ft and netdev once FT-over-Air
> is refactored.
> 
> The building was refactored to work off the callers
> buffer rather than internal stack buffers. An argument
> 'new_snonce' was included as FT-over-DS will generate
> a new snonce for the initial action frame, hence the
> handshakes snonce cannot be used.
> ---
>   src/ft.c | 62 +++++++++++++++++++++++++++++++-------------------------
>   src/ft.h |  4 ++++
>   2 files changed, 38 insertions(+), 28 deletions(-)
> 

<snip>

> diff --git a/src/ft.h b/src/ft.h
> index 6f6a7fd5..f24b3b5e 100644
> --- a/src/ft.h
> +++ b/src/ft.h
> @@ -25,6 +25,10 @@ typedef void (*ft_tx_authenticate_func_t)(struct iovec *iov, size_t iov_len,
>   typedef void (*ft_tx_associate_func_t)(struct iovec *ie_iov, size_t iov_len,
>   					void *user_data);
>   
> +bool ft_build_authenticate_ies(struct handshake_state *hs,

should this be declared const struct handshake_state ?

> +				const uint8_t *new_snonce, uint8_t *buf,
> +				size_t *len);
> +
>   struct auth_proto *ft_over_air_sm_new(struct handshake_state *hs,
>   				ft_tx_authenticate_func_t tx_auth,
>   				ft_tx_associate_func_t tx_assoc,
> 

Regards,
-Denis

[PATCH 3/8] ft: add ft_over_ds_handshake_new

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 3300 bytes --]

FT-over-DS will be refactored to better handle the initial
action frame stage which can be done prior to needing to
roam. To do this a new handshake object needs to be created
as we cannot change the existing handshake since its needed
for rekeys. Much of the existing handshake is kept as it
contains all the keys negotiated during the initial association.
When IWD is ready, this new handshake can be used to reassociate
to a new AP.
---
 src/ft.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 src/ft.h |  7 +++++++
 2 files changed, 49 insertions(+)

diff --git a/src/ft.c b/src/ft.c
index 08e6cdeb..9e6935e8 100644
--- a/src/ft.c
+++ b/src/ft.c
@@ -32,6 +32,9 @@
 #include "src/ft.h"
 #include "src/mpdu.h"
 #include "src/auth-proto.h"
+#include "src/scan.h"
+#include "src/netdev.h"
+#include "src/util.h"
 
 struct ft_sm {
 	struct auth_proto ap;
@@ -769,6 +772,45 @@ static bool ft_start(struct auth_proto *ap)
 	return true;
 }
 
+struct handshake_state *ft_over_ds_handshake_new(struct netdev *netdev,
+						struct scan_bss *target)
+{
+	uint8_t mde[5];
+	struct handshake_state *old = netdev_get_handshake(netdev);
+	struct handshake_state *hs = netdev_handshake_state_new(netdev);
+
+	/* copy info that will remain the same between handshakes */
+	handshake_state_set_supplicant_address(hs, old->spa);
+	handshake_state_set_supplicant_ie(hs, old->supplicant_ie);
+	handshake_state_set_ssid(hs, old->ssid, old->ssid_len);
+
+	if (old->settings_8021x)
+		handshake_state_set_8021x_config(hs, old->settings_8021x);
+
+	handshake_state_set_kh_ids(hs, old->r0khid, old->r0khid_len, old->r1khid);
+	memcpy(hs->pmk_r0, old->pmk_r0, sizeof(hs->pmk_r0));
+	memcpy(hs->pmk_r1, old->pmk_r1, sizeof(hs->pmk_r1));
+	memcpy(hs->pmk_r0_name, old->pmk_r0_name, sizeof(hs->pmk_r0_name));
+	memcpy(hs->pmk_r1_name, old->pmk_r1_name, sizeof(hs->pmk_r1_name));
+
+	handshake_state_set_pmk(hs, old->pmk, old->pmk_len);
+
+	mde[0] = IE_TYPE_MOBILITY_DOMAIN;
+	mde[1] = 3;
+	memcpy(mde + 2, target->mde, 3);
+
+	handshake_state_set_mde(hs, mde);
+
+	/* prepare new handshake for target BSS */
+	handshake_state_new_snonce(hs);
+	handshake_state_set_authenticator_address(hs, target->addr);
+
+	if (target->rsne)
+		handshake_state_set_authenticator_ie(hs, target->rsne);
+
+	return hs;
+}
+
 static struct auth_proto *ft_sm_new(struct handshake_state *hs,
 				ft_tx_authenticate_func_t tx_auth,
 				ft_tx_associate_func_t tx_assoc,
diff --git a/src/ft.h b/src/ft.h
index f24b3b5e..50090a03 100644
--- a/src/ft.h
+++ b/src/ft.h
@@ -20,6 +20,10 @@
  *
  */
 
+struct scan_bss;
+struct netdev;
+struct handshake_state;
+
 typedef void (*ft_tx_authenticate_func_t)(struct iovec *iov, size_t iov_len,
 					void *user_data);
 typedef void (*ft_tx_associate_func_t)(struct iovec *ie_iov, size_t iov_len,
@@ -29,6 +33,9 @@ bool ft_build_authenticate_ies(struct handshake_state *hs,
 				const uint8_t *new_snonce, uint8_t *buf,
 				size_t *len);
 
+struct handshake_state *ft_over_ds_handshake_new(struct netdev *netdev,
+						struct scan_bss *target);
+
 struct auth_proto *ft_over_air_sm_new(struct handshake_state *hs,
 				ft_tx_authenticate_func_t tx_auth,
 				ft_tx_associate_func_t tx_assoc,
-- 
2.26.2

[PATCH 4/8] ft: add ft_parse_action_response

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 1773 bytes --]

Parses the APs response to the initial FT-over-DS action
frame. This sets up the handshake passed in to get it
ready for re-association.
---
 src/ft.c | 34 ++++++++++++++++++++++++++++++++++
 src/ft.h |  4 ++++
 2 files changed, 38 insertions(+)

diff --git a/src/ft.c b/src/ft.c
index 9e6935e8..7826653f 100644
--- a/src/ft.c
+++ b/src/ft.c
@@ -478,6 +478,40 @@ ft_error:
 	return -EBADMSG;
 }
 
+int ft_parse_action_response(struct handshake_state *hs,
+				struct handshake_state *old,
+				const uint8_t *frame, size_t frame_len)
+{
+	uint16_t status = 0;
+
+	if (frame_len < 16)
+		goto parse_error;
+
+	/* Category FT */
+	if (frame[0] != 6)
+		goto parse_error;
+
+	/* FT Action */
+	if (frame[1] != 2)
+		goto parse_error;
+
+	if (memcmp(frame + 2, hs->spa, 6))
+		goto parse_error;
+
+	if (memcmp(frame + 8, hs->aa, 6))
+		goto parse_error;
+
+	status = l_get_le16(frame + 14);
+
+	if (status != 0)
+		return (int)status;
+
+	return ft_process_ies(hs, frame + 16, frame_len - 16);
+
+parse_error:
+	return -EBADMSG;
+}
+
 static int ft_rx_action(struct auth_proto *ap, const uint8_t *frame,
 				size_t frame_len)
 {
diff --git a/src/ft.h b/src/ft.h
index 50090a03..a22196b6 100644
--- a/src/ft.h
+++ b/src/ft.h
@@ -36,6 +36,10 @@ bool ft_build_authenticate_ies(struct handshake_state *hs,
 struct handshake_state *ft_over_ds_handshake_new(struct netdev *netdev,
 						struct scan_bss *target);
 
+int ft_parse_action_response(struct handshake_state *hs,
+				struct handshake_state *old,
+				const uint8_t *frame, size_t frame_len);
+
 struct auth_proto *ft_over_air_sm_new(struct handshake_state *hs,
 				ft_tx_authenticate_func_t tx_auth,
 				ft_tx_associate_func_t tx_assoc,
-- 
2.26.2

[PATCH 5/8] netdev: factor out FT handshake preparation

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 4450 bytes --]

This isolates the handshake/nhs preparation for
FT into its own function to be used by both
FT-over-Air and FT-over-DS after refactoring.
---
 src/netdev.c | 99 +++++++++++++++++++++++++++-------------------------
 1 file changed, 52 insertions(+), 47 deletions(-)

diff --git a/src/netdev.c b/src/netdev.c
index d8703ed2..4c28fb25 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -3529,6 +3529,56 @@ static void netdev_ft_tx_associate(struct iovec *ie_iov, size_t iov_len,
 	}
 }
 
+static void prepare_ft(struct netdev *netdev, struct scan_bss *target_bss)
+{
+	struct netdev_handshake_state *nhs;
+
+	memcpy(netdev->prev_bssid, netdev->handshake->aa, 6);
+	netdev->prev_frequency = netdev->frequency;
+	netdev->frequency = target_bss->frequency;
+
+	netdev->associated = false;
+	netdev->operational = false;
+	netdev->in_ft = true;
+
+	/*
+	 * Cancel commands that could be running because of EAPoL activity
+	 * like re-keying, this way the callbacks for those commands don't
+	 * have to check if failures resulted from the transition.
+	 */
+	nhs = l_container_of(netdev->handshake,
+				struct netdev_handshake_state, super);
+
+	/* reset key states just as we do in initialization */
+	nhs->complete = false;
+	nhs->ptk_installed = false;
+	nhs->gtk_installed = true;
+	nhs->igtk_installed = true;
+
+	if (nhs->group_new_key_cmd_id) {
+		l_genl_family_cancel(nl80211, nhs->group_new_key_cmd_id);
+		nhs->group_new_key_cmd_id = 0;
+	}
+
+	if (nhs->group_management_new_key_cmd_id) {
+		l_genl_family_cancel(nl80211,
+			nhs->group_management_new_key_cmd_id);
+		nhs->group_management_new_key_cmd_id = 0;
+	}
+
+	if (netdev->rekey_offload_cmd_id) {
+		l_genl_family_cancel(nl80211, netdev->rekey_offload_cmd_id);
+		netdev->rekey_offload_cmd_id = 0;
+	}
+
+	netdev_rssi_polling_update(netdev);
+
+	if (netdev->sm) {
+		eapol_sm_free(netdev->sm);
+		netdev->sm = NULL;
+	}
+}
+
 static void netdev_ft_request_cb(struct l_genl_msg *msg, void *user_data)
 {
 	struct netdev *netdev = user_data;
@@ -3636,8 +3686,6 @@ static int fast_transition(struct netdev *netdev, struct scan_bss *target_bss,
 				bool over_air,
 				netdev_connect_cb_t cb)
 {
-	struct netdev_handshake_state *nhs;
-
 	if (!netdev->operational)
 		return -ENOTCONN;
 
@@ -3651,14 +3699,9 @@ static int fast_transition(struct netdev *netdev, struct scan_bss *target_bss,
 	 * Could also create a new object and copy most of the state but
 	 * we would end up doing more work.
 	 */
-	memcpy(netdev->prev_bssid, netdev->handshake->aa, ETH_ALEN);
 	memcpy(netdev->prev_snonce, netdev->handshake->snonce, 32);
-
 	handshake_state_new_snonce(netdev->handshake);
 
-	netdev->prev_frequency = netdev->frequency;
-	netdev->frequency = target_bss->frequency;
-
 	handshake_state_set_authenticator_address(netdev->handshake,
 							target_bss->addr);
 
@@ -3667,47 +3710,9 @@ static int fast_transition(struct netdev *netdev, struct scan_bss *target_bss,
 							target_bss->rsne);
 	memcpy(netdev->handshake->mde + 2, target_bss->mde, 3);
 
-	netdev->associated = false;
-	netdev->operational = false;
-	netdev->in_ft = true;
-	netdev->connect_cb = cb;
-
-	/*
-	 * Cancel commands that could be running because of EAPoL activity
-	 * like re-keying, this way the callbacks for those commands don't
-	 * have to check if failures resulted from the transition.
-	 */
-	nhs = l_container_of(netdev->handshake,
-				struct netdev_handshake_state, super);
-
-	/* reset key states just as we do in initialization */
-	nhs->complete = false;
-	nhs->ptk_installed = false;
-	nhs->gtk_installed = true;
-	nhs->igtk_installed = true;
+	prepare_ft(netdev, target_bss);
 
-	if (nhs->group_new_key_cmd_id) {
-		l_genl_family_cancel(nl80211, nhs->group_new_key_cmd_id);
-		nhs->group_new_key_cmd_id = 0;
-	}
-
-	if (nhs->group_management_new_key_cmd_id) {
-		l_genl_family_cancel(nl80211,
-			nhs->group_management_new_key_cmd_id);
-		nhs->group_management_new_key_cmd_id = 0;
-	}
-
-	if (netdev->rekey_offload_cmd_id) {
-		l_genl_family_cancel(nl80211, netdev->rekey_offload_cmd_id);
-		netdev->rekey_offload_cmd_id = 0;
-	}
-
-	netdev_rssi_polling_update(netdev);
-
-	if (netdev->sm) {
-		eapol_sm_free(netdev->sm);
-		netdev->sm = NULL;
-	}
+	netdev->connect_cb = cb;
 
 	if (over_air)
 		netdev->ap = ft_over_air_sm_new(netdev->handshake,
-- 
2.26.2

Re: [PATCH 5/8] netdev: factor out FT handshake preparation

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 386 bytes --]

Hi James,

On 4/15/21 5:45 PM, James Prestwood wrote:
> This isolates the handshake/nhs preparation for
> FT into its own function to be used by both
> FT-over-Air and FT-over-DS after refactoring.
> ---
>   src/netdev.c | 99 +++++++++++++++++++++++++++-------------------------
>   1 file changed, 52 insertions(+), 47 deletions(-)
> 

Applied, thanks.

Regards,
-Denis

[PATCH 6/8] ft: netdev: refactor FT-over-DS to properly handle Auth failures

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 16469 bytes --]

FT-over-DS should be handled differently than FT-over-Air, at least in
terms of the initial action frame over-DS sends. This action frame can
actually be sent to multiple APs, even before IWD needs to roam. An
error condition while sending the frame or getting a response should
not be treated as fatal which it is currently.

To fix this FT-over-DS needed to be refactored and the initial action
frame was moved outside of the auth-proto mechanism and into netdev
itself. Since this is not a formal Authentication frame it did not fit
right with how auth-protos worked. In addition there was no feasible
way to separate failure in this path while still keeping the existing
connection alive (apart from storing loads of handshake data and
restoring it on failure).

To maintain the current connection while still starting the over-DS
process a new handshake object needs to be created
(ft_over_ds_handshake_new). This new handshake is held in netdev and
if there is any failure prior to Reassociation it can simply be freed
and thrown out. If FT-over-DS successfully authenticates then
the existing netdev handshake can be swapped for the new one, and
Reassociation can begin. For this an auth-proto is created for
FT-over-DS (ft_over_ds_sm_new). Once Reassociation starts the code path
behaves identically to FT-over-Air.

Some minor changes were also included like using frame-xchg rather
than sending the frame from netdev. This gets us free timeout handling
which reduced the complexity in netdev.

With this change, future improvements can be made much easier such
as starting FT-over-DS to multiple roam candidates immediate after
connecting. Then, once a roam is needed, IWD has already pre-authenticated
with these APs and can begin association immediately.
---
 src/ft.c     |  95 ++++++-------------------
 src/ft.h     |   1 -
 src/netdev.c | 191 ++++++++++++++++++++++++++++++++++++++++-----------
 3 files changed, 170 insertions(+), 117 deletions(-)

diff --git a/src/ft.c b/src/ft.c
index 7826653f..d1aaada7 100644
--- a/src/ft.c
+++ b/src/ft.c
@@ -159,40 +159,6 @@ static bool ft_parse_authentication_resp_frame(const uint8_t *data, size_t len,
 	return true;
 }
 
-static bool ft_parse_action_resp_frame(const uint8_t *frame, size_t frame_len,
-					const uint8_t *spa, const uint8_t *aa,
-					uint16_t *out_status,
-					const uint8_t **out_ies,
-					size_t *out_ies_len)
-{
-	uint16_t status = 0;
-
-	/* Category FT */
-	if (frame[0] != 6)
-		return false;
-
-	/* FT Action */
-	if (frame[1] != 2)
-		return false;
-
-	if (memcmp(frame + 2, spa, 6))
-		return false;
-	if (memcmp(frame + 8, aa, 6))
-		return false;
-
-	status = l_get_le16(frame + 14);
-
-	if (out_status)
-		*out_status = status;
-
-	if (status == 0 && out_ies) {
-		*out_ies = frame + 16;
-		*out_ies_len = frame_len - 16;
-	}
-
-	return true;
-}
-
 static bool ft_parse_associate_resp_frame(const uint8_t *frame, size_t frame_len,
 				uint16_t *out_status, const uint8_t **rsne,
 				const uint8_t **mde, const uint8_t **fte)
@@ -512,34 +478,6 @@ parse_error:
 	return -EBADMSG;
 }
 
-static int ft_rx_action(struct auth_proto *ap, const uint8_t *frame,
-				size_t frame_len)
-{
-	struct ft_sm *ft = l_container_of(ap, struct ft_sm, ap);
-	uint16_t status_code = MMPDU_STATUS_CODE_UNSPECIFIED;
-	const uint8_t *ies = NULL;
-	size_t ies_len;
-	int ret;
-
-	if (!ft_parse_action_resp_frame(frame, frame_len, ft->hs->spa,
-						ft->hs->aa, &status_code,
-						&ies, &ies_len))
-		return -EBADMSG;
-
-	/* AP Rejected the authenticate / associate */
-	if (status_code != 0)
-		goto auth_error;
-
-	ret = ft_process_ies(ft->hs, ies, ies_len);
-	if (ret < 0)
-		goto auth_error;
-
-	return ft_tx_reassociate(ft);
-
-auth_error:
-	return (int)status_code;
-}
-
 static int ft_rx_authenticate(struct auth_proto *ap, const uint8_t *frame,
 				size_t frame_len)
 {
@@ -716,6 +654,13 @@ static void ft_sm_free(struct auth_proto *ap)
 	l_free(ft);
 }
 
+static bool ft_over_ds_start(struct auth_proto *ap)
+{
+	struct ft_sm *ft = l_container_of(ap, struct ft_sm, ap);
+
+	return ft_tx_reassociate(ft) == 0;
+}
+
 bool ft_build_authenticate_ies(struct handshake_state *hs,
 				const uint8_t *new_snonce, uint8_t *buf,
 				size_t *len)
@@ -845,10 +790,9 @@ struct handshake_state *ft_over_ds_handshake_new(struct netdev *netdev,
 	return hs;
 }
 
-static struct auth_proto *ft_sm_new(struct handshake_state *hs,
+struct auth_proto *ft_over_air_sm_new(struct handshake_state *hs,
 				ft_tx_authenticate_func_t tx_auth,
 				ft_tx_associate_func_t tx_assoc,
-				bool over_air,
 				void *user_data)
 {
 	struct ft_sm *ft = l_new(struct ft_sm, 1);
@@ -858,7 +802,7 @@ static struct auth_proto *ft_sm_new(struct handshake_state *hs,
 	ft->hs = hs;
 	ft->user_data = user_data;
 
-	ft->ap.rx_authenticate = (over_air) ? ft_rx_authenticate : ft_rx_action;
+	ft->ap.rx_authenticate = ft_rx_authenticate;
 	ft->ap.rx_associate = ft_rx_associate;
 	ft->ap.start = ft_start;
 	ft->ap.free = ft_sm_free;
@@ -866,18 +810,19 @@ static struct auth_proto *ft_sm_new(struct handshake_state *hs,
 	return &ft->ap;
 }
 
-struct auth_proto *ft_over_air_sm_new(struct handshake_state *hs,
-				ft_tx_authenticate_func_t tx_auth,
-				ft_tx_associate_func_t tx_assoc,
-				void *user_data)
-{
-	return ft_sm_new(hs, tx_auth, tx_assoc, true, user_data);
-}
-
 struct auth_proto *ft_over_ds_sm_new(struct handshake_state *hs,
-				ft_tx_authenticate_func_t tx_auth,
 				ft_tx_associate_func_t tx_assoc,
 				void *user_data)
 {
-	return ft_sm_new(hs, tx_auth, tx_assoc, false, user_data);
+	struct ft_sm *ft = l_new(struct ft_sm, 1);
+
+	ft->tx_assoc = tx_assoc;
+	ft->hs = hs;
+	ft->user_data = user_data;
+
+	ft->ap.rx_associate = ft_rx_associate;
+	ft->ap.start = ft_over_ds_start;
+	ft->ap.free = ft_sm_free;
+
+	return &ft->ap;
 }
diff --git a/src/ft.h b/src/ft.h
index a22196b6..2dd806f2 100644
--- a/src/ft.h
+++ b/src/ft.h
@@ -46,6 +46,5 @@ struct auth_proto *ft_over_air_sm_new(struct handshake_state *hs,
 				void *user_data);
 
 struct auth_proto *ft_over_ds_sm_new(struct handshake_state *hs,
-				ft_tx_authenticate_func_t tx_auth,
 				ft_tx_associate_func_t tx_assoc,
 				void *user_data);
diff --git a/src/netdev.c b/src/netdev.c
index 4c28fb25..95fa6909 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -119,6 +119,7 @@ struct netdev {
 	uint32_t rekey_offload_cmd_id;
 	uint32_t qos_map_cmd_id;
 	uint32_t mac_change_cmd_id;
+	uint32_t ft_action_cmd_id;
 	enum netdev_result result;
 	uint16_t last_code; /* reason or status, depending on result */
 	struct l_timeout *neighbor_report_timeout;
@@ -136,6 +137,14 @@ struct netdev {
 	uint32_t rssi_poll_cmd_id;
 	uint8_t set_mac_once[6];
 
+	/*
+	 * TODO: It is possible to pre-authenticate with multiple FT-over-DS
+	 * targets so this could be changed to an array of BSS/handshake pairs,
+	 * but for now it is being limited to a single FT target
+	 */
+	struct scan_bss *ft_over_ds_target;
+	struct handshake_state *ft_over_ds_handshake;
+
 	struct scan_bss *fw_roam_bss;
 
 	uint32_t set_powered_cmd_id;
@@ -675,6 +684,11 @@ static void netdev_connect_free(struct netdev *netdev)
 		netdev->handshake = NULL;
 	}
 
+	if (netdev->ft_over_ds_handshake) {
+		handshake_state_free(netdev->ft_over_ds_handshake);
+		netdev->ft_over_ds_handshake = NULL;
+	}
+
 	if (netdev->neighbor_report_cb) {
 		netdev->neighbor_report_cb(netdev, -ENOTCONN, NULL, 0,
 						netdev->user_data);
@@ -810,6 +824,11 @@ static void netdev_free(void *data)
 		netdev->get_station_cmd_id = 0;
 	}
 
+	if (netdev->ft_action_cmd_id) {
+		frame_xchg_cancel(netdev->ft_action_cmd_id);
+		netdev->ft_action_cmd_id = 0;
+	}
+
 	if (netdev->fw_roam_bss)
 		scan_bss_free(netdev->fw_roam_bss);
 
@@ -3579,16 +3598,19 @@ static void prepare_ft(struct netdev *netdev, struct scan_bss *target_bss)
 	}
 }
 
-static void netdev_ft_request_cb(struct l_genl_msg *msg, void *user_data)
+static void netdev_ft_over_ds_auth_failed(struct netdev *netdev,
+						uint16_t status)
 {
-	struct netdev *netdev = user_data;
-
-	if (l_genl_msg_get_error(msg) < 0) {
-		l_error("Could not send CMD_FRAME");
-		netdev_connect_failed(netdev,
-					NETDEV_RESULT_AUTHENTICATION_FAILED,
-					MMPDU_STATUS_CODE_UNSPECIFIED);
+	if (netdev->ft_over_ds_handshake) {
+		netdev_handshake_state_free(netdev->ft_over_ds_handshake);
+		netdev->ft_over_ds_handshake = NULL;
 	}
+
+	netdev->ft_over_ds_target = NULL;
+
+	if (netdev->connect_cb)
+		netdev->connect_cb(netdev, NETDEV_RESULT_AUTHENTICATION_FAILED,
+					&status, netdev->user_data);
 }
 
 static void netdev_ft_response_frame_event(const struct mmpdu_header *hdr,
@@ -3598,11 +3620,11 @@ static void netdev_ft_response_frame_event(const struct mmpdu_header *hdr,
 	int ret;
 	uint16_t status_code = MMPDU_STATUS_CODE_UNSPECIFIED;
 	struct netdev *netdev = user_data;
+	struct handshake_state *netdev_hs = netdev->handshake;
 
-	if (!netdev->ap || !netdev->in_ft)
-		return;
-
-	ret = auth_proto_rx_authenticate(netdev->ap, body, body_len);
+	ret = ft_parse_action_response(netdev->ft_over_ds_handshake,
+					netdev->handshake,
+					body, body_len);
 	if (ret < 0)
 		goto ft_error;
 	else if (ret > 0) {
@@ -3610,12 +3632,36 @@ static void netdev_ft_response_frame_event(const struct mmpdu_header *hdr,
 		goto ft_error;
 	}
 
+	/*
+	 * At this point the target handshake could be saved off for use later
+	 * if authenticating to multiple APs. As it stands now we actually want
+	 * to FT so begin this process immediately.
+	 */
+	prepare_ft(netdev, netdev->ft_over_ds_target);
+
+	netdev->handshake = netdev->ft_over_ds_handshake;
+
+	netdev->ap = ft_over_ds_sm_new(netdev->ft_over_ds_handshake,
+					netdev_ft_tx_associate, netdev);
+
+	if (!auth_proto_start(netdev->ap))
+		goto ft_error;
+
+	/* Past the point of no return, free the old handshake */
+	netdev_handshake_state_free(netdev_hs);
+
 	return;
 
 ft_error:
-	netdev_connect_failed(netdev, NETDEV_RESULT_AUTHENTICATION_FAILED,
-				status_code);
-	return;
+	l_debug("FT-over-DS to "MAC" failed",
+				MAC_STR(netdev->ft_over_ds_target->addr));
+	/*
+	 * A failure here doesn't need to fully disconnect, hence why the
+	 * old netdev->handshake was saved
+	 */
+	netdev->handshake = netdev_hs;
+
+	netdev_ft_over_ds_auth_failed(netdev, status_code);
 }
 
 static void netdev_qos_map_frame_event(const struct mmpdu_header *hdr,
@@ -3640,26 +3686,74 @@ static void netdev_qos_map_frame_event(const struct mmpdu_header *hdr,
 	netdev_send_qos_map_set(netdev, body + 4, body_len - 4);
 }
 
-static void netdev_ft_over_ds_tx_authenticate(struct iovec *iov,
-					size_t iov_len, void *user_data)
+static void netdev_ft_over_ds_timeout(int error, void *user_data)
 {
 	struct netdev *netdev = user_data;
-	uint8_t ft_req[14];
+
+	if (error == 0)
+		return;
+
+	l_debug("Sending FT-over-DS action frame failed with %d", error);
+
+	netdev_ft_over_ds_auth_failed(netdev, MMPDU_STATUS_CODE_UNSPECIFIED);
+}
+
+static const struct frame_xchg_prefix netdev_ft_action_prefix = {
+	.data = (uint8_t []) {
+		0x06, 0x02,
+	},
+	.len = 2,
+};
+
+static bool netdev_ft_over_ds_ready(struct wiphy_radio_work_item *item)
+{
+	struct netdev *netdev = l_container_of(item, struct netdev, work);
+	uint8_t ft_req[40];
+	uint8_t *ptr = ft_req;
 	struct handshake_state *hs = netdev->handshake;
-	struct iovec iovs[iov_len + 1];
+	struct iovec iovs[5];
+	uint8_t buf[512];
+	size_t len;
 
-	ft_req[0] = 6; /* FT category */
-	ft_req[1] = 1; /* FT Request action */
-	memcpy(ft_req + 2, netdev->addr, 6);
-	memcpy(ft_req + 8, hs->aa, 6);
+	memset(ft_req, 0, sizeof(ft_req));
+
+	l_put_le16(0x00d0, ptr + 0);
+	memcpy(ptr + 4, hs->aa, 6);
+	memcpy(ptr + 10, netdev->addr, 6);
+	memcpy(ptr + 16, hs->aa, 6);
+
+	ptr = ft_req + 24;
+
+	ptr[0] = 6; /* FT category */
+	ptr[1] = 1; /* FT Request action */
+	memcpy(ptr + 2, netdev->addr, 6);
+	memcpy(ptr + 8, netdev->ft_over_ds_target->addr, 6);
 
 	iovs[0].iov_base = ft_req;
 	iovs[0].iov_len = sizeof(ft_req);
-	memcpy(iovs + 1, iov, sizeof(*iov) * iov_len);
 
-	netdev_send_action_framev(netdev, netdev->prev_bssid, iovs, iov_len + 1,
-					netdev->prev_frequency,
-					netdev_ft_request_cb);
+	if (!ft_build_authenticate_ies(hs, netdev->ft_over_ds_handshake->snonce,
+			buf, &len))
+		goto failed;
+
+	iovs[1].iov_base = buf;
+	iovs[1].iov_len = len;
+
+	iovs[2].iov_base = NULL;
+
+	netdev->ft_action_cmd_id = frame_xchg_start(netdev->wdev_id, iovs,
+					netdev->frequency, 0, 300, 0, 0,
+					netdev_ft_over_ds_timeout, netdev, NULL,
+					&netdev_ft_action_prefix,
+					netdev_ft_response_frame_event, NULL);
+	if (!netdev->ft_action_cmd_id)
+		goto failed;
+
+	return true;
+
+failed:
+	netdev_ft_over_ds_auth_failed(netdev, MMPDU_STATUS_CODE_UNSPECIFIED);
+	return true;
 }
 
 static bool netdev_ft_work_ready(struct wiphy_radio_work_item *item)
@@ -3682,8 +3776,11 @@ static const struct wiphy_radio_work_item_ops ft_work_ops = {
 	.do_work = netdev_ft_work_ready,
 };
 
+static const struct wiphy_radio_work_item_ops ft_over_ds_work_ops = {
+	.do_work = netdev_ft_over_ds_ready,
+};
+
 static int fast_transition(struct netdev *netdev, struct scan_bss *target_bss,
-				bool over_air,
 				netdev_connect_cb_t cb)
 {
 	if (!netdev->operational)
@@ -3714,14 +3811,9 @@ static int fast_transition(struct netdev *netdev, struct scan_bss *target_bss,
 
 	netdev->connect_cb = cb;
 
-	if (over_air)
-		netdev->ap = ft_over_air_sm_new(netdev->handshake,
+	netdev->ap = ft_over_air_sm_new(netdev->handshake,
 					netdev_ft_tx_authenticate,
 					netdev_ft_tx_associate, netdev);
-	else
-		netdev->ap = ft_over_ds_sm_new(netdev->handshake,
-					netdev_ft_over_ds_tx_authenticate,
-					netdev_ft_tx_associate, netdev);
 
 	wiphy_radio_work_insert(netdev->wiphy, &netdev->work, 1,
 				&ft_work_ops);
@@ -3732,14 +3824,36 @@ static int fast_transition(struct netdev *netdev, struct scan_bss *target_bss,
 int netdev_fast_transition(struct netdev *netdev, struct scan_bss *target_bss,
 				netdev_connect_cb_t cb)
 {
-	return fast_transition(netdev, target_bss, true, cb);
+	return fast_transition(netdev, target_bss, cb);
 }
 
 int netdev_fast_transition_over_ds(struct netdev *netdev,
 					struct scan_bss *target_bss,
 					netdev_connect_cb_t cb)
 {
-	return fast_transition(netdev, target_bss, false, cb);
+	if (!netdev->operational)
+		return -ENOTCONN;
+
+	if (!netdev->handshake->mde || !target_bss->mde_present ||
+			l_get_le16(netdev->handshake->mde + 2) !=
+			l_get_le16(target_bss->mde))
+		return -EINVAL;
+
+	/*
+	 * Create a new handshake to be used for the initial action frame and
+	 * response. Since failure could happen during these initial steps the
+	 * current handshake must be left pristine in order to maintain the
+	 * existing connection.
+	 */
+	netdev->ft_over_ds_handshake = ft_over_ds_handshake_new(netdev,
+								target_bss);
+	netdev->ft_over_ds_target = target_bss;
+	netdev->connect_cb = cb;
+
+	wiphy_radio_work_insert(netdev->wiphy, &netdev->work, 1,
+				&ft_over_ds_work_ops);
+
+	return 0;
 }
 
 static void netdev_preauth_cb(const uint8_t *pmk, void *user_data)
@@ -5340,7 +5454,6 @@ struct netdev *netdev_create_from_genl(struct l_genl_msg *msg,
 	const uint8_t action_neighbor_report_prefix[2] = { 0x05, 0x05 };
 	const uint8_t action_sa_query_resp_prefix[2] = { 0x08, 0x01 };
 	const uint8_t action_sa_query_req_prefix[2] = { 0x08, 0x00 };
-	const uint8_t action_ft_response_prefix[] =  { 0x06, 0x02 };
 	const uint8_t action_qos_map_prefix[] = { 0x01, 0x04 };
 	struct l_io *pae_io = NULL;
 
@@ -5428,10 +5541,6 @@ struct netdev *netdev_create_from_genl(struct l_genl_msg *msg,
 			sizeof(action_sa_query_req_prefix),
 			netdev_sa_query_req_frame_event, netdev, NULL);
 
-	frame_watch_add(wdev, 0, 0x00d0, action_ft_response_prefix,
-			sizeof(action_ft_response_prefix),
-			netdev_ft_response_frame_event, netdev, NULL);
-
 	if (wiphy_supports_qos_set_map(netdev->wiphy))
 		frame_watch_add(wdev, 0, 0x00d0, action_qos_map_prefix,
 				sizeof(action_qos_map_prefix),
-- 
2.26.2

Re: [PATCH 6/8] ft: netdev: refactor FT-over-DS to properly handle Auth failures

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 14932 bytes --]

Hi James,

On 4/15/21 5:45 PM, James Prestwood wrote:
> FT-over-DS should be handled differently than FT-over-Air, at least in
> terms of the initial action frame over-DS sends. This action frame can
> actually be sent to multiple APs, even before IWD needs to roam. An
> error condition while sending the frame or getting a response should
> not be treated as fatal which it is currently.
> 
> To fix this FT-over-DS needed to be refactored and the initial action
> frame was moved outside of the auth-proto mechanism and into netdev
> itself. Since this is not a formal Authentication frame it did not fit
> right with how auth-protos worked. In addition there was no feasible
> way to separate failure in this path while still keeping the existing
> connection alive (apart from storing loads of handshake data and
> restoring it on failure).
> 
> To maintain the current connection while still starting the over-DS
> process a new handshake object needs to be created
> (ft_over_ds_handshake_new). This new handshake is held in netdev and
> if there is any failure prior to Reassociation it can simply be freed
> and thrown out. If FT-over-DS successfully authenticates then
> the existing netdev handshake can be swapped for the new one, and
> Reassociation can begin. For this an auth-proto is created for
> FT-over-DS (ft_over_ds_sm_new). Once Reassociation starts the code path
> behaves identically to FT-over-Air.

This seems to be a bit heavy, particularly if we want to use FT-over-DS to 
pre-authenticate to multiple APs in the future.  All we really care about is 
storing the response IEs from the 'other' APs.  Do we really need a full blown 
handshake object to do that?  Can we just store the response IEs + the nonce 
used + target address (could even just store the action response frame) instead?

Then when we actually trigger the actual transition, we can essentially use the 
same code path as for FT-over-Air, where we take the current handshake and 
override certain parameters.

> 
> Some minor changes were also included like using frame-xchg rather
> than sending the frame from netdev. This gets us free timeout handling
> which reduced the complexity in netdev.

One thing I still hate about frame-xchg is that it creates a radio work object 
for every request.  This seems unnecessary for non-offchannel frames like action 
frames.  There's really no reason why we can't send a bunch of action frames all 
at once and wait for the responses to come in.  frame-xchg would want to 
serialize all of these unnecessarily.

> 
> With this change, future improvements can be made much easier such
> as starting FT-over-DS to multiple roam candidates immediate after
> connecting. Then, once a roam is needed, IWD has already pre-authenticated
> with these APs and can begin association immediately.
> ---
>   src/ft.c     |  95 ++++++-------------------
>   src/ft.h     |   1 -
>   src/netdev.c | 191 ++++++++++++++++++++++++++++++++++++++++-----------
>   3 files changed, 170 insertions(+), 117 deletions(-)
> 

<snip>

> diff --git a/src/netdev.c b/src/netdev.c
> index 4c28fb25..95fa6909 100644
> --- a/src/netdev.c
> +++ b/src/netdev.c
> @@ -119,6 +119,7 @@ struct netdev {
>   	uint32_t rekey_offload_cmd_id;
>   	uint32_t qos_map_cmd_id;
>   	uint32_t mac_change_cmd_id;
> +	uint32_t ft_action_cmd_id;

_cmd_id postfix might be a bit confusing if you're not actually sending a genl 
command but using frame-xchg.

>   	enum netdev_result result;
>   	uint16_t last_code; /* reason or status, depending on result */
>   	struct l_timeout *neighbor_report_timeout;
> @@ -136,6 +137,14 @@ struct netdev {
>   	uint32_t rssi_poll_cmd_id;
>   	uint8_t set_mac_once[6];
>   
> +	/*
> +	 * TODO: It is possible to pre-authenticate with multiple FT-over-DS
> +	 * targets so this could be changed to an array of BSS/handshake pairs,
> +	 * but for now it is being limited to a single FT target
> +	 */
> +	struct scan_bss *ft_over_ds_target;

You have to be very careful with how you manage struct scan_bss ownership.  I 
don't think storing a pointer here is a good idea.  A new scan might invalidate 
this pointer from under you.

> +	struct handshake_state *ft_over_ds_handshake;
> +
>   	struct scan_bss *fw_roam_bss;
>   
>   	uint32_t set_powered_cmd_id;
> @@ -675,6 +684,11 @@ static void netdev_connect_free(struct netdev *netdev)
>   		netdev->handshake = NULL;
>   	}
>   
> +	if (netdev->ft_over_ds_handshake) {
> +		handshake_state_free(netdev->ft_over_ds_handshake);
> +		netdev->ft_over_ds_handshake = NULL;
> +	}
> +
>   	if (netdev->neighbor_report_cb) {
>   		netdev->neighbor_report_cb(netdev, -ENOTCONN, NULL, 0,
>   						netdev->user_data);
> @@ -810,6 +824,11 @@ static void netdev_free(void *data)
>   		netdev->get_station_cmd_id = 0;
>   	}
>   
> +	if (netdev->ft_action_cmd_id) {
> +		frame_xchg_cancel(netdev->ft_action_cmd_id);
> +		netdev->ft_action_cmd_id = 0;
> +	}
> +
>   	if (netdev->fw_roam_bss)
>   		scan_bss_free(netdev->fw_roam_bss);
>   
> @@ -3579,16 +3598,19 @@ static void prepare_ft(struct netdev *netdev, struct scan_bss *target_bss)
>   	}
>   }
>   
> -static void netdev_ft_request_cb(struct l_genl_msg *msg, void *user_data)
> +static void netdev_ft_over_ds_auth_failed(struct netdev *netdev,
> +						uint16_t status)
>   {
> -	struct netdev *netdev = user_data;
> -
> -	if (l_genl_msg_get_error(msg) < 0) {
> -		l_error("Could not send CMD_FRAME");
> -		netdev_connect_failed(netdev,
> -					NETDEV_RESULT_AUTHENTICATION_FAILED,
> -					MMPDU_STATUS_CODE_UNSPECIFIED);
> +	if (netdev->ft_over_ds_handshake) {
> +		netdev_handshake_state_free(netdev->ft_over_ds_handshake);
> +		netdev->ft_over_ds_handshake = NULL;
>   	}
> +
> +	netdev->ft_over_ds_target = NULL;
> +
> +	if (netdev->connect_cb)
> +		netdev->connect_cb(netdev, NETDEV_RESULT_AUTHENTICATION_FAILED,
> +					&status, netdev->user_data);
>   }
>   
>   static void netdev_ft_response_frame_event(const struct mmpdu_header *hdr,
> @@ -3598,11 +3620,11 @@ static void netdev_ft_response_frame_event(const struct mmpdu_header *hdr,
>   	int ret;
>   	uint16_t status_code = MMPDU_STATUS_CODE_UNSPECIFIED;
>   	struct netdev *netdev = user_data;
> +	struct handshake_state *netdev_hs = netdev->handshake;
>   
> -	if (!netdev->ap || !netdev->in_ft)
> -		return;
> -
> -	ret = auth_proto_rx_authenticate(netdev->ap, body, body_len);
> +	ret = ft_parse_action_response(netdev->ft_over_ds_handshake,
> +					netdev->handshake,
> +					body, body_len);
>   	if (ret < 0)
>   		goto ft_error;
>   	else if (ret > 0) {
> @@ -3610,12 +3632,36 @@ static void netdev_ft_response_frame_event(const struct mmpdu_header *hdr,
>   		goto ft_error;
>   	}
>   
> +	/*
> +	 * At this point the target handshake could be saved off for use later
> +	 * if authenticating to multiple APs. As it stands now we actually want
> +	 * to FT so begin this process immediately.
> +	 */
> +	prepare_ft(netdev, netdev->ft_over_ds_target);
> +
> +	netdev->handshake = netdev->ft_over_ds_handshake;
> +
> +	netdev->ap = ft_over_ds_sm_new(netdev->ft_over_ds_handshake,
> +					netdev_ft_tx_associate, netdev);
> +
> +	if (!auth_proto_start(netdev->ap))
> +		goto ft_error;
> +
> +	/* Past the point of no return, free the old handshake */
> +	netdev_handshake_state_free(netdev_hs);
> +
>   	return;
>   
>   ft_error:
> -	netdev_connect_failed(netdev, NETDEV_RESULT_AUTHENTICATION_FAILED,
> -				status_code);
> -	return;
> +	l_debug("FT-over-DS to "MAC" failed",
> +				MAC_STR(netdev->ft_over_ds_target->addr));
> +	/*
> +	 * A failure here doesn't need to fully disconnect, hence why the
> +	 * old netdev->handshake was saved
> +	 */
> +	netdev->handshake = netdev_hs;
> +
> +	netdev_ft_over_ds_auth_failed(netdev, status_code);
>   }
>   
>   static void netdev_qos_map_frame_event(const struct mmpdu_header *hdr,
> @@ -3640,26 +3686,74 @@ static void netdev_qos_map_frame_event(const struct mmpdu_header *hdr,
>   	netdev_send_qos_map_set(netdev, body + 4, body_len - 4);
>   }
>   
> -static void netdev_ft_over_ds_tx_authenticate(struct iovec *iov,
> -					size_t iov_len, void *user_data)
> +static void netdev_ft_over_ds_timeout(int error, void *user_data)
>   {
>   	struct netdev *netdev = user_data;
> -	uint8_t ft_req[14];
> +
> +	if (error == 0)
> +		return;
> +
> +	l_debug("Sending FT-over-DS action frame failed with %d", error);
> +
> +	netdev_ft_over_ds_auth_failed(netdev, MMPDU_STATUS_CODE_UNSPECIFIED);
> +}
> +
> +static const struct frame_xchg_prefix netdev_ft_action_prefix = {
> +	.data = (uint8_t []) {
> +		0x06, 0x02,
> +	},
> +	.len = 2,
> +};
> +
> +static bool netdev_ft_over_ds_ready(struct wiphy_radio_work_item *item)
> +{
> +	struct netdev *netdev = l_container_of(item, struct netdev, work);
> +	uint8_t ft_req[40];
> +	uint8_t *ptr = ft_req;
>   	struct handshake_state *hs = netdev->handshake;
> -	struct iovec iovs[iov_len + 1];
> +	struct iovec iovs[5];
> +	uint8_t buf[512];
> +	size_t len;
>   
> -	ft_req[0] = 6; /* FT category */
> -	ft_req[1] = 1; /* FT Request action */
> -	memcpy(ft_req + 2, netdev->addr, 6);
> -	memcpy(ft_req + 8, hs->aa, 6);
> +	memset(ft_req, 0, sizeof(ft_req));
> +
> +	l_put_le16(0x00d0, ptr + 0);
> +	memcpy(ptr + 4, hs->aa, 6);
> +	memcpy(ptr + 10, netdev->addr, 6);
> +	memcpy(ptr + 16, hs->aa, 6);
> +
> +	ptr = ft_req + 24;
> +
> +	ptr[0] = 6; /* FT category */
> +	ptr[1] = 1; /* FT Request action */
> +	memcpy(ptr + 2, netdev->addr, 6);
> +	memcpy(ptr + 8, netdev->ft_over_ds_target->addr, 6);
>   
>   	iovs[0].iov_base = ft_req;
>   	iovs[0].iov_len = sizeof(ft_req);
> -	memcpy(iovs + 1, iov, sizeof(*iov) * iov_len);
>   
> -	netdev_send_action_framev(netdev, netdev->prev_bssid, iovs, iov_len + 1,
> -					netdev->prev_frequency,
> -					netdev_ft_request_cb);
> +	if (!ft_build_authenticate_ies(hs, netdev->ft_over_ds_handshake->snonce,
> +			buf, &len))
> +		goto failed;
> +
> +	iovs[1].iov_base = buf;
> +	iovs[1].iov_len = len;
> +
> +	iovs[2].iov_base = NULL;
> +
> +	netdev->ft_action_cmd_id = frame_xchg_start(netdev->wdev_id, iovs,
> +					netdev->frequency, 0, 300, 0, 0,
> +					netdev_ft_over_ds_timeout, netdev, NULL,
> +					&netdev_ft_action_prefix,
> +					netdev_ft_response_frame_event, NULL);

Yikes, why do we use a wiphy_work for this only to have frame_xchg also make up 
a wiphy_work?  I guess you're abusing the return value semantics here?  This may 
be a bit too subtle for good readability?

> +	if (!netdev->ft_action_cmd_id)
> +		goto failed;
> +
> +	return true;
> +
> +failed:
> +	netdev_ft_over_ds_auth_failed(netdev, MMPDU_STATUS_CODE_UNSPECIFIED);
> +	return true;
>   }
>   
>   static bool netdev_ft_work_ready(struct wiphy_radio_work_item *item)
> @@ -3682,8 +3776,11 @@ static const struct wiphy_radio_work_item_ops ft_work_ops = {
>   	.do_work = netdev_ft_work_ready,
>   };
>   
> +static const struct wiphy_radio_work_item_ops ft_over_ds_work_ops = {
> +	.do_work = netdev_ft_over_ds_ready,
> +};
> +
>   static int fast_transition(struct netdev *netdev, struct scan_bss *target_bss,
> -				bool over_air,
>   				netdev_connect_cb_t cb)
>   {
>   	if (!netdev->operational)
> @@ -3714,14 +3811,9 @@ static int fast_transition(struct netdev *netdev, struct scan_bss *target_bss,
>   
>   	netdev->connect_cb = cb;
>   
> -	if (over_air)
> -		netdev->ap = ft_over_air_sm_new(netdev->handshake,
> +	netdev->ap = ft_over_air_sm_new(netdev->handshake,
>   					netdev_ft_tx_authenticate,
>   					netdev_ft_tx_associate, netdev);
> -	else
> -		netdev->ap = ft_over_ds_sm_new(netdev->handshake,
> -					netdev_ft_over_ds_tx_authenticate,
> -					netdev_ft_tx_associate, netdev);
>   
>   	wiphy_radio_work_insert(netdev->wiphy, &netdev->work, 1,
>   				&ft_work_ops);
> @@ -3732,14 +3824,36 @@ static int fast_transition(struct netdev *netdev, struct scan_bss *target_bss,
>   int netdev_fast_transition(struct netdev *netdev, struct scan_bss *target_bss,
>   				netdev_connect_cb_t cb)
>   {
> -	return fast_transition(netdev, target_bss, true, cb);
> +	return fast_transition(netdev, target_bss, cb);
>   }
>   
>   int netdev_fast_transition_over_ds(struct netdev *netdev,
>   					struct scan_bss *target_bss,
>   					netdev_connect_cb_t cb)
>   {
> -	return fast_transition(netdev, target_bss, false, cb);
> +	if (!netdev->operational)
> +		return -ENOTCONN;
> +
> +	if (!netdev->handshake->mde || !target_bss->mde_present ||
> +			l_get_le16(netdev->handshake->mde + 2) !=
> +			l_get_le16(target_bss->mde))
> +		return -EINVAL;
> +
> +	/*
> +	 * Create a new handshake to be used for the initial action frame and
> +	 * response. Since failure could happen during these initial steps the
> +	 * current handshake must be left pristine in order to maintain the
> +	 * existing connection.
> +	 */
> +	netdev->ft_over_ds_handshake = ft_over_ds_handshake_new(netdev,
> +								target_bss);
> +	netdev->ft_over_ds_target = target_bss;
> +	netdev->connect_cb = cb;

This overloading of connect_cb is also something I think we shouldn't do.  I 
think we have abused this in the past, but ideally we should just create an 
object that holds the request data, sort of like how netdev_preauthenticate 
works.  If you want to make it a singleton for the initial version, that's fine. 
  But lets put all the data about the ft_over_ds request in there and not try 
and overload netdev members that are used for something else.

> +
> +	wiphy_radio_work_insert(netdev->wiphy, &netdev->work, 1,
> +				&ft_over_ds_work_ops);
> +
> +	return 0;
>   }
>   
>   static void netdev_preauth_cb(const uint8_t *pmk, void *user_data)
> @@ -5340,7 +5454,6 @@ struct netdev *netdev_create_from_genl(struct l_genl_msg *msg,
>   	const uint8_t action_neighbor_report_prefix[2] = { 0x05, 0x05 };
>   	const uint8_t action_sa_query_resp_prefix[2] = { 0x08, 0x01 };
>   	const uint8_t action_sa_query_req_prefix[2] = { 0x08, 0x00 };
> -	const uint8_t action_ft_response_prefix[] =  { 0x06, 0x02 };
>   	const uint8_t action_qos_map_prefix[] = { 0x01, 0x04 };
>   	struct l_io *pae_io = NULL;
>   
> @@ -5428,10 +5541,6 @@ struct netdev *netdev_create_from_genl(struct l_genl_msg *msg,
>   			sizeof(action_sa_query_req_prefix),
>   			netdev_sa_query_req_frame_event, netdev, NULL);
>   
> -	frame_watch_add(wdev, 0, 0x00d0, action_ft_response_prefix,
> -			sizeof(action_ft_response_prefix),
> -			netdev_ft_response_frame_event, netdev, NULL);
> -
>   	if (wiphy_supports_qos_set_map(netdev->wiphy))
>   		frame_watch_add(wdev, 0, 0x00d0, action_qos_map_prefix,
>   				sizeof(action_qos_map_prefix),
> 

Regards,
-Denis

[PATCH 7/8] station: specially handle FT-over-DS failure

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 2859 bytes --]

Depending on the netdev result, FT-over-DS failure may not need to
be fatal. If the result is an Authentication failure this just means
that FT-over-DS did not successfully send or receive the initial
action frames. This means we cannot roam, but it does not mean the
connection needs to be fully terminated. A new flag was added to
signal FT-over-DS, and the failure code treats an authentication
failure specially if this flag is set.
---
 src/station.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/src/station.c b/src/station.c
index 064872c6..80609607 100644
--- a/src/station.c
+++ b/src/station.c
@@ -112,6 +112,7 @@ struct station {
 	bool ap_directed_roaming : 1;
 	bool scanning : 1;
 	bool autoconnect : 1;
+	bool ft_over_ds : 1;
 };
 
 struct anqp_entry {
@@ -1625,10 +1626,17 @@ static void station_roam_failed(struct station *station)
 	l_debug("%u", netdev_get_ifindex(station->netdev));
 
 	/*
-	 * If we attempted a reassociation or a fast transition, and ended up
-	 * here then we are now disconnected.
+	 * If we attempted a reassociation or a fast transition (except DS),
+	 * and ended up here then we are now disconnected. In the case of
+	 * FT over DS we can remain connected to the AP even if the transition
+	 * fails.
 	 */
 	if (station->state == STATION_STATE_ROAMING) {
+		if (station->ft_over_ds) {
+			station_enter_state(station, STATION_STATE_CONNECTED);
+			goto delayed_retry;
+		}
+
 		station_disassociated(station);
 		return;
 	}
@@ -1657,6 +1665,7 @@ delayed_retry:
 	station->preparing_roam = false;
 	station->roam_scan_full = false;
 	station->ap_directed_roaming = false;
+	station->ft_over_ds = false;
 
 	if (station->signal_low)
 		station_roam_timeout_rearm(station, roam_retry_interval);
@@ -1708,6 +1717,16 @@ static void station_fast_transition_cb(struct netdev *netdev,
 	if (station->state != STATION_STATE_ROAMING)
 		return;
 
+	/*
+	 * Auth failures are handled special here for FT-over-DS since this only
+	 * means we cannot roam to that AP. Any other failure cannot be
+	 * salvaged. In this case unset the flag so station_roam_failed can
+	 * properly clean up.
+	 */
+	if (result != NETDEV_RESULT_AUTHENTICATION_FAILED &&
+				station->ft_over_ds)
+		station->ft_over_ds = false;
+
 	if (result == NETDEV_RESULT_OK)
 		station_roamed(station);
 	else
@@ -1839,6 +1858,8 @@ static void station_transition_start(struct station *station,
 		/* FT-over-DS can be better suited for these situations */
 		if ((hs->mde[4] & 1) && (station->ap_directed_roaming ||
 				station->signal_low)) {
+			station->ft_over_ds = true;
+
 			if (netdev_fast_transition_over_ds(station->netdev, bss,
 					station_fast_transition_cb) < 0) {
 				station_roam_failed(station);
-- 
2.26.2

[PATCH 8/8] ft: netdev: add return value to tx_associate

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 2450 bytes --]

Prior to this, an error sending the FT Reassociation was treated
as fatal, which is correct for FT-over-Air but not for FT-over-DS.
If the actual l_genl_family_send call fails for FT-over-DS the
existing connection can be maintained and there is no need to
call netdev_connect_failed.

Adding a return to the tx_associate function works for both FT
types. In the FT-over-Air case this return will ultimately get
sent back up to auth_proto_rx_authenticate in which case will
call netdev_connect_failed. For FT-over-DS tx_associate is
actually called from the 'start' operation which can fail and
still maintain the existing connection.
---
 src/ft.c     | 4 +---
 src/ft.h     | 2 +-
 src/netdev.c | 8 ++++----
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/src/ft.c b/src/ft.c
index d1aaada7..793e3724 100644
--- a/src/ft.c
+++ b/src/ft.c
@@ -290,9 +290,7 @@ static int ft_tx_reassociate(struct ft_sm *ft)
 		iov_elems += 1;
 	}
 
-	ft->tx_assoc(iov, iov_elems, ft->user_data);
-
-	return 0;
+	return ft->tx_assoc(iov, iov_elems, ft->user_data);
 
 error:
 	return -EINVAL;
diff --git a/src/ft.h b/src/ft.h
index 2dd806f2..72237a4e 100644
--- a/src/ft.h
+++ b/src/ft.h
@@ -26,7 +26,7 @@ struct handshake_state;
 
 typedef void (*ft_tx_authenticate_func_t)(struct iovec *iov, size_t iov_len,
 					void *user_data);
-typedef void (*ft_tx_associate_func_t)(struct iovec *ie_iov, size_t iov_len,
+typedef int (*ft_tx_associate_func_t)(struct iovec *ie_iov, size_t iov_len,
 					void *user_data);
 
 bool ft_build_authenticate_ies(struct handshake_state *hs,
diff --git a/src/netdev.c b/src/netdev.c
index 95fa6909..6495451f 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -3524,7 +3524,7 @@ restore_snonce:
 					MMPDU_STATUS_CODE_UNSPECIFIED);
 }
 
-static void netdev_ft_tx_associate(struct iovec *ie_iov, size_t iov_len,
+static int netdev_ft_tx_associate(struct iovec *ie_iov, size_t iov_len,
 					void *user_data)
 {
 	struct netdev *netdev = user_data;
@@ -3542,10 +3542,10 @@ static void netdev_ft_tx_associate(struct iovec *ie_iov, size_t iov_len,
 	if (!netdev->connect_cmd_id) {
 		l_genl_msg_unref(msg);
 
-		netdev_connect_failed(netdev, NETDEV_RESULT_ASSOCIATION_FAILED,
-					MMPDU_STATUS_CODE_UNSPECIFIED);
-		return;
+		return -EIO;
 	}
+
+	return 0;
 }
 
 static void prepare_ft(struct netdev *netdev, struct scan_bss *target_bss)
-- 
2.26.2

Re: [PATCH 1/8] ft: separate ft_sm from ft_process_ies

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 354 bytes --]

Hi James,

On 4/15/21 5:45 PM, James Prestwood wrote:
> To prepare for some refactoring remove the ft_sm dependency
> from ft_process_ies and instead only make it depend on the
> handshake_state object.
> ---
>   src/ft.c | 20 +++++++++++++++-----
>   1 file changed, 15 insertions(+), 5 deletions(-)
> 

Applied, thanks.

Regards,
-Denis