[PATCH v3 1/4] dpp: remove duplicate connected network check

[PATCH v3 1/4] dpp: remove duplicate connected network check

[James Prestwood]

---
 src/dpp.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

v3:
 * Just removed the extra check. Requiring DISCONNECTED is a bit
   strict since its ok if station is in an autoconnecting state
   (just as long as it doesn't actually connect)

diff --git a/src/dpp.c b/src/dpp.c
index 23010a05..3a740916 100644
--- a/src/dpp.c
+++ b/src/dpp.c
@@ -4001,14 +4001,8 @@ static uint32_t *dpp_default_freqs(struct dpp_sm *dpp, size_t *out_len)
 static bool dpp_start_pkex_enrollee(struct dpp_sm *dpp, const char *key,
 				const char *identifier)
 {
-	struct station *station = station_find(netdev_get_ifindex(dpp->netdev));
 	_auto_(l_ecc_point_free) struct l_ecc_point *qi = NULL;
 
-	if (station && station_get_connected_network(station)) {
-		l_debug("Already connected, disconnect before enrolling");
-		return false;
-	}
-
 	if (identifier)
 		dpp->pkex_id = l_strdup(identifier);
 
@@ -4120,7 +4114,7 @@ static struct l_dbus_message *dpp_dbus_pkex_start_enrollee(struct l_dbus *dbus,
 				dpp->interface != DPP_INTERFACE_UNBOUND)
 		return dbus_error_busy(message);
 
-	if (station_get_connected_network(station))
+	if (station && station_get_connected_network(station))
 		return dbus_error_busy(message);
 
 	if (!dpp_parse_pkex_args(message, &key, &id))
-- 
2.25.1

[PATCH v3 2/4] dpp: add station watch to DPP

[James Prestwood]

DPP (both DPP and PKEX) run the risk of odd behavior if station
decides to change state. DPP is completely unaware of this and
best case would just result in a protocol failure, worst case
duplicate calls to __station_connect_network.

Add a station watch and stop DPP if station changes state during
the protocol.
---
 src/dpp.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

v3:
 * Allow autoconnect states while enrolling. Its probably unlikely
   that a state change would happen but going between DISCONNECTED,
   AUTOCONNECT_QUICK, and AUTOCONNECT_FULL won't cause any issues
   while enrolling.

diff --git a/src/dpp.c b/src/dpp.c
index 3a740916..06ae2929 100644
--- a/src/dpp.c
+++ b/src/dpp.c
@@ -100,6 +100,7 @@ struct dpp_sm {
 	char *uri;
 	uint8_t role;
 	int refcount;
+	uint32_t station_watch;
 
 	uint64_t wdev_id;
 
@@ -536,6 +537,8 @@ static void dpp_reset(struct dpp_sm *dpp)
 
 static void dpp_free(struct dpp_sm *dpp)
 {
+	struct station *station = station_find(netdev_get_ifindex(dpp->netdev));
+
 	dpp_reset(dpp);
 
 	if (dpp->own_asn1) {
@@ -553,6 +556,13 @@ static void dpp_free(struct dpp_sm *dpp)
 		dpp->boot_private = NULL;
 	}
 
+	/*
+	 * Since this is called when the netdev goes down, station may already
+	 * be gone in which case the state watch will automatically go away.
+	 */
+	if (station)
+		station_remove_state_watch(station, dpp->station_watch);
+
 	l_free(dpp);
 }
 
@@ -3608,6 +3618,67 @@ static void dpp_frame_watch(struct dpp_sm *dpp, uint16_t frame_type,
 					L_UINT_TO_PTR(frame_type), NULL);
 }
 
+/*
+ * Station is unaware of DPP's state so we need to handle a few cases here so
+ * weird stuff doesn't happen:
+ *
+ *   - While configuring we should stay connected, a disconnection/roam should
+ *     stop DPP since it would fail regardless due to the hardware going idle
+ *     or changing channels since configurators assume all comms will be
+ *     on-channel.
+ *   - While enrolling we should stay disconnected. If station connects during
+ *     enrolling it would cause 2x calls to __station_connect_network after
+ *     DPP finishes.
+ *
+ * Other conditions shouldn't ever happen i.e. configuring and going into a
+ * connecting state or enrolling and going to a disconnected/roaming state.
+ */
+static void dpp_station_state_watch(enum station_state state, void *user_data)
+{
+	struct dpp_sm *dpp = user_data;
+
+	switch (state) {
+	case STATION_STATE_DISCONNECTED:
+	case STATION_STATE_DISCONNECTING:
+	case STATION_STATE_ROAMING:
+	case STATION_STATE_FT_ROAMING:
+	case STATION_STATE_FW_ROAMING:
+		if (L_WARN_ON(dpp->role == DPP_CAPABILITY_ENROLLEE))
+			dpp_reset(dpp);
+
+		if (dpp->role == DPP_CAPABILITY_CONFIGURATOR) {
+			l_debug("Disconnected while configuring, stopping DPP");
+			dpp_reset(dpp);
+		}
+
+		break;
+	case STATION_STATE_CONNECTING:
+	case STATION_STATE_CONNECTED:
+	case STATION_STATE_CONNECTING_AUTO:
+		if (L_WARN_ON(dpp->role == DPP_CAPABILITY_CONFIGURATOR))
+			dpp_reset(dpp);
+
+		if (dpp->role == DPP_CAPABILITY_ENROLLEE) {
+			l_debug("Connecting while enrolling, stopping DPP");
+			dpp_reset(dpp);
+		}
+
+		break;
+
+	/*
+	 * Autoconnect states are fine for enrollees. This makes it nicer for
+	 * the user since they don't need to explicity Disconnect() to disable
+	 * autoconnect, then re-enable it if DPP fails.
+	 */
+	case STATION_STATE_AUTOCONNECT_FULL:
+	case STATION_STATE_AUTOCONNECT_QUICK:
+		if (L_WARN_ON(dpp->role == DPP_CAPABILITY_CONFIGURATOR))
+			dpp_reset(dpp);
+
+		break;
+	}
+}
+
 static void dpp_create(struct netdev *netdev)
 {
 	struct l_dbus *dbus = dbus_get_bus();
@@ -3615,6 +3686,7 @@ static void dpp_create(struct netdev *netdev)
 	uint8_t dpp_conf_response_prefix[] = { 0x04, 0x0b };
 	uint8_t dpp_conf_request_prefix[] = { 0x04, 0x0a };
 	uint64_t wdev_id = netdev_get_wdev_id(netdev);
+	struct station *station = station_find(netdev_get_ifindex(netdev));
 
 	dpp->netdev = netdev;
 	dpp->state = DPP_STATE_NOTHING;
@@ -3660,6 +3732,9 @@ static void dpp_create(struct netdev *netdev)
 				sizeof(dpp_conf_request_prefix),
 				dpp_handle_config_request_frame, dpp, NULL);
 
+	dpp->station_watch = station_add_state_watch(station,
+					dpp_station_state_watch, dpp, NULL);
+
 	l_queue_push_tail(dpp_list, dpp);
 }
 
-- 
2.25.1

[PATCH v3 3/4] dpp: fix fragile scan/connecting logic

[James Prestwood]

The post-DPP connection was never done quite right due to station's
state being unknown. The state is now tracked in DPP by a previous
patch but the scan path in DPP is still wrong.

It relies on station autoconnect logic which has the potential to
connect to a different network than what was configured with DPP.
Its unlikely but still could happen in theory. In addition the scan
was not selectively filtering results by the SSID that DPP
configured.

This fixes the above problems by first filtering the scan by the
SSID. Then setting the scan results into station without triggering
autoconnect. And finally using network_autoconnect() directly
instead of relying on station to choose the SSID.
---
 src/dpp.c | 44 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/src/dpp.c b/src/dpp.c
index 06ae2929..a95f93e2 100644
--- a/src/dpp.c
+++ b/src/dpp.c
@@ -853,13 +853,42 @@ static bool dpp_scan_results(int err, struct l_queue *bss_list,
 {
 	struct dpp_sm *dpp = userdata;
 	struct station *station = station_find(netdev_get_ifindex(dpp->netdev));
+	struct scan_bss *bss;
+	char ssid[33];
+	struct network *network;
 
 	if (err < 0)
-		return false;
+		goto reset;
+
+	if (!bss_list || l_queue_length(bss_list) == 0)
+		goto reset;
+
+	/*
+	 * The station watch _should_ detect this and reset, which cancels the
+	 * scan. But just in case...
+	 */
+	if (L_WARN_ON(station_get_connected_network(station)))
+		goto reset;
+
+	/* Purely for grabbing the SSID */
+	bss = l_queue_peek_head(bss_list);
 
-	station_set_scan_results(station, bss_list, freqs, true);
+	memcpy(ssid, bss->ssid, bss->ssid_len);
+	ssid[bss->ssid_len] = '\0';
+
+	station_set_scan_results(station, bss_list, freqs, false);
+
+	network = station_network_find(station, ssid, SECURITY_PSK);
+
+	dpp_reset(dpp);
+
+	bss = network_bss_select(network, true);
+	network_autoconnect(network, bss);
 
 	return true;
+
+reset:
+	return false;
 }
 
 static void dpp_scan_destroy(void *userdata)
@@ -898,6 +927,7 @@ static void dpp_handle_config_response_frame(const struct mmpdu_header *frame,
 	struct network *network = NULL;
 	struct scan_bss *bss = NULL;
 	char ssid[33];
+	size_t ssid_len;
 
 	if (dpp->state != DPP_STATE_CONFIGURING)
 		return;
@@ -1027,6 +1057,7 @@ static void dpp_handle_config_response_frame(const struct mmpdu_header *frame,
 	 */
 	if (station) {
 		memcpy(ssid, config->ssid, config->ssid_len);
+		ssid_len = config->ssid_len;
 		ssid[config->ssid_len] = '\0';
 
 		network = station_network_find(station, ssid, SECURITY_PSK);
@@ -1045,7 +1076,14 @@ static void dpp_handle_config_response_frame(const struct mmpdu_header *frame,
 		__station_connect_network(station, network, bss,
 						STATION_STATE_CONNECTING);
 	else if (station) {
-		dpp->connect_scan_id = scan_active(dpp->wdev_id, NULL, 0,
+		struct scan_parameters params = {0};
+
+		params.ssid = (void *) ssid;
+		params.ssid_len = ssid_len;
+
+		l_debug("Scanning for %s", ssid);
+
+		dpp->connect_scan_id = scan_active_full(dpp->wdev_id, &params,
 						dpp_scan_triggered,
 						dpp_scan_results, dpp,
 						dpp_scan_destroy);
-- 
2.25.1

Re: [PATCH v3 3/4] dpp: fix fragile scan/connecting logic

[Denis Kenzior]

Hi James,

On 11/13/23 11:54, James Prestwood wrote:
> The post-DPP connection was never done quite right due to station's
> state being unknown. The state is now tracked in DPP by a previous
> patch but the scan path in DPP is still wrong.
> 
> It relies on station autoconnect logic which has the potential to
> connect to a different network than what was configured with DPP.
> Its unlikely but still could happen in theory. In addition the scan
> was not selectively filtering results by the SSID that DPP
> configured.
> 
> This fixes the above problems by first filtering the scan by the
> SSID. Then setting the scan results into station without triggering
> autoconnect. And finally using network_autoconnect() directly
> instead of relying on station to choose the SSID.
> ---
>   src/dpp.c | 44 +++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 41 insertions(+), 3 deletions(-)
> 

All 4 applied, but one comment:

> diff --git a/src/dpp.c b/src/dpp.c
> index 06ae2929..a95f93e2 100644
> --- a/src/dpp.c
> +++ b/src/dpp.c
> @@ -853,13 +853,42 @@ static bool dpp_scan_results(int err, struct l_queue *bss_list,
>   {
>   	struct dpp_sm *dpp = userdata;
>   	struct station *station = station_find(netdev_get_ifindex(dpp->netdev));
> +	struct scan_bss *bss;
> +	char ssid[33];
> +	struct network *network;
>   
>   	if (err < 0)
> -		return false;
> +		goto reset;
> +
> +	if (!bss_list || l_queue_length(bss_list) == 0)
> +		goto reset;
> +
> +	/*
> +	 * The station watch _should_ detect this and reset, which cancels the
> +	 * scan. But just in case...
> +	 */
> +	if (L_WARN_ON(station_get_connected_network(station)))
> +		goto reset;
> +
> +	/* Purely for grabbing the SSID */
> +	bss = l_queue_peek_head(bss_list);
>   
> -	station_set_scan_results(station, bss_list, freqs, true);
> +	memcpy(ssid, bss->ssid, bss->ssid_len);
> +	ssid[bss->ssid_len] = '\0';

Are you sure this SSID is UTF8? station_set_scan_results will filter any SSIDs 
that are not.

> +
> +	station_set_scan_results(station, bss_list, freqs, false);
> +
> +	network = station_network_find(station, ssid, SECURITY_PSK);

And so this can fail.

> +
> +	dpp_reset(dpp);
> +
> +	bss = network_bss_select(network, true);
> +	network_autoconnect(network, bss);
>   
>   	return true;
> +
> +reset:
> +	return false;
>   }
>   
>   static void dpp_scan_destroy(void *userdata)

Regards,
-Denis

Re: [PATCH v3 3/4] dpp: fix fragile scan/connecting logic

[James Prestwood]

Hi Denis,

On 11/16/23 07:18, Denis Kenzior wrote:
> Hi James,
>
> On 11/13/23 11:54, James Prestwood wrote:
>> The post-DPP connection was never done quite right due to station's
>> state being unknown. The state is now tracked in DPP by a previous
>> patch but the scan path in DPP is still wrong.
>>
>> It relies on station autoconnect logic which has the potential to
>> connect to a different network than what was configured with DPP.
>> Its unlikely but still could happen in theory. In addition the scan
>> was not selectively filtering results by the SSID that DPP
>> configured.
>>
>> This fixes the above problems by first filtering the scan by the
>> SSID. Then setting the scan results into station without triggering
>> autoconnect. And finally using network_autoconnect() directly
>> instead of relying on station to choose the SSID.
>> ---
>>   src/dpp.c | 44 +++++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 41 insertions(+), 3 deletions(-)
>>
>
> All 4 applied, but one comment:
>
>> diff --git a/src/dpp.c b/src/dpp.c
>> index 06ae2929..a95f93e2 100644
>> --- a/src/dpp.c
>> +++ b/src/dpp.c
>> @@ -853,13 +853,42 @@ static bool dpp_scan_results(int err, struct 
>> l_queue *bss_list,
>>   {
>>       struct dpp_sm *dpp = userdata;
>>       struct station *station = 
>> station_find(netdev_get_ifindex(dpp->netdev));
>> +    struct scan_bss *bss;
>> +    char ssid[33];
>> +    struct network *network;
>>         if (err < 0)
>> -        return false;
>> +        goto reset;
>> +
>> +    if (!bss_list || l_queue_length(bss_list) == 0)
>> +        goto reset;
>> +
>> +    /*
>> +     * The station watch _should_ detect this and reset, which 
>> cancels the
>> +     * scan. But just in case...
>> +     */
>> +    if (L_WARN_ON(station_get_connected_network(station)))
>> +        goto reset;
>> +
>> +    /* Purely for grabbing the SSID */
>> +    bss = l_queue_peek_head(bss_list);
>>   -    station_set_scan_results(station, bss_list, freqs, true);
>> +    memcpy(ssid, bss->ssid, bss->ssid_len);
>> +    ssid[bss->ssid_len] = '\0';
>
> Are you sure this SSID is UTF8? station_set_scan_results will filter 
> any SSIDs that are not.

Good catch, I hadn't considered that. BUT in theory this shouldn't 
happen since utf8 is verified when parsing the configuration object. 
Either way, I still sent a patch because an extra check doesn't hurt, 
and its results coming from the kernel so I'd rather avoid a crash if 
something ever changed in the future.

Thanks,

James

[PATCH v3 4/4] dpp: scan to pick up extra frequencies when enrolling

[James Prestwood]

The DPP-PKEX spec provides a very limited list of frequencies used
to discover configurators, only 3 on 2.4 and 5GHz bands. Since
configurators (at least in IWD's implementation) are only allowed
on the current operating frequency its very unlikely an enrollee
will find a configurator on these frequencies out of the entire
spectrum.

The spec does mention that the 3 default frequencies should be used
"In lieu of specific channel information obtained in a manner outside
the scope of this specification, ...". This allows the implementation
some flexibility in using a broader range of frequencies.

To increase the chances of finding a configurator shared code
enrollees will first issue a scan to determine what access points are
around, then iterate these frequencies. This is especially helpful
when the configurators are IWD-based since we know that they'll be
on the same channels as the APs in the area.
---
 src/dpp.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 88 insertions(+), 10 deletions(-)

diff --git a/src/dpp.c b/src/dpp.c
index a95f93e2..41b7c70e 100644
--- a/src/dpp.c
+++ b/src/dpp.c
@@ -183,6 +183,7 @@ struct dpp_sm {
 	size_t z_len;
 	uint8_t u[L_ECC_SCALAR_MAX_BYTES];
 	size_t u_len;
+	uint32_t pkex_scan_id;
 
 	bool mcast_support : 1;
 	bool roc_started : 1;
@@ -508,6 +509,11 @@ static void dpp_reset(struct dpp_sm *dpp)
 		dpp->retry_timeout = NULL;
 	}
 
+	if (dpp->pkex_scan_id) {
+		scan_cancel(dpp->wdev_id, dpp->pkex_scan_id);
+		dpp->pkex_scan_id = 0;
+	}
+
 	dpp->state = DPP_STATE_NOTHING;
 	dpp->new_freq = 0;
 	dpp->frame_retry = 0;
@@ -4073,6 +4079,14 @@ static struct l_dbus_message *dpp_dbus_stop(struct l_dbus *dbus,
 	return l_dbus_message_new_method_return(message);
 }
 
+static void dpp_pkex_scan_trigger(int err, void *user_data)
+{
+	struct dpp_sm *dpp = user_data;
+
+	if (err < 0)
+		dpp_reset(dpp);
+}
+
 /*
  * Section 5.6.1
  * In lieu of specific channel information obtained in a manner outside
@@ -4111,6 +4125,62 @@ static uint32_t *dpp_default_freqs(struct dpp_sm *dpp, size_t *out_len)
 	return freqs_out;
 }
 
+static bool dpp_pkex_scan_notify(int err, struct l_queue *bss_list,
+					const struct scan_freq_set *freqs,
+					void *user_data)
+{
+	struct dpp_sm *dpp = user_data;
+	const struct l_queue_entry *e;
+	_auto_(scan_freq_set_free) struct scan_freq_set *freq_set = NULL;
+
+	if (err < 0)
+		goto failed;
+
+	freq_set = scan_freq_set_new();
+
+	if (!bss_list || l_queue_isempty(bss_list)) {
+		dpp->freqs = dpp_default_freqs(dpp, &dpp->freqs_len);
+		if (!dpp->freqs)
+			goto failed;
+
+		l_debug("No BSS's seen, using default frequency list");
+		goto start;
+	}
+
+	for (e = l_queue_get_entries(bss_list); e; e = e->next) {
+		const struct scan_bss *bss = e->data;
+
+		scan_freq_set_add(freq_set, bss->frequency);
+	}
+
+	l_debug("Found %u frequencies to search for configurator",
+			l_queue_length(bss_list));
+
+	dpp->freqs = scan_freq_set_to_fixed_array(freq_set, &dpp->freqs_len);
+
+start:
+	dpp->current_freq = dpp->freqs[0];
+
+	dpp_reset_protocol_timer(dpp, DPP_PKEX_PROTO_TIMEOUT);
+
+	l_debug("PKEX start enrollee (id=%s)", dpp->pkex_id ?: "unset");
+
+	dpp_start_offchannel(dpp, dpp->current_freq);
+
+	return false;
+
+failed:
+	dpp_reset(dpp);
+	return false;
+}
+
+static void dpp_pkex_scan_destroy(void *user_data)
+{
+	struct dpp_sm *dpp = user_data;
+
+	dpp->pkex_scan_id = 0;
+}
+
 static bool dpp_start_pkex_enrollee(struct dpp_sm *dpp, const char *key,
 				const char *identifier)
 {
@@ -4156,17 +4226,25 @@ static bool dpp_start_pkex_enrollee(struct dpp_sm *dpp, const char *key,
 
 	dpp_property_changed_notify(dpp);
 
-	dpp->freqs = dpp_default_freqs(dpp, &dpp->freqs_len);
-	if (!dpp->freqs)
-		goto failed;
-
-	dpp->current_freq = dpp->freqs[dpp->freqs_idx];
-
-	dpp_reset_protocol_timer(dpp, DPP_PKEX_PROTO_TIMEOUT);
-
-	l_debug("PKEX start enrollee (id=%s)", dpp->pkex_id ?: "unset");
+	/*
+	 * The 'dpp_default_freqs' function returns the default frequencies
+	 * outlined in section 5.6.1. For 2.4/5GHz this is only 3 frequencies
+	 * which is unlikely to result in discovery of a configurator. The spec
+	 * does allow frequencies to be "obtained in a manner outside the scope
+	 * of this specification" which is what is being done here.
+	 *
+	 * This is mainly geared towards IWD-based configurators; banking on the
+	 * fact that they are currently connected to nearby APs. Scanning lets
+	 * us see nearby BSS's which should be the same frequencies as our
+	 * target configurator.
+	 */
+	l_debug("Performing scan for frequencies to start PKEX");
 
-	dpp_start_offchannel(dpp, dpp->current_freq);
+	dpp->pkex_scan_id = scan_active(dpp->wdev_id, NULL, 0,
+				dpp_pkex_scan_trigger, dpp_pkex_scan_notify,
+				dpp, dpp_pkex_scan_destroy);
+	if (!dpp->pkex_scan_id)
+		goto failed;
 
 	return true;
 
-- 
2.25.1