[PATCH 01/17] ap: Fix iteration over ap->rates

[PATCH 01/17] ap: Fix iteration over ap->rates

[Andrew Zaborowski]

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

In the for loops use a condition that works even if the uint32_t values
overflow since l_uintset_find_min/max() return UINT_MAX for empty sets.
If the condition is r <= l_uintset_find_max() or similar, it's always
true.

Make sure ap->rates is non-NULL both with and without no_cck_rates.

Update "count" in the for loop in ap_build_beacon_pr_head()
---
 src/ap.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index 1cee185c..84d4539a 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -371,7 +371,7 @@ static size_t ap_build_beacon_pr_head(struct ap_state *ap,
 	minr = l_uintset_find_min(ap->rates);
 	maxr = l_uintset_find_max(ap->rates);
 	count = 0;
-	for (r = minr; r <= maxr && count < 8; r++)
+	for (r = minr; (int32_t) (maxr - r) >= 0 && count < 8; r++)
 		if (l_uintset_contains(ap->rates, r)) {
 			uint8_t flag = 0;
 
@@ -380,10 +380,10 @@ static size_t ap_build_beacon_pr_head(struct ap_state *ap,
 				flag = 0x80;
 
 			*rates++ = r | flag;
+			count++;
 		}
 
-	ie_tlv_builder_set_length(&builder, rates -
-					ie_tlv_builder_get_data(&builder));
+	ie_tlv_builder_set_length(&builder, count);
 
 	/* DSSS Parameter Set IE for DSSS, HR, ERP and HT PHY rates */
 	ie_tlv_builder_next(&builder, IE_TYPE_DSSS_PARAMETER_SET);
@@ -983,7 +983,7 @@ static void ap_associate_sta(struct ap_state *ap, struct sta_state *sta)
 	minr = l_uintset_find_min(sta->rates);
 	maxr = l_uintset_find_max(sta->rates);
 
-	for (r = minr; r <= maxr; r++)
+	for (r = minr; (int32_t) (maxr - r) >= 0; r++)
 		if (l_uintset_contains(sta->rates, r))
 			rates[count++] = r;
 
@@ -1089,7 +1089,7 @@ static uint32_t ap_assoc_resp(struct ap_state *ap, struct sta_state *sta,
 	minr = l_uintset_find_min(ap->rates);
 	maxr = l_uintset_find_max(ap->rates);
 	count = 0;
-	for (r = minr; r <= maxr && count < 8; r++)
+	for (r = minr; (int32_t) (maxr - r) >= 0 && count < 8; r++)
 		if (l_uintset_contains(ap->rates, r)) {
 			uint8_t flag = 0;
 
@@ -2071,6 +2071,7 @@ struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
 	ap->ciphers = wiphy_select_cipher(wiphy, 0xffff);
 	ap->group_cipher = wiphy_select_cipher(wiphy, 0xffff);
 	ap->beacon_interval = 100;
+	ap->rates = l_uintset_new(200);
 
 	/* TODO: Pick from actual supported rates */
 	if (config->no_cck_rates) {
@@ -2083,7 +2084,6 @@ struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
 		l_uintset_put(ap->rates, 96); /* 48 Mbps*/
 		l_uintset_put(ap->rates, 108); /* 54 Mbps*/
 	} else {
-		ap->rates = l_uintset_new(200);
 		l_uintset_put(ap->rates, 2); /* 1 Mbps*/
 		l_uintset_put(ap->rates, 11); /* 5.5 Mbps*/
 		l_uintset_put(ap->rates, 22); /* 11 Mbps*/
-- 
2.25.1

[PATCH 02/17] scan: Add optional source_mac scan parameter

[Andrew Zaborowski]

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

This is similar to randomize_mac_addr_hint but it sets a specific source
MAC address for our probe frames.
---
 src/scan.c | 12 ++++++++++++
 src/scan.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/src/scan.c b/src/scan.c
index 889ff3a8..b4135d7c 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -361,6 +361,18 @@ static struct l_genl_msg *scan_build_cmd(struct scan_context *sc,
 		 */
 		flags |= NL80211_SCAN_FLAG_RANDOM_ADDR;
 
+	if (!is_passive && params->source_mac &&
+			wiphy_can_randomize_mac_addr(sc->wiphy)) {
+		static const uint8_t mask[6] =	/* No random bits */
+			{ 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
+
+		flags |= NL80211_SCAN_FLAG_RANDOM_ADDR;
+		l_genl_msg_append_attr(msg, NL80211_ATTR_MAC, 6,
+					params->source_mac);
+		l_genl_msg_append_attr(msg, NL80211_ATTR_MAC_MASK, 6,
+					mask);
+	}
+
 	if (!is_passive && wiphy_has_ext_feature(sc->wiphy,
 					NL80211_EXT_FEATURE_SCAN_RANDOM_SN))
 		flags |= NL80211_SCAN_FLAG_RANDOM_SN;
diff --git a/src/scan.h b/src/scan.h
index 61d7550f..82b03e87 100644
--- a/src/scan.h
+++ b/src/scan.h
@@ -104,6 +104,7 @@ struct scan_parameters {
 	bool no_cck_rates : 1;
 	bool duration_mandatory : 1;
 	const char *ssid;	/* Used for direct probe request */
+	const uint8_t *source_mac;
 };
 
 static inline int scan_bss_addr_cmp(const struct scan_bss *a1,
-- 
2.25.1

[PATCH 03/17] p2p: Do provisioning scan from the Interface Address

[Andrew Zaborowski]

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

For WSC we should have been sending our probe requests from the same
address we're going to be doing EAP-WSC with the GO.  Somehow I was able
to connect to most devices without that but other implementations seem
to use the Interface Address (the P2P-Client's MAC), not the Device
Address (P2P-Device's MAC).  We could switch the order to first create
the new interface and scan from it is simpler to use the scan_context we
already have created on the device interface and set a different mac.
---
 src/p2p.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/p2p.c b/src/p2p.c
index 34ff94bf..6c36d5b0 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -1300,6 +1300,13 @@ static void p2p_provision_scan_start(struct p2p_device *dev)
 						&params.extra_ie_size);
 	L_WARN_ON(!params.extra_ie);
 
+	/*
+	 * Rather than create the new interface and create a new
+	 * scan_context on it, use the P2P-Device interface and set
+	 * params.source_mac to our future P2P-Client address.
+	 */
+	params.source_mac = dev->conn_addr;
+
 	/*
 	 * Initially scan just the Operating Channel the GO reported
 	 * during the negotiation.  In theory there's no guarantee that
-- 
2.25.1

[PATCH 04/17] p2p: Move p2p_device_discovery_stop calls to connect_failed

[Andrew Zaborowski]

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

Move a few the calls to p2p_device_discovery_stop() done right after
p2p_connect_failed() directly to that function to reduce duplication.
---
 src/p2p.c | 26 +++-----------------------
 1 file changed, 3 insertions(+), 23 deletions(-)

diff --git a/src/p2p.c b/src/p2p.c
index 6c36d5b0..9983b0fc 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -518,6 +518,9 @@ static void p2p_connect_failed(struct p2p_device *dev)
 	if (dev->scan_id)
 		scan_cancel(dev->wdev_id, dev->scan_id);
 
+	if (l_queue_isempty(dev->discovery_users))
+		p2p_device_discovery_stop(dev);
+
 	if (peer->wsc.pending_connect)
 		dbus_pending_reply(&peer->wsc.pending_connect,
 				dbus_error_failed(peer->wsc.pending_connect));
@@ -1749,10 +1752,6 @@ static void p2p_device_go_negotiation_req_cb(const struct mmpdu_header *mpdu,
 		l_error("GO Negotiation Request parse error %s (%i)",
 			strerror(-r), -r);
 		p2p_connect_failed(dev);
-
-		if (l_queue_isempty(dev->discovery_users))
-			p2p_device_discovery_stop(dev);
-
 		status = P2P_STATUS_FAIL_INVALID_PARAMS;
 		goto respond;
 	}
@@ -1770,10 +1769,6 @@ static void p2p_device_go_negotiation_req_cb(const struct mmpdu_header *mpdu,
 		}
 
 		p2p_connect_failed(dev);
-
-		if (l_queue_isempty(dev->discovery_users))
-			p2p_device_discovery_stop(dev);
-
 		status = P2P_STATUS_FAIL_INCOMPATIBLE_PARAMS;
 		goto p2p_free;
 	}
@@ -1789,10 +1784,6 @@ static void p2p_device_go_negotiation_req_cb(const struct mmpdu_header *mpdu,
 
 		p2p_connect_failed(dev);
 		l_error("Persistent groups not supported");
-
-		if (l_queue_isempty(dev->discovery_users))
-			p2p_device_discovery_stop(dev);
-
 		status = P2P_STATUS_FAIL_INCOMPATIBLE_PARAMS;
 		goto p2p_free;
 	}
@@ -1800,10 +1791,6 @@ static void p2p_device_go_negotiation_req_cb(const struct mmpdu_header *mpdu,
 	if (req_info.device_password_id != dev->conn_password_id) {
 		p2p_connect_failed(dev);
 		l_error("Incompatible Password ID in the GO Negotiation Req");
-
-		if (l_queue_isempty(dev->discovery_users))
-			p2p_device_discovery_stop(dev);
-
 		status = P2P_STATUS_FAIL_INCOMPATIBLE_PROVISIONING;
 		goto p2p_free;
 	}
@@ -1812,10 +1799,6 @@ static void p2p_device_go_negotiation_req_cb(const struct mmpdu_header *mpdu,
 	if (!p2p_device_validate_conn_wfd(dev, req_info.wfd,
 						req_info.wfd_size)) {
 		p2p_connect_failed(dev);
-
-		if (l_queue_isempty(dev->discovery_users))
-			p2p_device_discovery_stop(dev);
-
 		status = P2P_STATUS_FAIL_INCOMPATIBLE_PARAMS;
 		goto p2p_free;
 	}
@@ -1921,9 +1904,6 @@ static void p2p_go_neg_req_timeout(struct l_timeout *timeout, void *user_data)
 	l_debug("");
 
 	p2p_connect_failed(dev);
-
-	if (l_queue_isempty(dev->discovery_users))
-		p2p_device_discovery_stop(dev);
 }
 
 static bool p2p_go_negotiation_resp_cb(const struct mmpdu_header *mpdu,
-- 
2.25.1

[PATCH 05/17] p2p: Use WSC_RF_BAND_2_4_GHZ constant instead of 0x01

[Andrew Zaborowski]

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

---
 src/p2p.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/p2p.c b/src/p2p.c
index 9983b0fc..0ec83ac3 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -3184,7 +3184,7 @@ static void p2p_device_send_probe_resp(struct p2p_device *dev,
 	wsc_info.primary_device_type = dev->device_info.primary_device_type;
 	l_strlcpy(wsc_info.device_name, dev->device_info.device_name,
 			sizeof(wsc_info.device_name));
-	wsc_info.rf_bands = 0x01;	/* 2.4GHz */
+	wsc_info.rf_bands = WSC_RF_BAND_2_4_GHZ;
 	wsc_info.version2 = true;
 
 	wsc_data = wsc_build_probe_response(&wsc_info, &wsc_data_size);
-- 
2.25.1

[PATCH 06/17] p2p: Free parsed frame data in p2p_go_negotiation_resp_cb

[Andrew Zaborowski]

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

---
 src/p2p.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/src/p2p.c b/src/p2p.c
index 0ec83ac3..5ab03c87 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -1943,7 +1943,7 @@ static bool p2p_go_negotiation_resp_cb(const struct mmpdu_header *mpdu,
 	if (resp_info.dialog_token != 1) {
 		l_error("GO Negotiation Response dialog token doesn't match");
 		p2p_connect_failed(dev);
-		return true;
+		goto p2p_free;
 	}
 
 	if (resp_info.status != P2P_STATUS_SUCCESS) {
@@ -1956,12 +1956,12 @@ static bool p2p_go_negotiation_resp_cb(const struct mmpdu_header *mpdu,
 						p2p_go_neg_req_timeout, dev,
 						p2p_go_neg_req_timeout_destroy);
 			p2p_device_discovery_start(dev);
-			return true;
+			goto p2p_free;
 		}
 
 		l_error("GO Negotiation Response status %i", resp_info.status);
 		p2p_connect_failed(dev);
-		return true;
+		goto p2p_free;
 	}
 
 	/*
@@ -1975,31 +1975,33 @@ static bool p2p_go_negotiation_resp_cb(const struct mmpdu_header *mpdu,
 		if (resp_info.go_intent == 0) {
 			/* Can't continue */
 			p2p_connect_failed(dev);
-			return true;
+			goto p2p_free;
 		}
 	}
 
 	if (resp_info.capability.group_caps & P2P_GROUP_CAP_PERSISTENT_GROUP) {
 		l_error("Persistent groups not supported");
 		p2p_connect_failed(dev);
-		return true;
+		goto p2p_free;
 	}
 
 	if (resp_info.device_password_id != dev->conn_password_id) {
 		l_error("GO Negotiation Response WSC device password ID wrong");
 		p2p_connect_failed(dev);
-		return true;
+		goto p2p_free;
 	}
 
 	if (!p2p_device_validate_channel_list(dev, &resp_info.channel_list,
-						&resp_info.operating_channel))
-		return true;
+						&resp_info.operating_channel)) {
+		p2p_connect_failed(dev);
+		goto p2p_free;
+	}
 
 	/* Check whether WFD IE is required, validate it if present */
 	if (!p2p_device_validate_conn_wfd(dev, resp_info.wfd,
 						resp_info.wfd_size)) {
 		p2p_connect_failed(dev);
-		return true;
+		goto p2p_free;
 	}
 
 	band = scan_oper_class_to_band(
@@ -2037,11 +2039,10 @@ static bool p2p_go_negotiation_resp_cb(const struct mmpdu_header *mpdu,
 	confirm_body = p2p_build_go_negotiation_confirmation(&confirm_info,
 								&confirm_len);
 	confirm_info.wfd = NULL;
-	p2p_clear_go_negotiation_resp(&resp_info);
 
 	if (!confirm_body) {
 		p2p_connect_failed(dev);
-		return true;
+		goto p2p_free;
 	}
 
 	iov[iov_len].iov_base = confirm_body;
@@ -2053,6 +2054,9 @@ static bool p2p_go_negotiation_resp_cb(const struct mmpdu_header *mpdu,
 	p2p_peer_frame_xchg(dev->conn_peer, iov, dev->conn_peer->device_addr,
 				0, 0, 0, false, FRAME_GROUP_CONNECT,
 				p2p_go_negotiation_confirm_done, NULL);
+
+p2p_free:
+	p2p_clear_go_negotiation_resp(&resp_info);
 	return true;
 }
 
-- 
2.25.1

[PATCH 07/17] p2p: Consistently use the conn_ prefix for variables

[Andrew Zaborowski]

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

Prefix all the struct p2p_device members that are part of the connection
state with the "conn_" string for consistency.  If we needed to support
multiple client connections, these members are the ones that would
probably land in a separate structure, without that prefix.
---
 src/p2p.c | 89 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 48 insertions(+), 41 deletions(-)

diff --git a/src/p2p.c b/src/p2p.c
index 5ab03c87..d2ae169d 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -100,14 +100,14 @@ struct p2p_device {
 	uint8_t conn_psk[32];
 	int conn_retry_count;
 
-	struct l_timeout *config_timeout;
-	unsigned long go_config_delay;
-	struct l_timeout *go_neg_req_timeout;
-	uint8_t go_dialog_token;
-	unsigned int go_scan_retry;
-	uint32_t go_oper_freq;
+	struct l_timeout *conn_peer_config_timeout;
+	unsigned long conn_config_delay;
+	struct l_timeout *conn_go_neg_req_timeout;
+	uint8_t conn_go_dialog_token;
+	unsigned int conn_go_scan_retry;
+	uint32_t conn_go_oper_freq;
 	struct p2p_group_id_attr go_group_id;
-	uint8_t go_interface_addr[6];
+	uint8_t conn_peer_interface_addr[6];
 
 	bool enabled : 1;
 	bool have_roc_cookie : 1;
@@ -425,8 +425,8 @@ static void p2p_connection_reset(struct p2p_device *dev)
 	l_dbus_property_changed(dbus_get_bus(), p2p_device_get_path(dev),
 				IWD_P2P_INTERFACE, "AvailableConnections");
 
-	l_timeout_remove(dev->config_timeout);
-	l_timeout_remove(dev->go_neg_req_timeout);
+	l_timeout_remove(dev->conn_peer_config_timeout);
+	l_timeout_remove(dev->conn_go_neg_req_timeout);
 	l_timeout_remove(dev->conn_dhcp_timeout);
 
 	if (dev->conn_netconfig) {
@@ -911,8 +911,8 @@ static void p2p_peer_provision_done(int err, struct wsc_credentials_info *creds,
 
 	dev->conn_enrollee = NULL;
 
-	l_timeout_remove(dev->config_timeout);
-	l_timeout_remove(dev->go_neg_req_timeout);
+	l_timeout_remove(dev->conn_peer_config_timeout);
+	l_timeout_remove(dev->conn_go_neg_req_timeout);
 
 	if (err < 0) {
 		if (err == -ECANCELED && peer->wsc.pending_cancel) {
@@ -1159,8 +1159,9 @@ static bool p2p_provision_scan_notify(int err, struct l_queue *bss_list,
 		if (dev->go_group_id.ssid[bss->ssid_len] != '\0')
 			continue;
 
-		if (!util_mem_is_zero(dev->go_interface_addr, 6) &&
-				memcmp(bss->addr, dev->go_interface_addr, 6))
+		if (!util_mem_is_zero(dev->conn_peer_interface_addr, 6) &&
+				memcmp(bss->addr, dev->conn_peer_interface_addr,
+					6))
 			l_debug("SSID matched but BSSID didn't match the GO's "
 				"intended interface addr, proceeding anyway");
 
@@ -1280,9 +1281,9 @@ static bool p2p_provision_scan_notify(int err, struct l_queue *bss_list,
 	}
 
 	/* Retry a few times if the WSC AP not found or not ready */
-	dev->go_scan_retry++;
+	dev->conn_go_scan_retry++;
 
-	if (dev->go_scan_retry > 15) {
+	if (dev->conn_go_scan_retry > 15) {
 		p2p_connect_failed(dev);
 		return false;
 	}
@@ -1320,9 +1321,9 @@ static void p2p_provision_scan_start(struct p2p_device *dev)
 	 * ourselves any work anyway as the Channel List is going to
 	 * contain all of the 2.4 and 5G channels.
 	 */
-	if (dev->go_scan_retry < 12) {
+	if (dev->conn_go_scan_retry < 12) {
 		params.freqs = scan_freq_set_new();
-		scan_freq_set_add(params.freqs, dev->go_oper_freq);
+		scan_freq_set_add(params.freqs, dev->conn_go_oper_freq);
 	}
 
 	dev->scan_id = scan_active_full(dev->wdev_id, &params, NULL,
@@ -1337,13 +1338,14 @@ static void p2p_start_client_provision(struct p2p_device *dev)
 {
 	char bssid_str[18];
 
-	memcpy(bssid_str, util_address_to_string(dev->go_interface_addr), 18);
-	l_debug("freq=%u ssid=%s group_addr=%s bssid=%s", dev->go_oper_freq,
-		dev->go_group_id.ssid,
+	memcpy(bssid_str, util_address_to_string(dev->conn_peer_interface_addr),
+		18);
+	l_debug("freq=%u ssid=%s group_addr=%s bssid=%s",
+		dev->conn_go_oper_freq, dev->go_group_id.ssid,
 		util_address_to_string(dev->go_group_id.device_addr),
 		bssid_str);
 
-	dev->go_scan_retry = 0;
+	dev->conn_go_scan_retry = 0;
 	p2p_provision_scan_start(dev);
 }
 
@@ -1351,14 +1353,14 @@ static void p2p_config_timeout_destroy(void *user_data)
 {
 	struct p2p_device *dev = user_data;
 
-	dev->config_timeout = NULL;
+	dev->conn_peer_config_timeout = NULL;
 }
 
 static void p2p_config_timeout(struct l_timeout *timeout, void *user_data)
 {
 	struct p2p_device *dev = user_data;
 
-	l_timeout_remove(dev->config_timeout);
+	l_timeout_remove(dev->conn_peer_config_timeout);
 
 	/* Ready to start WSC */
 	p2p_start_client_provision(dev);
@@ -1643,7 +1645,7 @@ static bool p2p_go_negotiation_confirm_cb(const struct mmpdu_header *mpdu,
 		return true;
 	}
 
-	if (info.dialog_token != dev->go_dialog_token) {
+	if (info.dialog_token != dev->conn_go_dialog_token) {
 		l_error("GO Negotiation Response dialog token doesn't match");
 		p2p_connect_failed(dev);
 		return true;
@@ -1676,7 +1678,7 @@ static bool p2p_go_negotiation_confirm_cb(const struct mmpdu_header *mpdu,
 		return true;
 	}
 
-	dev->go_oper_freq = frequency;
+	dev->conn_go_oper_freq = frequency;
 	memcpy(&dev->go_group_id, &info.group_id,
 		sizeof(struct p2p_group_id_attr));
 
@@ -1685,7 +1687,8 @@ static bool p2p_go_negotiation_confirm_cb(const struct mmpdu_header *mpdu,
 	 * of time indicated by the peer in the GO Negotiation Response's
 	 * Configuration Timeout attribute and start the provisioning phase.
 	 */
-	dev->config_timeout = l_timeout_create_ms(dev->go_config_delay,
+	dev->conn_peer_config_timeout = l_timeout_create_ms(
+						dev->conn_config_delay,
 						p2p_config_timeout, dev,
 						p2p_config_timeout_destroy);
 	return true;
@@ -1737,7 +1740,7 @@ static void p2p_device_go_negotiation_req_cb(const struct mmpdu_header *mpdu,
 	if (body_len < 8)
 		return;
 
-	if (!dev->go_neg_req_timeout || peer != dev->conn_peer) {
+	if (!dev->conn_go_neg_req_timeout || peer != dev->conn_peer) {
 		status = P2P_STATUS_FAIL_INFO_NOT_AVAIL;
 		goto respond;
 	}
@@ -1803,12 +1806,13 @@ static void p2p_device_go_negotiation_req_cb(const struct mmpdu_header *mpdu,
 		goto p2p_free;
 	}
 
-	l_timeout_remove(dev->go_neg_req_timeout);
+	l_timeout_remove(dev->conn_go_neg_req_timeout);
 	p2p_device_discovery_stop(dev);
 
-	dev->go_dialog_token = req_info.dialog_token;
-	dev->go_config_delay = req_info.config_timeout.go_config_timeout * 10;
-	memcpy(dev->go_interface_addr, req_info.intended_interface_addr, 6);
+	dev->conn_go_dialog_token = req_info.dialog_token;
+	dev->conn_config_delay = req_info.config_timeout.go_config_timeout * 10;
+	memcpy(dev->conn_peer_interface_addr, req_info.intended_interface_addr,
+		6);
 
 p2p_free:
 	tie_breaker = !req_info.go_tie_breaker;
@@ -1816,7 +1820,7 @@ p2p_free:
 
 respond:
 	/* Build and send the GO Negotiation Response */
-	resp_info.dialog_token = dev->go_dialog_token;
+	resp_info.dialog_token = dev->conn_go_dialog_token;
 	resp_info.status = status;
 	resp_info.capability.device_caps = dev->capability.device_caps;
 	resp_info.capability.group_caps = 0;	/* Reserved */
@@ -1885,7 +1889,8 @@ static void p2p_go_negotiation_confirm_done(int error, void *user_data)
 	 * time indicated by the peer in the GO Negotiation Response's
 	 * Configuration Timeout attribute and start the provisioning phase.
 	 */
-	dev->config_timeout = l_timeout_create_ms(dev->go_config_delay,
+	dev->conn_peer_config_timeout = l_timeout_create_ms(
+						dev->conn_config_delay,
 						p2p_config_timeout, dev,
 						p2p_config_timeout_destroy);
 }
@@ -1894,7 +1899,7 @@ static void p2p_go_neg_req_timeout_destroy(void *user_data)
 {
 	struct p2p_device *dev = user_data;
 
-	dev->go_neg_req_timeout = NULL;
+	dev->conn_go_neg_req_timeout = NULL;
 }
 
 static void p2p_go_neg_req_timeout(struct l_timeout *timeout, void *user_data)
@@ -1952,7 +1957,7 @@ static bool p2p_go_negotiation_resp_cb(const struct mmpdu_header *mpdu,
 			l_error("P2P_STATUS_FAIL_INFO_NOT_AVAIL: Will wait for "
 				"a new GO Negotiation Request before declaring "
 				"failure");
-			dev->go_neg_req_timeout = l_timeout_create(120,
+			dev->conn_go_neg_req_timeout = l_timeout_create(120,
 						p2p_go_neg_req_timeout, dev,
 						p2p_go_neg_req_timeout_destroy);
 			p2p_device_discovery_start(dev);
@@ -2016,11 +2021,13 @@ static bool p2p_go_negotiation_resp_cb(const struct mmpdu_header *mpdu,
 		return true;
 	}
 
-	dev->go_config_delay = resp_info.config_timeout.go_config_timeout * 10;
-	dev->go_oper_freq = frequency;
+	dev->conn_config_delay =
+			resp_info.config_timeout.go_config_timeout * 10;
+	dev->conn_go_oper_freq = frequency;
 	memcpy(&dev->go_group_id, &resp_info.group_id,
 		sizeof(struct p2p_group_id_attr));
-	memcpy(dev->go_interface_addr, resp_info.intended_interface_addr, 6);
+	memcpy(dev->conn_peer_interface_addr,
+		resp_info.intended_interface_addr, 6);
 
 	/* Build and send the GO Negotiation Confirmation */
 	confirm_info.dialog_token = resp_info.dialog_token;
@@ -2207,8 +2214,8 @@ static bool p2p_provision_disc_resp_cb(const struct mmpdu_header *mpdu,
 	 * We might want to make sure that Group Formation is false but the
 	 * Capability attribute is also optional.
 	 */
-	dev->go_oper_freq = dev->conn_peer->bss->frequency;
-	memset(dev->go_interface_addr, 0, 6);
+	dev->conn_go_oper_freq = dev->conn_peer->bss->frequency;
+	memset(dev->conn_peer_interface_addr, 0, 6);
 	memcpy(dev->go_group_id.device_addr, dev->conn_peer->device_addr, 6);
 	l_strlcpy(dev->go_group_id.ssid,
 			(const char *) dev->conn_peer->bss->ssid,
@@ -3264,7 +3271,7 @@ static void p2p_device_probe_cb(const struct mmpdu_header *mpdu,
 		return;
 
 	from_conn_peer =
-		dev->go_neg_req_timeout && dev->conn_peer &&
+		dev->conn_go_neg_req_timeout && dev->conn_peer &&
 		!memcmp(mpdu->address_2, dev->conn_peer->bss->addr, 6);
 
 	wsc_payload = ie_tlv_extract_wsc_payload(body, body_len, &wsc_len);
-- 
2.25.1

[PATCH 08/17] p2putil: Add p2p_random_char

[Andrew Zaborowski]

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

Add a utility to select random characters from the set defined in P2P
v1.7 Section 3.2.1.
---
 src/p2putil.c | 26 ++++++++++++++++++++++++++
 src/p2putil.h |  2 ++
 2 files changed, 28 insertions(+)

diff --git a/src/p2putil.c b/src/p2putil.c
index d0f3a444..99ab4ca0 100644
--- a/src/p2putil.c
+++ b/src/p2putil.c
@@ -2611,3 +2611,29 @@ uint8_t *p2p_build_go_disc_req(size_t *out_len)
 	return p2p_build_action_frame(false, P2P_ACTION_GO_DISCOVERABILITY_REQ,
 					0, NULL, NULL, NULL, 0, out_len);
 }
+
+/*
+ * Select a random char from the set defined in section 3.2.1:
+ *
+ * "Following "DIRECT-" the SSID shall contain two ASCII characters "xy",
+ * randomly selected with a uniform distribution from the following
+ * character set: upper case letters, lower case letters and numbers."
+ *
+ * "The WPA2-Personal pass-phrase shall contain at least eight ASCII
+ * characters randomly selected with a uniform distribution from the
+ * following character set: upper case letters, lower case letters and
+ * numbers."
+ */
+char p2p_random_char(void)
+{
+#define CHARSET "ABCDEFGHIJKLMNOPQRSTUVWXYZ" "abcdefghijklmnopqrstuvwxyz" \
+		"0123456789"
+	static const int set_size = strlen(CHARSET);
+	static const uint32_t limit = -(0x100000000LL % set_size);
+	uint32_t index;
+
+	while ((index = l_getrandom_uint32()) >= limit);
+
+	return CHARSET[index % set_size];
+#undef CHARSET
+}
diff --git a/src/p2putil.h b/src/p2putil.h
index 6aad5574..e1fb786b 100644
--- a/src/p2putil.h
+++ b/src/p2putil.h
@@ -603,3 +603,5 @@ uint8_t *p2p_build_presence_req(const struct p2p_presence_req *data,
 uint8_t *p2p_build_presence_resp(const struct p2p_presence_resp *data,
 					size_t *out_len);
 uint8_t *p2p_build_go_disc_req(size_t *out_len);
+
+char p2p_random_char(void);
-- 
2.25.1

[PATCH 09/17] p2p: Add GO-side of GO Negotiation (initiator)

[Andrew Zaborowski]

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

Allow the possibility of becoming the Group-owner when we build the GO
Negotiation Request, parse GO Negotiation Response and build the GO
Negotiation Confirmation, i.e. if we're the initiator of the
negotiation.

Until now the code assumed we can't become the GO or we'd report error.
---
 src/p2p.c | 180 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 143 insertions(+), 37 deletions(-)

diff --git a/src/p2p.c b/src/p2p.c
index d2ae169d..62dfef09 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -82,6 +82,7 @@ struct p2p_device {
 	unsigned int listen_duration;
 	struct l_queue *discovery_users;
 	struct l_queue *peer_list;
+	unsigned int next_tie_breaker;
 
 	struct p2p_peer *conn_peer;
 	uint16_t conn_config_method;
@@ -106,9 +107,10 @@ struct p2p_device {
 	uint8_t conn_go_dialog_token;
 	unsigned int conn_go_scan_retry;
 	uint32_t conn_go_oper_freq;
-	struct p2p_group_id_attr go_group_id;
 	uint8_t conn_peer_interface_addr[6];
 
+	struct p2p_group_id_attr go_group_id;
+
 	bool enabled : 1;
 	bool have_roc_cookie : 1;
 	/*
@@ -119,6 +121,8 @@ struct p2p_device {
 	 * netdev_disconnect, or similar, finishes.
 	 */
 	bool disconnecting : 1;
+	bool is_go : 1;
+	bool conn_go_tie_breaker : 1;
 };
 
 struct p2p_discovery_user {
@@ -163,6 +167,12 @@ static unsigned int p2p_wfd_disconnect_watch;
 static const int channels_social[] = { 1, 6, 11 };
 static const int channels_scan_2_4_other[] = { 2, 3, 4, 5, 7, 8, 9, 10 };
 
+/*
+ * The client side generally receives more testing and we know of fewer
+ * problematic drivers so set a low default Group Owner intent value.
+ */
+#define P2P_GO_INTENT 2
+
 enum {
 	FRAME_GROUP_DEFAULT = 0,
 	FRAME_GROUP_LISTEN,
@@ -501,6 +511,7 @@ static void p2p_connection_reset(struct p2p_device *dev)
 
 	explicit_bzero(dev->conn_psk, 32);
 	dev->conn_retry_count = 0;
+	dev->is_go = false;
 
 	if (dev->enabled && !dev->start_stop_cmd_id &&
 			!l_queue_isempty(dev->discovery_users))
@@ -1694,6 +1705,17 @@ static bool p2p_go_negotiation_confirm_cb(const struct mmpdu_header *mpdu,
 	return true;
 }
 
+static void p2p_set_group_id(struct p2p_device *dev)
+{
+	const char *name = dev->device_info.device_name;
+
+	/* SSID format following section 3.2.1 */
+	snprintf(dev->go_group_id.ssid, sizeof(dev->go_group_id.ssid),
+			"DIRECT-%c%c-%.22s",
+			p2p_random_char(), p2p_random_char(), name);
+	memcpy(dev->go_group_id.device_addr, dev->addr, 6);
+}
+
 static void p2p_device_go_negotiation_req_cb(const struct mmpdu_header *mpdu,
 						const void *body,
 						size_t body_len, int rssi,
@@ -1885,10 +1907,18 @@ static void p2p_go_negotiation_confirm_done(int error, void *user_data)
 	}
 
 	/*
-	 * Frame was ACKed.  For simplicity wait idly the maximum amount of
-	 * time indicated by the peer in the GO Negotiation Response's
-	 * Configuration Timeout attribute and start the provisioning phase.
+	 * Frame was ACKed.  On the GO start setting the group up right
+	 * away and we'll add the client's Configuation Timeout to the
+	 * WSC start timeout's value.  On the client wait idly the
+	 * maximum amount of time indicated by the peer in the GO
+	 * Negotiation Response's Configuration Timeout attribute and
+	 * start the provisioning phase.
 	 */
+	if (dev->is_go) {
+		p2p_device_interface_create(dev);
+		return;
+	}
+
 	dev->conn_peer_config_timeout = l_timeout_create_ms(
 						dev->conn_config_delay,
 						p2p_config_timeout, dev,
@@ -1974,17 +2004,17 @@ static bool p2p_go_negotiation_resp_cb(const struct mmpdu_header *mpdu,
 	 * shall be toggled from the corresponding GO Negotiation Request
 	 * frame."
 	 */
-	if (!resp_info.go_tie_breaker) {
+	if (resp_info.go_tie_breaker == dev->conn_go_tie_breaker) {
 		l_error("GO Negotiation Response tie breaker value wrong");
-
-		if (resp_info.go_intent == 0) {
-			/* Can't continue */
-			p2p_connect_failed(dev);
-			goto p2p_free;
-		}
+		/* Proceed anyway */
+		dev->conn_go_tie_breaker = !resp_info.go_tie_breaker;
 	}
 
-	if (resp_info.capability.group_caps & P2P_GROUP_CAP_PERSISTENT_GROUP) {
+	dev->is_go = P2P_GO_INTENT * 2 + dev->conn_go_tie_breaker >
+		resp_info.go_intent * 2;
+
+	if ((resp_info.capability.group_caps & P2P_GROUP_CAP_PERSISTENT_GROUP)
+			&& !dev->is_go) {
 		l_error("Persistent groups not supported");
 		p2p_connect_failed(dev);
 		goto p2p_free;
@@ -2002,6 +2032,44 @@ static bool p2p_go_negotiation_resp_cb(const struct mmpdu_header *mpdu,
 		goto p2p_free;
 	}
 
+	if (dev->is_go) {
+		const struct l_queue_entry *entry;
+		const struct p2p_channel_entries *entries;
+		int i;
+
+		/*
+		 * Check that our listen channel is in the set supported by the
+		 * Client.
+		 * TODO: if it's not, look for a different channel.
+		 */
+		for (entry = l_queue_get_entries(
+					resp_info.channel_list.channel_entries);
+				entry; entry = entry->next) {
+			entries = entry->data;
+
+			if (entries->oper_class == dev->listen_oper_class)
+				break;
+		}
+
+		if (!entry) {
+			l_error("Our Operating Class not listed in "
+				"the GO Negotiation Response");
+			p2p_connect_failed(dev);
+			goto p2p_free;
+		}
+
+		for (i = 0; i < entries->n_channels; i++)
+			if (entries->channels[i] == dev->listen_channel)
+				break;
+
+		if (i == entries->n_channels) {
+			l_error("Our listen channel not listed in "
+				"the GO Negotiation Response");
+			p2p_connect_failed(dev);
+			goto p2p_free;
+		}
+	}
+
 	/* Check whether WFD IE is required, validate it if present */
 	if (!p2p_device_validate_conn_wfd(dev, resp_info.wfd,
 						resp_info.wfd_size)) {
@@ -2009,33 +2077,63 @@ static bool p2p_go_negotiation_resp_cb(const struct mmpdu_header *mpdu,
 		goto p2p_free;
 	}
 
-	band = scan_oper_class_to_band(
+	memcpy(dev->conn_peer_interface_addr, resp_info.intended_interface_addr,
+		6);
+
+	if (dev->is_go) {
+		struct l_queue *channel_list = l_queue_new();
+		struct p2p_channel_entries *channel_entries =
+			l_malloc(sizeof(struct p2p_channel_entries) + 1);
+
+		p2p_set_group_id(dev);
+
+		dev->conn_config_delay =
+			resp_info.config_timeout.client_config_timeout * 10;
+
+		channel_entries->oper_class = dev->listen_oper_class;
+		channel_entries->n_channels = 1;
+		channel_entries->channels[0] = dev->listen_channel;
+		l_queue_push_tail(channel_list, channel_entries);
+
+		/* Build and send the GO Negotiation Confirmation */
+		confirm_info.capability = dev->capability;
+		memcpy(confirm_info.operating_channel.country,
+			dev->listen_country, 3);
+		confirm_info.operating_channel.oper_class = dev->listen_oper_class;
+		confirm_info.operating_channel.channel_num = dev->listen_channel;
+		memcpy(confirm_info.channel_list.country, dev->listen_country, 3);
+		confirm_info.channel_list.channel_entries = channel_list;
+		memcpy(&confirm_info.group_id, &dev->go_group_id,
+			sizeof(struct p2p_group_id_attr));
+	} else {
+		band = scan_oper_class_to_band(
 			(const uint8_t *) resp_info.operating_channel.country,
 			resp_info.operating_channel.oper_class);
-	frequency = scan_channel_to_freq(
+		frequency = scan_channel_to_freq(
 					resp_info.operating_channel.channel_num,
 					band);
-	if (!frequency) {
-		l_error("Bad operating channel in GO Negotiation Response");
-		p2p_connect_failed(dev);
-		return true;
-	}
+		if (!frequency) {
+			l_error("Bad operating channel in GO Negotiation Response");
+			p2p_connect_failed(dev);
+			goto p2p_free;
+		}
 
-	dev->conn_config_delay =
+		dev->conn_config_delay =
 			resp_info.config_timeout.go_config_timeout * 10;
-	dev->conn_go_oper_freq = frequency;
-	memcpy(&dev->go_group_id, &resp_info.group_id,
-		sizeof(struct p2p_group_id_attr));
-	memcpy(dev->conn_peer_interface_addr,
-		resp_info.intended_interface_addr, 6);
+		dev->conn_go_oper_freq = frequency;
+		memcpy(&dev->go_group_id, &resp_info.group_id,
+			sizeof(struct p2p_group_id_attr));
+
+		/* Build and send the GO Negotiation Confirmation */
+		confirm_info.capability.device_caps = 0;	/* Reserved */
+		confirm_info.capability.group_caps = 0;		/* Reserved */
+		confirm_info.operating_channel = resp_info.operating_channel;
+		confirm_info.channel_list = resp_info.channel_list;
+		resp_info.channel_list.channel_entries = NULL;
+	}
 
-	/* Build and send the GO Negotiation Confirmation */
 	confirm_info.dialog_token = resp_info.dialog_token;
 	confirm_info.status = P2P_STATUS_SUCCESS;
-	confirm_info.capability.device_caps = 0;	/* Reserved */
-	confirm_info.capability.group_caps = 0;		/* Reserved */
-	confirm_info.channel_list = resp_info.channel_list;
-	confirm_info.operating_channel = resp_info.operating_channel;
 
 	if (dev->conn_own_wfd) {
 		confirm_info.wfd = wfd_ie;
@@ -2046,6 +2144,7 @@ static bool p2p_go_negotiation_resp_cb(const struct mmpdu_header *mpdu,
 	confirm_body = p2p_build_go_negotiation_confirmation(&confirm_info,
 								&confirm_len);
 	confirm_info.wfd = NULL;
+	p2p_clear_go_negotiation_confirmation(&confirm_info);
 
 	if (!confirm_body) {
 		p2p_connect_failed(dev);
@@ -2099,10 +2198,12 @@ static void p2p_start_go_negotiation(struct p2p_device *dev)
 	 */
 	unsigned int resp_timeout = 600;
 
+	dev->conn_go_tie_breaker = dev->next_tie_breaker++ & 1;
+
 	info.dialog_token = 1;
 	info.capability = dev->capability;
-	info.go_intent = 0;	/* Don't want to be the GO */
-	info.go_tie_breaker = 0;
+	info.go_intent = P2P_GO_INTENT;
+	info.go_tie_breaker = dev->conn_go_tie_breaker;
 	info.config_timeout.go_config_timeout = 50;	/* 500ms */
 	info.config_timeout.client_config_timeout = 50;	/* 500ms */
 	memcpy(info.listen_channel.country, dev->listen_country, 3);
@@ -2111,11 +2212,9 @@ static void p2p_start_go_negotiation(struct p2p_device *dev)
 	memcpy(info.intended_interface_addr, dev->conn_addr, 6);
 
 	/*
-	 * In theory we support an empty set of operating channels for
-	 * our potential group as a GO but we have to include our
-	 * supported channels because the peer can only choose their
-	 * own channels from our list.  Use the listen channel as the
-	 * preferred operating channel because we have no preference.
+	 * Even if P2P_GO_INTENT is 0, we have to include our supported
+	 * channels because the peer can only choose their own channels
+	 * from our list.
 	 */
 	p2p_device_fill_channel_list(dev, &info.channel_list);
 	memcpy(info.operating_channel.country, dev->listen_country, 3);
@@ -3615,6 +3714,13 @@ struct p2p_device *p2p_device_update_from_genl(struct l_genl_msg *msg,
 	gethostname(hostname, sizeof(hostname));
 	dev->connections_left = 1;
 
+	/*
+	 * Section 3.1.4.2: "The Tie breaker bit in a first GO Negotiation
+	 * Request frame (for instance after power up) shall be set to 0 or 1
+	 * randomly, such that both values occur equally on average."
+	 */
+	dev->next_tie_breaker = l_getrandom_uint32();
+
 	/* TODO: allow masking capability bits through a setting? */
 	dev->capability.device_caps = P2P_DEVICE_CAP_CONCURRENT_OP;
 	dev->capability.group_caps = 0;
-- 
2.25.1

[PATCH 10/17] p2p: Add GO-side of GO Negotiation (responder)

[Andrew Zaborowski]

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

Allow the possibility of becoming the Group-owner when we parse the GO
Negotiation Request, build GO Negotiation Response and parse the GO
Negotiation Confirmation, i.e. if we're responding to a negotiation
initiated by the peer after it needed to request user action.

Until now the code assumed we can't become the GO or we'd report error.
---
 src/p2p.c | 217 ++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 169 insertions(+), 48 deletions(-)

diff --git a/src/p2p.c b/src/p2p.c
index 62dfef09..8e7da6d7 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -1636,8 +1636,6 @@ static bool p2p_go_negotiation_confirm_cb(const struct mmpdu_header *mpdu,
 {
 	struct p2p_go_negotiation_confirmation info;
 	int r;
-	enum scan_band band;
-	uint32_t frequency;
 
 	l_debug("");
 
@@ -1668,40 +1666,79 @@ static bool p2p_go_negotiation_confirm_cb(const struct mmpdu_header *mpdu,
 		return true;
 	}
 
-	if (!p2p_device_validate_channel_list(dev, &info.channel_list,
-						&info.operating_channel))
-		return true;
-
-	band = scan_oper_class_to_band(
-			(const uint8_t *) info.operating_channel.country,
-			info.operating_channel.oper_class);
-	frequency = scan_channel_to_freq(info.operating_channel.channel_num,
-						band);
-	if (!frequency) {
-		l_error("Bad operating channel in GO Negotiation Confirmation");
+	/* Check whether WFD IE is required, validate it if present */
+	if (!p2p_device_validate_conn_wfd(dev, info.wfd, info.wfd_size)) {
 		p2p_connect_failed(dev);
 		return true;
 	}
 
-	/* Check whether WFD IE is required, validate it if present */
-	if (!p2p_device_validate_conn_wfd(dev, info.wfd, info.wfd_size)) {
+	/*
+	 * In both scenarios the Channel List is a subset of what we previously
+	 * sent in the GO Negotiation Response and must contain the Operating
+	 * Channel.
+	 */
+	if (!p2p_device_validate_channel_list(dev, &info.channel_list,
+						&info.operating_channel)) {
 		p2p_connect_failed(dev);
 		return true;
 	}
 
-	dev->conn_go_oper_freq = frequency;
-	memcpy(&dev->go_group_id, &info.group_id,
-		sizeof(struct p2p_group_id_attr));
-
 	/*
-	 * Confirmation received.  For simplicity wait idly the maximum amount
-	 * of time indicated by the peer in the GO Negotiation Response's
-	 * Configuration Timeout attribute and start the provisioning phase.
+	 * Not validating .capability.group_caps, it's either reserved
+	 * (dev->is_go) or has to be identical to that in the GO Negotiation
+	 * Request (!dev->is_go).
 	 */
-	dev->conn_peer_config_timeout = l_timeout_create_ms(
+
+	if (dev->is_go) {
+		if (memcmp(info.operating_channel.country,
+				dev->listen_country, 3) ||
+				info.operating_channel.oper_class !=
+				dev->listen_oper_class ||
+				info.operating_channel.channel_num !=
+				dev->listen_channel) {
+			l_error("Bad operating channel in GO Negotiation "
+				"Confirmation");
+			p2p_connect_failed(dev);
+			return true;
+		}
+
+		/*
+		 * Start setting the group up right away and we'll add the
+		 * client's Configuation Timeout to the WSC start timeout's
+		 * value.
+		 */
+		p2p_device_interface_create(dev);
+	} else {
+		enum scan_band band = scan_oper_class_to_band(
+			(const uint8_t *) info.operating_channel.country,
+			info.operating_channel.oper_class);
+		uint32_t frequency = scan_channel_to_freq(
+					info.operating_channel.channel_num,
+					band);
+
+		if (!frequency) {
+			l_error("Bad operating channel in GO Negotiation "
+				"Confirmation");
+			p2p_connect_failed(dev);
+			return true;
+		}
+
+		dev->conn_go_oper_freq = frequency;
+		memcpy(&dev->go_group_id, &info.group_id,
+			sizeof(struct p2p_group_id_attr));
+
+		/*
+		 * Confirmation received.  For simplicity wait idly the maximum
+		 * amount of time indicated by the peer in the GO Negotiation
+		 * Request's Configuration Timeout attribute and start the
+		 * provisioning phase.
+		 */
+		dev->conn_peer_config_timeout = l_timeout_create_ms(
 						dev->conn_config_delay,
 						p2p_config_timeout, dev,
 						p2p_config_timeout_destroy);
+	}
+
 	return true;
 }
 
@@ -1732,7 +1769,6 @@ static void p2p_device_go_negotiation_req_cb(const struct mmpdu_header *mpdu,
 	int iov_len = 0;
 	struct p2p_peer *peer;
 	enum p2p_attr_status_code status = P2P_STATUS_SUCCESS;
-	bool tie_breaker = false;
 
 	l_debug("");
 
@@ -1781,24 +1817,12 @@ static void p2p_device_go_negotiation_req_cb(const struct mmpdu_header *mpdu,
 		goto respond;
 	}
 
-	if (req_info.go_intent == 0 && !req_info.go_tie_breaker) {
-		l_error("Can't negotiate client role and GO operation not "
-			"supported");
-
-		if (peer->wsc.pending_connect) {
-			struct l_dbus_message *reply =
-				dbus_error_not_supported(
-						peer->wsc.pending_connect);
-
-			dbus_pending_reply(&peer->wsc.pending_connect, reply);
-		}
-
-		p2p_connect_failed(dev);
-		status = P2P_STATUS_FAIL_INCOMPATIBLE_PARAMS;
-		goto p2p_free;
-	}
+	dev->conn_go_tie_breaker = !req_info.go_tie_breaker;
+	dev->is_go = P2P_GO_INTENT * 2 + dev->conn_go_tie_breaker >
+		req_info.go_intent * 2;
 
-	if (req_info.capability.group_caps & P2P_GROUP_CAP_PERSISTENT_GROUP) {
+	if ((req_info.capability.group_caps & P2P_GROUP_CAP_PERSISTENT_GROUP) &&
+			!dev->is_go) {
 		if (peer->wsc.pending_connect) {
 			struct l_dbus_message *reply =
 				dbus_error_not_supported(
@@ -1820,6 +1844,70 @@ static void p2p_device_go_negotiation_req_cb(const struct mmpdu_header *mpdu,
 		goto p2p_free;
 	}
 
+	if (!p2p_device_validate_channel_list(dev, &req_info.channel_list,
+						&req_info.operating_channel)) {
+		p2p_connect_failed(dev);
+		status = P2P_STATUS_FAIL_INCOMPATIBLE_PARAMS;
+		goto p2p_free;
+	}
+
+	if (dev->is_go) {
+		const struct l_queue_entry *entry;
+		const struct p2p_channel_entries *entries;
+		int i;
+
+		/*
+		 * Section 3.1.4.2.1: "The Channel List attribute shall
+		 * indicate the channels that the P2P Device can support as
+		 * Operating Channel of the P2P Group if it becomes P2P Group
+		 * Owner."
+		 * Section 3.1.4.2.2: "The channels indicated in the Channel
+		 * List shall only include channels from the Channel List
+		 * attribute in the GO Negotiation Request frame. [...]
+		 * The channel indicated in the Operating Channel attribute
+		 * shall be one of the channels in the Channel List attribute
+		 * in the GO Negotiation Response frame."
+		 *
+		 * Since the sender is not becoming the GO this shouldn't
+		 * affect us but following 3.1.4.2.2 our operating channel
+		 * should be one of those listed in the GO Negotiation
+		 * Response which in turn are the subset of those in the
+		 * Request.  So effectively the list in the Request limits
+		 * the peer's set of supported operating channels both as the
+		 * GO and the Client.  Check that our listen channel is in
+		 * that set.
+		 * TODO: if it's not, look for a different channel.
+		 */
+		for (entry = l_queue_get_entries(
+					req_info.channel_list.channel_entries);
+				entry; entry = entry->next) {
+			entries = entry->data;
+
+			if (entries->oper_class == dev->listen_oper_class)
+				break;
+		}
+
+		if (!entry) {
+			l_error("Our Operating Class not listed in "
+				"the GO Negotiation Request");
+			p2p_connect_failed(dev);
+			status = P2P_STATUS_FAIL_NO_COMMON_CHANNELS;
+			goto p2p_free;
+		}
+
+		for (i = 0; i < entries->n_channels; i++)
+			if (entries->channels[i] == dev->listen_channel)
+				break;
+
+		if (i == entries->n_channels) {
+			l_error("Our listen channel not listed in "
+				"the GO Negotiation Request");
+			p2p_connect_failed(dev);
+			status = P2P_STATUS_FAIL_NO_COMMON_CHANNELS;
+			goto p2p_free;
+		}
+	}
+
 	/* Check whether WFD IE is required, validate it if present */
 	if (!p2p_device_validate_conn_wfd(dev, req_info.wfd,
 						req_info.wfd_size)) {
@@ -1832,29 +1920,62 @@ static void p2p_device_go_negotiation_req_cb(const struct mmpdu_header *mpdu,
 	p2p_device_discovery_stop(dev);
 
 	dev->conn_go_dialog_token = req_info.dialog_token;
-	dev->conn_config_delay = req_info.config_timeout.go_config_timeout * 10;
 	memcpy(dev->conn_peer_interface_addr, req_info.intended_interface_addr,
 		6);
 
+	if (dev->is_go && dev->conn_peer) {
+		p2p_set_group_id(dev);
+
+		dev->conn_config_delay =
+			req_info.config_timeout.client_config_timeout * 10;
+	} else {
+		dev->conn_config_delay =
+			req_info.config_timeout.go_config_timeout * 10;
+	}
+
 p2p_free:
-	tie_breaker = !req_info.go_tie_breaker;
+	dev->conn_go_tie_breaker = !req_info.go_tie_breaker;
 	p2p_clear_go_negotiation_req(&req_info);
 
 respond:
 	/* Build and send the GO Negotiation Response */
 	resp_info.dialog_token = dev->conn_go_dialog_token;
 	resp_info.status = status;
-	resp_info.capability.device_caps = dev->capability.device_caps;
-	resp_info.capability.group_caps = 0;	/* Reserved */
-	resp_info.go_intent = 0;		/* Don't want to be the GO */
-	resp_info.go_tie_breaker = tie_breaker;
+
+	if (dev->is_go && dev->conn_peer) {
+		struct l_queue *channel_list = l_queue_new();
+		struct p2p_channel_entries *channel_entries =
+			l_malloc(sizeof(struct p2p_channel_entries) + 1);
+
+		resp_info.capability = dev->capability;
+		memcpy(resp_info.operating_channel.country,
+			dev->listen_country, 3);
+		resp_info.operating_channel.oper_class = dev->listen_oper_class;
+		resp_info.operating_channel.channel_num = dev->listen_channel;
+		memcpy(&resp_info.group_id, &dev->go_group_id,
+			sizeof(struct p2p_group_id_attr));
+
+		channel_entries->oper_class = dev->listen_oper_class;
+		channel_entries->n_channels = 1;
+		channel_entries->channels[0] = dev->listen_channel;
+		l_queue_push_tail(channel_list, channel_entries);
+
+		memcpy(resp_info.channel_list.country, dev->listen_country, 3);
+		resp_info.channel_list.channel_entries = channel_list;
+	} else {
+		resp_info.capability.device_caps = dev->capability.device_caps;
+		resp_info.capability.group_caps = 0;	/* Reserved */
+		p2p_device_fill_channel_list(dev, &resp_info.channel_list);
+	}
+
+	resp_info.go_intent = P2P_GO_INTENT;
+	resp_info.go_tie_breaker = dev->conn_go_tie_breaker;
 	resp_info.config_timeout.go_config_timeout = 50;	/* 500ms */
 	resp_info.config_timeout.client_config_timeout = 50;	/* 500ms */
 
 	if (dev->conn_peer)
 		memcpy(resp_info.intended_interface_addr, dev->conn_addr, 6);
 
-	p2p_device_fill_channel_list(dev, &resp_info.channel_list);
 	resp_info.device_info = dev->device_info;
 	resp_info.device_info.wsc_config_methods = dev->conn_config_method;
 	resp_info.device_password_id = dev->conn_password_id;
-- 
2.25.1

[PATCH 11/17] ap: Pass "ops" struct to ap_start()

[Andrew Zaborowski]

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

Pass the event callback function pointer in a "struct ap_ops" instead of
as individual ap_start() argument to make adding new callbacks easier.
---
 src/ap.c | 65 +++++++++++++++++++++++++++++++-------------------------
 src/ap.h |  9 +++++---
 2 files changed, 42 insertions(+), 32 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index 84d4539a..fab7c20e 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -52,7 +52,7 @@
 struct ap_state {
 	struct netdev *netdev;
 	struct l_genl_family *nl80211;
-	ap_event_func_t event_func;
+	const struct ap_ops *ops;
 	ap_stopped_func_t stopped_func;
 	void *user_data;
 	struct ap_config *config;
@@ -210,7 +210,7 @@ static void ap_del_station(struct sta_state *sta, uint16_t reason,
 	sta->associated = false;
 
 	if (sta->rsna) {
-		if (ap->event_func) {
+		if (ap->ops->handle_event) {
 			memset(&event_data, 0, sizeof(event_data));
 			event_data.mac = sta->addr;
 			event_data.reason = reason;
@@ -228,8 +228,8 @@ static void ap_del_station(struct sta_state *sta, uint16_t reason,
 	ap_stop_handshake(sta);
 
 	if (send_event)
-		ap->event_func(AP_EVENT_STATION_REMOVED, &event_data,
-				ap->user_data);
+		ap->ops->handle_event(AP_EVENT_STATION_REMOVED, &event_data,
+					ap->user_data);
 }
 
 static bool ap_sta_match_addr(const void *a, const void *b)
@@ -269,12 +269,12 @@ static void ap_new_rsna(struct sta_state *sta)
 
 	sta->rsna = true;
 
-	if (ap->event_func) {
+	if (ap->ops->handle_event) {
 		struct ap_event_station_added_data event_data = {};
 		event_data.mac = sta->addr;
 		event_data.rsn_ie = sta->assoc_rsne;
-		ap->event_func(AP_EVENT_STATION_ADDED, &event_data,
-				ap->user_data);
+		ap->ops->handle_event(AP_EVENT_STATION_ADDED, &event_data,
+					ap->user_data);
 	}
 }
 
@@ -308,11 +308,11 @@ static void ap_drop_rsna(struct sta_state *sta)
 
 	ap_stop_handshake(sta);
 
-	if (ap->event_func) {
+	if (ap->ops->handle_event) {
 		struct ap_event_station_removed_data event_data = {};
 		event_data.mac = sta->addr;
-		ap->event_func(AP_EVENT_STATION_REMOVED, &event_data,
-				ap->user_data);
+		ap->ops->handle_event(AP_EVENT_STATION_REMOVED, &event_data,
+					ap->user_data);
 	}
 }
 
@@ -543,7 +543,7 @@ static void ap_wsc_exit_pbc(struct ap_state *ap)
 	ap->wsc_dpid = 0;
 	ap_update_beacon(ap);
 
-	ap->event_func(AP_EVENT_PBC_MODE_EXIT, NULL, ap->user_data);
+	ap->ops->handle_event(AP_EVENT_PBC_MODE_EXIT, NULL, ap->user_data);
 }
 
 static uint32_t ap_send_mgmt_frame(struct ap_state *ap,
@@ -750,8 +750,9 @@ static void ap_wsc_handshake_event(struct handshake_state *hs,
 					&expiry_data);
 
 		event_data.mac = sta->addr;
-		sta->ap->event_func(AP_EVENT_REGISTRATION_SUCCESS, &event_data,
-					sta->ap->user_data);
+		sta->ap->ops->handle_event(AP_EVENT_REGISTRATION_SUCCESS,
+						&event_data,
+						sta->ap->user_data);
 		break;
 	default:
 		break;
@@ -1335,8 +1336,8 @@ static void ap_assoc_reassoc(struct sta_state *sta, bool reassoc,
 		sta->wsc_v2 = wsc_req.version2;
 
 		event_data.mac = sta->addr;
-		ap->event_func(AP_EVENT_REGISTRATION_START, &event_data,
-				ap->user_data);
+		ap->ops->handle_event(AP_EVENT_REGISTRATION_START, &event_data,
+					ap->user_data);
 
 		/*
 		 * Since we're starting the PBC Registration Protocol
@@ -1916,7 +1917,8 @@ static void ap_start_cb(struct l_genl_msg *msg, void *user_data)
 	if (l_genl_msg_get_error(msg) < 0) {
 		l_error("START_AP failed: %i", l_genl_msg_get_error(msg));
 
-		ap->event_func(AP_EVENT_START_FAILED, NULL, ap->user_data);
+		ap->ops->handle_event(AP_EVENT_START_FAILED, NULL,
+					ap->user_data);
 		ap_reset(ap);
 		l_genl_family_free(ap->nl80211);
 		l_free(ap);
@@ -1924,7 +1926,7 @@ static void ap_start_cb(struct l_genl_msg *msg, void *user_data)
 	}
 
 	ap->started = true;
-	ap->event_func(AP_EVENT_STARTED, NULL, ap->user_data);
+	ap->ops->handle_event(AP_EVENT_STARTED, NULL, ap->user_data);
 }
 
 static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
@@ -2011,11 +2013,12 @@ static void ap_mlme_notify(struct l_genl_msg *msg, void *user_data)
 			l_genl_family_cancel(ap->nl80211,
 						ap->start_stop_cmd_id);
 			ap->start_stop_cmd_id = 0;
-			ap->event_func(AP_EVENT_START_FAILED, NULL,
-					ap->user_data);
+			ap->ops->handle_event(AP_EVENT_START_FAILED, NULL,
+						ap->user_data);
 		} else if (ap->started) {
 			ap->started = false;
-			ap->event_func(AP_EVENT_STOPPING, NULL, ap->user_data);
+			ap->ops->handle_event(AP_EVENT_STOPPING, NULL,
+						ap->user_data);
 		}
 
 		ap_reset(ap);
@@ -2028,17 +2031,17 @@ static void ap_mlme_notify(struct l_genl_msg *msg, void *user_data)
 /*
  * Start a simple independent WPA2 AP on given netdev.
  *
- * @event_func is required and must react to AP_EVENT_START_FAILED
- * and AP_EVENT_STOPPING by forgetting the ap_state struct, which
- * is going to be freed automatically.
- * In the @config struct only .ssid and .psk need to be non-NUL,
+ * @ops.handle_event is required and must react to AP_EVENT_START_FAILED
+ * and AP_EVENT_STOPPING by forgetting the ap_state struct, which is
+ * going to be freed automatically.
+ * In the @config struct only .ssid and .psk need to be non-NULL,
  * other fields are optional.  If @ap_start succeeds, the returned
  * ap_state takes ownership of @config and the caller shouldn't
  * free it or any of the memory pointed to by its members (they
  * also can't be static).
  */
 struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
-				ap_event_func_t event_func, void *user_data)
+				const struct ap_ops *ops, void *user_data)
 {
 	struct ap_state *ap;
 	struct wiphy *wiphy = netdev_get_wiphy(netdev);
@@ -2049,7 +2052,7 @@ struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
 	ap->nl80211 = l_genl_family_new(iwd_get_genl(), NL80211_GENL_NAME);
 	ap->config = config;
 	ap->netdev = netdev;
-	ap->event_func = event_func;
+	ap->ops = ops;
 	ap->user_data = user_data;
 
 	if (!config->channel)
@@ -2180,7 +2183,7 @@ static struct l_genl_msg *ap_build_cmd_stop_ap(struct ap_state *ap)
 
 /*
  * Schedule the running @ap to be stopped and freed.  The original
- * event_func and user_data are forgotten and a new callback can be
+ * ops and user_data are forgotten and a new callback can be
  * provided if the caller needs to know when the interface becomes
  * free, for example for a new ap_start call.
  *
@@ -2197,7 +2200,7 @@ void ap_shutdown(struct ap_state *ap, ap_stopped_func_t stopped_func,
 
 	if (ap->started) {
 		ap->started = false;
-		ap->event_func(AP_EVENT_STOPPING, NULL, ap->user_data);
+		ap->ops->handle_event(AP_EVENT_STOPPING, NULL, ap->user_data);
 	}
 
 	ap_reset(ap);
@@ -2367,6 +2370,10 @@ static void ap_if_event_func(enum ap_event_type type, const void *event_data,
 	}
 }
 
+static const struct ap_ops ap_dbus_ops = {
+	.handle_event = ap_if_event_func,
+};
+
 static struct l_dbus_message *ap_dbus_start(struct l_dbus *dbus,
 		struct l_dbus_message *message, void *user_data)
 {
@@ -2387,7 +2394,7 @@ static struct l_dbus_message *ap_dbus_start(struct l_dbus *dbus,
 	config->ssid = l_strdup(ssid);
 	config->psk = l_strdup(wpa2_psk);
 
-	ap_if->ap = ap_start(ap_if->netdev, config, ap_if_event_func, ap_if);
+	ap_if->ap = ap_start(ap_if->netdev, config, &ap_dbus_ops, ap_if);
 	if (!ap_if->ap) {
 		ap_config_free(config);
 		return dbus_error_invalid_args(message);
diff --git a/src/ap.h b/src/ap.h
index cb5238d1..bbfecd6d 100644
--- a/src/ap.h
+++ b/src/ap.h
@@ -52,8 +52,6 @@ struct ap_event_registration_success_data {
 	const uint8_t *mac;
 };
 
-typedef void (*ap_event_func_t)(enum ap_event_type type, const void *event_data,
-				void *user_data);
 typedef void (*ap_stopped_func_t)(void *user_data);
 
 struct ap_config {
@@ -67,10 +65,15 @@ struct ap_config {
 	bool no_cck_rates : 1;
 };
 
+struct ap_ops {
+	void (*handle_event)(enum ap_event_type type, const void *event_data,
+				void *user_data);
+};
+
 void ap_config_free(struct ap_config *config);
 
 struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
-				ap_event_func_t event_func, void *user_data);
+				const struct ap_ops *ops, void *user_data);
 void ap_shutdown(struct ap_state *ap, ap_stopped_func_t stopped_func,
 			void *user_data);
 void ap_free(struct ap_state *ap);
-- 
2.25.1

[PATCH 12/17] ap: Accept P2P wildcard SSIDs in probe requests

[Andrew Zaborowski]

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

Add the special case "DIRECT-" SSID, called the P2P Wildcard SSID, in
ap_probe_req_cb so as not to reject those Probe Requests on the basis of
ssid mismatch.  I'd have preferred to keep all the P2P-specific bits in
p2p.c but in this case there's little point in adding a generic
config setting for SSID-matching quirks.
---
 src/ap.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/ap.c b/src/ap.c
index fab7c20e..3579d30f 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -1683,6 +1683,9 @@ static void ap_probe_req_cb(const struct mmpdu_header *hdr, const void *body,
 	else if (ssid && ssid_len == strlen(ap->config->ssid) && /* One SSID */
 			!memcmp(ssid, ap->config->ssid, ssid_len))
 		match = true;
+	else if (ssid && ssid_len == 7 && !memcmp(ssid, "DIRECT-", 7) &&
+			!memcmp(ssid, ap->config->ssid, 7)) /* P2P wildcard */
+		match = true;
 	else if (ssid_list) { /* SSID List */
 		ie_tlv_iter_init(&iter, ssid_list, ssid_list_len);
 
-- 
2.25.1

[PATCH 13/17] p2p: Start a basic P2P Group after GO Negotiation

[Andrew Zaborowski]

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

Use the ap.c API to start an AP on a P2P_GO interface after we've been
selected as the GO in the GO Negotiation.
---
 src/p2p.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 123 insertions(+), 13 deletions(-)

diff --git a/src/p2p.c b/src/p2p.c
index 8e7da6d7..db4b0750 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -55,6 +55,7 @@
 #include "src/frame-xchg.h"
 #include "src/nl80211util.h"
 #include "src/netconfig.h"
+#include "src/ap.h"
 #include "src/p2p.h"
 
 struct p2p_device {
@@ -110,6 +111,7 @@ struct p2p_device {
 	uint8_t conn_peer_interface_addr[6];
 
 	struct p2p_group_id_attr go_group_id;
+	struct ap_state *group;
 
 	bool enabled : 1;
 	bool have_roc_cookie : 1;
@@ -123,6 +125,7 @@ struct p2p_device {
 	bool disconnecting : 1;
 	bool is_go : 1;
 	bool conn_go_tie_breaker : 1;
+	bool conn_peer_added : 1;
 };
 
 struct p2p_discovery_user {
@@ -217,8 +220,9 @@ static void p2p_discovery_user_free(void *data)
 
 static inline bool p2p_peer_operational(struct p2p_peer *peer)
 {
-	return peer && peer->dev->conn_netdev && !peer->dev->conn_wsc_bss &&
-		!peer->wsc.pending_connect && !peer->dev->disconnecting;
+	return peer && peer->dev->conn_netdev && !peer->dev->disconnecting &&
+		((!peer->dev->is_go && !peer->dev->conn_wsc_bss) ||
+		 (peer->dev->is_go && peer->dev->conn_peer_added));
 }
 
 static bool p2p_peer_match(const void *a, const void *b)
@@ -457,6 +461,14 @@ static void p2p_connection_reset(struct p2p_device *dev)
 	if (dev->conn_enrollee)
 		wsc_enrollee_cancel(dev->conn_enrollee, false);
 
+	if (dev->group) {
+		ap_free(dev->group);
+		dev->group = NULL;
+		dev->conn_peer_added = false;
+	}
+
+	dev->capability.group_caps = 0;
+
 	if (dev->conn_netdev) {
 		struct l_genl_msg *msg;
 		uint64_t wdev_id = netdev_get_wdev_id(dev->conn_netdev);
@@ -628,10 +640,12 @@ static void p2p_peer_connect_done(struct p2p_device *dev)
 {
 	struct p2p_peer *peer = dev->conn_peer;
 
-	/* We can free anything potentially needed for a retry */
-	scan_bss_free(dev->conn_wsc_bss);
-	dev->conn_wsc_bss = NULL;
-	explicit_bzero(dev->conn_psk, 32);
+	if (!dev->is_go) {
+		/* We can free anything potentially needed for a retry */
+		scan_bss_free(dev->conn_wsc_bss);
+		dev->conn_wsc_bss = NULL;
+		explicit_bzero(dev->conn_psk, 32);
+	}
 
 	dbus_pending_reply(&peer->wsc.pending_connect,
 				l_dbus_message_new_method_return(
@@ -649,6 +663,98 @@ static void p2p_peer_connect_done(struct p2p_device *dev)
 				"ConnectedIP");
 }
 
+static void p2p_group_event(enum ap_event_type type, const void *event_data,
+				void *user_data)
+{
+	struct p2p_device *dev = user_data;
+
+	l_debug("type=%i", type);
+
+	switch (type) {
+	case AP_EVENT_START_FAILED:
+	case AP_EVENT_STOPPING:
+		dev->group = NULL;
+		p2p_connect_failed(dev);
+		break;
+
+	case AP_EVENT_STARTED:
+		ap_push_button(dev->group);
+		break;
+
+	case AP_EVENT_STATION_ADDED:
+		dev->conn_peer_added = true;
+		p2p_peer_connect_done(dev);
+		break;
+
+	case AP_EVENT_STATION_REMOVED:
+		dev->conn_peer_added = false;
+		p2p_connect_failed(dev);
+		break;
+
+	case AP_EVENT_REGISTRATION_START:
+		/* Don't validate the P2P IE or WFD IE at this stage */
+		break;
+	case AP_EVENT_REGISTRATION_SUCCESS:
+		dev->capability.group_caps &= ~P2P_GROUP_CAP_GROUP_FORMATION;
+		break;
+	case AP_EVENT_PBC_MODE_EXIT:
+		break;
+	};
+}
+
+static const struct ap_ops p2p_go_ops = {
+	.handle_event = p2p_group_event,
+};
+
+static void p2p_group_start(struct p2p_device *dev)
+{
+	struct ap_config *config = l_new(struct ap_config, 1);
+	char passphrase[20];
+	unsigned int i;
+
+	/*
+	 * Section 3.2.1: "The WPA2-Personal pass-phrase shall contain at
+	 * least eight ASCII characters randomly selected with a uniform
+	 * distribution from the following character set: upper case letters,
+	 * lower case letters and numbers."
+	 *
+	 * However we don't currently respect the following text because
+	 * our credentials will contain the passphrase and not the PSK.
+	 * Section 3.2.1: "The Credentials for a P2P Group issued to a
+	 * P2P Device shall: [...]
+	 *  - Use a Network Key Type of 64 Hex characters."
+	 */
+	for (i = 0; i < sizeof(passphrase) - 1; i++)
+		passphrase[i] = p2p_random_char();
+
+	passphrase[i] = '\0';
+
+	config->ssid = l_strdup(dev->go_group_id.ssid);
+	config->psk = l_strdup(passphrase);
+	config->channel = dev->listen_channel;
+	config->wsc_name = l_strdup(dev->device_info.device_name);
+	config->wsc_primary_device_type = dev->device_info.primary_device_type;
+	config->no_cck_rates = true;
+	/*
+	 * Section 3.1.4.4: "It shall only allow association by the
+	 * P2P Device that it is currently in Group Formation with."
+	 */
+	config->authorized_macs = l_memdup(dev->conn_peer_interface_addr, 6);
+	config->authorized_macs_num = 1;
+
+	explicit_bzero(passphrase, sizeof(passphrase));
+
+	dev->capability.group_caps |= P2P_GROUP_CAP_GO;
+	dev->capability.group_caps |= P2P_GROUP_CAP_GROUP_FORMATION;
+
+	dev->group = ap_start(dev->conn_netdev, config, &p2p_go_ops, dev);
+	if (!dev->group) {
+		ap_config_free(config);
+		p2p_connect_failed(dev);
+		return;
+	}
+}
+
 static void p2p_netconfig_event_handler(enum netconfig_event event,
 					void *user_data)
 {
@@ -682,7 +788,7 @@ static void p2p_dhcp_timeout_destroy(void *user_data)
 	dev->conn_dhcp_timeout = NULL;
 }
 
-static void p2p_start_dhcp(struct p2p_device *dev)
+static void p2p_start_dhcp_client(struct p2p_device *dev)
 {
 	uint32_t ifindex = netdev_get_ifindex(dev->conn_netdev);
 	unsigned int dhcp_timeout_val;
@@ -723,7 +829,7 @@ static void p2p_netdev_connect_cb(struct netdev *netdev,
 
 	switch (result) {
 	case NETDEV_RESULT_OK:
-		p2p_start_dhcp(dev);
+		p2p_start_dhcp_client(dev);
 		break;
 	case NETDEV_RESULT_AUTHENTICATION_FAILED:
 	case NETDEV_RESULT_ASSOCIATION_FAILED:
@@ -1034,10 +1140,14 @@ static void p2p_device_netdev_notify(struct netdev *netdev,
 	case NETDEV_WATCH_EVENT_NEW:
 		/* Ignore the event if we're already connecting/connected */
 		if (dev->conn_enrollee || dev->conn_retry_count ||
-				!netdev_get_is_up(netdev))
+				dev->group || !netdev_get_is_up(netdev))
 			break;
 
-		p2p_provision_connect(dev);
+		if (dev->is_go)
+			p2p_group_start(dev);
+		else
+			p2p_provision_connect(dev);
+
 		break;
 	case NETDEV_WATCH_EVENT_DEL:
 		dev->conn_netdev = NULL;
@@ -1094,13 +1203,14 @@ static void p2p_device_new_interface_destroy(void *user_data)
 
 static void p2p_device_interface_create(struct p2p_device *dev)
 {
-	uint32_t iftype = NL80211_IFTYPE_P2P_CLIENT;
+	uint32_t iftype = dev->is_go ? NL80211_IFTYPE_P2P_GO :
+		NL80211_IFTYPE_P2P_CLIENT;
 	char ifname[32];
 	uint32_t wiphy_id = dev->wdev_id >> 32;
 	struct l_genl_msg *msg;
 
-	snprintf(ifname, sizeof(ifname), "wlan%i-p2p-cl%i",
-			wiphy_id, dev->conn_num++);
+	snprintf(ifname, sizeof(ifname), "wlan%i-p2p-%s%i",
+			wiphy_id, dev->is_go ? "go" : "cl", dev->conn_num++);
 	l_debug("creating %s", ifname);
 
 	msg = l_genl_msg_new(NL80211_CMD_NEW_INTERFACE);
-- 
2.25.1

[PATCH 14/17] ap: Pass extra IEs between mgmt frames and user

[Andrew Zaborowski]

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

Add user callbacks to add extra IEs to the AP's beacons and other
frames.  I considered a few ways for the user to pass this data
without callbacks but so far this seems to be the easiest and most
versatile way.

Similarly pass the string of IEs from STA association frames to the user
through the AP events.  I dropped ap_event_station_added_data.rsn_ie
because that probably would have not been very useful and the RSN IE
is included in the IE array in any case.
---
 src/ap.c | 57 +++++++++++++++++++++++++++++++++++++++++++++-----------
 src/ap.h | 10 +++++++++-
 2 files changed, 55 insertions(+), 12 deletions(-)

diff --git a/src/ap.c b/src/ap.c
index 3579d30f..c1e1e389 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -88,6 +88,8 @@ struct sta_state {
 	struct l_uintset *rates;
 	uint32_t assoc_resp_cmd_id;
 	struct ap_state *ap;
+	uint8_t *assoc_ies;
+	size_t assoc_ies_len;
 	uint8_t *assoc_rsne;
 	struct eapol_sm *sm;
 	struct handshake_state *hs;
@@ -159,7 +161,7 @@ static void ap_sta_free(void *data)
 	struct ap_state *ap = sta->ap;
 
 	l_uintset_free(sta->rates);
-	l_free(sta->assoc_rsne);
+	l_free(sta->assoc_ies);
 
 	if (sta->assoc_resp_cmd_id)
 		l_genl_family_cancel(ap->nl80211, sta->assoc_resp_cmd_id);
@@ -272,7 +274,8 @@ static void ap_new_rsna(struct sta_state *sta)
 	if (ap->ops->handle_event) {
 		struct ap_event_station_added_data event_data = {};
 		event_data.mac = sta->addr;
-		event_data.rsn_ie = sta->assoc_rsne;
+		event_data.assoc_ies = sta->assoc_ies;
+		event_data.assoc_ies_len = sta->assoc_ies_len;
 		ap->ops->handle_event(AP_EVENT_STATION_ADDED, &event_data,
 					ap->user_data);
 	}
@@ -491,6 +494,12 @@ static size_t ap_build_beacon_pr_tail(struct ap_state *ap, bool pr,
 	len += wsc_ie_size;
 	l_free(wsc_ie);
 
+	if (ap->ops->write_mgmt_frame_ies)
+		len += ap->ops->write_mgmt_frame_ies(
+				pr ? MPDU_MANAGEMENT_SUBTYPE_PROBE_RESPONSE :
+				MPDU_MANAGEMENT_SUBTYPE_BEACON,
+				out_buf + len, ap->user_data);
+
 	return len;
 }
 
@@ -505,7 +514,11 @@ static void ap_set_beacon_cb(struct l_genl_msg *msg, void *user_data)
 static void ap_update_beacon(struct ap_state *ap)
 {
 	struct l_genl_msg *cmd;
-	uint8_t head[256], tail[256];
+	size_t extra_ies_len = ap->ops->get_mgmt_frame_ies_len ?
+		ap->ops->get_mgmt_frame_ies_len(MPDU_MANAGEMENT_SUBTYPE_BEACON,
+						ap->user_data) : 0;
+	uint8_t head[256];
+	uint8_t tail[256 + extra_ies_len];
 	size_t head_len, tail_len;
 	uint64_t wdev_id = netdev_get_wdev_id(ap->netdev);
 	static const uint8_t bcast_addr[6] = {
@@ -1059,7 +1072,11 @@ static uint32_t ap_assoc_resp(struct ap_state *ap, struct sta_state *sta,
 				bool reassoc, l_genl_msg_func_t callback)
 {
 	const uint8_t *addr = netdev_get_address(ap->netdev);
-	uint8_t mpdu_buf[128];
+	size_t extra_ies_len = ap->ops->get_mgmt_frame_ies_len ?
+		ap->ops->get_mgmt_frame_ies_len(
+				MPDU_MANAGEMENT_SUBTYPE_ASSOCIATION_RESPONSE,
+				ap->user_data) : 0;
+	uint8_t mpdu_buf[128 + extra_ies_len];
 	struct mmpdu_header *mpdu = (void *) mpdu_buf;
 	struct mmpdu_association_response *resp;
 	size_t ies_len = 0;
@@ -1135,6 +1152,11 @@ static uint32_t ap_assoc_resp(struct ap_state *ap, struct sta_state *sta,
 		l_free(wsc_ie);
 	}
 
+	if (ap->ops->write_mgmt_frame_ies)
+		ies_len += ap->ops->write_mgmt_frame_ies(
+				MPDU_MANAGEMENT_SUBTYPE_ASSOCIATION_RESPONSE,
+				resp->ies + ies_len, ap->user_data);
+
 send_frame:
 	return ap_send_mgmt_frame(ap, mpdu, resp->ies + ies_len - mpdu_buf,
 					true, callback, sta);
@@ -1336,6 +1358,8 @@ static void ap_assoc_reassoc(struct sta_state *sta, bool reassoc,
 		sta->wsc_v2 = wsc_req.version2;
 
 		event_data.mac = sta->addr;
+		event_data.assoc_ies = ies;
+		event_data.assoc_ies_len = ies_len;
 		ap->ops->handle_event(AP_EVENT_REGISTRATION_START, &event_data,
 					ap->user_data);
 
@@ -1385,13 +1409,16 @@ static void ap_assoc_reassoc(struct sta_state *sta, bool reassoc,
 
 	sta->rates = rates;
 
-	if (sta->assoc_rsne)
-		l_free(sta->assoc_rsne);
+	l_free(sta->assoc_ies);
 
-	if (rsn)
-		sta->assoc_rsne = l_memdup(rsn, rsn[1] + 2);
-	else
+	if (rsn) {
+		sta->assoc_ies = l_memdup(ies, ies_len);
+		sta->assoc_ies_len = ies_len;
+		sta->assoc_rsne = sta->assoc_ies + (rsn - ies);
+	} else {
+		sta->assoc_ies = NULL;
 		sta->assoc_rsne = NULL;
+	}
 
 	sta->assoc_resp_cmd_id = ap_assoc_resp(ap, sta, sta->addr, 0, reassoc,
 						ap_success_assoc_resp_cb);
@@ -1636,7 +1663,11 @@ static void ap_probe_req_cb(const struct mmpdu_header *hdr, const void *body,
 	struct ie_tlv_iter iter;
 	const uint8_t *bssid = netdev_get_address(ap->netdev);
 	bool match = false;
-	uint8_t resp[512];
+	size_t extra_ies_len = ap->ops->get_mgmt_frame_ies_len ?
+		ap->ops->get_mgmt_frame_ies_len(
+					MPDU_MANAGEMENT_SUBTYPE_PROBE_RESPONSE,
+					ap->user_data) : 0;
+	uint8_t resp[512 + extra_ies_len];
 	uint8_t *wsc_data;
 	ssize_t wsc_data_len;
 
@@ -1936,7 +1967,11 @@ static struct l_genl_msg *ap_build_cmd_start_ap(struct ap_state *ap)
 {
 	struct l_genl_msg *cmd;
 
-	uint8_t head[256], tail[256];
+	size_t extra_ies_len = ap->ops->get_mgmt_frame_ies_len ?
+		ap->ops->get_mgmt_frame_ies_len(MPDU_MANAGEMENT_SUBTYPE_BEACON,
+						ap->user_data) : 0;
+	uint8_t head[256];
+	uint8_t tail[256 + extra_ies_len];
 	size_t head_len, tail_len;
 
 	uint32_t dtim_period = 3;
diff --git a/src/ap.h b/src/ap.h
index bbfecd6d..25ecc15a 100644
--- a/src/ap.h
+++ b/src/ap.h
@@ -22,6 +22,7 @@
 
 struct ap_state;
 struct iovec;
+enum mpdu_management_subtype;
 
 enum ap_event_type {
 	AP_EVENT_START_FAILED,
@@ -36,7 +37,8 @@ enum ap_event_type {
 
 struct ap_event_station_added_data {
 	const uint8_t *mac;
-	const uint8_t *rsn_ie;
+	const uint8_t *assoc_ies;
+	size_t assoc_ies_len;
 };
 
 struct ap_event_station_removed_data {
@@ -46,6 +48,8 @@ struct ap_event_station_removed_data {
 
 struct ap_event_registration_start_data {
 	const uint8_t *mac;
+	const uint8_t *assoc_ies;
+	size_t assoc_ies_len;
 };
 
 struct ap_event_registration_success_data {
@@ -68,6 +72,10 @@ struct ap_config {
 struct ap_ops {
 	void (*handle_event)(enum ap_event_type type, const void *event_data,
 				void *user_data);
+	size_t (*get_mgmt_frame_ies_len)(enum mpdu_management_subtype type,
+						void *user_data);
+	size_t (*write_mgmt_frame_ies)(enum mpdu_management_subtype type,
+					uint8_t *out_buf, void *user_data);
 };
 
 void ap_config_free(struct ap_config *config);
-- 
2.25.1

[PATCH 15/17] p2p: Parse P2P IEs and WFD IEs in Association Requests

[Andrew Zaborowski]

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

Roughly validate the IEs and save some information for use in our own
IEs. p2p_extract_wfd_properties and p2p_device_validate_conn_wfd are
being moved unchanged to be usable in p2p_group_event without forward
declarations and to be next to p2p_build_wfd_ie.
---
 src/p2p.c | 386 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 229 insertions(+), 157 deletions(-)

diff --git a/src/p2p.c b/src/p2p.c
index db4b0750..50559bae 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -109,6 +109,8 @@ struct p2p_device {
 	unsigned int conn_go_scan_retry;
 	uint32_t conn_go_oper_freq;
 	uint8_t conn_peer_interface_addr[6];
+	struct p2p_capability_attr conn_client_capability;
+	struct p2p_device_info_attr conn_client_dev_info;
 
 	struct p2p_group_id_attr go_group_id;
 	struct ap_state *group;
@@ -329,6 +331,163 @@ static size_t p2p_build_wfd_ie(const struct p2p_wfd_properties *wfd,
 	return size;
 }
 
+static bool p2p_extract_wfd_properties(const uint8_t *ie, size_t ie_size,
+					struct p2p_wfd_properties *out)
+{
+	struct wfd_subelem_iter iter;
+	const uint8_t *devinfo = NULL;
+	const uint8_t *ext_caps = NULL;
+	const uint8_t *r2 = NULL;
+
+	if (!ie)
+		return false;
+
+	wfd_subelem_iter_init(&iter, ie, ie_size);
+
+	while (wfd_subelem_iter_next(&iter)) {
+		enum wfd_subelem_type type = wfd_subelem_iter_get_type(&iter);
+		size_t len = wfd_subelem_iter_get_length(&iter);
+		const uint8_t *data = wfd_subelem_iter_get_data(&iter);
+
+		switch (type) {
+#define SUBELEM_CHECK(var, expected_len, name)		\
+			if (len != expected_len) {	\
+				l_debug(name " length wrong in WFD IE");\
+				return false;		\
+			}				\
+							\
+			if (var) {			\
+				l_debug("Duplicate" name " in WFD IE");\
+				return false;		\
+			}				\
+							\
+			var = data;
+		case WFD_SUBELEM_WFD_DEVICE_INFORMATION:
+			SUBELEM_CHECK(devinfo, 6, "Device Information");
+			break;
+		case WFD_SUBELEM_EXTENDED_CAPABILITY:
+			SUBELEM_CHECK(ext_caps, 2, "Extended Capability");
+			break;
+		case WFD_SUBELEM_R2_DEVICE_INFORMATION:
+			SUBELEM_CHECK(r2, 2, "R2 Device Information");
+			break;
+#undef SUBELEM_CHECK
+		default:
+			/* Skip unknown IEs */
+			break;
+		}
+	}
+
+	if (devinfo) {
+		uint16_t capability = l_get_be16(devinfo + 0);
+		bool source;
+		bool sink;
+		uint16_t port;
+
+		source = (capability & WFD_DEV_INFO_DEVICE_TYPE) ==
+			WFD_DEV_INFO_TYPE_SOURCE ||
+			(capability & WFD_DEV_INFO_DEVICE_TYPE) ==
+			WFD_DEV_INFO_TYPE_DUAL_ROLE;
+		sink = (capability & WFD_DEV_INFO_DEVICE_TYPE) ==
+			WFD_DEV_INFO_TYPE_PRIMARY_SINK ||
+			(capability & WFD_DEV_INFO_DEVICE_TYPE) ==
+			WFD_DEV_INFO_TYPE_DUAL_ROLE;
+
+		if (!source && !sink)
+			return false;
+
+		port = l_get_be16(devinfo + 2);
+
+		if (source && port == 0) {
+			l_debug("0 port number in WFD IE Device Information");
+			return false;
+		}
+
+		memset(out, 0, sizeof(*out));
+		out->available =
+			(capability & WFD_DEV_INFO_SESSION_AVAILABILITY) ==
+			WFD_DEV_INFO_SESSION_AVAILABLE;
+		out->source = source;
+		out->sink = sink;
+		out->port = port;
+		out->cp = capability & WFD_DEV_INFO_CONTENT_PROTECTION_SUPPORT;
+		out->audio = !sink ||
+			!(capability & WFD_DEV_INFO_NO_AUDIO_AT_PRIMARY_SINK);
+	} else {
+		l_error("Device Information missing in WFD IE");
+		return false;
+	}
+
+	if (ext_caps && (l_get_be16(ext_caps) & 1))
+		out->uibc = 1;
+
+	if (r2) {
+		uint8_t role = l_get_be16(r2) & 3;
+
+		if ((out->source && role != 0 && role != 3) ||
+				(out->sink && role != 1 && role != 3))
+			l_debug("Invalid role in WFD R2 Device Information");
+		else
+			out->r2 = true;
+	}
+
+	return true;
+}
+
+static bool p2p_device_validate_conn_wfd(struct p2p_device *dev,
+						const uint8_t *ie,
+						ssize_t ie_size)
+{
+	struct p2p_wfd_properties wfd;
+
+	if (!dev->conn_own_wfd)
+		return true;
+
+	/*
+	 * WFD IEs are optional in Association Request/Response and P2P Public
+	 * Action frames for R2 devices and required for R1 devices.
+	 * Wi-Fi Display Technical Specification v2.1.0 section 5.2:
+	 * "A WFD R2 Device shall include the WFD IE in Beacon, Probe
+	 * Request/Response, Association Request/Response and P2P Public Action
+	 * frames in order to be interoperable with R1 devices. If a WFD R2
+	 * Device discovers that the peer device is also a WFD R2 Device, then
+	 * it may include the WFD IE in Association Request/Response and P2P
+	 * Public Action frames."
+	 */
+	if (!ie)
+		return dev->conn_own_wfd->r2;
+
+	if (!p2p_extract_wfd_properties(ie, ie_size, &wfd)) {
+		l_error("Could not parse the WFD IE contents");
+		return false;
+	}
+
+	if ((dev->conn_own_wfd->source && !wfd.sink) ||
+			(dev->conn_own_wfd->sink && !wfd.source)) {
+		l_error("Wrong role in peer's WFD IE");
+		return false;
+	}
+
+	if (wfd.r2 != dev->conn_own_wfd->r2) {
+		l_error("Wrong version in peer's WFD IE");
+		return false;
+	}
+
+	/*
+	 * Ignore the session available state because it's not 100% clear
+	 * at what point the peer switches to SESSION_NOT_AVAILABLE in its
+	 * Device Information.
+	 * But we might want to check that other bits have not changed from
+	 * what the peer reported during discovery.
+	 * Wi-Fi Display Technical Specification v2.1.0 section 4.5.2.1:
+	 * "The content of the WFD Device Information subelement should be
+	 * immutable during the period of P2P connection establishment, with
+	 * [...] exceptions [...]"
+	 */
+
+	return true;
+}
+
 /* TODO: convert to iovecs */
 static uint8_t *p2p_build_scan_ies(struct p2p_device *dev, uint8_t *buf,
 					size_t buf_len, size_t *out_len)
@@ -682,9 +841,72 @@ static void p2p_group_event(enum ap_event_type type, const void *event_data,
 		break;
 
 	case AP_EVENT_STATION_ADDED:
+	{
+		const struct ap_event_station_added_data *data = event_data;
+		L_AUTO_FREE_VAR(uint8_t *, p2p_data) = NULL;
+		ssize_t p2p_data_len;
+		L_AUTO_FREE_VAR(uint8_t *, wfd_data) = NULL;
+		ssize_t wfd_data_len;
+		struct p2p_association_req req_info;
+		int r;
+
+		p2p_data = ie_tlv_extract_p2p_payload(data->assoc_ies,
+							data->assoc_ies_len,
+							&p2p_data_len);
+		if (!p2p_data) {
+			l_error("Missing or invalid P2P IEs: %s (%i)",
+				strerror(-p2p_data_len), (int) -p2p_data_len);
+			goto invalid_ie;
+		}
+
+		/*
+		 * We don't need to validate most of the Association Request
+		 * P2P IE contents as we already have all the information there
+		 * may be but we need to save some of the attributes because
+		 * section 3.2.3 requires that our Group Info includes
+		 * specifically the data from the association, not any of the
+		 * earlier exchanges:
+		 * "When a P2P Client associates with a P2P Group Owner, it
+		 * provides [...] the P2P Device Info attribute (see Section
+		 * 4.1.15) and the P2P Capability attribute (see Section 4.1.4)
+		 * in the P2P IE in the Association Request frame.  This
+		 * information shall be used by the P2P Group Owner for Group
+		 * Information Advertisement.
+		 */
+		r = p2p_parse_association_req(p2p_data, p2p_data_len,
+						&req_info);
+		if (r < 0) {
+			l_error("Can't parse P2P Association Request: %s (%i)",
+				strerror(-r), -r);
+			goto invalid_ie;
+		}
+
+		/*
+		 * Most of this duplicates the information we already have in
+		 * dev->conn_peer.
+		 */
+		dev->conn_client_capability = req_info.capability;
+		dev->conn_client_dev_info = req_info.device_info;
+		p2p_clear_association_req(&req_info);
+
+		if (dev->conn_own_wfd)
+			wfd_data = ie_tlv_extract_p2p_payload(data->assoc_ies,
+							data->assoc_ies_len,
+							&wfd_data_len);
+
+		if (!p2p_device_validate_conn_wfd(dev, wfd_data,
+							wfd_data_len))
+			goto invalid_ie;
+
+		/* Take the chance to update WFD attributes for Session Info */
+		if (wfd_data)
+			p2p_extract_wfd_properties(wfd_data, wfd_data_len,
+							dev->conn_peer->wfd);
+
 		dev->conn_peer_added = true;
 		p2p_peer_connect_done(dev);
 		break;
+	}
 
 	case AP_EVENT_STATION_REMOVED:
 		dev->conn_peer_added = false;
@@ -700,6 +922,13 @@ static void p2p_group_event(enum ap_event_type type, const void *event_data,
 	case AP_EVENT_PBC_MODE_EXIT:
 		break;
 	};
+
+	return;
+
+invalid_ie:
+	ap_station_disconnect(dev->group, dev->conn_peer_interface_addr,
+				MMPDU_REASON_CODE_INVALID_IE);
+	p2p_connect_failed(dev);
 }
 
 static const struct ap_ops p2p_go_ops = {
@@ -1583,163 +1812,6 @@ static void p2p_device_fill_channel_list(struct p2p_device *dev,
 	l_queue_push_tail(attr->channel_entries, channel_entry);
 }
 
-static bool p2p_extract_wfd_properties(const uint8_t *ie, size_t ie_size,
-					struct p2p_wfd_properties *out)
-{
-	struct wfd_subelem_iter iter;
-	const uint8_t *devinfo = NULL;
-	const uint8_t *ext_caps = NULL;
-	const uint8_t *r2 = NULL;
-
-	if (!ie)
-		return false;
-
-	wfd_subelem_iter_init(&iter, ie, ie_size);
-
-	while (wfd_subelem_iter_next(&iter)) {
-		enum wfd_subelem_type type = wfd_subelem_iter_get_type(&iter);
-		size_t len = wfd_subelem_iter_get_length(&iter);
-		const uint8_t *data = wfd_subelem_iter_get_data(&iter);
-
-		switch (type) {
-#define SUBELEM_CHECK(var, expected_len, name)		\
-			if (len != expected_len) {	\
-				l_debug(name " length wrong in WFD IE");\
-				return false;		\
-			}				\
-							\
-			if (var) {			\
-				l_debug("Duplicate" name " in WFD IE");\
-				return false;		\
-			}				\
-							\
-			var = data;
-		case WFD_SUBELEM_WFD_DEVICE_INFORMATION:
-			SUBELEM_CHECK(devinfo, 6, "Device Information");
-			break;
-		case WFD_SUBELEM_EXTENDED_CAPABILITY:
-			SUBELEM_CHECK(ext_caps, 2, "Extended Capability");
-			break;
-		case WFD_SUBELEM_R2_DEVICE_INFORMATION:
-			SUBELEM_CHECK(r2, 2, "R2 Device Information");
-			break;
-#undef SUBELEM_CHECK
-		default:
-			/* Skip unknown IEs */
-			break;
-		}
-	}
-
-	if (devinfo) {
-		uint16_t capability = l_get_be16(devinfo + 0);
-		bool source;
-		bool sink;
-		uint16_t port;
-
-		source = (capability & WFD_DEV_INFO_DEVICE_TYPE) ==
-			WFD_DEV_INFO_TYPE_SOURCE ||
-			(capability & WFD_DEV_INFO_DEVICE_TYPE) ==
-			WFD_DEV_INFO_TYPE_DUAL_ROLE;
-		sink = (capability & WFD_DEV_INFO_DEVICE_TYPE) ==
-			WFD_DEV_INFO_TYPE_PRIMARY_SINK ||
-			(capability & WFD_DEV_INFO_DEVICE_TYPE) ==
-			WFD_DEV_INFO_TYPE_DUAL_ROLE;
-
-		if (!source && !sink)
-			return false;
-
-		port = l_get_be16(devinfo + 2);
-
-		if (source && port == 0) {
-			l_debug("0 port number in WFD IE Device Information");
-			return false;
-		}
-
-		memset(out, 0, sizeof(*out));
-		out->available =
-			(capability & WFD_DEV_INFO_SESSION_AVAILABILITY) ==
-			WFD_DEV_INFO_SESSION_AVAILABLE;
-		out->source = source;
-		out->sink = sink;
-		out->port = port;
-		out->cp = capability & WFD_DEV_INFO_CONTENT_PROTECTION_SUPPORT;
-		out->audio = !sink ||
-			!(capability & WFD_DEV_INFO_NO_AUDIO_AT_PRIMARY_SINK);
-	} else {
-		l_error("Device Information missing in WFD IE");
-		return false;
-	}
-
-	if (ext_caps && (l_get_be16(ext_caps) & 1))
-		out->uibc = 1;
-
-	if (r2) {
-		uint8_t role = l_get_be16(r2) & 3;
-
-		if ((out->source && role != 0 && role != 3) ||
-				(out->sink && role != 1 && role != 3))
-			l_debug("Invalid role in WFD R2 Device Information");
-		else
-			out->r2 = true;
-	}
-
-	return true;
-}
-
-static bool p2p_device_validate_conn_wfd(struct p2p_device *dev,
-						const uint8_t *ie,
-						ssize_t ie_size)
-{
-	struct p2p_wfd_properties wfd;
-
-	if (!dev->conn_own_wfd)
-		return true;
-
-	/*
-	 * WFD IEs are optional in Association Request/Response and P2P Public
-	 * Action frames for R2 devices and required for R1 devices.
-	 * Wi-Fi Display Technical Specification v2.1.0 section 5.2:
-	 * "A WFD R2 Device shall include the WFD IE in Beacon, Probe
-	 * Request/Response, Association Request/Response and P2P Public Action
-	 * frames in order to be interoperable with R1 devices. If a WFD R2
-	 * Device discovers that the peer device is also a WFD R2 Device, then
-	 * it may include the WFD IE in Association Request/Response and P2P
-	 * Public Action frames."
-	 */
-	if (!ie)
-		return dev->conn_own_wfd->r2;
-
-	if (!p2p_extract_wfd_properties(ie, ie_size, &wfd)) {
-		l_error("Could not parse the WFD IE contents");
-		return false;
-	}
-
-	if ((dev->conn_own_wfd->source && !wfd.sink) ||
-			(dev->conn_own_wfd->sink && !wfd.source)) {
-		l_error("Wrong role in peer's WFD IE");
-		return false;
-	}
-
-	if (wfd.r2 != dev->conn_own_wfd->r2) {
-		l_error("Wrong version in peer's WFD IE");
-		return false;
-	}
-
-	/*
-	 * Ignore the session available state because it's not 100% clear
-	 * at what point the peer switches to SESSION_NOT_AVAILABLE in its
-	 * Device Information.
-	 * But we might want to check that other bits have not changed from
-	 * what the peer reported during discovery.
-	 * Wi-Fi Display Technical Specification v2.1.0 section 4.5.2.1:
-	 * "The content of the WFD Device Information subelement should be
-	 * immutable during the period of P2P connection establishment, with
-	 * [...] exceptions [...]"
-	 */
-
-	return true;
-}
-
 static bool p2p_go_negotiation_confirm_cb(const struct mmpdu_header *mpdu,
 					const void *body, size_t body_len,
 					int rssi, struct p2p_device *dev)
-- 
2.25.1

[PATCH 16/17] ap: Make ap_update_beacon public

[Andrew Zaborowski]

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

Let users call ap_update_beacon when they've changed something in the
beacon IEs returned by ops->write_mgmt_frame_ies(BEACON) to cause a new
call to that method.
---
 src/ap.c | 5 ++++-
 src/ap.h | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/src/ap.c b/src/ap.c
index c1e1e389..8ef42c77 100644
--- a/src/ap.c
+++ b/src/ap.c
@@ -511,7 +511,7 @@ static void ap_set_beacon_cb(struct l_genl_msg *msg, void *user_data)
 		l_error("SET_BEACON failed: %s (%i)", strerror(-error), -error);
 }
 
-static void ap_update_beacon(struct ap_state *ap)
+void ap_update_beacon(struct ap_state *ap)
 {
 	struct l_genl_msg *cmd;
 	size_t extra_ies_len = ap->ops->get_mgmt_frame_ies_len ?
@@ -525,6 +525,9 @@ static void ap_update_beacon(struct ap_state *ap)
 		0xff, 0xff, 0xff, 0xff, 0xff, 0xff
 	};
 
+	if (L_WARN_ON(!ap->started))
+		return;
+
 	head_len = ap_build_beacon_pr_head(ap, MPDU_MANAGEMENT_SUBTYPE_BEACON,
 						bcast_addr, head, sizeof(head));
 	tail_len = ap_build_beacon_pr_tail(ap, false, tail);
diff --git a/src/ap.h b/src/ap.h
index 25ecc15a..233e9269 100644
--- a/src/ap.h
+++ b/src/ap.h
@@ -90,3 +90,4 @@ bool ap_station_disconnect(struct ap_state *ap, const uint8_t *mac,
 				enum mmpdu_reason_code reason);
 
 bool ap_push_button(struct ap_state *ap);
+void ap_update_beacon(struct ap_state *ap);
-- 
2.25.1

[PATCH 17/17] p2p: Build P2P and WFD IEs for group's management frames

[Andrew Zaborowski]

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

Handle our P2P group's ops.write_mgmt_frame_ies callbacks to build and
attach the necessary P2P IE and WFD IEs to the (Re)Association Response,
Probe Response and Beacon frames sent by the GO.
---
 src/p2p.c | 191 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 180 insertions(+), 11 deletions(-)

diff --git a/src/p2p.c b/src/p2p.c
index 50559bae..7717d838 100644
--- a/src/p2p.c
+++ b/src/p2p.c
@@ -158,6 +158,10 @@ struct p2p_wfd_properties {
 	bool uibc;
 	bool cp;
 	bool r2;
+	uint16_t raw_dev_info;
+	uint8_t associated_bssid[6];
+	uint8_t raw_coupled_sink_status;
+	uint8_t coupled_sink_mac[6];
 };
 
 static struct l_queue *p2p_device_list;
@@ -272,9 +276,9 @@ static void p2p_peer_put(void *user_data)
 static void p2p_device_discovery_start(struct p2p_device *dev);
 static void p2p_device_discovery_stop(struct p2p_device *dev);
 
-/* Callers should reserve 32 bytes */
+/* Callers should reserve 32 bytes, 64 with non-NULL @wfd_clients */
 static size_t p2p_build_wfd_ie(const struct p2p_wfd_properties *wfd,
-				uint8_t *buf)
+				const struct p2p_peer *wfd_client, uint8_t *buf)
 {
 	/*
 	 * Wi-Fi Display Technical Specification v2.1.0
@@ -319,6 +323,30 @@ static size_t p2p_build_wfd_ie(const struct p2p_wfd_properties *wfd,
 		buf[size++] = 0x01;	/* UIBC Support */
 	}
 
+	/*
+	 * Wi-Fi Display Technical Specification v2.1.0 section 5.2.3:
+	 * "If a WFD Capable GO has at least one associated client that is
+	 * WFD capable, the WFD capable GO shall include the WFD Session
+	 * Information subelement in the WFD IE in the Probe Response
+	 * frames it transmits."
+	 */
+	if (wfd_client && !L_WARN_ON(!wfd_client->wfd)) {
+		buf[size++] = WFD_SUBELEM_SESION_INFORMATION;
+		buf[size++] = 0;		/* WFD Subelement length */
+		buf[size++] = 23;
+		memcpy(buf + size, wfd_client->device_addr, 6);
+		size += 6;
+		memcpy(buf + size, wfd_client->wfd->associated_bssid, 6);
+		size += 6;
+		buf[size++] = wfd_client->wfd->raw_dev_info >> 8;
+		buf[size++] = wfd_client->wfd->raw_dev_info & 255;
+		buf[size++] = wfd_client->wfd->throughput >> 8;
+		buf[size++] = wfd_client->wfd->throughput & 255;
+		buf[size++] = wfd_client->wfd->raw_coupled_sink_status;
+		memcpy(buf + size, wfd_client->wfd->coupled_sink_mac, 6);
+		size += 6;
+	}
+
 	if (wfd->r2) {
 		buf[size++] = WFD_SUBELEM_R2_DEVICE_INFORMATION;
 		buf[size++] = 0;	/* WFD Subelement length */
@@ -336,6 +364,8 @@ static bool p2p_extract_wfd_properties(const uint8_t *ie, size_t ie_size,
 {
 	struct wfd_subelem_iter iter;
 	const uint8_t *devinfo = NULL;
+	const uint8_t *associated_bssid = NULL;
+	const uint8_t *coupled_sink_info = NULL;
 	const uint8_t *ext_caps = NULL;
 	const uint8_t *r2 = NULL;
 
@@ -365,6 +395,13 @@ static bool p2p_extract_wfd_properties(const uint8_t *ie, size_t ie_size,
 		case WFD_SUBELEM_WFD_DEVICE_INFORMATION:
 			SUBELEM_CHECK(devinfo, 6, "Device Information");
 			break;
+		case WFD_SUBELEM_ASSOCIATED_BSSID:
+			SUBELEM_CHECK(associated_bssid, 6, "Associated BSSID");
+			break;
+		case WFD_SUBELEM_COUPLED_SINK_INFORMATION:
+			SUBELEM_CHECK(coupled_sink_info, 7,
+					"Coupled Sink Information");
+			break;
 		case WFD_SUBELEM_EXTENDED_CAPABILITY:
 			SUBELEM_CHECK(ext_caps, 2, "Extended Capability");
 			break;
@@ -413,11 +450,20 @@ static bool p2p_extract_wfd_properties(const uint8_t *ie, size_t ie_size,
 		out->cp = capability & WFD_DEV_INFO_CONTENT_PROTECTION_SUPPORT;
 		out->audio = !sink ||
 			!(capability & WFD_DEV_INFO_NO_AUDIO_AT_PRIMARY_SINK);
+		out->raw_dev_info = l_get_be16(devinfo);
 	} else {
 		l_error("Device Information missing in WFD IE");
 		return false;
 	}
 
+	if (associated_bssid)
+		memcpy(out->associated_bssid, associated_bssid, 6);
+
+	if (coupled_sink_info) {
+		out->raw_coupled_sink_status = coupled_sink_info[0];
+		memcpy(out->coupled_sink_mac, coupled_sink_info + 1, 6);
+	}
+
 	if (ext_caps && (l_get_be16(ext_caps) & 1))
 		out->uibc = 1;
 
@@ -554,7 +600,7 @@ static uint8_t *p2p_build_scan_ies(struct p2p_device *dev, uint8_t *buf,
 		return NULL;
 
 	if (p2p_own_wfd)
-		wfd_ie_size = p2p_build_wfd_ie(p2p_own_wfd, wfd_ie);
+		wfd_ie_size = p2p_build_wfd_ie(p2p_own_wfd, NULL, wfd_ie);
 	else
 		wfd_ie_size = 0;
 
@@ -917,7 +963,9 @@ static void p2p_group_event(enum ap_event_type type, const void *event_data,
 		/* Don't validate the P2P IE or WFD IE at this stage */
 		break;
 	case AP_EVENT_REGISTRATION_SUCCESS:
+		/* Update the Group Formation bit in our beacons */
 		dev->capability.group_caps &= ~P2P_GROUP_CAP_GROUP_FORMATION;
+		ap_update_beacon(dev->group);
 		break;
 	case AP_EVENT_PBC_MODE_EXIT:
 		break;
@@ -931,8 +979,126 @@ invalid_ie:
 	p2p_connect_failed(dev);
 }
 
+static size_t p2p_group_get_ies_len(enum mpdu_management_subtype type,
+					void *user_data)
+{
+	switch (type) {
+	case MPDU_MANAGEMENT_SUBTYPE_ASSOCIATION_RESPONSE:
+	case MPDU_MANAGEMENT_SUBTYPE_REASSOCIATION_RESPONSE:
+	case MPDU_MANAGEMENT_SUBTYPE_PROBE_RESPONSE:
+	case MPDU_MANAGEMENT_SUBTYPE_BEACON:
+		return 256;
+	default:
+		return 0;
+	}
+}
+
+static size_t p2p_group_write_ies(enum mpdu_management_subtype type,
+					uint8_t *out_buf, void *user_data)
+{
+	struct p2p_device *dev = user_data;
+	uint8_t *ptr = out_buf;
+	L_AUTO_FREE_VAR(uint8_t *, p2p_ie) = NULL;
+	size_t p2p_ie_len;
+	struct p2p_wfd_properties *wfd = NULL;
+	const struct p2p_peer *wfd_client = NULL;
+
+	switch (type) {
+	case MPDU_MANAGEMENT_SUBTYPE_ASSOCIATION_RESPONSE:
+	case MPDU_MANAGEMENT_SUBTYPE_REASSOCIATION_RESPONSE:
+	{
+		/*
+		 * Wi-Fi P2P Technical Specification v1.7 Section 4.2.5:
+		 * "If neither P2P attribute is required according to the
+		 * conditions in Table 55, then a P2P IE containing no P2P
+		 * attributes is included."
+		 * This is going to be our case.
+		 */
+		struct p2p_association_resp info = {};
+
+		p2p_ie = p2p_build_association_resp(&info, &p2p_ie_len);
+
+		if (dev->conn_own_wfd && !dev->conn_own_wfd->r2)
+			wfd = dev->conn_own_wfd;
+
+		break;
+	}
+
+	case MPDU_MANAGEMENT_SUBTYPE_PROBE_RESPONSE:
+	{
+		/*
+		 * 3.2.2: "A P2P Group Owner shall not include a P2P IE
+		 * in the Probe Response frame if the received Probe Request
+		 * frame does not contain a P2P IE."
+		 * For simplicity include it always, same applies to the
+		 * WFD IE too.
+		 */
+		struct p2p_probe_resp info = {};
+
+		info.capability = dev->capability;
+		info.device_info = dev->device_info;
+
+		if (dev->conn_peer_added) {
+			struct p2p_client_info_descriptor client = {};
+
+			memcpy(client.device_addr,
+				dev->conn_client_dev_info.device_addr, 6);
+			memcpy(client.interface_addr,
+				dev->conn_peer_interface_addr, 6);
+			client.device_caps =
+				dev->conn_client_capability.device_caps;
+			client.wsc_config_methods =
+				dev->conn_client_dev_info.wsc_config_methods;
+			client.primary_device_type =
+				dev->conn_client_dev_info.primary_device_type;
+			l_strlcpy(client.device_name,
+					dev->conn_client_dev_info.device_name,
+					sizeof(client.device_name));
+
+			info.group_clients = l_queue_new();
+			l_queue_push_tail(info.group_clients,
+					l_memdup(&client, sizeof(client)));
+		}
+
+		p2p_ie = p2p_build_probe_resp(&info, &p2p_ie_len);
+		p2p_clear_probe_resp(&info);
+
+		if (dev->conn_own_wfd)
+			wfd_client = dev->conn_peer;
+
+		wfd = p2p_own_wfd;
+		break;
+	}
+
+	case MPDU_MANAGEMENT_SUBTYPE_BEACON:
+	{
+		struct p2p_beacon info = {};
+
+		info.capability = dev->capability;
+		memcpy(info.device_addr, dev->addr, 6);
+		p2p_ie = p2p_build_beacon(&info, &p2p_ie_len);
+
+		wfd = p2p_own_wfd;
+		break;
+	}
+
+	default:
+		return 0;
+	}
+
+	memcpy(ptr, p2p_ie, p2p_ie_len);
+	ptr += p2p_ie_len;
+
+	if (wfd)
+		ptr += p2p_build_wfd_ie(wfd, wfd_client, ptr);
+
+	return ptr - out_buf;
+}
+
 static const struct ap_ops p2p_go_ops = {
 	.handle_event = p2p_group_event,
+	.get_mgmt_frame_ies_len = p2p_group_get_ies_len,
+	.write_mgmt_frame_ies = p2p_group_write_ies,
 };
 
 static void p2p_group_start(struct p2p_device *dev)
@@ -1190,7 +1356,7 @@ static void p2p_try_connect_group(struct p2p_device *dev)
 	if (dev->conn_own_wfd) {
 		ie_iov[ie_num].iov_base = wfd_ie;
 		ie_iov[ie_num].iov_len = p2p_build_wfd_ie(dev->conn_own_wfd,
-								wfd_ie);
+								NULL, wfd_ie);
 		ie_num++;
 	}
 
@@ -1336,7 +1502,7 @@ static void p2p_provision_connect(struct p2p_device *dev)
 	if (dev->conn_own_wfd) {
 		iov[iov_num].iov_base = wfd_ie;
 		iov[iov_num].iov_len = p2p_build_wfd_ie(dev->conn_own_wfd,
-							wfd_ie);
+							NULL, wfd_ie);
 		iov_num++;
 	}
 
@@ -2165,7 +2331,7 @@ respond:
 	if (dev->conn_own_wfd) {
 		resp_info.wfd = wfd_ie;
 		resp_info.wfd_size = p2p_build_wfd_ie(dev->conn_own_wfd,
-							wfd_ie);
+							NULL, wfd_ie);
 	}
 
 	resp_body = p2p_build_go_negotiation_resp(&resp_info, &resp_len);
@@ -2441,7 +2607,7 @@ static bool p2p_go_negotiation_resp_cb(const struct mmpdu_header *mpdu,
 	if (dev->conn_own_wfd) {
 		confirm_info.wfd = wfd_ie;
 		confirm_info.wfd_size = p2p_build_wfd_ie(dev->conn_own_wfd,
-								wfd_ie);
+								NULL, wfd_ie);
 	}
 
 	confirm_body = p2p_build_go_negotiation_confirmation(&confirm_info,
@@ -2528,7 +2694,8 @@ static void p2p_start_go_negotiation(struct p2p_device *dev)
 
 	if (dev->conn_own_wfd) {
 		info.wfd = wfd_ie;
-		info.wfd_size = p2p_build_wfd_ie(dev->conn_own_wfd, wfd_ie);
+		info.wfd_size = p2p_build_wfd_ie(dev->conn_own_wfd,
+							NULL, wfd_ie);
 	}
 
 	req_body = p2p_build_go_negotiation_req(&info, &req_len);
@@ -2665,7 +2832,8 @@ static void p2p_start_provision_discovery(struct p2p_device *dev)
 
 	if (dev->conn_own_wfd) {
 		info.wfd = wfd_ie;
-		info.wfd_size = p2p_build_wfd_ie(dev->conn_own_wfd, wfd_ie);
+		info.wfd_size = p2p_build_wfd_ie(dev->conn_own_wfd,
+							NULL, wfd_ie);
 	}
 
 	req_body = p2p_build_provision_disc_req(&info, &req_len);
@@ -3631,11 +3799,12 @@ static void p2p_device_send_probe_resp(struct p2p_device *dev,
 	if (to_conn_peer && dev->conn_own_wfd) {
 		iov[iov_len].iov_base = wfd_ie;
 		iov[iov_len].iov_len = p2p_build_wfd_ie(dev->conn_own_wfd,
-							wfd_ie);
+							NULL, wfd_ie);
 		iov_len++;
 	} else if (p2p_own_wfd) {
 		iov[iov_len].iov_base = wfd_ie;
-		iov[iov_len].iov_len = p2p_build_wfd_ie(p2p_own_wfd, wfd_ie);
+		iov[iov_len].iov_len = p2p_build_wfd_ie(p2p_own_wfd,
+							NULL, wfd_ie);
 		iov_len++;
 	}
 
-- 
2.25.1

Re: [PATCH 01/17] ap: Fix iteration over ap->rates

[Denis Kenzior]

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

Hi Andrew,

On 9/8/20 6:49 PM, Andrew Zaborowski wrote:
> In the for loops use a condition that works even if the uint32_t values
> overflow since l_uintset_find_min/max() return UINT_MAX for empty sets.

So it only returns UINT_MAX for set objects that are NULL, not empty.   How are 
you even passing in NULL objects that you're hitting this condition?  That seems 
to be a logic error?

> If the condition is r <= l_uintset_find_max() or similar, it's always
> true.
> 
> Make sure ap->rates is non-NULL both with and without no_cck_rates.
> 
> Update "count" in the for loop in ap_build_beacon_pr_head()
> ---
>   src/ap.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/src/ap.c b/src/ap.c
> index 1cee185c..84d4539a 100644
> --- a/src/ap.c
> +++ b/src/ap.c
> @@ -371,7 +371,7 @@ static size_t ap_build_beacon_pr_head(struct ap_state *ap,
>   	minr = l_uintset_find_min(ap->rates);
>   	maxr = l_uintset_find_max(ap->rates);
>   	count = 0;
> -	for (r = minr; r <= maxr && count < 8; r++)
> +	for (r = minr; (int32_t) (maxr - r) >= 0 && count < 8; r++)
>   		if (l_uintset_contains(ap->rates, r)) {
>   			uint8_t flag = 0;

Looking at this code closely I totally hate how this is done.  You are only 
going to be calling l_uintset_contains a few million times even though you know 
the basic rates are only 7 bits and the set cannot have more than 127 entries.

>   
> @@ -380,10 +380,10 @@ static size_t ap_build_beacon_pr_head(struct ap_state *ap,
>   				flag = 0x80;
>   
>   			*rates++ = r | flag;
> +			count++;
>   		}
>   
> -	ie_tlv_builder_set_length(&builder, rates -
> -					ie_tlv_builder_get_data(&builder));
> +	ie_tlv_builder_set_length(&builder, count);
>   
>   	/* DSSS Parameter Set IE for DSSS, HR, ERP and HT PHY rates */
>   	ie_tlv_builder_next(&builder, IE_TYPE_DSSS_PARAMETER_SET);
> @@ -983,7 +983,7 @@ static void ap_associate_sta(struct ap_state *ap, struct sta_state *sta)
>   	minr = l_uintset_find_min(sta->rates);
>   	maxr = l_uintset_find_max(sta->rates);
>   
> -	for (r = minr; r <= maxr; r++)
> +	for (r = minr; (int32_t) (maxr - r) >= 0; r++)
>   		if (l_uintset_contains(sta->rates, r))
>   			rates[count++] = r;
>   
> @@ -1089,7 +1089,7 @@ static uint32_t ap_assoc_resp(struct ap_state *ap, struct sta_state *sta,
>   	minr = l_uintset_find_min(ap->rates);
>   	maxr = l_uintset_find_max(ap->rates);
>   	count = 0;
> -	for (r = minr; r <= maxr && count < 8; r++)
> +	for (r = minr; (int32_t) (maxr - r) >= 0 && count < 8; r++)
>   		if (l_uintset_contains(ap->rates, r)) {
>   			uint8_t flag = 0;
>   
> @@ -2071,6 +2071,7 @@ struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
>   	ap->ciphers = wiphy_select_cipher(wiphy, 0xffff);
>   	ap->group_cipher = wiphy_select_cipher(wiphy, 0xffff);
>   	ap->beacon_interval = 100;
> +	ap->rates = l_uintset_new(200);
>   
>   	/* TODO: Pick from actual supported rates */
>   	if (config->no_cck_rates) {
> @@ -2083,7 +2084,6 @@ struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
>   		l_uintset_put(ap->rates, 96); /* 48 Mbps*/
>   		l_uintset_put(ap->rates, 108); /* 54 Mbps*/
>   	} else {
> -		ap->rates = l_uintset_new(200);
>   		l_uintset_put(ap->rates, 2); /* 1 Mbps*/
>   		l_uintset_put(ap->rates, 11); /* 5.5 Mbps*/
>   		l_uintset_put(ap->rates, 22); /* 11 Mbps*/
> 

Regards,
-Denis

Re: [PATCH 01/17] ap: Fix iteration over ap->rates

[Denis Kenzior]

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

Hi Andrew,

On 9/9/20 11:51 AM, Andrew Zaborowski wrote:
> Hi Denis,
> 
> On Wed, 9 Sep 2020 at 17:37, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 9/8/20 6:49 PM, Andrew Zaborowski wrote:
>>> In the for loops use a condition that works even if the uint32_t values
>>> overflow since l_uintset_find_min/max() return UINT_MAX for empty sets.
>>
>> So it only returns UINT_MAX for set objects that are NULL, not empty.
> 
> Ah, true.  If it's empty we'll get 201 as set->max is 200.

So why is the set 200 anyway when the max is 127? ;)

> 
>> How are
>> you even passing in NULL objects that you're hitting this condition?  That seems
>> to be a logic error?
> 
> It is/was, this is the other fix in this patch.

Okay, then the rest of it is a big hammer for no good reason.  I'd just fix the 
logic error where ap->rates was NULL and leave the rest as is.

Or use l_uintset_foreach...

>>> -     for (r = minr; r <= maxr && count < 8; r++)
>>> +     for (r = minr; (int32_t) (maxr - r) >= 0 && count < 8; r++)
>>>                if (l_uintset_contains(ap->rates, r)) {
>>>                        uint8_t flag = 0;
>>
>> Looking at this code closely I totally hate how this is done.  You are only
>> going to be calling l_uintset_contains a few million times even though you know
>> the basic rates are only 7 bits and the set cannot have more than 127 entries.
> 
> If the set contains rates between 2 and 22 we only make 11 calls, the
> loop iterates from l_uintset_find_min(ap->rates) to
> l_uintset_find_max(ap->rates), or am I missing something?  If the set
> is empty we make one call... we could replace that with an isempty()
> call if needed.

If the set is valid, sure.  But if the set wasn't valid you're iterating 2 
billion times or something, when a static 0..127 would have been enough.

> 
> Granted we're iterating over values that don't correspond to standard
> rates, we could store indexes into the array of known rates to save
> some iterations.
> 

Maybe consider just using wiphy_get_supported_rates() instead.

Regards,
-Denis

Re: [PATCH 01/17] ap: Fix iteration over ap->rates

[Andrew Zaborowski]

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

Hi Denis,

On Wed, 9 Sep 2020 at 17:37, Denis Kenzior <denkenz@gmail.com> wrote:
> On 9/8/20 6:49 PM, Andrew Zaborowski wrote:
> > In the for loops use a condition that works even if the uint32_t values
> > overflow since l_uintset_find_min/max() return UINT_MAX for empty sets.
>
> So it only returns UINT_MAX for set objects that are NULL, not empty.

Ah, true.  If it's empty we'll get 201 as set->max is 200.

> How are
> you even passing in NULL objects that you're hitting this condition?  That seems
> to be a logic error?

It is/was, this is the other fix in this patch.

>
> > If the condition is r <= l_uintset_find_max() or similar, it's always
> > true.
> >
> > Make sure ap->rates is non-NULL both with and without no_cck_rates.
> >
> > Update "count" in the for loop in ap_build_beacon_pr_head()
> > ---
> >   src/ap.c | 12 ++++++------
> >   1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/ap.c b/src/ap.c
> > index 1cee185c..84d4539a 100644
> > --- a/src/ap.c
> > +++ b/src/ap.c
> > @@ -371,7 +371,7 @@ static size_t ap_build_beacon_pr_head(struct ap_state *ap,
> >       minr = l_uintset_find_min(ap->rates);
> >       maxr = l_uintset_find_max(ap->rates);
> >       count = 0;
> > -     for (r = minr; r <= maxr && count < 8; r++)
> > +     for (r = minr; (int32_t) (maxr - r) >= 0 && count < 8; r++)
> >               if (l_uintset_contains(ap->rates, r)) {
> >                       uint8_t flag = 0;
>
> Looking at this code closely I totally hate how this is done.  You are only
> going to be calling l_uintset_contains a few million times even though you know
> the basic rates are only 7 bits and the set cannot have more than 127 entries.

If the set contains rates between 2 and 22 we only make 11 calls, the
loop iterates from l_uintset_find_min(ap->rates) to
l_uintset_find_max(ap->rates), or am I missing something?  If the set
is empty we make one call... we could replace that with an isempty()
call if needed.

Granted we're iterating over values that don't correspond to standard
rates, we could store indexes into the array of known rates to save
some iterations.

>
> >
> > @@ -2071,6 +2071,7 @@ struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
> >       ap->ciphers = wiphy_select_cipher(wiphy, 0xffff);
> >       ap->group_cipher = wiphy_select_cipher(wiphy, 0xffff);
> >       ap->beacon_interval = 100;
> > +     ap->rates = l_uintset_new(200);
> >
> >       /* TODO: Pick from actual supported rates */
> >       if (config->no_cck_rates) {
> > @@ -2083,7 +2084,6 @@ struct ap_state *ap_start(struct netdev *netdev, struct ap_config *config,
> >               l_uintset_put(ap->rates, 96); /* 48 Mbps*/
> >               l_uintset_put(ap->rates, 108); /* 54 Mbps*/
> >       } else {
> > -             ap->rates = l_uintset_new(200);
> >               l_uintset_put(ap->rates, 2); /* 1 Mbps*/

These two changes fix not allocating ap->rates if no_cck_rates was
set, this is why it was NULL.

Best regards

Re: [PATCH 02/17] scan: Add optional source_mac scan parameter

[Denis Kenzior]

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

Hi Andrew,

On 9/8/20 6:49 PM, Andrew Zaborowski wrote:
> This is similar to randomize_mac_addr_hint but it sets a specific source
> MAC address for our probe frames.
> ---
>   src/scan.c | 12 ++++++++++++
>   src/scan.h |  1 +
>   2 files changed, 13 insertions(+)
> 

Patch 2 - 7 applied, thanks.

Regards,
-Denis

Re: [PATCH 08/17] p2putil: Add p2p_random_char

[Denis Kenzior]

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

Hi Andrew,

On 9/8/20 6:49 PM, Andrew Zaborowski wrote:
> Add a utility to select random characters from the set defined in P2P
> v1.7 Section 3.2.1.
> ---
>   src/p2putil.c | 26 ++++++++++++++++++++++++++
>   src/p2putil.h |  2 ++
>   2 files changed, 28 insertions(+)
> 
> diff --git a/src/p2putil.c b/src/p2putil.c
> index d0f3a444..99ab4ca0 100644
> --- a/src/p2putil.c
> +++ b/src/p2putil.c
> @@ -2611,3 +2611,29 @@ uint8_t *p2p_build_go_disc_req(size_t *out_len)
>   	return p2p_build_action_frame(false, P2P_ACTION_GO_DISCOVERABILITY_REQ,
>   					0, NULL, NULL, NULL, 0, out_len);
>   }
> +
> +/*
> + * Select a random char from the set defined in section 3.2.1:
> + *
> + * "Following "DIRECT-" the SSID shall contain two ASCII characters "xy",
> + * randomly selected with a uniform distribution from the following
> + * character set: upper case letters, lower case letters and numbers."
> + *
> + * "The WPA2-Personal pass-phrase shall contain at least eight ASCII
> + * characters randomly selected with a uniform distribution from the
> + * following character set: upper case letters, lower case letters and
> + * numbers."
> + */
> +char p2p_random_char(void)
> +{
> +#define CHARSET "ABCDEFGHIJKLMNOPQRSTUVWXYZ" "abcdefghijklmnopqrstuvwxyz" \
> +		"0123456789"

might as well make this static char[]

> +	static const int set_size = strlen(CHARSET);
> +	static const uint32_t limit = -(0x100000000LL % set_size);

I find this written like:

limit = UINT_MAX - UINTMAX % set_size

a bit more clear.

> +	uint32_t index;
> +
> +	while ((index = l_getrandom_uint32()) >= limit);

So I actually wonder if this should be an ell utility.  I'm not too sure I like 
the idea of hitting getrandom in a loop.  The chances are pretty small but 
still...  Perhaps using rand_r  with a random seed would be better.

But if you want to direct code, then in theory you require only 5 bits of 
randomness here since the set is 62 long.  You could make due with just grabbing 
a single byte of randomness and you'd have 3 attempts at being in the range (low 
5 bits, then 6..1, 7..2)

Maybe I'm overthinking this, what do you think?

> +
> +	return CHARSET[index % set_size];

I'm sure this syntax works, but man I find this weird ;)

> +#undef CHARSET
> +}

Regards,
-Denis

Re: [PATCH 12/17] ap: Accept P2P wildcard SSIDs in probe requests

[Denis Kenzior]

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

Hi Andrew,

On 9/8/20 6:49 PM, Andrew Zaborowski wrote:
> Add the special case "DIRECT-" SSID, called the P2P Wildcard SSID, in
> ap_probe_req_cb so as not to reject those Probe Requests on the basis of
> ssid mismatch.  I'd have preferred to keep all the P2P-specific bits in
> p2p.c but in this case there's little point in adding a generic
> config setting for SSID-matching quirks.
> ---
>   src/ap.c | 3 +++
>   1 file changed, 3 insertions(+)
> 

Applied, thanks.

Regards,
-Denis

Re: [PATCH 17/17] p2p: Build P2P and WFD IEs for group's management frames

[Denis Kenzior]

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

Hi Andrew,

On 9/8/20 6:49 PM, Andrew Zaborowski wrote:
> Handle our P2P group's ops.write_mgmt_frame_ies callbacks to build and
> attach the necessary P2P IE and WFD IEs to the (Re)Association Response,
> Probe Response and Beacon frames sent by the GO.
> ---
>   src/p2p.c | 191 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 180 insertions(+), 11 deletions(-)
> 
<snip>

> @@ -931,8 +979,126 @@ invalid_ie:
>   	p2p_connect_failed(dev);
>   }
>   
> +static size_t p2p_group_get_ies_len(enum mpdu_management_subtype type,
> +					void *user_data)
> +{
> +	switch (type) {
> +	case MPDU_MANAGEMENT_SUBTYPE_ASSOCIATION_RESPONSE:
> +	case MPDU_MANAGEMENT_SUBTYPE_REASSOCIATION_RESPONSE:
> +	case MPDU_MANAGEMENT_SUBTYPE_PROBE_RESPONSE:
> +	case MPDU_MANAGEMENT_SUBTYPE_BEACON:
> +		return 256;

This seems rather brittle

> +	default:
> +		return 0;
> +	}
> +}
> +
> +static size_t p2p_group_write_ies(enum mpdu_management_subtype type,
> +					uint8_t *out_buf, void *user_data)
> +{
> +	struct p2p_device *dev = user_data;
> +	uint8_t *ptr = out_buf;
> +	L_AUTO_FREE_VAR(uint8_t *, p2p_ie) = NULL;
> +	size_t p2p_ie_len;
> +	struct p2p_wfd_properties *wfd = NULL;
> +	const struct p2p_peer *wfd_client = NULL;
> +
> +	switch (type) {
> +	case MPDU_MANAGEMENT_SUBTYPE_ASSOCIATION_RESPONSE:
> +	case MPDU_MANAGEMENT_SUBTYPE_REASSOCIATION_RESPONSE:
> +	{
> +		/*
> +		 * Wi-Fi P2P Technical Specification v1.7 Section 4.2.5:
> +		 * "If neither P2P attribute is required according to the
> +		 * conditions in Table 55, then a P2P IE containing no P2P
> +		 * attributes is included."
> +		 * This is going to be our case.
> +		 */
> +		struct p2p_association_resp info = {};
> +
> +		p2p_ie = p2p_build_association_resp(&info, &p2p_ie_len);
> +
> +		if (dev->conn_own_wfd && !dev->conn_own_wfd->r2)
> +			wfd = dev->conn_own_wfd;
> +
> +		break;
> +	}
> +
> +	case MPDU_MANAGEMENT_SUBTYPE_PROBE_RESPONSE:
> +	{
> +		/*
> +		 * 3.2.2: "A P2P Group Owner shall not include a P2P IE
> +		 * in the Probe Response frame if the received Probe Request
> +		 * frame does not contain a P2P IE."
> +		 * For simplicity include it always, same applies to the
> +		 * WFD IE too.

???

> +		 */
> +		struct p2p_probe_resp info = {};
> +
> +		info.capability = dev->capability;
> +		info.device_info = dev->device_info;
> +
> +		if (dev->conn_peer_added) {
> +			struct p2p_client_info_descriptor client = {};
> +
> +			memcpy(client.device_addr,
> +				dev->conn_client_dev_info.device_addr, 6);
> +			memcpy(client.interface_addr,
> +				dev->conn_peer_interface_addr, 6);
> +			client.device_caps =
> +				dev->conn_client_capability.device_caps;
> +			client.wsc_config_methods =
> +				dev->conn_client_dev_info.wsc_config_methods;
> +			client.primary_device_type =
> +				dev->conn_client_dev_info.primary_device_type;
> +			l_strlcpy(client.device_name,
> +					dev->conn_client_dev_info.device_name,
> +					sizeof(client.device_name));
> +
> +			info.group_clients = l_queue_new();
> +			l_queue_push_tail(info.group_clients,
> +					l_memdup(&client, sizeof(client)));
> +		}
> +
> +		p2p_ie = p2p_build_probe_resp(&info, &p2p_ie_len);
> +		p2p_clear_probe_resp(&info);
> +
> +		if (dev->conn_own_wfd)
> +			wfd_client = dev->conn_peer;
> +
> +		wfd = p2p_own_wfd;
> +		break;
> +	}
> +
> +	case MPDU_MANAGEMENT_SUBTYPE_BEACON:
> +	{
> +		struct p2p_beacon info = {};
> +
> +		info.capability = dev->capability;
> +		memcpy(info.device_addr, dev->addr, 6);
> +		p2p_ie = p2p_build_beacon(&info, &p2p_ie_len);
> +
> +		wfd = p2p_own_wfd;
> +		break;
> +	}
> +
> +	default:
> +		return 0;
> +	}
> +
> +	memcpy(ptr, p2p_ie, p2p_ie_len);
> +	ptr += p2p_ie_len;

So are you leaking p2p_ie here?

And also, p2p_build utilities use a dynamically-growing buffer to build all the 
attributes and ie_tlv_encapsulate_p2p_payload can produce a ton of IEs, not just 
a single one.  So I think you need something a bit smarter than 
get_mgmt_frame_ies_len() method.

Perhaps ie_tlv_builder should be passed in directly and that entire class be 
made smarter.  Including adding the ability to encapsulate p2p/wsc/vendor TLVs.

> +
> +	if (wfd)
> +		ptr += p2p_build_wfd_ie(wfd, wfd_client, ptr);
> +
> +	return ptr - out_buf;
> +}
> +

Regards,
-Denis

Re: [PATCH 14/17] ap: Pass extra IEs between mgmt frames and user

[Denis Kenzior]

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

Hi Andrew,

On 9/8/20 6:49 PM, Andrew Zaborowski wrote:
> Add user callbacks to add extra IEs to the AP's beacons and other
> frames.  I considered a few ways for the user to pass this data
> without callbacks but so far this seems to be the easiest and most
> versatile way.
> 
> Similarly pass the string of IEs from STA association frames to the user
> through the AP events.  I dropped ap_event_station_added_data.rsn_ie
> because that probably would have not been very useful and the RSN IE
> is included in the IE array in any case.
> ---
>   src/ap.c | 57 +++++++++++++++++++++++++++++++++++++++++++++-----------
>   src/ap.h | 10 +++++++++-
>   2 files changed, 55 insertions(+), 12 deletions(-)
> 

<snip>

> @@ -505,7 +514,11 @@ static void ap_set_beacon_cb(struct l_genl_msg *msg, void *user_data)
>   static void ap_update_beacon(struct ap_state *ap)
>   {
>   	struct l_genl_msg *cmd;
> -	uint8_t head[256], tail[256];
> +	size_t extra_ies_len = ap->ops->get_mgmt_frame_ies_len ?
> +		ap->ops->get_mgmt_frame_ies_len(MPDU_MANAGEMENT_SUBTYPE_BEACON,
> +						ap->user_data) : 0;
> +	uint8_t head[256];
> +	uint8_t tail[256 + extra_ies_len];
>   	size_t head_len, tail_len;
>   	uint64_t wdev_id = netdev_get_wdev_id(ap->netdev);
>   	static const uint8_t bcast_addr[6] = {

Given that this is semi-static, and the caller has to call ap_update_beacon 
anyway, wouldn't it make more sense to just have the caller pass in the extra 
ies so you could just use append_attrv() on the NL80211_ATTR_BEACON_TAIL?

Regards,
-Denis

Re: [PATCH 01/17] ap: Fix iteration over ap->rates

[Andrew Zaborowski]

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

Hi Denis,

On Wed, 9 Sep 2020 at 19:02, Denis Kenzior <denkenz@gmail.com> wrote:
> On 9/9/20 11:51 AM, Andrew Zaborowski wrote:
> > On Wed, 9 Sep 2020 at 17:37, Denis Kenzior <denkenz@gmail.com> wrote:
> >> On 9/8/20 6:49 PM, Andrew Zaborowski wrote:
> >>> In the for loops use a condition that works even if the uint32_t values
> >>> overflow since l_uintset_find_min/max() return UINT_MAX for empty sets.
> >>
> >> So it only returns UINT_MAX for set objects that are NULL, not empty.
> >
> > Ah, true.  If it's empty we'll get 201 as set->max is 200.
>
> So why is the set 200 anyway when the max is 127? ;)

I don't remember, I guess I could fix it in this same patch.

>
> >
> >> How are
> >> you even passing in NULL objects that you're hitting this condition?  That seems
> >> to be a logic error?
> >
> > It is/was, this is the other fix in this patch.
>
> Okay, then the rest of it is a big hammer for no good reason.  I'd just fix the
> logic error where ap->rates was NULL and leave the rest as is.

I thought that was a very small change and since the uintset functions
under some conditions do return UINT_MAX maybe it's the better way to
write those loops.  But I can drop this change.

>
> Or use l_uintset_foreach...
>
> >>> -     for (r = minr; r <= maxr && count < 8; r++)
> >>> +     for (r = minr; (int32_t) (maxr - r) >= 0 && count < 8; r++)
> >>>                if (l_uintset_contains(ap->rates, r)) {
> >>>                        uint8_t flag = 0;
> >>
> >> Looking at this code closely I totally hate how this is done.  You are only
> >> going to be calling l_uintset_contains a few million times even though you know
> >> the basic rates are only 7 bits and the set cannot have more than 127 entries.
> >
> > If the set contains rates between 2 and 22 we only make 11 calls, the
> > loop iterates from l_uintset_find_min(ap->rates) to
> > l_uintset_find_max(ap->rates), or am I missing something?  If the set
> > is empty we make one call... we could replace that with an isempty()
> > call if needed.
>
> If the set is valid, sure.  But if the set wasn't valid you're iterating 2
> billion times or something,

You're iterating indefinitely.

Oh ok, yes, the original code contained an overflow bug, once I
understood it I didn't like it either ;-)  Hence the fix.

> when a static 0..127 would have been enough.

For an invalid set, 0..127 is still wasteful ;-)

>
> >
> > Granted we're iterating over values that don't correspond to standard
> > rates, we could store indexes into the array of known rates to save
> > some iterations.
> >
>
> Maybe consider just using wiphy_get_supported_rates() instead.

Right, but we're giving the user (of the ap.h API) some control over
which rates to support so the plan is to eventually call it but during
ap_start only.

Best regards

Re: [PATCH 08/17] p2putil: Add p2p_random_char

[Andrew Zaborowski]

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

On Wed, 9 Sep 2020 at 21:16, Denis Kenzior <denkenz@gmail.com> wrote:
> On 9/8/20 6:49 PM, Andrew Zaborowski wrote:
> > Add a utility to select random characters from the set defined in P2P
> > v1.7 Section 3.2.1.
> > ---
> >   src/p2putil.c | 26 ++++++++++++++++++++++++++
> >   src/p2putil.h |  2 ++
> >   2 files changed, 28 insertions(+)
> >
> > diff --git a/src/p2putil.c b/src/p2putil.c
> > index d0f3a444..99ab4ca0 100644
> > --- a/src/p2putil.c
> > +++ b/src/p2putil.c
> > @@ -2611,3 +2611,29 @@ uint8_t *p2p_build_go_disc_req(size_t *out_len)
> >       return p2p_build_action_frame(false, P2P_ACTION_GO_DISCOVERABILITY_REQ,
> >                                       0, NULL, NULL, NULL, 0, out_len);
> >   }
> > +
> > +/*
> > + * Select a random char from the set defined in section 3.2.1:
> > + *
> > + * "Following "DIRECT-" the SSID shall contain two ASCII characters "xy",
> > + * randomly selected with a uniform distribution from the following
> > + * character set: upper case letters, lower case letters and numbers."
> > + *
> > + * "The WPA2-Personal pass-phrase shall contain at least eight ASCII
> > + * characters randomly selected with a uniform distribution from the
> > + * following character set: upper case letters, lower case letters and
> > + * numbers."
> > + */
> > +char p2p_random_char(void)
> > +{
> > +#define CHARSET "ABCDEFGHIJKLMNOPQRSTUVWXYZ" "abcdefghijklmnopqrstuvwxyz" \
> > +             "0123456789"
>
> might as well make this static char[]
>
> > +     static const int set_size = strlen(CHARSET);
> > +     static const uint32_t limit = -(0x100000000LL % set_size);
>
> I find this written like:
>
> limit = UINT_MAX - UINTMAX % set_size
>
> a bit more clear.

Ok, maybe UINT32_MAX to make it clear it's based on 32-bit input.

>
> > +     uint32_t index;
> > +
> > +     while ((index = l_getrandom_uint32()) >= limit);
>
> So I actually wonder if this should be an ell utility.  I'm not too sure I like
> the idea of hitting getrandom in a loop.  The chances are pretty small but
> still...  Perhaps using rand_r  with a random seed would be better.

Do you mean adding something like l_getrandom_range()?

However if we just use a single random seed of say 32 bits, we're
guaranteed a non-uniform distribution, unless 1 << 32 is divisible by
the size of the set (not the case).  I'm no expert but as far as I
know (and googled) there's no way to get uniform distribution with a
predefined number of random bits.

The difficulty here is really only the "uniform distribution" part
which I don't think it super important, as long as it's *nearly*
uniform.  I'd have normally sacrificed it to avoid the loop and
thinking of it, the way we ended up doing it in
l_key_generate_dh_private is completely non-uniform.  However we've
also later added l_ecc_scalar_new_random and l_ecdh_generate_key_pair
which do loop, with a much higher probability and discard many more
random bits in each iteration, so I didn't think you'd care.

>
> But if you want to direct code, then in theory you require only 5 bits of
> randomness here since the set is 62 long.  You could make due with just grabbing
> a single byte of randomness and you'd have 3 attempts at being in the range (low
> 5 bits, then 6..1, 7..2)

So the idea is to discard just the right-most (or left-most) bit in
each iteration and shift the other 4, and if we run out of bits we
draw another 8 and continue shifting, right?  This sounds reasonable.

On a second thought though... if our first 5-bit number was >= 62, we
went into a second iteration and reused bits 5...1, we start favouring
some numbers and the uniform distribution goes out the window.  In
fact, in this case the first number must have been 62 or 63... we
shift right, we get 31 and add a new topmost bit, we can only end up
drawing 31 from there on.

We probably have to choose between looping and generating independent
random bits each time, or doing (random % 62), which is better than
the above idea and than what we do in l_key_generate_dh_private.

Best regards

Re: [PATCH 17/17] p2p: Build P2P and WFD IEs for group's management frames

[Andrew Zaborowski]

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

Hi Denis,

On Wed, 9 Sep 2020 at 22:35, Denis Kenzior <denkenz@gmail.com> wrote:
> On 9/8/20 6:49 PM, Andrew Zaborowski wrote:
> > +static size_t p2p_group_get_ies_len(enum mpdu_management_subtype type,
> > +                                     void *user_data)
> > +{
> > +     switch (type) {
> > +     case MPDU_MANAGEMENT_SUBTYPE_ASSOCIATION_RESPONSE:
> > +     case MPDU_MANAGEMENT_SUBTYPE_REASSOCIATION_RESPONSE:
> > +     case MPDU_MANAGEMENT_SUBTYPE_PROBE_RESPONSE:
> > +     case MPDU_MANAGEMENT_SUBTYPE_BEACON:
> > +             return 256;
>
> This seems rather brittle
>
> > +     default:
> > +             return 0;
> > +     }
> > +}
> > +
> > +static size_t p2p_group_write_ies(enum mpdu_management_subtype type,
> > +                                     uint8_t *out_buf, void *user_data)
> > +{
> > +     struct p2p_device *dev = user_data;
> > +     uint8_t *ptr = out_buf;
> > +     L_AUTO_FREE_VAR(uint8_t *, p2p_ie) = NULL;
> > +     size_t p2p_ie_len;
> > +     struct p2p_wfd_properties *wfd = NULL;
> > +     const struct p2p_peer *wfd_client = NULL;
> > +
> > +     switch (type) {
> > +     case MPDU_MANAGEMENT_SUBTYPE_ASSOCIATION_RESPONSE:
> > +     case MPDU_MANAGEMENT_SUBTYPE_REASSOCIATION_RESPONSE:
> > +     {
> > +             /*
> > +              * Wi-Fi P2P Technical Specification v1.7 Section 4.2.5:
> > +              * "If neither P2P attribute is required according to the
> > +              * conditions in Table 55, then a P2P IE containing no P2P
> > +              * attributes is included."
> > +              * This is going to be our case.
> > +              */
> > +             struct p2p_association_resp info = {};
> > +
> > +             p2p_ie = p2p_build_association_resp(&info, &p2p_ie_len);
> > +
> > +             if (dev->conn_own_wfd && !dev->conn_own_wfd->r2)
> > +                     wfd = dev->conn_own_wfd;
> > +
> > +             break;
> > +     }
> > +
> > +     case MPDU_MANAGEMENT_SUBTYPE_PROBE_RESPONSE:
> > +     {
> > +             /*
> > +              * 3.2.2: "A P2P Group Owner shall not include a P2P IE
> > +              * in the Probe Response frame if the received Probe Request
> > +              * frame does not contain a P2P IE."
> > +              * For simplicity include it always, same applies to the
> > +              * WFD IE too.
>
> ???

I was thinking of adding the contents of the request frame as another
parameter to ops.write_mgmt_frame_ies() and
ops.get_mgmt_frame_ies_len() but it seemed to be an overkill.
Shouldn't the receiving device just discard the IEs it doesn't care
about?

>
> > +              */
> > +             struct p2p_probe_resp info = {};
> > +
> > +             info.capability = dev->capability;
> > +             info.device_info = dev->device_info;
> > +
> > +             if (dev->conn_peer_added) {
> > +                     struct p2p_client_info_descriptor client = {};
> > +
> > +                     memcpy(client.device_addr,
> > +                             dev->conn_client_dev_info.device_addr, 6);
> > +                     memcpy(client.interface_addr,
> > +                             dev->conn_peer_interface_addr, 6);
> > +                     client.device_caps =
> > +                             dev->conn_client_capability.device_caps;
> > +                     client.wsc_config_methods =
> > +                             dev->conn_client_dev_info.wsc_config_methods;
> > +                     client.primary_device_type =
> > +                             dev->conn_client_dev_info.primary_device_type;
> > +                     l_strlcpy(client.device_name,
> > +                                     dev->conn_client_dev_info.device_name,
> > +                                     sizeof(client.device_name));
> > +
> > +                     info.group_clients = l_queue_new();
> > +                     l_queue_push_tail(info.group_clients,
> > +                                     l_memdup(&client, sizeof(client)));
> > +             }
> > +
> > +             p2p_ie = p2p_build_probe_resp(&info, &p2p_ie_len);
> > +             p2p_clear_probe_resp(&info);
> > +
> > +             if (dev->conn_own_wfd)
> > +                     wfd_client = dev->conn_peer;
> > +
> > +             wfd = p2p_own_wfd;
> > +             break;
> > +     }
> > +
> > +     case MPDU_MANAGEMENT_SUBTYPE_BEACON:
> > +     {
> > +             struct p2p_beacon info = {};
> > +
> > +             info.capability = dev->capability;
> > +             memcpy(info.device_addr, dev->addr, 6);
> > +             p2p_ie = p2p_build_beacon(&info, &p2p_ie_len);
> > +
> > +             wfd = p2p_own_wfd;
> > +             break;
> > +     }
> > +
> > +     default:
> > +             return 0;
> > +     }
> > +
> > +     memcpy(ptr, p2p_ie, p2p_ie_len);
> > +     ptr += p2p_ie_len;
>
> So are you leaking p2p_ie here?

It is defined with L_AUTO_FREE_VAR.

>
> And also, p2p_build utilities use a dynamically-growing buffer to build all the
> attributes and ie_tlv_encapsulate_p2p_payload can produce a ton of IEs, not just
> a single one.  So I think you need something a bit smarter than
> get_mgmt_frame_ies_len() method.

But we know how much data we put into those frames and there's no way
we can produce more than one P2P IE probably unless we start
implementing "P2P Services" specification.  Even then we can easily
calculate an upper bound on the size of the IE string.

>
> Perhaps ie_tlv_builder should be passed in directly and that entire class be
> made smarter.  Including adding the ability to encapsulate p2p/wsc/vendor TLVs.

We could do that, or we could also return a new uint8_t buffer and its
length from write_mgmt_frame_ies().  Or keep this proposed "zerocopy"
API (more like "less copy"). I'm ok with either option.

Best regards

Re: [PATCH 14/17] ap: Pass extra IEs between mgmt frames and user

[Andrew Zaborowski]

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

Hi Denis,

On Wed, 9 Sep 2020 at 22:44, Denis Kenzior <denkenz@gmail.com> wrote:
> On 9/8/20 6:49 PM, Andrew Zaborowski wrote:
> > @@ -505,7 +514,11 @@ static void ap_set_beacon_cb(struct l_genl_msg *msg, void *user_data)
> >   static void ap_update_beacon(struct ap_state *ap)
> >   {
> >       struct l_genl_msg *cmd;
> > -     uint8_t head[256], tail[256];
> > +     size_t extra_ies_len = ap->ops->get_mgmt_frame_ies_len ?
> > +             ap->ops->get_mgmt_frame_ies_len(MPDU_MANAGEMENT_SUBTYPE_BEACON,
> > +                                             ap->user_data) : 0;
> > +     uint8_t head[256];
> > +     uint8_t tail[256 + extra_ies_len];
> >       size_t head_len, tail_len;
> >       uint64_t wdev_id = netdev_get_wdev_id(ap->netdev);
> >       static const uint8_t bcast_addr[6] = {
>
> Given that this is semi-static, and the caller has to call ap_update_beacon
> anyway, wouldn't it make more sense to just have the caller pass in the extra
> ies so you could just use append_attrv() on the NL80211_ATTR_BEACON_TAIL?

I can do that I guess, but since we're already adding the
write_mgmt_frame_ies() API we can save some effort by using it here.

ops->write_mgmt_frame_ies() is also called when we set up the initial beacon.

Best regards

Re: [PATCH 01/17] ap: Fix iteration over ap->rates

[Denis Kenzior]

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

Hi Andrew,

> Oh ok, yes, the original code contained an overflow bug, once I
> understood it I didn't like it either ;-)  Hence the fix.

Ok, I'm not following now.  Are you saying there was a possibility of an empty 
(not NULL) set somehow?  If so, shouldn't this have been caught in ap_start 
somewhere?  In the end, a NULL or empty set would produce an invalid IE. 
Assuming a set is valid, then I don't see anything wrong with the current code? 
Or am I missing something?

i.e. what is wrong with
if (L_WARN_ON(!ap->rates || l_uintset_isempty(ap->rates))
	return 0;

.. proceed as before.

> 
>> when a static 0..127 would have been enough.
> 
> For an invalid set, 0..127 is still wasteful ;-)
> 

Of course, but at least you don't have to think about the cast and the 
arithmetic and go 'what in the world is that doing'...

But really, l_uintset_foreach seems to be the better candidate here.

>>
>>>
>>> Granted we're iterating over values that don't correspond to standard
>>> rates, we could store indexes into the array of known rates to save
>>> some iterations.
>>>
>>
>> Maybe consider just using wiphy_get_supported_rates() instead.
> 
> Right, but we're giving the user (of the ap.h API) some control over
> which rates to support so the plan is to eventually call it but during
> ap_start only.

You're giving a no_cck hint, not control over the rates, right? So the rates 
should still come from wiphy_get_supported_rates().  If you want to stuff these 
into the uintset or just carry the no_cck hint into the IE builders is a 
different question.  I would think the latter would be easier.

Regards,
-Denis

Re: [PATCH 08/17] p2putil: Add p2p_random_char

[Denis Kenzior]

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

Hi Andrew,

>>> +     while ((index = l_getrandom_uint32()) >= limit);
>>
>> So I actually wonder if this should be an ell utility.  I'm not too sure I like
>> the idea of hitting getrandom in a loop.  The chances are pretty small but
>> still...  Perhaps using rand_r  with a random seed would be better.
> 
> Do you mean adding something like l_getrandom_range()?

I was thinking:

l_int_distribution_new(min, max);
l_int_distribution_next();
l_int_distribution_desroy();

where _new would grab a random seed and after that use rand.  The numbers 
produced should be uniform enough.

> 
> However if we just use a single random seed of say 32 bits, we're
> guaranteed a non-uniform distribution, unless 1 << 32 is divisible by
> the size of the set (not the case).  I'm no expert but as far as I
> know (and googled) there's no way to get uniform distribution with a
> predefined number of random bits.

You only need log2(max - min + 1) bits of randomness.  If the resulting number 
is too large, you throw it away and grab the next random bits.  Essentially the 
same as what you have now.

> 
> The difficulty here is really only the "uniform distribution" part
> which I don't think it super important, as long as it's *nearly*
> uniform.  I'd have normally sacrificed it to avoid the loop and
> thinking of it, the way we ended up doing it in

I'm not worried about looping here as much as us wasting resources.  You are 
grabbing 32 bits of randomness at a time, and you only need 5.

> l_key_generate_dh_private is completely non-uniform.  However we've
> also later added l_ecc_scalar_new_random and l_ecdh_generate_key_pair
> which do loop, with a much higher probability and discard many more
> random bits in each iteration, so I didn't think you'd care.

Right, I wasn't happy about those either, but had no concrete suggestions at the 
time.  I might need to revisit these.

> 
>>
>> But if you want to direct code, then in theory you require only 5 bits of
>> randomness here since the set is 62 long.  You could make due with just grabbing
>> a single byte of randomness and you'd have 3 attempts at being in the range (low
>> 5 bits, then 6..1, 7..2)
> 
> So the idea is to discard just the right-most (or left-most) bit in
> each iteration and shift the other 4, and if we run out of bits we
> draw another 8 and continue shifting, right?  This sounds reasonable.

Ah, I was thinking more simply:

- Draw 8 bits of randomness
- Try the lower 5 bits
- Try the middle 5 bits
- Try the upper 5 bits
- If we're here, go back to the beginning

Assuming the 8 bits are truly random, you basically get 3 tries to get a usable 
number.

I think what you're thinking (correct me if I'm wrong) is a 'sliding window' of 
sorts.  So you get 8 bits, slide the 5-bit window to the right and try for a 
number.  If that doesn't work, slide the window 1 bit left, try again.  Once a 
number is found, discard all bits in the scale and to the right.  If you run out 
of bits, you grab more randomness and push it to the left.

> 
> On a second thought though... if our first 5-bit number was >= 62, we
> went into a second iteration and reused bits 5...1, we start favouring
> some numbers and the uniform distribution goes out the window.  In
> fact, in this case the first number must have been 62 or 63... we
> shift right, we get 31 and add a new topmost bit, we can only end up
> drawing 31 from there on.

Since the bits are truly random, what difference does it make if we take bits 
0..4 or 1..5?

> 
> We probably have to choose between looping and generating independent
> random bits each time, or doing (random % 62), which is better than
> the above idea and than what we do in l_key_generate_dh_private.

No, random % 62 would definitely result in an uneven distribution.

0 % 62 -> 0
1 % 62 -> 1
...
62 % 62 -> 0
63 % 62 -> 1

So you have 2/64 chance for a 0 and 1.  You must loop if the random is outside 
the limit.

Regards.
-Denis

Re: [PATCH 08/17] p2putil: Add p2p_random_char

[Denis Kenzior]

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

Hi,

> I'm not worried about looping here as much as us wasting resources.  You are 
> grabbing 32 bits of randomness at a time, and you only need 5.

And I'm not sure why I keep saying 5 bits.  I of course mean 6.  But hopefully 
you groked what I was saying anyway.

Regards,
-Denis

Re: [PATCH 01/17] ap: Fix iteration over ap->rates

[Andrew Zaborowski]

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

On Thu, 10 Sep 2020 at 02:47, Denis Kenzior <denkenz@gmail.com> wrote:
> > Oh ok, yes, the original code contained an overflow bug, once I
> > understood it I didn't like it either ;-)  Hence the fix.
>
> Ok, I'm not following now.  Are you saying there was a possibility of an empty
> (not NULL) set somehow?  If so, shouldn't this have been caught in ap_start
> somewhere?  In the end, a NULL or empty set would produce an invalid IE.
> Assuming a set is valid, then I don't see anything wrong with the current code?

Currently we add either 3 or 8 numbers to the set, but we had a bug there.

You said:

> Looking at this code closely I totally hate how this is done.  You are only
> going to be calling l_uintset_contains a few million times

I was trying to understand whether you referred to the old code (which
was fine except if ap->rates was NULL where l_uintset_contains was
called in an endless loop...) or the new code (which is fine either
way).  I guess you must have referred to the old code in the ap->rates
== NULL scenario... in that case I guess I also dislike that code
because of the corner case overflow bug but I can't see anything wrong
with it otherwise.  And both versions should be fine once we fix the
ap_start bug...

Hence the fix ;-)

> Or am I missing something?
>
> i.e. what is wrong with
> if (L_WARN_ON(!ap->rates || l_uintset_isempty(ap->rates))
>         return 0;
>
> .. proceed as before.

Since ap->rates shouldn't need to ever change after ap_start, I'd
rather not add explicit checks everywhere.

>
> >
> >> when a static 0..127 would have been enough.
> >
> > For an invalid set, 0..127 is still wasteful ;-)
> >
>
> Of course, but at least you don't have to think about the cast and the
> arithmetic and go 'what in the world is that doing'...
>
> But really, l_uintset_foreach seems to be the better candidate here.

Ok.

>
> >>
> >>>
> >>> Granted we're iterating over values that don't correspond to standard
> >>> rates, we could store indexes into the array of known rates to save
> >>> some iterations.
> >>>
> >>
> >> Maybe consider just using wiphy_get_supported_rates() instead.
> >
> > Right, but we're giving the user (of the ap.h API) some control over
> > which rates to support so the plan is to eventually call it but during
> > ap_start only.
>
> You're giving a no_cck hint, not control over the rates, right? So the rates
> should still come from wiphy_get_supported_rates().

To be specific I think the final set should be a result of multiple
calls to wiphy_get_supported_rates and the no_cck_rates setting.

>  If you want to stuff these
> into the uintset or just carry the no_cck hint into the IE builders is a
> different question.  I would think the latter would be easier.

Not sure about that, but I didn't mean to refactor this code, only fix
a few one-liners.  The refactoring if needed can happen in an
independent commit without functionality changes.

Best regards

Re: [PATCH 01/17] ap: Fix iteration over ap->rates

[Denis Kenzior]

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

Hi Andrew,

> called in an endless loop...) or the new code (which is fine either

I'm not sure its fine though?  By my reading it enters into the loop at least 
once, so you're sending a bogus IE anyway (with 0xff or something).  But anyhow, 
this is academic.

>> You're giving a no_cck hint, not control over the rates, right? So the rates
>> should still come from wiphy_get_supported_rates().
> 
> To be specific I think the final set should be a result of multiple
> calls to wiphy_get_supported_rates and the no_cck_rates setting.

Why multiple? You're just operating on one band, no?

> 
>>   If you want to stuff these
>> into the uintset or just carry the no_cck hint into the IE builders is a
>> different question.  I would think the latter would be easier.
> 
> Not sure about that, but I didn't mean to refactor this code, only fix
> a few one-liners.  The refactoring if needed can happen in an
> independent commit without functionality changes.

Okay, then lets just fix the NULL set issue and move on without touching the loops.

Regards,
-Denis

Re: [PATCH 08/17] p2putil: Add p2p_random_char

[Andrew Zaborowski]

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

On Thu, 10 Sep 2020 at 03:11, Denis Kenzior <denkenz@gmail.com> wrote:
> >>> +     while ((index = l_getrandom_uint32()) >= limit);
> >>
> >> So I actually wonder if this should be an ell utility.  I'm not too sure I like
> >> the idea of hitting getrandom in a loop.  The chances are pretty small but
> >> still...  Perhaps using rand_r  with a random seed would be better.
> >
> > Do you mean adding something like l_getrandom_range()?
>
> I was thinking:
>
> l_int_distribution_new(min, max);
> l_int_distribution_next();
> l_int_distribution_desroy();
>
> where _new would grab a random seed and after that use rand.  The numbers
> produced should be uniform enough.

They'd be uniform enough for me, but not any better than doing
l_getrandom_uint32() % 62.  Essentially you only get 32 bits of
randomness and regardless of how many times you call rand() you still
end up with a function of those original 32 bits.  Basically your loop
produces a mapping, a lookup table, of 2^32 numbers into 62 numbers.
There's always going to be at least 2 numbers that pop up with a
higher frequency than the other 60 in that set.

>
> >
> > However if we just use a single random seed of say 32 bits, we're
> > guaranteed a non-uniform distribution, unless 1 << 32 is divisible by
> > the size of the set (not the case).  I'm no expert but as far as I
> > know (and googled) there's no way to get uniform distribution with a
> > predefined number of random bits.
>
> You only need log2(max - min + 1) bits of randomness.  If the resulting number
> is too large, you throw it away and grab the next random bits.  Essentially the
> same as what you have now.

This will work but only with a new call to l_getrandom.

>
> >
> > The difficulty here is really only the "uniform distribution" part
> > which I don't think it super important, as long as it's *nearly*
> > uniform.  I'd have normally sacrificed it to avoid the loop and
> > thinking of it, the way we ended up doing it in
>
> I'm not worried about looping here as much as us wasting resources.  You are
> grabbing 32 bits of randomness at a time, and you only need 5.
>
> > l_key_generate_dh_private is completely non-uniform.  However we've
> > also later added l_ecc_scalar_new_random and l_ecdh_generate_key_pair
> > which do loop, with a much higher probability and discard many more
> > random bits in each iteration, so I didn't think you'd care.
>
> Right, I wasn't happy about those either, but had no concrete suggestions at the
> time.  I might need to revisit these.
>
> >
> >>
> >> But if you want to direct code, then in theory you require only 5 bits of
> >> randomness here since the set is 62 long.  You could make due with just grabbing
> >> a single byte of randomness and you'd have 3 attempts at being in the range (low
> >> 5 bits, then 6..1, 7..2)
> >
> > So the idea is to discard just the right-most (or left-most) bit in
> > each iteration and shift the other 4, and if we run out of bits we
> > draw another 8 and continue shifting, right?  This sounds reasonable.
>
> Ah, I was thinking more simply:
>
> - Draw 8 bits of randomness
> - Try the lower 5 bits
> - Try the middle 5 bits
> - Try the upper 5 bits
> - If we're here, go back to the beginning
>
> Assuming the 8 bits are truly random, you basically get 3 tries to get a usable
> number.
>
> I think what you're thinking (correct me if I'm wrong) is a 'sliding window' of
> sorts.  So you get 8 bits, slide the 5-bit window to the right and try for a
> number.  If that doesn't work, slide the window 1 bit left, try again.  Once a
> number is found, discard all bits in the scale and to the right.  If you run out
> of bits, you grab more randomness and push it to the left.

So I think this produces an uneven distribution, I'll explain why
below.  But we could definitely re-use the 2 bits we didn't use from
the first byte, after we didn't like the first 6-bit window.

>
> >
> > On a second thought though... if our first 5-bit number was >= 62, we
> > went into a second iteration and reused bits 5...1, we start favouring
> > some numbers and the uniform distribution goes out the window.  In
> > fact, in this case the first number must have been 62 or 63... we
> > shift right, we get 31 and add a new topmost bit, we can only end up
> > drawing 31 from there on.
>
> Since the bits are truly random, what difference does it make if we take bits
> 0..4 or 1..5?

It doesn't, but it does make a difference that we re-use some bits
after we already know something about them.  At that point they're no
longer random.

Our 62 character set is a good example, think about this:

We looked at the bits 0...5.  The value was 62 or 63.  In binary 62
and 63 are 111110 and 111111.  Depending on how you ordered your bits,
bits 1-5 may have been all ones.  Now you try bits 1...6.  You already
know that the value is going to be 011111 or 111111.  If it is the
former, we generated 31.  If it is the latter, we have to loop again,
but again we're starting with 5 ones and one random bit, and we can
only draw 31.

So in effect that is less uniform than l_getrandom_uint32() % 62.

>
> >
> > We probably have to choose between looping and generating independent
> > random bits each time, or doing (random % 62), which is better than
> > the above idea and than what we do in l_key_generate_dh_private.
>
> No, random % 62 would definitely result in an uneven distribution.

Right, but it's not bad enough that it really helps an attacker.

>
> 0 % 62 -> 0
> 1 % 62 -> 1
> ...
> 62 % 62 -> 0
> 63 % 62 -> 1
>
> So you have 2/64 chance for a 0 and 1.  You must loop if the random is outside
> the limit.

That depends, if you use a full byte % 62, you have a 5/256 chance for
a [0-7] number and 4/256 for a [8-62] number.  If you use
l_getrandom_uint32() % 62 you have a 69273667 / 2^32 chance for [0-3]
and a 69273666 / 2^32 for [4-62].

Best regards

Re: [PATCH 01/17] ap: Fix iteration over ap->rates

[Andrew Zaborowski]

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

On Thu, 10 Sep 2020 at 04:04, Denis Kenzior <denkenz@gmail.com> wrote:
> > called in an endless loop...) or the new code (which is fine either
>
> I'm not sure its fine though?  By my reading it enters into the loop at least
> once, so you're sending a bogus IE anyway (with 0xff or something).  But anyhow,
> this is academic.

l_uintset_contains() would be false, so the IE would be empty but in
any case invalid.

>
> >> You're giving a no_cck hint, not control over the rates, right? So the rates
> >> should still come from wiphy_get_supported_rates().
> >
> > To be specific I think the final set should be a result of multiple
> > calls to wiphy_get_supported_rates and the no_cck_rates setting.
>
> Why multiple? You're just operating on one band, no?

Yes, true, we probably need just one call.

>
> >
> >>   If you want to stuff these
> >> into the uintset or just carry the no_cck hint into the IE builders is a
> >> different question.  I would think the latter would be easier.
> >
> > Not sure about that, but I didn't mean to refactor this code, only fix
> > a few one-liners.  The refactoring if needed can happen in an
> > independent commit without functionality changes.
>
> Okay, then lets just fix the NULL set issue and move on without touching the loops.

Ok.

Best regards

Re: [PATCH 08/17] p2putil: Add p2p_random_char

[Denis Kenzior]

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

Hi Andrew,

On 9/9/20 9:23 PM, Andrew Zaborowski wrote:
> On Thu, 10 Sep 2020 at 03:11, Denis Kenzior <denkenz@gmail.com> wrote:
>>>>> +     while ((index = l_getrandom_uint32()) >= limit);
>>>>
>>>> So I actually wonder if this should be an ell utility.  I'm not too sure I like
>>>> the idea of hitting getrandom in a loop.  The chances are pretty small but
>>>> still...  Perhaps using rand_r  with a random seed would be better.
>>>
>>> Do you mean adding something like l_getrandom_range()?
>>
>> I was thinking:
>>
>> l_int_distribution_new(min, max);
>> l_int_distribution_next();
>> l_int_distribution_desroy();
>>
>> where _new would grab a random seed and after that use rand.  The numbers
>> produced should be uniform enough.
> 
> They'd be uniform enough for me, but not any better than doing
> l_getrandom_uint32() % 62.  Essentially you only get 32 bits of
> randomness and regardless of how many times you call rand() you still
> end up with a function of those original 32 bits.  Basically your loop
> produces a mapping, a lookup table, of 2^32 numbers into 62 numbers.
> There's always going to be at least 2 numbers that pop up with a
> higher frequency than the other 60 in that set.

So to re-state it another way, you're saying that the pseudo-random approach is 
not suitable for certain things?  I agree.  We should not generate the 
passphrase using this.  But for generating the SSID or other stuff?

That reminds me, why are we generating the passphrase and not the PSK for P2P? 
We could then generate the PSK directly and skip this entirely.

Also the rules for passphrases in 802.11 allow a different set of characters 
than what is covered here.  That is also a bit strange.  But I'm getting 
off-topic...

> 
>>
>>>
>>> However if we just use a single random seed of say 32 bits, we're
>>> guaranteed a non-uniform distribution, unless 1 << 32 is divisible by
>>> the size of the set (not the case).  I'm no expert but as far as I
>>> know (and googled) there's no way to get uniform distribution with a
>>> predefined number of random bits.
>>
>> You only need log2(max - min + 1) bits of randomness.  If the resulting number
>> is too large, you throw it away and grab the next random bits.  Essentially the
>> same as what you have now.
> 
> This will work but only with a new call to l_getrandom.

My point here was, you were grabbing 32 bit blocks of randomness at a time when 
doing 8 at a time would have been enough.

<snip>

>>>
>>> On a second thought though... if our first 5-bit number was >= 62, we
>>> went into a second iteration and reused bits 5...1, we start favouring
>>> some numbers and the uniform distribution goes out the window.  In
>>> fact, in this case the first number must have been 62 or 63... we
>>> shift right, we get 31 and add a new topmost bit, we can only end up
>>> drawing 31 from there on.
>>
>> Since the bits are truly random, what difference does it make if we take bits
>> 0..4 or 1..5?
> 
> It doesn't, but it does make a difference that we re-use some bits
> after we already know something about them.  At that point they're no
> longer random.
> 
> Our 62 character set is a good example, think about this:
> 
> We looked at the bits 0...5.  The value was 62 or 63.  In binary 62
> and 63 are 111110 and 111111.  Depending on how you ordered your bits,
> bits 1-5 may have been all ones.  Now you try bits 1...6.  You already
> know that the value is going to be 011111 or 111111.  If it is the
> former, we generated 31.  If it is the latter, we have to loop again,
> but again we're starting with 5 ones and one random bit, and we can
> only draw 31.
> 
> So in effect that is less uniform than l_getrandom_uint32() % 62.

Not sure I buy that, but I don't have enough background to really argue.  I 
would tend to agree that unless proven otherwise, throwing the bits away is a 
safer approach.  A quick test might be useful though.

> 
>>
>>>
>>> We probably have to choose between looping and generating independent
>>> random bits each time, or doing (random % 62), which is better than
>>> the above idea and than what we do in l_key_generate_dh_private.
>>
>> No, random % 62 would definitely result in an uneven distribution.
> 
> Right, but it's not bad enough that it really helps an attacker.
>

Yeah I dunno if we're qualified to say that ;)

>>
>> 0 % 62 -> 0
>> 1 % 62 -> 1
>> ...
>> 62 % 62 -> 0
>> 63 % 62 -> 1
>>
>> So you have 2/64 chance for a 0 and 1.  You must loop if the random is outside
>> the limit.
> 
> That depends, if you use a full byte % 62, you have a 5/256 chance for
> a [0-7] number and 4/256 for a [8-62] number.  If you use
> l_getrandom_uint32() % 62 you have a 69273667 / 2^32 chance for [0-3]
> and a 69273666 / 2^32 for [4-62].

I can buy the argument here that with a simple modulus operation, without 
re-rolling things, the 32 bit block wins in producing a more uniform 
distribution.  But fundamentally, without re-rolling, the distribution is still 
skewed.  So to be on the safe side, we need to re-roll regardless.

The question then becomes, would we end up using less randomness on average 
using 32 bit blocks or smaller ones, and does it matter.  Given that 
l_getrandom() can block for lack of entropy, I would guess that using less 
should be a goal.

Also, you're probably aware of this, but l_getrandom_uint32 was designed not to 
block and falls back to rand() in case the entropy pool is empty.  It was meant 
more for use by dhcp where crypto randomness is not needed.  So using it here as 
it is implemented right now is probably not a good idea anyway.

Regards,
-Denis

Re: [PATCH 08/17] p2putil: Add p2p_random_char

[Andrew Zaborowski]

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

Hi Denis,

On Thu, 10 Sep 2020 at 05:58, Denis Kenzior <denkenz@gmail.com> wrote:
> On 9/9/20 9:23 PM, Andrew Zaborowski wrote:
> > They'd be uniform enough for me, but not any better than doing
> > l_getrandom_uint32() % 62.  Essentially you only get 32 bits of
> > randomness and regardless of how many times you call rand() you still
> > end up with a function of those original 32 bits.  Basically your loop
> > produces a mapping, a lookup table, of 2^32 numbers into 62 numbers.
> > There's always going to be at least 2 numbers that pop up with a
> > higher frequency than the other 60 in that set.
>
> So to re-state it another way, you're saying that the pseudo-random approach is
> not suitable for certain things?  I agree.  We should not generate the
> passphrase using this.  But for generating the SSID or other stuff?

It's a matter of whether we want to implement this part of the spec
literally, it defines the random chars in the passphrase and the SSID
the same way.  But we're probably worrying about it too much.  Also
the SSID uses only two random characters.

>
> That reminds me, why are we generating the passphrase and not the PSK for P2P?
> We could then generate the PSK directly and skip this entirely.

Right, it's only because the spec says we should generate the
passphrase first, then the PSK from the passphrase.  The reasoning in
it is about legacy clients.

>
> Also the rules for passphrases in 802.11 allow a different set of characters
> than what is covered here.  That is also a bit strange.  But I'm getting
> off-topic...
>
> >
> >>
> >>>
> >>> However if we just use a single random seed of say 32 bits, we're
> >>> guaranteed a non-uniform distribution, unless 1 << 32 is divisible by
> >>> the size of the set (not the case).  I'm no expert but as far as I
> >>> know (and googled) there's no way to get uniform distribution with a
> >>> predefined number of random bits.
> >>
> >> You only need log2(max - min + 1) bits of randomness.  If the resulting number
> >> is too large, you throw it away and grab the next random bits.  Essentially the
> >> same as what you have now.
> >
> > This will work but only with a new call to l_getrandom.
>
> My point here was, you were grabbing 32 bit blocks of randomness at a time when
> doing 8 at a time would have been enough.

Ok, right.

>
> <snip>
>
> >>>
> >>> On a second thought though... if our first 5-bit number was >= 62, we
> >>> went into a second iteration and reused bits 5...1, we start favouring
> >>> some numbers and the uniform distribution goes out the window.  In
> >>> fact, in this case the first number must have been 62 or 63... we
> >>> shift right, we get 31 and add a new topmost bit, we can only end up
> >>> drawing 31 from there on.
> >>
> >> Since the bits are truly random, what difference does it make if we take bits
> >> 0..4 or 1..5?
> >
> > It doesn't, but it does make a difference that we re-use some bits
> > after we already know something about them.  At that point they're no
> > longer random.
> >
> > Our 62 character set is a good example, think about this:
> >
> > We looked at the bits 0...5.  The value was 62 or 63.  In binary 62
> > and 63 are 111110 and 111111.  Depending on how you ordered your bits,
> > bits 1-5 may have been all ones.  Now you try bits 1...6.  You already
> > know that the value is going to be 011111 or 111111.  If it is the
> > former, we generated 31.  If it is the latter, we have to loop again,
> > but again we're starting with 5 ones and one random bit, and we can
> > only draw 31.
> >
> > So in effect that is less uniform than l_getrandom_uint32() % 62.
>
> Not sure I buy that, but I don't have enough background to really argue.  I

You can try it for inputs in [0-255], like you did for random() % 62 ;)

> would tend to agree that unless proven otherwise, throwing the bits away is a
> safer approach.  A quick test might be useful though.
>
> >
> >>
> >>>
> >>> We probably have to choose between looping and generating independent
> >>> random bits each time, or doing (random % 62), which is better than
> >>> the above idea and than what we do in l_key_generate_dh_private.
> >>
> >> No, random % 62 would definitely result in an uneven distribution.
> >
> > Right, but it's not bad enough that it really helps an attacker.
> >
>
> Yeah I dunno if we're qualified to say that ;)

What we're doing if we use a slightly non-uniform random distribution
is we reduce the strength of the passphrase, but by less than one bit
over the whole passphrase.  The spec says that the passphrase "shall
contain at least eight ASCII characters randomly selected with a
uniform distribution from the following character set: upper case
letters, lower case letters and numbers."  In the patches I went for a
20-char passphrase.  Even if I'd used only hex digits (4 bits per
char) the passphrase would stlil be stronger than the 8 character
passphrase generated according to the spec.  Note in
l_key_generate_dh_private we're openly throwing away some bits of the
key.

But the cost of doing exactly what the spec says is low so I'd just do that.

>
> >>
> >> 0 % 62 -> 0
> >> 1 % 62 -> 1
> >> ...
> >> 62 % 62 -> 0
> >> 63 % 62 -> 1
> >>
> >> So you have 2/64 chance for a 0 and 1.  You must loop if the random is outside
> >> the limit.
> >
> > That depends, if you use a full byte % 62, you have a 5/256 chance for
> > a [0-7] number and 4/256 for a [8-62] number.  If you use
> > l_getrandom_uint32() % 62 you have a 69273667 / 2^32 chance for [0-3]
> > and a 69273666 / 2^32 for [4-62].
>
> I can buy the argument here that with a simple modulus operation, without
> re-rolling things, the 32 bit block wins in producing a more uniform
> distribution.  But fundamentally, without re-rolling, the distribution is still
> skewed.  So to be on the safe side, we need to re-roll regardless.
>
> The question then becomes, would we end up using less randomness on average
> using 32 bit blocks or smaller ones, and does it matter.  Given that
> l_getrandom() can block for lack of entropy, I would guess that using less
> should be a goal.

Ok, let me rewrite this using l_getrandom(&byte, 1) I guess.  We could
optimize it a little by trying to generate multiple characters at once
but the code gets more complicated, I think it's not worth it?

Best regards

Re: [PATCH 08/17] p2putil: Add p2p_random_char

[Denis Kenzior]

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

Hi Andrew,

> What we're doing if we use a slightly non-uniform random distribution
> is we reduce the strength of the passphrase, but by less than one bit
> over the whole passphrase.  The spec says that the passphrase "shall
> contain at least eight ASCII characters randomly selected with a
> uniform distribution from the following character set: upper case
> letters, lower case letters and numbers."  In the patches I went for a
> 20-char passphrase.  Even if I'd used only hex digits (4 bits per
> char) the passphrase would stlil be stronger than the 8 character
> passphrase generated according to the spec.  Note in

Right, but the 8 char passphrase the spec recommends is basically completely 
inadequate these days.  Since the set is reduced to only numbers / letters, and 
no special or other printable characters, brute-forcing an 8 character 
passphrase is trivial.  So I think your 20 character passphrase is closer to 
what we should shoot for.

> l_key_generate_dh_private we're openly throwing away some bits of the
> key.

So can we do better there?

I'm less concerned about DH because the key is used only once to deliver the 
credentials.  Whereas a passphrase can be brute-forced offline.  This is where 
slight bias to the first 8 characters or so can really hurt.

> 
> But the cost of doing exactly what the spec says is low so I'd just do that.
> 

My thinking as well.

>>
>>>>
>>>> 0 % 62 -> 0
>>>> 1 % 62 -> 1
>>>> ...
>>>> 62 % 62 -> 0
>>>> 63 % 62 -> 1
>>>>
>>>> So you have 2/64 chance for a 0 and 1.  You must loop if the random is outside
>>>> the limit.
>>>
>>> That depends, if you use a full byte % 62, you have a 5/256 chance for
>>> a [0-7] number and 4/256 for a [8-62] number.  If you use
>>> l_getrandom_uint32() % 62 you have a 69273667 / 2^32 chance for [0-3]
>>> and a 69273666 / 2^32 for [4-62].
>>
>> I can buy the argument here that with a simple modulus operation, without
>> re-rolling things, the 32 bit block wins in producing a more uniform
>> distribution.  But fundamentally, without re-rolling, the distribution is still
>> skewed.  So to be on the safe side, we need to re-roll regardless.
>>
>> The question then becomes, would we end up using less randomness on average
>> using 32 bit blocks or smaller ones, and does it matter.  Given that
>> l_getrandom() can block for lack of entropy, I would guess that using less
>> should be a goal.
> 
> Ok, let me rewrite this using l_getrandom(&byte, 1) I guess.  We could
> optimize it a little by trying to generate multiple characters at once
> but the code gets more complicated, I think it's not worth it?
> 

I'd think just being greedy and generating the entire passphrase in one go if 
possible would be the better approach.  We could even just use the first 2 chars 
of the generated passphrase as the SSID 'xy' and use the rest as the passphrase.

Also, since we have some leeway in how big the passphrase is, why not just 
request one bigger than we need and if it comes out several chars smaller due to 
re-rolls, then we don't worry about it as long as it is bigger than what we need.

Regards,
-Denis

Re: [PATCH 13/17] p2p: Start a basic P2P Group after GO Negotiation

[Denis Kenzior]

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

Hi Andrew,

To follow up from the other discussion:

> +	/*
> +	 * Section 3.2.1: "The WPA2-Personal pass-phrase shall contain at
> +	 * least eight ASCII characters randomly selected with a uniform
> +	 * distribution from the following character set: upper case letters,
> +	 * lower case letters and numbers."
> +	 *
> +	 * However we don't currently respect the following text because
> +	 * our credentials will contain the passphrase and not the PSK.
> +	 * Section 3.2.1: "The Credentials for a P2P Group issued to a
> +	 * P2P Device shall: [...]
> +	 *  - Use a Network Key Type of 64 Hex characters."

So it looks to me like the spec wants you to deliver the credentials in Network 
Key form, not a passphrase.  The exception is for 'Legacy Clients', which can't 
connect anyway due to authorized_macs being restricted.  If we were to allow 
that, how would we distinguish a legacy client ?

Also, this begs the question of whether we should generate per client keys.  I 
think that would actually be safer & more secure in the long run.  Thoughts?

Regards,
-Denis

Re: [PATCH 13/17] p2p: Start a basic P2P Group after GO Negotiation

[Andrew Zaborowski]

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

Hi Denis,

On Thu, 10 Sep 2020 at 16:58, Denis Kenzior <denkenz@gmail.com> wrote:
> > +      * Section 3.2.1: "The Credentials for a P2P Group issued to a
> > +      * P2P Device shall: [...]
> > +      *  - Use a Network Key Type of 64 Hex characters."
>
> So it looks to me like the spec wants you to deliver the credentials in Network
> Key form, not a passphrase.  The exception is for 'Legacy Clients', which can't
> connect anyway due to authorized_macs being restricted.  If we were to allow
> that, how would we distinguish a legacy client ?

So for a legacy client, as far as I understand, you're supposed to
"manually" provision the client, i.e. obtain the passphrase from the
GO in an unspecified way, type it into the client through, e.g.
Network Manager and it'll do the WPA2 Personal 4-way handshake.  So it
matters that there *exists* a passphrase but it doesn't matter whether
we send the passphrase or the PSK inside the Network Key.  For the
record, the Network Key is assumed to be the PSK if it's 64 Hex chars
long, and a passphrase if it's between 8 and 63...  So it's the "64
Hex characters" phrase that implies we should use the PSK form, but it
makes no difference for the GO or the client, and as far as I know it
makes no difference for security either.

>
> Also, this begs the question of whether we should generate per client keys.  I
> think that would actually be safer & more secure in the long run.  Thoughts?

P2P and WSC don't really consider this, we could do it but for now we
can only have a single client so technically we generate a per-client
key ;-)

Best regards

Re: [PATCH 13/17] p2p: Start a basic P2P Group after GO Negotiation

[Denis Kenzior]

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

Hi Andrew,

On 9/10/20 10:28 AM, Andrew Zaborowski wrote:
> Hi Denis,
> 
> On Thu, 10 Sep 2020 at 16:58, Denis Kenzior <denkenz@gmail.com> wrote:
>>> +      * Section 3.2.1: "The Credentials for a P2P Group issued to a
>>> +      * P2P Device shall: [...]
>>> +      *  - Use a Network Key Type of 64 Hex characters."
>>
>> So it looks to me like the spec wants you to deliver the credentials in Network
>> Key form, not a passphrase.  The exception is for 'Legacy Clients', which can't
>> connect anyway due to authorized_macs being restricted.  If we were to allow
>> that, how would we distinguish a legacy client ?
> 
> So for a legacy client, as far as I understand, you're supposed to
> "manually" provision the client, i.e. obtain the passphrase from the
> GO in an unspecified way, type it into the client through, e.g.
> Network Manager and it'll do the WPA2 Personal 4-way handshake.  So it

That's one way.  But it also means WSC as well.  That same section:

"This may be delivered using WSC, or in the case of a Legacy Client that does 
not support WSC, by means outside the scope of this specification".  This is 
probably the most likely method for legacy clients.

> matters that there *exists* a passphrase but it doesn't matter whether
> we send the passphrase or the PSK inside the Network Key.  For the

Not sure I agree.  The passphrase must exist for Legacy clients, but in the case 
of P2P provisioning, the spec uses the words 'shall':

"Use a Network Key Type of 64 Hex characters."

> record, the Network Key is assumed to be the PSK if it's 64 Hex chars
> long, and a passphrase if it's between 8 and 63...  So it's the "64
> Hex characters" phrase that implies we should use the PSK form, but it

Right

> makes no difference for the GO or the client, and as far as I know it

Probably doesn't since they would support both, but the spec says to use the key 
form.  Note that this is sort of wrong if SAE is used, but SAE wasn't around 
back then...

> makes no difference for security either.

The key is derived from the passphrase and thus it can be much more easily 
brute-forced compared to a 32 byte key.  With a long enough passphrase it 
doesn't matter of course, but the spec is recommending a bare minimum.

With SAE it is less of a concern due to forward-secrecy properties.  With WPA1/2 
it is a major issue since simply sniffing the traffic allows attacks against the 
passphrase.

Also consider that deriving the key from the passphrase on the peer takes extra 
time.  On a slow CPU this can be quite significant and thus increases latency.

> 
>>
>> Also, this begs the question of whether we should generate per client keys.  I
>> think that would actually be safer & more secure in the long run.  Thoughts?
> 
> P2P and WSC don't really consider this, we could do it but for now we
> can only have a single client so technically we generate a per-client
> key ;-)

Lol, sure.  But then you're giving $reasons for generating a passphrase but turn 
around and argue that you don't support the use case for those $reasons ;)

I'm also not quite sure how your proposed design could be made to support legacy 
or multiple clients nicely, but that's another conversation.

Seriously though, I don't see how a per-client key is precluded by the spec.  It 
doesn't support these outright, but it has no verbiage to limit such use. 
Security wise it is much better to generate a per-client key and I wouldn't even 
generate a passphrase unless needed for some cert test.  I doubt legacy use 
cases even exist in the wild.

Regards,
-Denis

Re: [PATCH 13/17] p2p: Start a basic P2P Group after GO Negotiation

[Andrew Zaborowski]

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

On Thu, 10 Sep 2020 at 18:11, Denis Kenzior <denkenz@gmail.com> wrote:
> On 9/10/20 10:28 AM, Andrew Zaborowski wrote:
> > On Thu, 10 Sep 2020 at 16:58, Denis Kenzior <denkenz@gmail.com> wrote:
> >>> +      * Section 3.2.1: "The Credentials for a P2P Group issued to a
> >>> +      * P2P Device shall: [...]
> >>> +      *  - Use a Network Key Type of 64 Hex characters."
> >>
> >> So it looks to me like the spec wants you to deliver the credentials in Network
> >> Key form, not a passphrase.  The exception is for 'Legacy Clients', which can't
> >> connect anyway due to authorized_macs being restricted.  If we were to allow
> >> that, how would we distinguish a legacy client ?
> >
> > So for a legacy client, as far as I understand, you're supposed to
> > "manually" provision the client, i.e. obtain the passphrase from the
> > GO in an unspecified way, type it into the client through, e.g.
> > Network Manager and it'll do the WPA2 Personal 4-way handshake.  So it
>
> That's one way.  But it also means WSC as well.  That same section:
>
> "This may be delivered using WSC, or in the case of a Legacy Client that does
> not support WSC, by means outside the scope of this specification".  This is
> probably the most likely method for legacy clients.

Ok.

>
> > matters that there *exists* a passphrase but it doesn't matter whether
> > we send the passphrase or the PSK inside the Network Key.  For the
>
> Not sure I agree.  The passphrase must exist for Legacy clients, but in the case
> of P2P provisioning, the spec uses the words 'shall':
>
> "Use a Network Key Type of 64 Hex characters."

Do you mean that it doesn't say 'must', or?

I mean, like I said in the code comment, sending the passphrase
instead of the PSK as the Network Key is agaist the spec but from a
practical point of view a client who supports WSC supports both forms.
Legacy clients (supporting WSC or not) are probably not the reason for
the PSK requirement.

>
> > record, the Network Key is assumed to be the PSK if it's 64 Hex chars
> > long, and a passphrase if it's between 8 and 63...  So it's the "64
> > Hex characters" phrase that implies we should use the PSK form, but it
>
> Right
>
> > makes no difference for the GO or the client, and as far as I know it
>
> Probably doesn't since they would support both, but the spec says to use the key
> form.  Note that this is sort of wrong if SAE is used, but SAE wasn't around
> back then...
>
> > makes no difference for security either.
>
> The key is derived from the passphrase and thus it can be much more easily
> brute-forced compared to a 32 byte key.

Sorry, which are you saying is easier to brute-force?

The PSK is 32 * 8 bits while the passphrase is up to about 63 * 6 bits
which is stronger but not by much.  You can brute-force either.  With
the passphrase you get the PSK.  With the PSK you don't get the
passphrase but you don't need it.  So I don't see much difference.

>  With a long enough passphrase it
> doesn't matter of course, but the spec is recommending a bare minimum.
>
> With SAE it is less of a concern due to forward-secrecy properties.  With WPA1/2
> it is a major issue since simply sniffing the traffic allows attacks against the
> passphrase.
>
> Also consider that deriving the key from the passphrase on the peer takes extra
> time.  On a slow CPU this can be quite significant and thus increases latency.

So you're saying we should derive the PSK from the passphrase and send
the PSK in Network Key, right?

I don't think it's a noticeable task on a device that implements diffie-hellman.

>
> >
> >>
> >> Also, this begs the question of whether we should generate per client keys.  I
> >> think that would actually be safer & more secure in the long run.  Thoughts?
> >
> > P2P and WSC don't really consider this, we could do it but for now we
> > can only have a single client so technically we generate a per-client
> > key ;-)
>
> Lol, sure.  But then you're giving $reasons for generating a passphrase but turn
> around and argue that you don't support the use case for those $reasons ;)

Which use case?  We don't support multiple clients because we didn't
get there yet, no other reasons.  In fact we don't support one client
yet.

>
> I'm also not quite sure how your proposed design could be made to support legacy
> or multiple clients nicely, but that's another conversation.

Do you mean the design of the struct p2p_device and such?  It will
need to evolve obviously, we'll need to use l_queue's etc, but should
we already be thinking about this?

>
> Seriously though, I don't see how a per-client key is precluded by the spec.  It
> doesn't support these outright, but it has no verbiage to limit such use.

I only said it doesn't consider that... but then it does say that you
generate the credentials once and keep them until the group is torn
down.

Or do you mean it doesn't preclude it as in, it wouldn't break
anything? As I said I think we can do it.  But throughout this thread
you had been arguing for implementing the spec literally.

Best regards

Re: [PATCH 13/17] p2p: Start a basic P2P Group after GO Negotiation

[Denis Kenzior]

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

Hi Andrew,

>>> matters that there *exists* a passphrase but it doesn't matter whether
>>> we send the passphrase or the PSK inside the Network Key.  For the
>>
>> Not sure I agree.  The passphrase must exist for Legacy clients, but in the case
>> of P2P provisioning, the spec uses the words 'shall':
>>
>> "Use a Network Key Type of 64 Hex characters."
> 
> Do you mean that it doesn't say 'must', or?

What are we arguing here?  The spec clearly says:

1 Send a PSK to P2P provisioning clients
2 Send a PSK or Passphrase to WSC Legacy Clients
3 Share a passphrase with Legacy Clients via carrier pigeons.

3 is basically pointless, so unless you are doing certification, I'd completely 
ignore this.

> 
> I mean, like I said in the code comment, sending the passphrase
> instead of the PSK as the Network Key is agaist the spec but from a
> practical point of view a client who supports WSC supports both forms.
> Legacy clients (supporting WSC or not) are probably not the reason for
> the PSK requirement.

I don't even know why the spec mandates this.  But it does, so practicalities 
aside we should send the 64 byte version for now.

> 
>>
>>> record, the Network Key is assumed to be the PSK if it's 64 Hex chars
>>> long, and a passphrase if it's between 8 and 63...  So it's the "64
>>> Hex characters" phrase that implies we should use the PSK form, but it
>>
>> Right
>>
>>> makes no difference for the GO or the client, and as far as I know it
>>
>> Probably doesn't since they would support both, but the spec says to use the key
>> form.  Note that this is sort of wrong if SAE is used, but SAE wasn't around
>> back then...
>>
>>> makes no difference for security either.
>>
>> The key is derived from the passphrase and thus it can be much more easily
>> brute-forced compared to a 32 byte key.
> 
> Sorry, which are you saying is easier to brute-force?
> 
> The PSK is 32 * 8 bits while the passphrase is up to about 63 * 6 bits
> which is stronger but not by much.  You can brute-force either.  With
> the passphrase you get the PSK.  With the PSK you don't get the
> passphrase but you don't need it.  So I don't see much difference.

But you're not generating a 63 byte passphrase! A 20 character passphrase is 
weaker.  And 8 char passphrase is likely too weak.  And if you're generating 30+ 
char passphrases, just generate a PSK and be done with it.

> 
>>   With a long enough passphrase it
>> doesn't matter of course, but the spec is recommending a bare minimum.
>>
>> With SAE it is less of a concern due to forward-secrecy properties.  With WPA1/2
>> it is a major issue since simply sniffing the traffic allows attacks against the
>> passphrase.
>>
>> Also consider that deriving the key from the passphrase on the peer takes extra
>> time.  On a slow CPU this can be quite significant and thus increases latency.
> 
> So you're saying we should derive the PSK from the passphrase and send
> the PSK in Network Key, right?

We can, but the simpler approach would be to just generate the PSK and forget 
the passphrase business for  now.

> 
> I don't think it's a noticeable task on a device that implements diffie-hellman.
> 

Key derivation runs 4K iterations of hmac(sha1), so its not trivial.  I need to 
run some numbers, I remember I was shocked how long this actually takes, which 
is why we cache the PSK in settings.  Maybe it is our implementation that needs 
fixing as well.

But still, I see no reason to make the other side suffer if we don't need to.

>>
>> Seriously though, I don't see how a per-client key is precluded by the spec.  It
>> doesn't support these outright, but it has no verbiage to limit such use.
> 
> I only said it doesn't consider that... but then it does say that you
> generate the credentials once and keep them until the group is torn
> down.

Which we would do ;)

> 
> Or do you mean it doesn't preclude it as in, it wouldn't break
> anything? As I said I think we can do it.  But throughout this thread
> you had been arguing for implementing the spec literally.
> 

I am arguing for that, but doesn't mean we should be doing pointless stuff. 
Right now I consider passphrases pointless.  We should not use them until a 
legacy client actually has to connect, which would be never.

Regards,
-Denis