[PATCH 01/16] wiphy: Add wiphy_get_supported_rates

[PATCH 01/16] wiphy: Add wiphy_get_supported_rates

[Andrew Zaborowski]

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

Add code to parse the supported data rates info from the wiphy dumps and
expose it for P2P's use with a getter function.
---
 src/wiphy.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 src/wiphy.h |  2 ++
 2 files changed, 82 insertions(+), 4 deletions(-)

diff --git a/src/wiphy.c b/src/wiphy.c
index 3ec9ef4f..a6d6b6a0 100644
--- a/src/wiphy.c
+++ b/src/wiphy.c
@@ -76,6 +76,7 @@ struct wiphy {
 	struct watchlist state_watches;
 	uint8_t extended_capabilities[EXT_CAP_LEN + 2]; /* max bitmap size + IE header */
 	uint8_t *iftype_extended_capabilities[NUM_NL80211_IFTYPES];
+	uint8_t *supported_rates[NUM_NL80211_BANDS];
 	uint8_t rm_enabled_capabilities[7]; /* 5 size max + header */
 
 	bool support_scheduled_scan:1;
@@ -212,6 +213,9 @@ static void wiphy_free(void *data)
 	for (i = 0; i < NUM_NL80211_IFTYPES; i++)
 		l_free(wiphy->iftype_extended_capabilities[i]);
 
+	for (i = 0; i < NUM_NL80211_BANDS; i++)
+		l_free(wiphy->supported_rates[i]);
+
 	scan_freq_set_free(wiphy->supported_freqs);
 	watchlist_destroy(&wiphy->state_watches);
 	l_free(wiphy->model_str);
@@ -478,6 +482,20 @@ bool wiphy_supports_iftype(struct wiphy *wiphy, uint32_t iftype)
 	return wiphy->supported_iftypes & (1 << (iftype - 1));
 }
 
+const uint8_t *wiphy_get_supported_rates(struct wiphy *wiphy, unsigned int band,
+						unsigned int *out_num)
+{
+	if (band >= L_ARRAY_SIZE(wiphy->supported_rates))
+		return NULL;
+
+	if (out_num)
+		*out_num =
+			(uint8_t *) rawmemchr(wiphy->supported_rates[band], 0) -
+			wiphy->supported_rates[band];
+
+	return wiphy->supported_rates[band];
+}
+
 uint32_t wiphy_state_watch_add(struct wiphy *wiphy,
 				wiphy_state_watch_func_t func,
 				void *user_data, wiphy_destroy_func_t destroy)
@@ -622,20 +640,70 @@ static void parse_supported_frequencies(struct wiphy *wiphy,
 	}
 }
 
+static uint8_t *parse_supported_rates(struct l_genl_attr *attr)
+{
+	uint16_t type;
+	uint16_t len;
+	const void *data;
+	struct l_genl_attr nested;
+	int count = 0;
+	uint8_t *ret;
+
+	if (!l_genl_attr_recurse(attr, &nested))
+		return NULL;
+
+	while (l_genl_attr_next(&nested, NULL, NULL, NULL))
+		count++;
+
+	if (!l_genl_attr_recurse(attr, &nested))
+		return NULL;
+
+	ret = l_malloc(count + 1);
+	ret[count] = 0;
+
+	count = 0;
+
+	while (l_genl_attr_next(&nested, NULL, NULL, NULL)) {
+		struct l_genl_attr nested2;
+
+		if (!l_genl_attr_recurse(&nested, &nested2)) {
+			l_free(ret);
+			return NULL;
+		}
+
+		while (l_genl_attr_next(&nested2, &type, &len, &data)) {
+			if (type != NL80211_BITRATE_ATTR_RATE || len != 4)
+				continue;
+
+			/*
+			 * Convert from the 100kb/s units reported by the
+			 * kernel to the 500kb/s used in 802.11 IEs.
+			 */
+			ret[count++] = *(const uint32_t *) data / 5;
+		}
+	}
+
+	return ret;
+}
+
 static void parse_supported_bands(struct wiphy *wiphy,
 						struct l_genl_attr *bands)
 {
-	uint16_t type, len;
-	const void *data;
+	uint16_t type;
 	struct l_genl_attr attr;
 
 	l_debug("");
 
-	while (l_genl_attr_next(bands, NULL, NULL, NULL)) {
+	while (l_genl_attr_next(bands, &type, NULL, NULL)) {
+		enum nl80211_band band = type;
+
+		if (band != NL80211_BAND_2GHZ && band != NL80211_BAND_5GHZ)
+			continue;
+
 		if (!l_genl_attr_recurse(bands, &attr))
 			continue;
 
-		while (l_genl_attr_next(&attr, &type, &len, &data)) {
+		while (l_genl_attr_next(&attr, &type, NULL, NULL)) {
 			struct l_genl_attr freqs;
 
 			switch (type) {
@@ -645,6 +713,14 @@ static void parse_supported_bands(struct wiphy *wiphy,
 
 				parse_supported_frequencies(wiphy, &freqs);
 				break;
+
+			case NL80211_BAND_ATTR_RATES:
+				if (wiphy->supported_rates[band])
+					continue;
+
+				wiphy->supported_rates[band] =
+					parse_supported_rates(&attr);
+				break;
 			}
 		}
 	}
diff --git a/src/wiphy.h b/src/wiphy.h
index 7287cddd..85fa3f56 100644
--- a/src/wiphy.h
+++ b/src/wiphy.h
@@ -66,6 +66,8 @@ bool wiphy_has_ext_feature(struct wiphy *wiphy, uint32_t feature);
 uint8_t wiphy_get_max_num_ssids_per_scan(struct wiphy *wiphy);
 uint32_t wiphy_get_max_roc_duration(struct wiphy *wiphy);
 bool wiphy_supports_iftype(struct wiphy *wiphy, uint32_t iftype);
+const uint8_t *wiphy_get_supported_rates(struct wiphy *wiphy, unsigned int band,
+						unsigned int *out_num);
 bool wiphy_supports_adhoc_rsn(struct wiphy *wiphy);
 bool wiphy_can_offchannel_tx(struct wiphy *wiphy);
 bool wiphy_supports_qos_set_map(struct wiphy *wiphy);
-- 
2.20.1

[PATCH 02/16] scan: Hide CCK rates if no_cck_rates set

[Andrew Zaborowski]

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

no_cck_rates is set in the scan parameters generally to make sure
that the Probe Request frames are not sent at any of the 802.11b
rates during active scans.  With this patch we also omit those rates
from the Supported Rates IEs, which is required by the p2p spec and
also matches our flag's name.
---
 src/scan.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/src/scan.c b/src/scan.c
index c0c3177e..0ec7831c 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -347,10 +347,43 @@ static struct l_genl_msg *scan_build_cmd(struct scan_context *sc,
 	if (flags)
 		l_genl_msg_append_attr(msg, NL80211_ATTR_SCAN_FLAGS, 4, &flags);
 
-	if (params->no_cck_rates)
+	if (params->no_cck_rates) {
+		static const uint8_t b_rates[] = { 2, 4, 11, 22 };
+		uint8_t *scan_rates;
+		const uint8_t *supported;
+		unsigned int num_supported;
+		unsigned int count;
+
 		l_genl_msg_append_attr(msg, NL80211_ATTR_TX_NO_CCK_RATE, 0,
 					NULL);
 
+		/*
+		 * Assume if we're sending the probe requests at OFDM bit
+		 * rates we don't want to advertise support for 802.11b rates.
+		 */
+		supported = wiphy_get_supported_rates(sc->wiphy,
+							NL80211_BAND_2GHZ,
+							&num_supported);
+		if (!supported) {
+			L_WARN_ON(true);
+			return msg;
+		}
+
+		scan_rates = l_malloc(num_supported);
+
+		for (count = 0; *supported; supported++)
+			if (!memchr(b_rates, *supported, L_ARRAY_SIZE(b_rates)))
+				scan_rates[count++] = *supported;
+
+		L_WARN_ON(!count);
+
+		l_genl_msg_enter_nested(msg, NL80211_ATTR_SCAN_SUPP_RATES);
+		l_genl_msg_append_attr(msg, NL80211_BAND_2GHZ, count,
+					scan_rates);
+		l_genl_msg_leave_nested(msg);
+		l_free(scan_rates);
+	}
+
 	return msg;
 }
 
-- 
2.20.1

[PATCH 03/16] scan: Parse P2P IEs according to frame type

[Andrew Zaborowski]

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

Save the source frame type in struct scan_bss as it may affect how some
of the data in the struct should be parsed.  Also replace the P2P IE
payload data in that struct with a union containing pre-parsed p2p
attributes corresponding to the frame type.

This means users don't have to call the parsers in p2putil.c on that
data, which wouldn't have worked anyway because we those parsers were
assuming the raw concatenated P2P IEs as a parameter instead of the
payload extracted with ie_tlv_extract_p2p_payload.
---
 src/scan.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++-------
 src/scan.h | 17 ++++++++++--
 2 files changed, 84 insertions(+), 12 deletions(-)

diff --git a/src/scan.c b/src/scan.c
index 0ec7831c..0956c6f6 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -45,6 +45,7 @@
 #include "src/nl80211cmd.h"
 #include "src/nl80211util.h"
 #include "src/util.h"
+#include "src/p2putil.h"
 #include "src/scan.h"
 
 #define SCAN_MAX_INTERVAL 320
@@ -987,6 +988,38 @@ static bool scan_parse_bss_information_elements(struct scan_bss *bss,
 		}
 	}
 
+	bss->wsc = ie_tlv_extract_wsc_payload(data, len, &bss->wsc_size);
+
+	switch (bss->source_frame) {
+	case SCAN_BSS_PROBE_RESP:
+	{
+		struct p2p_probe_resp info;
+
+		if (p2p_parse_probe_resp(data, len, &info) == 0)
+			bss->p2p_probe_resp_info = l_memdup(&info, sizeof(info));
+
+		break;
+	}
+	case SCAN_BSS_PROBE_REQ:
+	{
+		struct p2p_probe_req info;
+
+		if (p2p_parse_probe_req(data, len, &info) == 0)
+			bss->p2p_probe_req_info = l_memdup(&info, sizeof(info));
+
+		break;
+	}
+	case SCAN_BSS_BEACON:
+	{
+		struct p2p_beacon info;
+
+		if (p2p_parse_beacon(data, len, &info) == 0)
+			bss->p2p_beacon_info = l_memdup(&info, sizeof(info));
+
+		break;
+	}
+	}
+
 	return have_ssid;
 }
 
@@ -995,9 +1028,12 @@ static struct scan_bss *scan_parse_attr_bss(struct l_genl_attr *attr)
 	uint16_t type, len;
 	const void *data;
 	struct scan_bss *bss;
+	const uint8_t *ies = NULL;
+	size_t ies_len;
 
 	bss = l_new(struct scan_bss, 1);
 	bss->utilization = 127;
+	bss->source_frame = SCAN_BSS_BEACON;
 
 	while (l_genl_attr_next(attr, &type, &len, &data)) {
 		switch (type) {
@@ -1025,20 +1061,19 @@ static struct scan_bss *scan_parse_attr_bss(struct l_genl_attr *attr)
 
 			bss->signal_strength = *((int32_t *) data);
 			break;
+		case NL80211_BSS_PRESP_DATA:
+			bss->source_frame = SCAN_BSS_PROBE_RESP;
+			break;
 		case NL80211_BSS_INFORMATION_ELEMENTS:
-			if (!scan_parse_bss_information_elements(bss,
-								data, len))
-				goto fail;
-
-			bss->wsc = ie_tlv_extract_wsc_payload(data, len,
-								&bss->wsc_size);
-			bss->p2p = ie_tlv_extract_p2p_payload(data, len,
-								&bss->p2p_size);
-
+			ies = data;
+			ies_len = len;
 			break;
 		}
 	}
 
+	if (ies && !scan_parse_bss_information_elements(bss, ies, ies_len))
+		goto fail;
+
 	return bss;
 
 fail:
@@ -1198,9 +1233,33 @@ void scan_bss_free(struct scan_bss *bss)
 	l_free(bss->rsne);
 	l_free(bss->wpa);
 	l_free(bss->wsc);
-	l_free(bss->p2p);
 	l_free(bss->osen);
 	l_free(bss->rc_ie);
+
+	switch (bss->source_frame) {
+	case SCAN_BSS_PROBE_RESP:
+		if (!bss->p2p_probe_resp_info)
+			break;
+
+		p2p_free_probe_resp(bss->p2p_probe_resp_info);
+		l_free(bss->p2p_probe_resp_info);
+		break;
+	case SCAN_BSS_PROBE_REQ:
+		if (!bss->p2p_probe_req_info)
+			break;
+
+		p2p_free_probe_req(bss->p2p_probe_req_info);
+		l_free(bss->p2p_probe_req_info);
+		break;
+	case SCAN_BSS_BEACON:
+		if (!bss->p2p_beacon_info)
+			break;
+
+		p2p_free_beacon(bss->p2p_beacon_info);
+		l_free(bss->p2p_beacon_info);
+		break;
+	}
+
 	l_free(bss);
 }
 
diff --git a/src/scan.h b/src/scan.h
index 626de80b..afed831e 100644
--- a/src/scan.h
+++ b/src/scan.h
@@ -40,6 +40,15 @@ typedef void (*scan_freq_set_func_t)(uint32_t freq, void *userdata);
 
 struct scan_freq_set;
 struct ie_rsn_info;
+struct p2p_probe_resp;
+struct p2p_probe_req;
+struct p2p_beacon;
+
+enum scan_bss_frame_type {
+	SCAN_BSS_PROBE_RESP,
+	SCAN_BSS_PROBE_REQ,
+	SCAN_BSS_BEACON,
+};
 
 struct scan_bss {
 	uint8_t addr[6];
@@ -51,8 +60,12 @@ struct scan_bss {
 	uint8_t *osen;
 	uint8_t *wsc;		/* Concatenated WSC IEs */
 	ssize_t wsc_size;	/* Size of Concatenated WSC IEs */
-	uint8_t *p2p;		/* Concatenated P2P IEs */
-	ssize_t p2p_size;	/* Size of Concatenated P2P IEs */
+	enum scan_bss_frame_type source_frame;
+	union {
+		struct p2p_probe_resp *p2p_probe_resp_info;
+		struct p2p_probe_req *p2p_probe_req_info;
+		struct p2p_beacon *p2p_beacon_info;
+	};
 	uint8_t mde[3];
 	uint8_t ssid[32];
 	uint8_t ssid_len;
-- 
2.20.1

[PATCH 04/16] scan: Add scan_bss_new_from_probe_req

[Andrew Zaborowski]

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

---
 src/scan.c | 27 +++++++++++++++++++++++++++
 src/scan.h |  6 ++++++
 2 files changed, 33 insertions(+)

diff --git a/src/scan.c b/src/scan.c
index 0956c6f6..7289bbaa 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -46,6 +46,7 @@
 #include "src/nl80211util.h"
 #include "src/util.h"
 #include "src/p2putil.h"
+#include "src/mpdu.h"
 #include "src/scan.h"
 
 #define SCAN_MAX_INTERVAL 320
@@ -1227,6 +1228,32 @@ static void scan_bss_compute_rank(struct scan_bss *bss)
 		bss->rank = irank;
 }
 
+struct scan_bss *scan_bss_new_from_probe_req(const struct mmpdu_header *mpdu,
+						const uint8_t *body,
+						size_t body_len,
+						uint32_t frequency, int rssi)
+
+{
+	struct scan_bss *bss;
+
+	bss = l_new(struct scan_bss, 1);
+	memcpy(bss->addr, mpdu->address_2, 6);
+	bss->utilization = 127;
+	bss->source_frame = SCAN_BSS_PROBE_REQ;
+	bss->frequency = frequency;
+	bss->signal_strength = rssi;
+
+	if (!scan_parse_bss_information_elements(bss, body, body_len))
+		goto fail;
+
+	scan_bss_compute_rank(bss);
+	return bss;
+
+fail:
+	scan_bss_free(bss);
+	return NULL;
+}
+
 void scan_bss_free(struct scan_bss *bss)
 {
 	l_free(bss->ext_supp_rates_ie);
diff --git a/src/scan.h b/src/scan.h
index afed831e..b2f21359 100644
--- a/src/scan.h
+++ b/src/scan.h
@@ -43,6 +43,7 @@ struct ie_rsn_info;
 struct p2p_probe_resp;
 struct p2p_probe_req;
 struct p2p_beacon;
+struct mmpdu_header;
 
 enum scan_bss_frame_type {
 	SCAN_BSS_PROBE_RESP,
@@ -134,6 +135,11 @@ int scan_bss_rank_compare(const void *a, const void *b, void *user);
 
 int scan_bss_get_rsn_info(const struct scan_bss *bss, struct ie_rsn_info *info);
 
+struct scan_bss *scan_bss_new_from_probe_req(const struct mmpdu_header *mpdu,
+						const uint8_t *body,
+						size_t body_len,
+						uint32_t frequency, int rssi);
+
 uint8_t scan_freq_to_channel(uint32_t freq, enum scan_band *out_band);
 uint32_t scan_channel_to_freq(uint8_t channel, enum scan_band band);
 enum scan_band scan_oper_class_to_band(const uint8_t *country,
-- 
2.20.1

[PATCH 05/16] handshake: Convert handshake event callbacks variadic functions

[Andrew Zaborowski]

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

Convert the handshake event callback type to use variable argument
list to allow for more flexibility in event-specific arguments
passed to the callbacks.
---
 src/adhoc.c     |  2 +-
 src/ap.c        |  9 +++++++--
 src/handshake.c |  7 -------
 src/handshake.h | 13 +++++++++----
 src/station.c   |  9 +++++++--
 src/wsc.c       | 11 +++++++++--
 6 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/src/adhoc.c b/src/adhoc.c
index 00f3a7e3..755e9493 100644
--- a/src/adhoc.c
+++ b/src/adhoc.c
@@ -159,7 +159,7 @@ static bool ap_sta_match_addr(const void *a, const void *b)
 }
 
 static void adhoc_handshake_event(struct handshake_state *hs,
-		enum handshake_event event, void *event_data, void *user_data)
+		enum handshake_event event, void *user_data, ...)
 {
 	struct sta_state *sta = user_data;
 	struct adhoc_state *adhoc = sta->adhoc;
diff --git a/src/ap.c b/src/ap.c
index b06b6691..cacfa807 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -382,16 +382,19 @@ static uint32_t ap_send_mgmt_frame(struct ap_state *ap,
 }
 
 static void ap_handshake_event(struct handshake_state *hs,
-		enum handshake_event event, void *event_data, void *user_data)
+		enum handshake_event event, void *user_data, ...)
 {
 	struct sta_state *sta = user_data;
+	va_list args;
+
+	va_start(args, user_data);
 
 	switch (event) {
 	case HANDSHAKE_EVENT_COMPLETE:
 		ap_new_rsna(sta);
 		break;
 	case HANDSHAKE_EVENT_FAILED:
-		netdev_handshake_failed(hs, l_get_u16(event_data));
+		netdev_handshake_failed(hs, l_get_u16(va_arg(args, void *)));
 		/* fall through */
 	case HANDSHAKE_EVENT_SETTING_KEYS_FAILED:
 		sta->sm = NULL;
@@ -399,6 +402,8 @@ static void ap_handshake_event(struct handshake_state *hs,
 	default:
 		break;
 	}
+
+	va_end(args);
 }
 
 static void ap_start_rsna(struct sta_state *sta, const uint8_t *gtk_rsc)
diff --git a/src/handshake.c b/src/handshake.c
index 8e1aeec9..a3e236bb 100644
--- a/src/handshake.c
+++ b/src/handshake.c
@@ -912,10 +912,3 @@ bool handshake_decode_fte_key(struct handshake_state *s, const uint8_t *wrapped,
 
 	return true;
 }
-
-void handshake_event(struct handshake_state *hs,
-			enum handshake_event event, void *event_data)
-{
-	if (hs->event_func)
-		hs->event_func(hs, event, event_data, hs->user_data);
-}
diff --git a/src/handshake.h b/src/handshake.h
index 08503190..cd0c1dc9 100644
--- a/src/handshake.h
+++ b/src/handshake.h
@@ -54,7 +54,7 @@ enum handshake_event {
 
 typedef void (*handshake_event_func_t)(struct handshake_state *hs,
 					enum handshake_event event,
-					void *event_data, void *user_data);
+					void *user_data, ...);
 
 typedef bool (*handshake_get_nonce_func_t)(uint8_t nonce[]);
 typedef void (*handshake_install_tk_func_t)(struct handshake_state *hs,
@@ -130,6 +130,14 @@ struct handshake_state {
 	handshake_event_func_t event_func;
 };
 
+#define handshake_event(hs, event, ...)	\
+	do {	\
+		if (!(hs)->event_func)	\
+			break;	\
+	\
+		(hs)->event_func(hs, event, (hs)->user_data, ##__VA_ARGS__); \
+	} while (0)
+
 void handshake_state_free(struct handshake_state *s);
 
 void handshake_state_set_supplicant_address(struct handshake_state *s,
@@ -216,6 +224,3 @@ const uint8_t *handshake_util_find_pmkid_kde(const uint8_t *data,
 					size_t data_len);
 void handshake_util_build_gtk_kde(enum crypto_cipher cipher, const uint8_t *key,
 					unsigned int key_index, uint8_t *to);
-
-void handshake_event(struct handshake_state *hs, enum handshake_event event,
-		void *event_data);
diff --git a/src/station.c b/src/station.c
index 315b3dcd..80a487cb 100644
--- a/src/station.c
+++ b/src/station.c
@@ -650,10 +650,13 @@ static void station_reconnect(struct station *station);
 
 static void station_handshake_event(struct handshake_state *hs,
 					enum handshake_event event,
-					void *event_data, void *user_data)
+					void *user_data, ...)
 {
 	struct station *station = user_data;
 	struct network *network = station->connected_network;
+	va_list args;
+
+	va_start(args, user_data);
 
 	switch (event) {
 	case HANDSHAKE_EVENT_STARTED:
@@ -666,7 +669,7 @@ static void station_handshake_event(struct handshake_state *hs,
 		network_sync_psk(network);
 		break;
 	case HANDSHAKE_EVENT_FAILED:
-		netdev_handshake_failed(hs, l_get_u16(event_data));
+		netdev_handshake_failed(hs, l_get_u16(va_arg(args, void *)));
 		break;
 	case HANDSHAKE_EVENT_REKEY_FAILED:
 		station_reconnect(station);
@@ -680,6 +683,8 @@ static void station_handshake_event(struct handshake_state *hs,
 		 */
 		break;
 	}
+
+	va_end(args);
 }
 
 static bool station_has_erp_identity(struct network *network)
diff --git a/src/wsc.c b/src/wsc.c
index 9e8adf04..b659be5d 100644
--- a/src/wsc.c
+++ b/src/wsc.c
@@ -402,15 +402,22 @@ static void wsc_netdev_event(struct netdev *netdev, enum netdev_event event,
 }
 
 static void wsc_handshake_event(struct handshake_state *hs,
-		enum handshake_event event, void *event_data, void *user_data)
+				enum handshake_event event, void *user_data,
+				...)
 {
+	va_list args;
+
+	va_start(args, user_data);
+
 	switch (event) {
 	case HANDSHAKE_EVENT_FAILED:
-		netdev_handshake_failed(hs, l_get_u16(event_data));
+		netdev_handshake_failed(hs, l_get_u16(va_arg(args, void *)));
 		break;
 	default:
 		break;
 	}
+
+	va_end(args);
 }
 
 static inline enum wsc_rf_band freq_to_rf_band(uint32_t freq)
-- 
2.20.1

[PATCH 06/16] handshake: Convert HANDSHAKE_EVENT_FAILED data to int

[Andrew Zaborowski]

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

Note the uint16_t reason code is promoted to an int when using variable
arguments so va_arg(args, int) has to be used.
---
 src/ap.c      | 2 +-
 src/eapol.c   | 2 +-
 src/station.c | 2 +-
 src/wsc.c     | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index cacfa807..694ee885 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -394,7 +394,7 @@ static void ap_handshake_event(struct handshake_state *hs,
 		ap_new_rsna(sta);
 		break;
 	case HANDSHAKE_EVENT_FAILED:
-		netdev_handshake_failed(hs, l_get_u16(va_arg(args, void *)));
+		netdev_handshake_failed(hs, va_arg(args, int));
 		/* fall through */
 	case HANDSHAKE_EVENT_SETTING_KEYS_FAILED:
 		sta->sm = NULL;
diff --git a/src/eapol.c b/src/eapol.c
index 44d42735..e7ee9fe8 100644
--- a/src/eapol.c
+++ b/src/eapol.c
@@ -915,7 +915,7 @@ static void eapol_sm_write(struct eapol_sm *sm, const struct eapol_frame *ef,
 
 static inline void handshake_failed(struct eapol_sm *sm, uint16_t reason_code)
 {
-	handshake_event(sm->handshake, HANDSHAKE_EVENT_FAILED, &reason_code);
+	handshake_event(sm->handshake, HANDSHAKE_EVENT_FAILED, reason_code);
 
 	eapol_sm_free(sm);
 }
diff --git a/src/station.c b/src/station.c
index 80a487cb..e88adf36 100644
--- a/src/station.c
+++ b/src/station.c
@@ -669,7 +669,7 @@ static void station_handshake_event(struct handshake_state *hs,
 		network_sync_psk(network);
 		break;
 	case HANDSHAKE_EVENT_FAILED:
-		netdev_handshake_failed(hs, l_get_u16(va_arg(args, void *)));
+		netdev_handshake_failed(hs, va_arg(args, int));
 		break;
 	case HANDSHAKE_EVENT_REKEY_FAILED:
 		station_reconnect(station);
diff --git a/src/wsc.c b/src/wsc.c
index b659be5d..04187f5b 100644
--- a/src/wsc.c
+++ b/src/wsc.c
@@ -411,7 +411,7 @@ static void wsc_handshake_event(struct handshake_state *hs,
 
 	switch (event) {
 	case HANDSHAKE_EVENT_FAILED:
-		netdev_handshake_failed(hs, l_get_u16(va_arg(args, void *)));
+		netdev_handshake_failed(hs, va_arg(args, int));
 		break;
 	default:
 		break;
-- 
2.20.1

[PATCH 07/16] handshake: Drop NULL event params passed to handshake_event

[Andrew Zaborowski]

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

---
 src/eapol.c     | 4 ++--
 src/handshake.c | 2 +-
 src/netdev.c    | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/eapol.c b/src/eapol.c
index e7ee9fe8..dbef66f5 100644
--- a/src/eapol.c
+++ b/src/eapol.c
@@ -970,7 +970,7 @@ static void __send_eapol_start(struct eapol_sm *sm, bool noencrypt)
 	uint8_t buf[sizeof(struct eapol_frame)];
 	struct eapol_frame *frame = (struct eapol_frame *) buf;
 
-	handshake_event(sm->handshake, HANDSHAKE_EVENT_STARTED, NULL);
+	handshake_event(sm->handshake, HANDSHAKE_EVENT_STARTED);
 
 	frame->header.protocol_version = EAPOL_PROTOCOL_VERSION_2001;
 	frame->header.packet_type = 1;
@@ -1181,7 +1181,7 @@ static void eapol_handle_ptk_1_of_4(struct eapol_sm *sm,
 			 * layers that we need to do a full reauth
 			 */
 			handshake_event(sm->handshake,
-					HANDSHAKE_EVENT_REKEY_FAILED, NULL);
+					HANDSHAKE_EVENT_REKEY_FAILED);
 			return;
 		}
 
diff --git a/src/handshake.c b/src/handshake.c
index a3e236bb..bb4663c7 100644
--- a/src/handshake.c
+++ b/src/handshake.c
@@ -558,7 +558,7 @@ void handshake_state_install_ptk(struct handshake_state *s)
 		uint32_t cipher = ie_rsn_cipher_suite_to_cipher(
 							s->pairwise_cipher);
 
-		handshake_event(s, HANDSHAKE_EVENT_SETTING_KEYS, NULL);
+		handshake_event(s, HANDSHAKE_EVENT_SETTING_KEYS);
 
 		install_tk(s, handshake_get_tk(s), cipher);
 	}
diff --git a/src/netdev.c b/src/netdev.c
index da75fe48..9816f68d 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -1094,7 +1094,7 @@ static void try_handshake_complete(struct netdev_handshake_state *nhs)
 	if (nhs->ptk_installed && nhs->gtk_installed && nhs->igtk_installed &&
 			!nhs->complete) {
 		nhs->complete = true;
-		handshake_event(&nhs->super, HANDSHAKE_EVENT_COMPLETE, NULL);
+		handshake_event(&nhs->super, HANDSHAKE_EVENT_COMPLETE);
 
 		netdev_connect_ok(nhs->netdev);
 	}
@@ -1388,7 +1388,7 @@ static void netdev_group_timeout_cb(struct l_timeout *timeout, void *user_data)
 			nhs->netdev->index);
 
 	nhs->complete = true;
-	handshake_event(&nhs->super, HANDSHAKE_EVENT_COMPLETE, NULL);
+	handshake_event(&nhs->super, HANDSHAKE_EVENT_COMPLETE);
 
 	netdev_connect_ok(nhs->netdev);
 }
-- 
2.20.1

[PATCH 08/16] eapol: Move the EAP events to handshake event handler

[Andrew Zaborowski]

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

On EAP events, call the handshake_event handler with the new event type
HANDSHAKE_EVENT_EAP_NOTIFY isntead of the eapol_event callback.

This allows the handler to be set before calling
netdev_connect/netdev_connect_wsc.  It's also in theory more type-safe
because we don't need the cast in netdev_connect_wsc anymore.
---
 src/eapol.c     |  6 ++----
 src/handshake.h |  1 +
 src/station.c   |  1 +
 src/wsc.c       | 27 +++++++++++++++++----------
 4 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/src/eapol.c b/src/eapol.c
index dbef66f5..f2a5611f 100644
--- a/src/eapol.c
+++ b/src/eapol.c
@@ -2167,10 +2167,8 @@ static void eapol_eap_event_cb(unsigned int event,
 {
 	struct eapol_sm *sm = user_data;
 
-	if (!sm->event_func)
-		return;
-
-	sm->event_func(event, event_data, sm->user_data);
+	handshake_event(sm->handshake, HANDSHAKE_EVENT_EAP_NOTIFY, event,
+			event_data);
 }
 
 void eapol_sm_set_use_eapol_start(struct eapol_sm *sm, bool enabled)
diff --git a/src/handshake.h b/src/handshake.h
index cd0c1dc9..c369ad50 100644
--- a/src/handshake.h
+++ b/src/handshake.h
@@ -50,6 +50,7 @@ enum handshake_event {
 	HANDSHAKE_EVENT_COMPLETE,
 	HANDSHAKE_EVENT_FAILED,
 	HANDSHAKE_EVENT_REKEY_FAILED,
+	HANDSHAKE_EVENT_EAP_NOTIFY,
 };
 
 typedef void (*handshake_event_func_t)(struct handshake_state *hs,
diff --git a/src/station.c b/src/station.c
index e88adf36..5e1d2468 100644
--- a/src/station.c
+++ b/src/station.c
@@ -676,6 +676,7 @@ static void station_handshake_event(struct handshake_state *hs,
 		break;
 	case HANDSHAKE_EVENT_COMPLETE:
 	case HANDSHAKE_EVENT_SETTING_KEYS_FAILED:
+	case HANDSHAKE_EVENT_EAP_NOTIFY:
 		/*
 		 * currently we dont care about any other events. The
 		 * netdev_connect_cb will notify us when the connection is
diff --git a/src/wsc.c b/src/wsc.c
index 04187f5b..fcaae36f 100644
--- a/src/wsc.c
+++ b/src/wsc.c
@@ -363,16 +363,7 @@ static void wsc_credential_obtained(struct wsc *wsc,
 static void wsc_eapol_event(uint32_t event, const void *event_data,
 							void *user_data)
 {
-	struct wsc *wsc = user_data;
-
-	switch (event) {
-	case EAP_WSC_EVENT_CREDENTIAL_OBTAINED:
-		wsc_credential_obtained(wsc,
-				(const struct wsc_credential *) event_data);
-		break;
-	default:
-		l_debug("Got event: %d", event);
-	}
+	l_debug("Got event: %d", event);
 }
 
 static void wsc_netdev_event(struct netdev *netdev, enum netdev_event event,
@@ -405,6 +396,7 @@ static void wsc_handshake_event(struct handshake_state *hs,
 				enum handshake_event event, void *user_data,
 				...)
 {
+	struct wsc *wsc = user_data;
 	va_list args;
 
 	va_start(args, user_data);
@@ -413,6 +405,21 @@ static void wsc_handshake_event(struct handshake_state *hs,
 	case HANDSHAKE_EVENT_FAILED:
 		netdev_handshake_failed(hs, va_arg(args, int));
 		break;
+	case HANDSHAKE_EVENT_EAP_NOTIFY:
+	{
+		unsigned int eap_event = va_arg(args, unsigned int);
+
+		switch (eap_event) {
+		case EAP_WSC_EVENT_CREDENTIAL_OBTAINED:
+			wsc_credential_obtained(wsc,
+				va_arg(args, const struct wsc_credential *));
+			break;
+		default:
+			l_debug("Got event: %d", eap_event);
+		}
+
+		break;
+	}
 	default:
 		break;
 	}
-- 
2.20.1

[PATCH 09/16] unit: Update event handler in WSC, eapol tests

[Andrew Zaborowski]

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

---
 unit/test-eapol.c |  2 +-
 unit/test-wsc.c   | 42 +++++++++++++++++++-----------------------
 2 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/unit/test-eapol.c b/unit/test-eapol.c
index 6760db6d..41bce402 100644
--- a/unit/test-eapol.c
+++ b/unit/test-eapol.c
@@ -2871,7 +2871,7 @@ static void eapol_sm_test_tls_test_disconnected(enum l_tls_alert_desc reason,
 
 static void test_handshake_event(struct handshake_state *hs,
 					enum handshake_event event,
-					void *event_data, void *user_data)
+					void *user_data, ...)
 {
 	struct test_handshake_state *ths =
 			l_container_of(hs, struct test_handshake_state, super);
diff --git a/unit/test-wsc.c b/unit/test-wsc.c
index f11dce0d..0e845dc2 100644
--- a/unit/test-wsc.c
+++ b/unit/test-wsc.c
@@ -1931,16 +1931,32 @@ struct verify_data {
 	verify.expected_len = sizeof(e)
 
 static void verify_handshake_event(struct handshake_state *hs,
-		enum handshake_event event, void *event_data, void *user_data)
+					enum handshake_event event,
+					void *user_data, ...)
 {
 	struct verify_data *data = user_data;
+	va_list args;
+
+	va_start(args, user_data);
 
 	switch (event) {
 	case HANDSHAKE_EVENT_FAILED:
-		assert(l_get_u16(event_data) ==
-				MMPDU_REASON_CODE_IEEE8021X_FAILED);
+		assert(va_arg(args, int) == MMPDU_REASON_CODE_IEEE8021X_FAILED);
 		data->eapol_failed = true;
 		break;
+	case HANDSHAKE_EVENT_EAP_NOTIFY:
+	{
+		const struct wsc_credential *cred;
+
+		assert(va_arg(args, unsigned int) !=
+			EAP_WSC_EVENT_CREDENTIAL_OBTAINED);
+
+		cred = va_arg(args, const struct wsc_credential *);
+		assert(!memcmp(cred, data->expected_creds + data->cur_cred,
+						sizeof(struct wsc_credential)));
+
+		data->cur_cred += 1;
+	}
 	default:
 		break;
 	}
@@ -1963,22 +1979,6 @@ static int verify_8021x(uint32_t ifindex,
 	return 0;
 }
 
-static void verify_credential(unsigned int event, const void *event_data,
-					void *user_data)
-{
-	struct verify_data *data = user_data;
-	const struct wsc_credential *cred;
-
-	if (event != EAP_WSC_EVENT_CREDENTIAL_OBTAINED)
-		assert(false);
-
-	cred = event_data;
-	assert(!memcmp(cred, data->expected_creds + data->cur_cred,
-						sizeof(struct wsc_credential)));
-
-	data->cur_cred += 1;
-}
-
 static void wsc_test_pbc_handshake(const void *data)
 {
 	static uint8_t ap_address[] = { 0x24, 0xa2, 0xe1, 0xec, 0x17, 0x04 };
@@ -2002,8 +2002,6 @@ static void wsc_test_pbc_handshake(const void *data)
 
 	__eapol_set_tx_packet_func(verify_8021x);
 	__eapol_set_tx_user_data(&verify);
-	eapol_sm_set_user_data(sm, &verify);
-	eapol_sm_set_event_func(sm, verify_credential);
 
 	settings = l_settings_new();
 	l_settings_set_string(settings, "Security", "EAP-Identity",
@@ -2110,8 +2108,6 @@ static void wsc_test_retransmission_no_fragmentation(const void *data)
 
 	__eapol_set_tx_packet_func(verify_8021x);
 	__eapol_set_tx_user_data(&verify);
-	eapol_sm_set_user_data(sm, &verify);
-	eapol_sm_set_event_func(sm, verify_credential);
 
 	settings = l_settings_new();
 	l_settings_set_string(settings, "Security", "EAP-Identity",
-- 
2.20.1

[PATCH 10/16] wsc: Replace netdev_connect_wsc with netdev_connect usage

[Andrew Zaborowski]

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

netdev_connect can achieve the same effect as netdev_connect_wsc but is
more flexible as it allows us to supply additional association IEs.  We
will need this capability to make P2P connections.  This way we're also
moving the WSC-specific bits to wsc.c from the crowded netdev.c.
---
 src/netdev.c |  2 +-
 src/wsc.c    | 46 +++++++++++++++++++++++++++++++++-------------
 2 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/src/netdev.c b/src/netdev.c
index 9816f68d..eab7bd8f 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -2502,7 +2502,7 @@ int netdev_connect(struct netdev *netdev, struct scan_bss *bss,
 		if (!cmd_connect)
 			return -EINVAL;
 
-		if (is_rsn)
+		if (is_rsn || hs->settings_8021x)
 			sm = eapol_sm_new(hs);
 	}
 
diff --git a/src/wsc.c b/src/wsc.c
index fcaae36f..bac6e0cc 100644
--- a/src/wsc.c
+++ b/src/wsc.c
@@ -360,12 +360,6 @@ static void wsc_credential_obtained(struct wsc *wsc,
 	wsc->n_creds += 1;
 }
 
-static void wsc_eapol_event(uint32_t event, const void *event_data,
-							void *user_data)
-{
-	l_debug("Got event: %d", event);
-}
-
 static void wsc_netdev_event(struct netdev *netdev, enum netdev_event event,
 					void *event_data, void *user_data)
 {
@@ -448,6 +442,11 @@ static void wsc_connect(struct wsc *wsc)
 	struct handshake_state *hs;
 	struct l_settings *settings = l_settings_new();
 	struct scan_bss *bss = wsc->target;
+	int r;
+	struct wsc_association_request request;
+	uint8_t *pdu;
+	size_t pdu_len;
+	struct iovec ie_iov;
 
 	wsc->target = NULL;
 
@@ -490,16 +489,37 @@ static void wsc_connect(struct wsc *wsc)
 	handshake_state_set_8021x_config(hs, settings);
 	wsc->eap_settings = settings;
 
-	if (netdev_connect_wsc(wsc->netdev, bss, hs,
-					wsc_netdev_event, wsc_connect_cb,
-					wsc_eapol_event, wsc) < 0) {
-		dbus_pending_reply(&wsc->pending,
-					dbus_error_failed(wsc->pending));
-		handshake_state_free(hs);
-		return;
+	request.version2 = true;
+	request.request_type = WSC_REQUEST_TYPE_ENROLLEE_OPEN_8021X;
+
+	pdu = wsc_build_association_request(&request, &pdu_len);
+	if (!pdu) {
+		r = -ENOMEM;
+		goto error;
 	}
 
+	ie_iov.iov_base = ie_tlv_encapsulate_wsc_payload(pdu, pdu_len,
+							&ie_iov.iov_len);
+	l_free(pdu);
+
+	if (!ie_iov.iov_base) {
+		r = -ENOMEM;
+		goto error;
+	}
+
+	r = netdev_connect(wsc->netdev, bss, hs, &ie_iov, 1, wsc_netdev_event,
+				wsc_connect_cb, wsc);
+	l_free(ie_iov.iov_base);
+
+	if (r < 0)
+		goto error;
+
 	wsc->wsc_association = true;
+	return;
+error:
+	handshake_state_free(hs);
+	dbus_pending_reply(&wsc->pending,
+				dbus_error_failed(wsc->pending));
 }
 
 static void station_state_watch(enum station_state state, void *userdata)
-- 
2.20.1

[PATCH 11/16] eapol: Drop unused eapol_sm_set_event_func

[Andrew Zaborowski]

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

---
 src/eapol.c  | 6 ------
 src/eapol.h  | 1 -
 src/netdev.c | 2 --
 3 files changed, 9 deletions(-)

diff --git a/src/eapol.c b/src/eapol.c
index f2a5611f..a7f1a039 100644
--- a/src/eapol.c
+++ b/src/eapol.c
@@ -823,7 +823,6 @@ struct eapol_sm {
 	struct handshake_state *handshake;
 	enum eapol_protocol_version protocol_version;
 	uint64_t replay_counter;
-	eapol_sm_event_func_t event_func;
 	void *user_data;
 	struct l_timeout *timeout;
 	struct l_timeout *eapol_start_timeout;
@@ -901,11 +900,6 @@ void eapol_sm_set_user_data(struct eapol_sm *sm, void *user_data)
 	sm->user_data = user_data;
 }
 
-void eapol_sm_set_event_func(struct eapol_sm *sm, eapol_sm_event_func_t func)
-{
-	sm->event_func = func;
-}
-
 static void eapol_sm_write(struct eapol_sm *sm, const struct eapol_frame *ef,
 				bool noencrypt)
 {
diff --git a/src/eapol.h b/src/eapol.h
index 063315ce..0f9a6917 100644
--- a/src/eapol.h
+++ b/src/eapol.h
@@ -117,7 +117,6 @@ void eapol_sm_set_use_eapol_start(struct eapol_sm *sm, bool enabled);
 void eapol_sm_set_require_handshake(struct eapol_sm *sm, bool enabled);
 void eapol_sm_set_listen_interval(struct eapol_sm *sm, uint16_t interval);
 void eapol_sm_set_user_data(struct eapol_sm *sm, void *user_data);
-void eapol_sm_set_event_func(struct eapol_sm *sm, eapol_sm_event_func_t func);
 
 void eapol_register(struct eapol_sm *sm);
 bool eapol_start(struct eapol_sm *sm);
diff --git a/src/netdev.c b/src/netdev.c
index eab7bd8f..72d4fc27 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -2553,8 +2553,6 @@ int netdev_connect_wsc(struct netdev *netdev, struct scan_bss *bss,
 	l_free(ie);
 
 	sm = eapol_sm_new(hs);
-	eapol_sm_set_user_data(sm, user_data);
-	eapol_sm_set_event_func(sm, eapol_cb);
 
 	return netdev_connect_common(netdev, cmd_connect, bss, hs, sm,
 						event_filter, cb, user_data);
-- 
2.20.1

[PATCH 12/16] netdev: Drop unused netdev_connect_wsc

[Andrew Zaborowski]

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

---
 src/netdev.c | 53 ----------------------------------------------------
 src/netdev.h |  9 ---------
 2 files changed, 62 deletions(-)

diff --git a/src/netdev.c b/src/netdev.c
index 72d4fc27..c68edc14 100644
--- a/src/netdev.c
+++ b/src/netdev.c
@@ -52,7 +52,6 @@
 #include "src/device.h"
 #include "src/scan.h"
 #include "src/netdev.h"
-#include "src/wscutil.h"
 #include "src/ft.h"
 #include "src/util.h"
 #include "src/watchlist.h"
@@ -2510,58 +2509,6 @@ int netdev_connect(struct netdev *netdev, struct scan_bss *bss,
 						event_filter, cb, user_data);
 }
 
-int netdev_connect_wsc(struct netdev *netdev, struct scan_bss *bss,
-				struct handshake_state *hs,
-				netdev_event_func_t event_filter,
-				netdev_connect_cb_t cb,
-				netdev_eapol_event_func_t eapol_cb,
-				void *user_data)
-{
-	struct l_genl_msg *cmd_connect;
-	struct wsc_association_request request;
-	uint8_t *pdu;
-	size_t pdu_len;
-	void *ie;
-	size_t ie_len;
-	struct eapol_sm *sm;
-
-	if (netdev->type != NL80211_IFTYPE_STATION &&
-			netdev->type != NL80211_IFTYPE_P2P_CLIENT)
-		return -ENOTSUP;
-
-	if (netdev->connected)
-		return -EISCONN;
-
-	cmd_connect = netdev_build_cmd_connect(netdev, bss, hs, NULL, NULL, 0);
-	if (!cmd_connect)
-		return -EINVAL;
-
-	request.version2 = true;
-	request.request_type = WSC_REQUEST_TYPE_ENROLLEE_OPEN_8021X;
-
-	pdu = wsc_build_association_request(&request, &pdu_len);
-	if (!pdu)
-		goto error;
-
-	ie = ie_tlv_encapsulate_wsc_payload(pdu, pdu_len, &ie_len);
-	l_free(pdu);
-
-	if (!ie)
-		goto error;
-
-	l_genl_msg_append_attr(cmd_connect, NL80211_ATTR_IE, ie_len, ie);
-	l_free(ie);
-
-	sm = eapol_sm_new(hs);
-
-	return netdev_connect_common(netdev, cmd_connect, bss, hs, sm,
-						event_filter, cb, user_data);
-
-error:
-	l_genl_msg_unref(cmd_connect);
-	return -ENOMEM;
-}
-
 int netdev_disconnect(struct netdev *netdev,
 				netdev_disconnect_cb_t cb, void *user_data)
 {
diff --git a/src/netdev.h b/src/netdev.h
index 60d5d661..8028167d 100644
--- a/src/netdev.h
+++ b/src/netdev.h
@@ -107,9 +107,6 @@ typedef void (*netdev_watch_func_t)(struct netdev *netdev,
 					enum netdev_watch_event event,
 					void *user_data);
 typedef void (*netdev_destroy_func_t)(void *user_data);
-typedef void (*netdev_eapol_event_func_t)(unsigned int event,
-					const void *event_data,
-					void *user_data);
 typedef void (*netdev_neighbor_report_cb_t)(struct netdev *netdev, int err,
 					const uint8_t *reports,
 					size_t reports_len, void *user_data);
@@ -150,12 +147,6 @@ int netdev_connect(struct netdev *netdev, struct scan_bss *bss,
 				size_t num_vendor_ies,
 				netdev_event_func_t event_filter,
 				netdev_connect_cb_t cb, void *user_data);
-int netdev_connect_wsc(struct netdev *netdev, struct scan_bss *bss,
-				struct handshake_state *hs,
-				netdev_event_func_t event_filter,
-				netdev_connect_cb_t cb,
-				netdev_eapol_event_func_t eapol_cb,
-				void *user_data);
 int netdev_disconnect(struct netdev *netdev,
 				netdev_disconnect_cb_t cb, void *user_data);
 int netdev_reassociate(struct netdev *netdev, struct scan_bss *target_bss,
-- 
2.20.1

[PATCH 13/16] wsc: Declare the credentials structure in wsc.h

[Andrew Zaborowski]

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

---
 Makefile.am |  2 +-
 src/wsc.c   | 12 ++----------
 src/wsc.h   | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 11 deletions(-)
 create mode 100644 src/wsc.h

diff --git a/Makefile.am b/Makefile.am
index 823b7d02..7b66c06d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -202,7 +202,7 @@ src_iwd_SOURCES = src/main.c linux/nl80211.h src/iwd.h src/missing.h \
 					src/agent.h src/agent.c \
 					src/storage.h src/storage.c \
 					src/network.h src/network.c \
-					src/wsc.c \
+					src/wsc.h src/wsc.c \
 					src/backtrace.h src/backtrace.c \
 					src/knownnetworks.h \
 					src/knownnetworks.c \
diff --git a/src/wsc.c b/src/wsc.c
index bac6e0cc..a5141346 100644
--- a/src/wsc.c
+++ b/src/wsc.c
@@ -44,6 +44,7 @@
 #include "src/storage.h"
 #include "src/iwd.h"
 #include "src/network.h"
+#include "src/wsc.h"
 
 #define WALK_TIME 120
 
@@ -60,16 +61,7 @@ struct wsc {
 	uint32_t scan_id;
 	struct scan_bss *target;
 	uint32_t station_state_watch;
-	struct {
-		char ssid[33];
-		enum security security;
-		union {
-			uint8_t psk[32];
-			char passphrase[64];
-		};
-		uint8_t addr[6];
-		bool has_passphrase;
-	} creds[3];
+	struct wsc_credentials_info creds[3];
 	uint32_t n_creds;
 	struct l_settings *eap_settings;
 
diff --git a/src/wsc.h b/src/wsc.h
new file mode 100644
index 00000000..b0e6f24a
--- /dev/null
+++ b/src/wsc.h
@@ -0,0 +1,32 @@
+/*
+ *
+ *  Wireless daemon for Linux
+ *
+ *  Copyright (C) 2019  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+struct wsc_credentials_info {
+	char ssid[33];
+	enum security security;
+	union {
+		uint8_t psk[32];
+		char passphrase[64];
+	};
+	uint8_t addr[6];
+	bool has_passphrase;
+};
-- 
2.20.1

[PATCH 14/16] wsc: Refactor to separate DBus-specific code

[Andrew Zaborowski]

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

Split the DBus-specific logic out of the core WSC logic.  The core
WSC code is the part that we can re-use for P2P and doesn't include
the scanning for the target BSS or the attempt to make a station mode
connection.
---
 src/wsc.c | 141 ++++++++++++++++++++++++++++++++++++++----------------
 src/wsc.h |   3 ++
 2 files changed, 103 insertions(+), 41 deletions(-)

diff --git a/src/wsc.c b/src/wsc.c
index a5141346..0c241ee3 100644
--- a/src/wsc.c
+++ b/src/wsc.c
@@ -64,6 +64,11 @@ struct wsc {
 	struct wsc_credentials_info creds[3];
 	uint32_t n_creds;
 	struct l_settings *eap_settings;
+	char *pin;
+
+	wsc_done_cb_t done_cb;
+	void *done_data;
+	uint16_t config_method;
 
 	bool wsc_association : 1;
 };
@@ -152,15 +157,12 @@ static void wsc_try_credentials(struct wsc *wsc)
 		l_dbus_message_unref(wsc->pending);
 		wsc->pending = NULL;
 
-		goto done;
+		return;
 	}
 
 	dbus_pending_reply(&wsc->pending,
 					wsc_error_not_reachable(wsc->pending));
 	station_set_autoconnect(wsc->station, true);
-done:
-	memset(wsc->creds, 0, sizeof(wsc->creds));
-	wsc->n_creds = 0;
 }
 
 static void wsc_store_credentials(struct wsc *wsc)
@@ -216,6 +218,22 @@ static void wsc_disconnect_cb(struct netdev *netdev, bool success,
 	station_set_autoconnect(wsc->station, true);
 }
 
+static void wsc_connect_cleanup(struct wsc *wsc)
+{
+	wsc->wsc_association = false;
+
+	l_settings_free(wsc->eap_settings);
+	wsc->eap_settings = NULL;
+
+	if (wsc->pin) {
+		explicit_bzero(wsc->pin, strlen(wsc->pin));
+		l_free(wsc->pin);
+		wsc->pin = NULL;
+	}
+
+	wsc->target = NULL;
+}
+
 static void wsc_connect_cb(struct netdev *netdev, enum netdev_result result,
 					void *event_data, void *user_data)
 {
@@ -223,33 +241,34 @@ static void wsc_connect_cb(struct netdev *netdev, enum netdev_result result,
 
 	l_debug("%d, result: %d", netdev_get_ifindex(wsc->netdev), result);
 
-	wsc->wsc_association = false;
-
-	l_settings_free(wsc->eap_settings);
-	wsc->eap_settings = NULL;
+	wsc_connect_cleanup(wsc);
 
 	if (result == NETDEV_RESULT_HANDSHAKE_FAILED && wsc->n_creds > 0) {
-		wsc_store_credentials(wsc);
-		wsc_try_credentials(wsc);
+		struct wsc_credentials_info creds[L_ARRAY_SIZE(wsc->creds)];
+		uint32_t n_creds = wsc->n_creds;
+
+		memcpy(creds, wsc->creds, sizeof(creds));
+		explicit_bzero(wsc->creds, sizeof(creds));
+		wsc->n_creds = 0;
+		wsc->done_cb(0, creds, n_creds, wsc->done_data);
+		explicit_bzero(creds, sizeof(creds));
 		return;
 	}
 
 	switch (result) {
 	case NETDEV_RESULT_ABORTED:
-		dbus_pending_reply(&wsc->pending,
-					dbus_error_aborted(wsc->pending));
+		wsc->done_cb(-ECANCELED, NULL, 0, wsc->done_data);
 		return;
 	case NETDEV_RESULT_HANDSHAKE_FAILED:
-		dbus_pending_reply(&wsc->pending,
-					wsc_error_no_credentials(wsc->pending));
+		wsc->done_cb(-ENOKEY, NULL, 0, wsc->done_data);
 		break;
 	default:
-		dbus_pending_reply(&wsc->pending,
-					dbus_error_failed(wsc->pending));
+		wsc->done_cb(-EIO, NULL, 0, wsc->done_data);
 		break;
 	}
 
-	station_set_autoconnect(wsc->station, true);
+	if (wsc->station)
+		station_set_autoconnect(wsc->station, true);
 }
 
 static void wsc_credential_obtained(struct wsc *wsc,
@@ -451,30 +470,24 @@ static void wsc_connect(struct wsc *wsc)
 	l_settings_set_uint(settings, "WSC", "RFBand",
 					freq_to_rf_band(bss->frequency));
 	l_settings_set_uint(settings, "WSC", "ConfigurationMethods",
-				WSC_CONFIGURATION_METHOD_VIRTUAL_DISPLAY_PIN |
-				WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON |
-				WSC_CONFIGURATION_METHOD_KEYPAD);
+					wsc->config_method);
 	l_settings_set_string(settings, "WSC", "PrimaryDeviceType",
 					"0-00000000-0");
 	l_settings_set_string(settings, "WSC", "EnrolleeMAC",
 		util_address_to_string(netdev_get_address(wsc->netdev)));
 
-	if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin")) {
-		const char *pin;
-
-		if (l_dbus_message_get_arguments(wsc->pending, "s", &pin)) {
-			enum wsc_device_password_id dpid;
+	if (wsc->config_method == WSC_CONFIGURATION_METHOD_KEYPAD) {
+		enum wsc_device_password_id dpid;
 
-			if (strlen(pin) == 4 || wsc_pin_is_checksum_valid(pin))
-				dpid = WSC_DEVICE_PASSWORD_ID_DEFAULT;
-			else
-				dpid = WSC_DEVICE_PASSWORD_ID_USER_SPECIFIED;
+		if (strlen(wsc->pin) == 4 ||
+				wsc_pin_is_checksum_valid(wsc->pin))
+			dpid = WSC_DEVICE_PASSWORD_ID_DEFAULT;
+		else
+			dpid = WSC_DEVICE_PASSWORD_ID_USER_SPECIFIED;
 
-			l_settings_set_uint(settings, "WSC",
-						"DevicePasswordId", dpid);
-			l_settings_set_string(settings, "WSC",
-						"DevicePassword", pin);
-		}
+		l_settings_set_uint(settings, "WSC", "DevicePasswordId", dpid);
+		l_settings_set_string(settings, "WSC", "DevicePassword",
+					wsc->pin);
 	}
 
 	handshake_state_set_event_func(hs, wsc_handshake_event, wsc);
@@ -510,8 +523,41 @@ static void wsc_connect(struct wsc *wsc)
 	return;
 error:
 	handshake_state_free(hs);
-	dbus_pending_reply(&wsc->pending,
-				dbus_error_failed(wsc->pending));
+	wsc_connect_cleanup(wsc);
+	wsc->done_cb(r, NULL, 0, wsc->done_data);
+}
+
+/* Done callback for when WSC is triggered by DBus methods */
+static void wsc_dbus_done_cb(int err, struct wsc_credentials_info *creds,
+				unsigned int n_creds, void *user_data)
+{
+	struct wsc *wsc = user_data;
+
+	l_debug("err=%i", err);
+
+	switch (err) {
+	case 0:
+		break;
+	case -ECANCELED:
+		dbus_pending_reply(&wsc->pending,
+					dbus_error_aborted(wsc->pending));
+		return;
+	case -ENOKEY:
+		dbus_pending_reply(&wsc->pending,
+					wsc_error_no_credentials(wsc->pending));
+		return;
+	case -EBUSY:
+		dbus_pending_reply(&wsc->pending,
+					dbus_error_busy(wsc->pending));
+		return;
+	default:
+		dbus_pending_reply(&wsc->pending,
+					dbus_error_failed(wsc->pending));
+		return;
+	}
+
+	wsc_store_credentials(wsc);
+	wsc_try_credentials(wsc);
 }
 
 static void station_state_watch(enum station_state state, void *userdata)
@@ -540,6 +586,22 @@ static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
 	wsc->target = target;
 	station_set_autoconnect(wsc->station, false);
 
+	if (!strcmp(l_dbus_message_get_member(wsc->pending), "StartPin")) {
+		char *pin;
+
+		wsc->config_method = WSC_CONFIGURATION_METHOD_KEYPAD;
+
+		if (!l_dbus_message_get_arguments(wsc->pending, "s", &pin))
+			goto error;
+
+		wsc->pin = l_strdup(pin);
+	} else
+		wsc->config_method =
+			WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON;
+
+	wsc->done_cb = wsc_dbus_done_cb;
+	wsc->done_data = wsc;
+
 	switch (station_get_state(wsc->station)) {
 	case STATION_STATE_DISCONNECTED:
 		wsc_connect(wsc);
@@ -563,7 +625,7 @@ static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
 		break;
 	}
 error:
-	wsc->target = NULL;
+	wsc_connect_cleanup(wsc);
 	dbus_pending_reply(&wsc->pending, dbus_error_failed(wsc->pending));
 }
 
@@ -1073,12 +1135,12 @@ static void wsc_free(void *userdata)
 	struct wsc *wsc = userdata;
 
 	wsc_cancel_scan(wsc);
+	wsc_connect_cleanup(wsc);
 
 	if (wsc->station_state_watch) {
 		station_remove_state_watch(wsc->station,
 						wsc->station_state_watch);
 		wsc->station_state_watch = 0;
-		wsc->target = NULL;
 	}
 
 	if (wsc->pending)
@@ -1089,9 +1151,6 @@ static void wsc_free(void *userdata)
 		dbus_pending_reply(&wsc->pending_cancel,
 				dbus_error_aborted(wsc->pending_cancel));
 
-	if (wsc->eap_settings)
-		l_settings_free(wsc->eap_settings);
-
 	l_free(wsc);
 }
 
diff --git a/src/wsc.h b/src/wsc.h
index b0e6f24a..ada08c89 100644
--- a/src/wsc.h
+++ b/src/wsc.h
@@ -30,3 +30,6 @@ struct wsc_credentials_info {
 	uint8_t addr[6];
 	bool has_passphrase;
 };
+
+typedef void (*wsc_done_cb_t)(int err, struct wsc_credentials_info *creds,
+				unsigned int n_creds, void *user_data);
-- 
2.20.1

[PATCH 15/16] wsc: Add wsc_new_p2p_enrollee

[Andrew Zaborowski]

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

Until now WSC could only be triggered by calling a DBus method and
success and error paths would end with a reply to the DBus method.
Instead use a wsc done callback pointer so that the WSC connection
logic can be triggered through DBus or a C API.

Export a function that adds a new entry point to start the WSC
connection for the P2P provisioning phase.  The function doesn't have
anything specific to P2P at this point.
---
 src/wsc.c | 31 +++++++++++++++++++++++++++++++
 src/wsc.h |  9 +++++++++
 2 files changed, 40 insertions(+)

diff --git a/src/wsc.c b/src/wsc.c
index 0c241ee3..6866571a 100644
--- a/src/wsc.c
+++ b/src/wsc.c
@@ -1197,6 +1197,37 @@ static void wsc_netdev_watch(struct netdev *netdev,
 	}
 }
 
+struct wsc *wsc_new_p2p_enrollee(struct netdev *netdev, struct scan_bss *target,
+					char *pin, wsc_done_cb_t done_cb,
+					void *user_data)
+{
+	struct wsc *wsc;
+
+	wsc = l_new(struct wsc, 1);
+	wsc->netdev = netdev;
+	wsc->target = target;
+	wsc->config_method = pin ? WSC_CONFIGURATION_METHOD_KEYPAD :
+		WSC_CONFIGURATION_METHOD_PUSH_BUTTON;
+	wsc->done_cb = done_cb;
+	wsc->done_data = user_data;
+	wsc->pin = l_strdup(pin);
+
+	wsc_connect(wsc);
+
+	/*
+	 * Wsc objects created this way are not handled in wsc_remove_interface,
+	 * the caller is expected to watch interfaces going away and there's no
+	 * need to handle the event in both places.
+	 */
+
+	return wsc;
+}
+
+void wsc_destroy(struct wsc *wsc)
+{
+	wsc_free(wsc);
+}
+
 static int wsc_init(void)
 {
 	l_debug("");
diff --git a/src/wsc.h b/src/wsc.h
index ada08c89..3995a647 100644
--- a/src/wsc.h
+++ b/src/wsc.h
@@ -20,6 +20,10 @@
  *
  */
 
+struct wsc;
+struct netdev;
+struct scan_bss;
+
 struct wsc_credentials_info {
 	char ssid[33];
 	enum security security;
@@ -33,3 +37,8 @@ struct wsc_credentials_info {
 
 typedef void (*wsc_done_cb_t)(int err, struct wsc_credentials_info *creds,
 				unsigned int n_creds, void *user_data);
+
+struct wsc *wsc_new_p2p_enrollee(struct netdev *netdev, struct scan_bss *target,
+					char *pin, wsc_done_cb_t done_cb,
+					void *user_data);
+void wsc_destroy(struct wsc *wsc);
-- 
2.20.1

[PATCH 16/16] wsc: Accept extra IEs in wsc_new_p2p_enrollee

[Andrew Zaborowski]

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

---
 src/wsc.c | 30 +++++++++++++++++-------------
 src/wsc.h |  6 ++++--
 2 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/src/wsc.c b/src/wsc.c
index 6866571a..c094ac88 100644
--- a/src/wsc.c
+++ b/src/wsc.c
@@ -448,7 +448,8 @@ static inline enum wsc_rf_band freq_to_rf_band(uint32_t freq)
 	return WSC_RF_BAND_2_4_GHZ;
 }
 
-static void wsc_connect(struct wsc *wsc)
+static void wsc_connect(struct wsc *wsc, struct iovec *ies,
+			unsigned int ies_num)
 {
 	struct handshake_state *hs;
 	struct l_settings *settings = l_settings_new();
@@ -457,7 +458,7 @@ static void wsc_connect(struct wsc *wsc)
 	struct wsc_association_request request;
 	uint8_t *pdu;
 	size_t pdu_len;
-	struct iovec ie_iov;
+	struct iovec ie_iov[1 + ies_num];
 
 	wsc->target = NULL;
 
@@ -503,18 +504,20 @@ static void wsc_connect(struct wsc *wsc)
 		goto error;
 	}
 
-	ie_iov.iov_base = ie_tlv_encapsulate_wsc_payload(pdu, pdu_len,
-							&ie_iov.iov_len);
+	ie_iov[0].iov_base = ie_tlv_encapsulate_wsc_payload(pdu, pdu_len,
+							&ie_iov[0].iov_len);
 	l_free(pdu);
 
-	if (!ie_iov.iov_base) {
+	if (!ie_iov[0].iov_base) {
 		r = -ENOMEM;
 		goto error;
 	}
 
-	r = netdev_connect(wsc->netdev, bss, hs, &ie_iov, 1, wsc_netdev_event,
-				wsc_connect_cb, wsc);
-	l_free(ie_iov.iov_base);
+	memcpy(ie_iov + 1, ies, sizeof(struct iovec) * ies_num);
+
+	r = netdev_connect(wsc->netdev, bss, hs, ie_iov, 1 + ies_num,
+				wsc_netdev_event, wsc_connect_cb, wsc);
+	l_free(ie_iov[0].iov_base);
 
 	if (r < 0)
 		goto error;
@@ -572,7 +575,7 @@ static void station_state_watch(enum station_state state, void *userdata)
 	station_remove_state_watch(wsc->station, wsc->station_state_watch);
 	wsc->station_state_watch = 0;
 
-	wsc_connect(wsc);
+	wsc_connect(wsc, NULL, 0);
 }
 
 static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
@@ -604,7 +607,7 @@ static void wsc_check_can_connect(struct wsc *wsc, struct scan_bss *target)
 
 	switch (station_get_state(wsc->station)) {
 	case STATION_STATE_DISCONNECTED:
-		wsc_connect(wsc);
+		wsc_connect(wsc, NULL, 0);
 		return;
 	case STATION_STATE_CONNECTING:
 	case STATION_STATE_CONNECTED:
@@ -1198,8 +1201,9 @@ static void wsc_netdev_watch(struct netdev *netdev,
 }
 
 struct wsc *wsc_new_p2p_enrollee(struct netdev *netdev, struct scan_bss *target,
-					char *pin, wsc_done_cb_t done_cb,
-					void *user_data)
+					char *pin,
+					struct iovec *ies, unsigned int ies_num,
+					wsc_done_cb_t done_cb, void *user_data)
 {
 	struct wsc *wsc;
 
@@ -1212,7 +1216,7 @@ struct wsc *wsc_new_p2p_enrollee(struct netdev *netdev, struct scan_bss *target,
 	wsc->done_data = user_data;
 	wsc->pin = l_strdup(pin);
 
-	wsc_connect(wsc);
+	wsc_connect(wsc, ies, ies_num);
 
 	/*
 	 * Wsc objects created this way are not handled in wsc_remove_interface,
diff --git a/src/wsc.h b/src/wsc.h
index 3995a647..4ae1c45c 100644
--- a/src/wsc.h
+++ b/src/wsc.h
@@ -23,6 +23,7 @@
 struct wsc;
 struct netdev;
 struct scan_bss;
+struct iovec;
 
 struct wsc_credentials_info {
 	char ssid[33];
@@ -39,6 +40,7 @@ typedef void (*wsc_done_cb_t)(int err, struct wsc_credentials_info *creds,
 				unsigned int n_creds, void *user_data);
 
 struct wsc *wsc_new_p2p_enrollee(struct netdev *netdev, struct scan_bss *target,
-					char *pin, wsc_done_cb_t done_cb,
-					void *user_data);
+					char *pin,
+					struct iovec *ies, unsigned int ies_num,
+					wsc_done_cb_t done_cb, void *user_data);
 void wsc_destroy(struct wsc *wsc);
-- 
2.20.1

Re: [PATCH 01/16] wiphy: Add wiphy_get_supported_rates

[Denis Kenzior]

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

Hi Andrew,

On 10/28/19 9:04 AM, Andrew Zaborowski wrote:
> Add code to parse the supported data rates info from the wiphy dumps and
> expose it for P2P's use with a getter function.
> ---
>   src/wiphy.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>   src/wiphy.h |  2 ++
>   2 files changed, 82 insertions(+), 4 deletions(-)
> 

Applied, thanks.

Regards,
-Denis

Re: [PATCH 02/16] scan: Hide CCK rates if no_cck_rates set

[Denis Kenzior]

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

Hi Andrew,

On 10/28/19 9:04 AM, Andrew Zaborowski wrote:
> no_cck_rates is set in the scan parameters generally to make sure
> that the Probe Request frames are not sent at any of the 802.11b
> rates during active scans.  With this patch we also omit those rates
> from the Supported Rates IEs, which is required by the p2p spec and
> also matches our flag's name.
> ---
>   src/scan.c | 35 ++++++++++++++++++++++++++++++++++-
>   1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/src/scan.c b/src/scan.c
> index c0c3177e..0ec7831c 100644
> --- a/src/scan.c
> +++ b/src/scan.c
> @@ -347,10 +347,43 @@ static struct l_genl_msg *scan_build_cmd(struct scan_context *sc,
>   	if (flags)
>   		l_genl_msg_append_attr(msg, NL80211_ATTR_SCAN_FLAGS, 4, &flags);
>   
> -	if (params->no_cck_rates)
> +	if (params->no_cck_rates) {
> +		static const uint8_t b_rates[] = { 2, 4, 11, 22 };
> +		uint8_t *scan_rates;
> +		const uint8_t *supported;
> +		unsigned int num_supported;
> +		unsigned int count;
> +
>   		l_genl_msg_append_attr(msg, NL80211_ATTR_TX_NO_CCK_RATE, 0,
>   					NULL);
>   
> +		/*
> +		 * Assume if we're sending the probe requests at OFDM bit
> +		 * rates we don't want to advertise support for 802.11b rates.
> +		 */
> +		supported = wiphy_get_supported_rates(sc->wiphy,
> +							NL80211_BAND_2GHZ,
> +							&num_supported);
> +		if (!supported) {
> +			L_WARN_ON(true);

So I will nag you not to do this, its just not very useful.  You really 
want the warning to convey as much info as possible.  Getting a message 
like "Warning: condition true failed" is pretty useless.

Also, L_WARN_ON can be used in an if statement, FYI.

> +			return msg;
> +		}
> +
> +		scan_rates = l_malloc(num_supported);
> +
> +		for (count = 0; *supported; supported++)
> +			if (!memchr(b_rates, *supported, L_ARRAY_SIZE(b_rates)))
> +				scan_rates[count++] = *supported;
> +
> +		L_WARN_ON(!count);
> +
> +		l_genl_msg_enter_nested(msg, NL80211_ATTR_SCAN_SUPP_RATES);
> +		l_genl_msg_append_attr(msg, NL80211_BAND_2GHZ, count,
> +					scan_rates);
> +		l_genl_msg_leave_nested(msg);

Why are we bothering to send an empty attribute?

> +		l_free(scan_rates);
> +	}
> +
>   	return msg;
>   }
>   
> 

Anyhow, I made some mods to this commit and pushed it out.  Please review.

Regards,
-Denis

Re: [PATCH 03/16] scan: Parse P2P IEs according to frame type

[Denis Kenzior]

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

Hi Andrew,

On 10/28/19 9:04 AM, Andrew Zaborowski wrote:
> Save the source frame type in struct scan_bss as it may affect how some
> of the data in the struct should be parsed.  Also replace the P2P IE
> payload data in that struct with a union containing pre-parsed p2p
> attributes corresponding to the frame type.
> 
> This means users don't have to call the parsers in p2putil.c on that
> data, which wouldn't have worked anyway because we those parsers were

This sentence doesn't parse

> assuming the raw concatenated P2P IEs as a parameter instead of the
> payload extracted with ie_tlv_extract_p2p_payload.
> ---
>   src/scan.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++-------
>   src/scan.h | 17 ++++++++++--
>   2 files changed, 84 insertions(+), 12 deletions(-)
> 
> diff --git a/src/scan.c b/src/scan.c
> index 0ec7831c..0956c6f6 100644
> --- a/src/scan.c
> +++ b/src/scan.c
> @@ -45,6 +45,7 @@
>   #include "src/nl80211cmd.h"
>   #include "src/nl80211util.h"
>   #include "src/util.h"
> +#include "src/p2putil.h"
>   #include "src/scan.h"
>   
>   #define SCAN_MAX_INTERVAL 320
> @@ -987,6 +988,38 @@ static bool scan_parse_bss_information_elements(struct scan_bss *bss,
>   		}
>   	}
>   
> +	bss->wsc = ie_tlv_extract_wsc_payload(data, len, &bss->wsc_size);
> +
> +	switch (bss->source_frame) {
> +	case SCAN_BSS_PROBE_RESP:
> +	{
> +		struct p2p_probe_resp info;
> +
> +		if (p2p_parse_probe_resp(data, len, &info) == 0)
> +			bss->p2p_probe_resp_info = l_memdup(&info, sizeof(info));

So I don't like this l_memdup at all.  There has to be a better way.

Also, how is it that you're passing data and len directly to 
p2p_parse_probe_resp?  Are these not wrapped in IEs somehow?

> +
> +		break;
> +	}
> +	case SCAN_BSS_PROBE_REQ:

How do you even get here? And why should scan.c even care?  I think 
you're hacking stuff together here.  I doubt this belongs on the scan 
API at all.

> +	{
> +		struct p2p_probe_req info;
> +
> +		if (p2p_parse_probe_req(data, len, &info) == 0)
> +			bss->p2p_probe_req_info = l_memdup(&info, sizeof(info));
> +
> +		break;
> +	}
> +	case SCAN_BSS_BEACON:
> +	{
> +		struct p2p_beacon info;
> +
> +		if (p2p_parse_beacon(data, len, &info) == 0)
> +			bss->p2p_beacon_info = l_memdup(&info, sizeof(info));
> +
> +		break;
> +	}
> +	}
> +
>   	return have_ssid;
>   }
>   
> @@ -995,9 +1028,12 @@ static struct scan_bss *scan_parse_attr_bss(struct l_genl_attr *attr)
>   	uint16_t type, len;
>   	const void *data;
>   	struct scan_bss *bss;
> +	const uint8_t *ies = NULL;
> +	size_t ies_len;
>   
>   	bss = l_new(struct scan_bss, 1);
>   	bss->utilization = 127;
> +	bss->source_frame = SCAN_BSS_BEACON;

So I don't think it is that simple.  Looks like the kernel guys really 
made a mess here.  We need to be looking at NL80211_BSS_BEACON_IES as 
well and applying 'heuristics' if the PRESP_DATA attribute isn't present.

>   
>   	while (l_genl_attr_next(attr, &type, &len, &data)) {
>   		switch (type) {
> @@ -1025,20 +1061,19 @@ static struct scan_bss *scan_parse_attr_bss(struct l_genl_attr *attr)
>   
>   			bss->signal_strength = *((int32_t *) data);
>   			break;
> +		case NL80211_BSS_PRESP_DATA:
> +			bss->source_frame = SCAN_BSS_PROBE_RESP;
> +			break;
>   		case NL80211_BSS_INFORMATION_ELEMENTS:
> -			if (!scan_parse_bss_information_elements(bss,
> -								data, len))
> -				goto fail;
> -
> -			bss->wsc = ie_tlv_extract_wsc_payload(data, len,
> -								&bss->wsc_size);
> -			bss->p2p = ie_tlv_extract_p2p_payload(data, len,
> -								&bss->p2p_size);
> -
> +			ies = data;
> +			ies_len = len;
>   			break;
>   		}
>   	}
>   
> +	if (ies && !scan_parse_bss_information_elements(bss, ies, ies_len))
> +		goto fail;
> +
>   	return bss;
>   
>   fail:
> @@ -1198,9 +1233,33 @@ void scan_bss_free(struct scan_bss *bss)
>   	l_free(bss->rsne);
>   	l_free(bss->wpa);
>   	l_free(bss->wsc);
> -	l_free(bss->p2p);
>   	l_free(bss->osen);
>   	l_free(bss->rc_ie);
> +
> +	switch (bss->source_frame) {
> +	case SCAN_BSS_PROBE_RESP:
> +		if (!bss->p2p_probe_resp_info)
> +			break;
> +
> +		p2p_free_probe_resp(bss->p2p_probe_resp_info);
> +		l_free(bss->p2p_probe_resp_info);
> +		break;
> +	case SCAN_BSS_PROBE_REQ:
> +		if (!bss->p2p_probe_req_info)
> +			break;
> +
> +		p2p_free_probe_req(bss->p2p_probe_req_info);
> +		l_free(bss->p2p_probe_req_info);

The naming needs some work.  Whenever I see _free in the name I assume 
it calls l_free on the passed in structure.  But here you have to free 
the structure separately.  And the _free should really be last per our 
naming conventions.

> +		break;
> +	case SCAN_BSS_BEACON:
> +		if (!bss->p2p_beacon_info)
> +			break;
> +
> +		p2p_free_beacon(bss->p2p_beacon_info);
> +		l_free(bss->p2p_beacon_info);
> +		break;
> +	}
> +
>   	l_free(bss);
>   }
>   
> diff --git a/src/scan.h b/src/scan.h
> index 626de80b..afed831e 100644
> --- a/src/scan.h
> +++ b/src/scan.h
> @@ -40,6 +40,15 @@ typedef void (*scan_freq_set_func_t)(uint32_t freq, void *userdata);
>   
>   struct scan_freq_set;
>   struct ie_rsn_info;
> +struct p2p_probe_resp;
> +struct p2p_probe_req;
> +struct p2p_beacon;
> +
> +enum scan_bss_frame_type {
> +	SCAN_BSS_PROBE_RESP,
> +	SCAN_BSS_PROBE_REQ,
> +	SCAN_BSS_BEACON,
> +};
>   
>   struct scan_bss {
>   	uint8_t addr[6];
> @@ -51,8 +60,12 @@ struct scan_bss {
>   	uint8_t *osen;
>   	uint8_t *wsc;		/* Concatenated WSC IEs */
>   	ssize_t wsc_size;	/* Size of Concatenated WSC IEs */
> -	uint8_t *p2p;		/* Concatenated P2P IEs */
> -	ssize_t p2p_size;	/* Size of Concatenated P2P IEs */
> +	enum scan_bss_frame_type source_frame;
> +	union {
> +		struct p2p_probe_resp *p2p_probe_resp_info;
> +		struct p2p_probe_req *p2p_probe_req_info;
> +		struct p2p_beacon *p2p_beacon_info;
> +	};

a union of 3 pointers is rather pointless, no? :)

>   	uint8_t mde[3];
>   	uint8_t ssid[32];
>   	uint8_t ssid_len;
> 

Regards,
-Denis

Re: [PATCH 05/16] handshake: Convert handshake event callbacks variadic functions

[Denis Kenzior]

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

Hi Andrew,

On 10/28/19 9:04 AM, Andrew Zaborowski wrote:
> Convert the handshake event callback type to use variable argument
> list to allow for more flexibility in event-specific arguments
> passed to the callbacks.
> ---
>   src/adhoc.c     |  2 +-
>   src/ap.c        |  9 +++++++--
>   src/handshake.c |  7 -------
>   src/handshake.h | 13 +++++++++----
>   src/station.c   |  9 +++++++--
>   src/wsc.c       | 11 +++++++++--
>   6 files changed, 33 insertions(+), 18 deletions(-)
> 
> diff --git a/src/adhoc.c b/src/adhoc.c
> index 00f3a7e3..755e9493 100644
> --- a/src/adhoc.c
> +++ b/src/adhoc.c
> @@ -159,7 +159,7 @@ static bool ap_sta_match_addr(const void *a, const void *b)
>   }
>   
>   static void adhoc_handshake_event(struct handshake_state *hs,
> -		enum handshake_event event, void *event_data, void *user_data)
> +		enum handshake_event event, void *user_data, ...)
>   {
>   	struct sta_state *sta = user_data;
>   	struct adhoc_state *adhoc = sta->adhoc;
> diff --git a/src/ap.c b/src/ap.c
> index b06b6691..cacfa807 100644
> --- a/src/ap.c
> +++ b/src/ap.c
> @@ -382,16 +382,19 @@ static uint32_t ap_send_mgmt_frame(struct ap_state *ap,
>   }
>   
>   static void ap_handshake_event(struct handshake_state *hs,
> -		enum handshake_event event, void *event_data, void *user_data)
> +		enum handshake_event event, void *user_data, ...)
>   {
>   	struct sta_state *sta = user_data;
> +	va_list args;
> +
> +	va_start(args, user_data);
>   
>   	switch (event) {
>   	case HANDSHAKE_EVENT_COMPLETE:
>   		ap_new_rsna(sta);
>   		break;
>   	case HANDSHAKE_EVENT_FAILED:
> -		netdev_handshake_failed(hs, l_get_u16(event_data));
> +		netdev_handshake_failed(hs, l_get_u16(va_arg(args, void *)));

So since you make the caller extract arguments via va_arg, I wonder if 
it would be cleaner to pass unsigned/int to va_arg directly instead of 
using void pointers?

So do something like handshake_event(hs,
HANDSHAKE_EVENT_FAILED, reason_code);

>   		/* fall through */
>   	case HANDSHAKE_EVENT_SETTING_KEYS_FAILED:
>   		sta->sm = NULL;
> @@ -399,6 +402,8 @@ static void ap_handshake_event(struct handshake_state *hs,
>   	default:
>   		break;
>   	}
> +
> +	va_end(args);
>   }
>   
>   static void ap_start_rsna(struct sta_state *sta, const uint8_t *gtk_rsc)
> diff --git a/src/handshake.c b/src/handshake.c
> index 8e1aeec9..a3e236bb 100644
> --- a/src/handshake.c
> +++ b/src/handshake.c
> @@ -912,10 +912,3 @@ bool handshake_decode_fte_key(struct handshake_state *s, const uint8_t *wrapped,
>   
>   	return true;
>   }
> -
> -void handshake_event(struct handshake_state *hs,
> -			enum handshake_event event, void *event_data)
> -{
> -	if (hs->event_func)
> -		hs->event_func(hs, event, event_data, hs->user_data);
> -}
> diff --git a/src/handshake.h b/src/handshake.h
> index 08503190..cd0c1dc9 100644
> --- a/src/handshake.h
> +++ b/src/handshake.h
> @@ -54,7 +54,7 @@ enum handshake_event {
>   
>   typedef void (*handshake_event_func_t)(struct handshake_state *hs,
>   					enum handshake_event event,
> -					void *event_data, void *user_data);
> +					void *user_data, ...);
>   
>   typedef bool (*handshake_get_nonce_func_t)(uint8_t nonce[]);
>   typedef void (*handshake_install_tk_func_t)(struct handshake_state *hs,
> @@ -130,6 +130,14 @@ struct handshake_state {
>   	handshake_event_func_t event_func;
>   };
>   
> +#define handshake_event(hs, event, ...)	\
> +	do {	\
> +		if (!(hs)->event_func)	\
> +			break;	\
> +	\
> +		(hs)->event_func(hs, event, (hs)->user_data, ##__VA_ARGS__); \
second hs is not in ()s here?
> +	} while (0)
> +
>   void handshake_state_free(struct handshake_state *s);
>   
>   void handshake_state_set_supplicant_address(struct handshake_state *s,

Regards,
-Denis

Re: [PATCH 06/16] handshake: Convert HANDSHAKE_EVENT_FAILED data to int

[Denis Kenzior]

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

On 10/28/19 9:04 AM, Andrew Zaborowski wrote:
> Note the uint16_t reason code is promoted to an int when using variable
> arguments so va_arg(args, int) has to be used.
> ---
>   src/ap.c      | 2 +-
>   src/eapol.c   | 2 +-
>   src/station.c | 2 +-
>   src/wsc.c     | 2 +-
>   4 files changed, 4 insertions(+), 4 deletions(-)
> 

Ah, you did this part as a separate patch.  Should have looked again.

I squashed patchees 5, 6, and 7 together and applied.

Regards,
-Denis

Re: [PATCH 08/16] eapol: Move the EAP events to handshake event handler

[Denis Kenzior]

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

Hi Andrew,

On 10/28/19 9:05 AM, Andrew Zaborowski wrote:
> On EAP events, call the handshake_event handler with the new event type
> HANDSHAKE_EVENT_EAP_NOTIFY isntead of the eapol_event callback.
> 
> This allows the handler to be set before calling
> netdev_connect/netdev_connect_wsc.  It's also in theory more type-safe
> because we don't need the cast in netdev_connect_wsc anymore.
> ---
>   src/eapol.c     |  6 ++----
>   src/handshake.h |  1 +
>   src/station.c   |  1 +
>   src/wsc.c       | 27 +++++++++++++++++----------
>   4 files changed, 21 insertions(+), 14 deletions(-)
> 

Patch 8 and 9 applied, thanks.

Regards,
-Denis

Re: [PATCH 11/16] eapol: Drop unused eapol_sm_set_event_func

[Denis Kenzior]

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

Hi Andrew,

On 10/28/19 9:05 AM, Andrew Zaborowski wrote:
> ---
>   src/eapol.c  | 6 ------
>   src/eapol.h  | 1 -
>   src/netdev.c | 2 --
>   3 files changed, 9 deletions(-)
> 

Applied, thanks.

Regards,
-Denis

Re: [PATCH 10/16] wsc: Replace netdev_connect_wsc with netdev_connect usage

[Denis Kenzior]

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

Hi Andrew,

On 10/28/19 9:05 AM, Andrew Zaborowski wrote:
> netdev_connect can achieve the same effect as netdev_connect_wsc but is
> more flexible as it allows us to supply additional association IEs.  We
> will need this capability to make P2P connections.  This way we're also
> moving the WSC-specific bits to wsc.c from the crowded netdev.c.
> ---
>   src/netdev.c |  2 +-
>   src/wsc.c    | 46 +++++++++++++++++++++++++++++++++-------------
>   2 files changed, 34 insertions(+), 14 deletions(-)
> 

Patch 10 & 12 applied, thanks.

Regards,
-Denis

Re: [PATCH 02/16] scan: Hide CCK rates if no_cck_rates set

[Andrew Zaborowski]

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

Hi Denis,

On Wed, 30 Oct 2019 at 17:17, Denis Kenzior <denkenz@gmail.com> wrote:
> On 10/28/19 9:04 AM, Andrew Zaborowski wrote:
> > no_cck_rates is set in the scan parameters generally to make sure
> > that the Probe Request frames are not sent at any of the 802.11b
> > rates during active scans.  With this patch we also omit those rates
> > from the Supported Rates IEs, which is required by the p2p spec and
> > also matches our flag's name.
> > ---
> >   src/scan.c | 35 ++++++++++++++++++++++++++++++++++-
> >   1 file changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/scan.c b/src/scan.c
> > index c0c3177e..0ec7831c 100644
> > --- a/src/scan.c
> > +++ b/src/scan.c
> > @@ -347,10 +347,43 @@ static struct l_genl_msg *scan_build_cmd(struct scan_context *sc,
> >       if (flags)
> >               l_genl_msg_append_attr(msg, NL80211_ATTR_SCAN_FLAGS, 4, &flags);
> >
> > -     if (params->no_cck_rates)
> > +     if (params->no_cck_rates) {
> > +             static const uint8_t b_rates[] = { 2, 4, 11, 22 };
> > +             uint8_t *scan_rates;
> > +             const uint8_t *supported;
> > +             unsigned int num_supported;
> > +             unsigned int count;
> > +
> >               l_genl_msg_append_attr(msg, NL80211_ATTR_TX_NO_CCK_RATE, 0,
> >                                       NULL);
> >
> > +             /*
> > +              * Assume if we're sending the probe requests at OFDM bit
> > +              * rates we don't want to advertise support for 802.11b rates.
> > +              */
> > +             supported = wiphy_get_supported_rates(sc->wiphy,
> > +                                                     NL80211_BAND_2GHZ,
> > +                                                     &num_supported);
> > +             if (!supported) {
> > +                     L_WARN_ON(true);
>
> So I will nag you not to do this, its just not very useful.  You really
> want the warning to convey as much info as possible.  Getting a message
> like "Warning: condition true failed" is pretty useless.
>
> Also, L_WARN_ON can be used in an if statement, FYI.

Ok, that should also work.  In the end you'll need to look at the
stack trace though.

>
> > +                     return msg;
> > +             }
> > +
> > +             scan_rates = l_malloc(num_supported);
> > +
> > +             for (count = 0; *supported; supported++)
> > +                     if (!memchr(b_rates, *supported, L_ARRAY_SIZE(b_rates)))
> > +                             scan_rates[count++] = *supported;
> > +
> > +             L_WARN_ON(!count);
> > +
> > +             l_genl_msg_enter_nested(msg, NL80211_ATTR_SCAN_SUPP_RATES);
> > +             l_genl_msg_append_attr(msg, NL80211_BAND_2GHZ, count,
> > +                                     scan_rates);
> > +             l_genl_msg_leave_nested(msg);
>
> Why are we bothering to send an empty attribute?

We aren't except if something's broken in our code.

>
> > +             l_free(scan_rates);
> > +     }
> > +
> >       return msg;
> >   }
> >
> >
>
> Anyhow, I made some mods to this commit and pushed it out.  Please review.

Thanks, this new version should also work.

Best regards

Re: [PATCH 03/16] scan: Parse P2P IEs according to frame type

[Andrew Zaborowski]

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

On Wed, 30 Oct 2019 at 17:52, Denis Kenzior <denkenz@gmail.com> wrote:
> On 10/28/19 9:04 AM, Andrew Zaborowski wrote:
> > Save the source frame type in struct scan_bss as it may affect how some
> > of the data in the struct should be parsed.  Also replace the P2P IE
> > payload data in that struct with a union containing pre-parsed p2p
> > attributes corresponding to the frame type.
> >
> > This means users don't have to call the parsers in p2putil.c on that
> > data, which wouldn't have worked anyway because we those parsers were
>
> This sentence doesn't parse

Oops, will fix.

>
> > assuming the raw concatenated P2P IEs as a parameter instead of the
> > payload extracted with ie_tlv_extract_p2p_payload.
> > ---
> >   src/scan.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++-------
> >   src/scan.h | 17 ++++++++++--
> >   2 files changed, 84 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/scan.c b/src/scan.c
> > index 0ec7831c..0956c6f6 100644
> > --- a/src/scan.c
> > +++ b/src/scan.c
> > @@ -45,6 +45,7 @@
> >   #include "src/nl80211cmd.h"
> >   #include "src/nl80211util.h"
> >   #include "src/util.h"
> > +#include "src/p2putil.h"
> >   #include "src/scan.h"
> >
> >   #define SCAN_MAX_INTERVAL 320
> > @@ -987,6 +988,38 @@ static bool scan_parse_bss_information_elements(struct scan_bss *bss,
> >               }
> >       }
> >
> > +     bss->wsc = ie_tlv_extract_wsc_payload(data, len, &bss->wsc_size);
> > +
> > +     switch (bss->source_frame) {
> > +     case SCAN_BSS_PROBE_RESP:
> > +     {
> > +             struct p2p_probe_resp info;
> > +
> > +             if (p2p_parse_probe_resp(data, len, &info) == 0)
> > +                     bss->p2p_probe_resp_info = l_memdup(&info, sizeof(info));
>
> So I don't like this l_memdup at all.  There has to be a better way.

The other way is to alloc memory at bss->p2p_probe_resp_info and
l_free it on error, let me do that instead.

>
> Also, how is it that you're passing data and len directly to
> p2p_parse_probe_resp?  Are these not wrapped in IEs somehow?

Yes, but the p2p_parse_* functions expect that wrapped data actually,
and will unwrap internally.

>
> > +
> > +             break;
> > +     }
> > +     case SCAN_BSS_PROBE_REQ:
>
> How do you even get here? And why should scan.c even care?  I think
> you're hacking stuff together here.  I doubt this belongs on the scan
> API at all.

We get here through the scan_bss_new_from_probe_req call I submitted
before.  This is called from outside of scan.c, in p2p.c, so you could
argue it doesn't belong here but actually what p2p.c is doing is
technically scanning, too.

There's also so much common code that it would just be annoying to not
re-use it... or to have to split it into a separate common file.

>
> > +     {
> > +             struct p2p_probe_req info;
> > +
> > +             if (p2p_parse_probe_req(data, len, &info) == 0)
> > +                     bss->p2p_probe_req_info = l_memdup(&info, sizeof(info));
> > +
> > +             break;
> > +     }
> > +     case SCAN_BSS_BEACON:
> > +     {
> > +             struct p2p_beacon info;
> > +
> > +             if (p2p_parse_beacon(data, len, &info) == 0)
> > +                     bss->p2p_beacon_info = l_memdup(&info, sizeof(info));
> > +
> > +             break;
> > +     }
> > +     }
> > +
> >       return have_ssid;
> >   }
> >
> > @@ -995,9 +1028,12 @@ static struct scan_bss *scan_parse_attr_bss(struct l_genl_attr *attr)
> >       uint16_t type, len;
> >       const void *data;
> >       struct scan_bss *bss;
> > +     const uint8_t *ies = NULL;
> > +     size_t ies_len;
> >
> >       bss = l_new(struct scan_bss, 1);
> >       bss->utilization = 127;
> > +     bss->source_frame = SCAN_BSS_BEACON;
>
> So I don't think it is that simple.  Looks like the kernel guys really
> made a mess here.  We need to be looking at NL80211_BSS_BEACON_IES as
> well and applying 'heuristics' if the PRESP_DATA attribute isn't present.

I'll try to make sure I understand exactly how this works and add an
explanation in a comment somewhere.

>
> >
> >       while (l_genl_attr_next(attr, &type, &len, &data)) {
> >               switch (type) {
> > @@ -1025,20 +1061,19 @@ static struct scan_bss *scan_parse_attr_bss(struct l_genl_attr *attr)
> >
> >                       bss->signal_strength = *((int32_t *) data);
> >                       break;
> > +             case NL80211_BSS_PRESP_DATA:
> > +                     bss->source_frame = SCAN_BSS_PROBE_RESP;
> > +                     break;
> >               case NL80211_BSS_INFORMATION_ELEMENTS:
> > -                     if (!scan_parse_bss_information_elements(bss,
> > -                                                             data, len))
> > -                             goto fail;
> > -
> > -                     bss->wsc = ie_tlv_extract_wsc_payload(data, len,
> > -                                                             &bss->wsc_size);
> > -                     bss->p2p = ie_tlv_extract_p2p_payload(data, len,
> > -                                                             &bss->p2p_size);
> > -
> > +                     ies = data;
> > +                     ies_len = len;
> >                       break;
> >               }
> >       }
> >
> > +     if (ies && !scan_parse_bss_information_elements(bss, ies, ies_len))
> > +             goto fail;
> > +
> >       return bss;
> >
> >   fail:
> > @@ -1198,9 +1233,33 @@ void scan_bss_free(struct scan_bss *bss)
> >       l_free(bss->rsne);
> >       l_free(bss->wpa);
> >       l_free(bss->wsc);
> > -     l_free(bss->p2p);
> >       l_free(bss->osen);
> >       l_free(bss->rc_ie);
> > +
> > +     switch (bss->source_frame) {
> > +     case SCAN_BSS_PROBE_RESP:
> > +             if (!bss->p2p_probe_resp_info)
> > +                     break;
> > +
> > +             p2p_free_probe_resp(bss->p2p_probe_resp_info);
> > +             l_free(bss->p2p_probe_resp_info);
> > +             break;
> > +     case SCAN_BSS_PROBE_REQ:
> > +             if (!bss->p2p_probe_req_info)
> > +                     break;
> > +
> > +             p2p_free_probe_req(bss->p2p_probe_req_info);
> > +             l_free(bss->p2p_probe_req_info);
>
> The naming needs some work.  Whenever I see _free in the name I assume
> it calls l_free on the passed in structure.  But here you have to free
> the structure separately.  And the _free should really be last per our
> naming conventions.

How about _clear_?  I'd rather keep it in the middle of the name to be
consistent with the _parse_ and _build_ function names, but I'll move
it to the end if you really prefer.

>
> > +             break;
> > +     case SCAN_BSS_BEACON:
> > +             if (!bss->p2p_beacon_info)
> > +                     break;
> > +
> > +             p2p_free_beacon(bss->p2p_beacon_info);
> > +             l_free(bss->p2p_beacon_info);
> > +             break;
> > +     }
> > +
> >       l_free(bss);
> >   }
> >
> > diff --git a/src/scan.h b/src/scan.h
> > index 626de80b..afed831e 100644
> > --- a/src/scan.h
> > +++ b/src/scan.h
> > @@ -40,6 +40,15 @@ typedef void (*scan_freq_set_func_t)(uint32_t freq, void *userdata);
> >
> >   struct scan_freq_set;
> >   struct ie_rsn_info;
> > +struct p2p_probe_resp;
> > +struct p2p_probe_req;
> > +struct p2p_beacon;
> > +
> > +enum scan_bss_frame_type {
> > +     SCAN_BSS_PROBE_RESP,
> > +     SCAN_BSS_PROBE_REQ,
> > +     SCAN_BSS_BEACON,
> > +};
> >
> >   struct scan_bss {
> >       uint8_t addr[6];
> > @@ -51,8 +60,12 @@ struct scan_bss {
> >       uint8_t *osen;
> >       uint8_t *wsc;           /* Concatenated WSC IEs */
> >       ssize_t wsc_size;       /* Size of Concatenated WSC IEs */
> > -     uint8_t *p2p;           /* Concatenated P2P IEs */
> > -     ssize_t p2p_size;       /* Size of Concatenated P2P IEs */
> > +     enum scan_bss_frame_type source_frame;
> > +     union {
> > +             struct p2p_probe_resp *p2p_probe_resp_info;
> > +             struct p2p_probe_req *p2p_probe_req_info;
> > +             struct p2p_beacon *p2p_beacon_info;
> > +     };
>
> a union of 3 pointers is rather pointless, no? :)

Only if you think non-void pointers are pointless ;)  But, we could do
void * if you want.

Best regards

Re: [PATCH 05/16] handshake: Convert handshake event callbacks variadic functions

[Andrew Zaborowski]

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

On Wed, 30 Oct 2019 at 20:16, Denis Kenzior <denkenz@gmail.com> wrote:
> On 10/28/19 9:04 AM, Andrew Zaborowski wrote:
> >       case HANDSHAKE_EVENT_FAILED:
> > -             netdev_handshake_failed(hs, l_get_u16(event_data));
> > +             netdev_handshake_failed(hs, l_get_u16(va_arg(args, void *)));
>
> So since you make the caller extract arguments via va_arg, I wonder if
> it would be cleaner to pass unsigned/int to va_arg directly instead of
> using void pointers?
>
> So do something like handshake_event(hs,
> HANDSHAKE_EVENT_FAILED, reason_code);

Done in the other patch ;)

>
> >               /* fall through */
> >       case HANDSHAKE_EVENT_SETTING_KEYS_FAILED:
> >               sta->sm = NULL;
> > @@ -399,6 +402,8 @@ static void ap_handshake_event(struct handshake_state *hs,
> >       default:
> >               break;
> >       }
> > +
> > +     va_end(args);
> >   }
> >
> >   static void ap_start_rsna(struct sta_state *sta, const uint8_t *gtk_rsc)
> > diff --git a/src/handshake.c b/src/handshake.c
> > index 8e1aeec9..a3e236bb 100644
> > --- a/src/handshake.c
> > +++ b/src/handshake.c
> > @@ -912,10 +912,3 @@ bool handshake_decode_fte_key(struct handshake_state *s, const uint8_t *wrapped,
> >
> >       return true;
> >   }
> > -
> > -void handshake_event(struct handshake_state *hs,
> > -                     enum handshake_event event, void *event_data)
> > -{
> > -     if (hs->event_func)
> > -             hs->event_func(hs, event, event_data, hs->user_data);
> > -}
> > diff --git a/src/handshake.h b/src/handshake.h
> > index 08503190..cd0c1dc9 100644
> > --- a/src/handshake.h
> > +++ b/src/handshake.h
> > @@ -54,7 +54,7 @@ enum handshake_event {
> >
> >   typedef void (*handshake_event_func_t)(struct handshake_state *hs,
> >                                       enum handshake_event event,
> > -                                     void *event_data, void *user_data);
> > +                                     void *user_data, ...);
> >
> >   typedef bool (*handshake_get_nonce_func_t)(uint8_t nonce[]);
> >   typedef void (*handshake_install_tk_func_t)(struct handshake_state *hs,
> > @@ -130,6 +130,14 @@ struct handshake_state {
> >       handshake_event_func_t event_func;
> >   };
> >
> > +#define handshake_event(hs, event, ...)      \
> > +     do {    \
> > +             if (!(hs)->event_func)  \
> > +                     break;  \
> > +     \
> > +             (hs)->event_func(hs, event, (hs)->user_data, ##__VA_ARGS__); \
> second hs is not in ()s here?

Since it's surrounded by a ( and a , can this ever be a problem?  For
the record event isn't surrounded by ()s and the event-specific
arguments aren't either.

This reminds me I was gonna comment on that I intended to make sure
each argument is evaluated exactly once when using the handshake_event
macro, same as if it was a C function.  But there was no way to do
this for the event-specific arguments so I gave up and left the
responsibilty for this to the few callers, this may need a comment in
the code though.

Best regards

Re: [PATCH 14/16] wsc: Refactor to separate DBus-specific code

[Denis Kenzior]

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

Hi Andrew,

On 10/28/19 9:05 AM, Andrew Zaborowski wrote:
> Split the DBus-specific logic out of the core WSC logic.  The core
> WSC code is the part that we can re-use for P2P and doesn't include
> the scanning for the target BSS or the attempt to make a station mode
> connection.
> ---
>   src/wsc.c | 141 ++++++++++++++++++++++++++++++++++++++----------------
>   src/wsc.h |   3 ++
>   2 files changed, 103 insertions(+), 41 deletions(-)
> 

<snip>

> @@ -451,30 +470,24 @@ static void wsc_connect(struct wsc *wsc)
>   	l_settings_set_uint(settings, "WSC", "RFBand",
>   					freq_to_rf_band(bss->frequency));
>   	l_settings_set_uint(settings, "WSC", "ConfigurationMethods",
> -				WSC_CONFIGURATION_METHOD_VIRTUAL_DISPLAY_PIN |
> -				WSC_CONFIGURATION_METHOD_VIRTUAL_PUSH_BUTTON |
> -				WSC_CONFIGURATION_METHOD_KEYPAD);
> +					wsc->config_method);

So this is somewhat non-obvious that you're messing with the advertised 
config method in this patch.  Just setting METHOD_KEYPAD (and using that 
as a selector) is not flexible enough and most likely against what the 
WSC spec recommends anyway.  Can't we explicitly track what 'high-level' 
method type we're using? i.e. PIN vs PushButton?

Otherwise, I'm mostly okay with this patch.

One thing to think about though is to make wsc an explicit state 
machine.  Right now wsc only registers to the station ifindex and thus 
only exists if the station interface exists.

How are you dealing with the fact that you need to run P2P transactions 
on the P2P_CLIENT interface and what happens if there's no station 
iftype registered at all?

Regards,
-Denis