[PATCH 1/5] station: Make Disconnect() cancel ConnectHiddenNetwork()

[PATCH 1/5] station: Make Disconnect() cancel ConnectHiddenNetwork()

[Andrew Zaborowski]

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

ConnectHiddenNetwork can be seen a triggering this sequence:
1. the active scan,
2. the optional agent request,
3. the Authentication/Association/4-Way Handshake/netconfig,
4. connected state

Currently Disconnect() interrupts 3 and 4, allow it to also interrupt
state 1.  It's difficult to tell whether we're in state 2 from within
station.c.
---
 src/station.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/station.c b/src/station.c
index 7fb4ae77..6ac9e53b 100644
--- a/src/station.c
+++ b/src/station.c
@@ -2749,6 +2749,15 @@ static struct l_dbus_message *station_dbus_disconnect(struct l_dbus *dbus,
 	 */
 	station_set_autoconnect(station, false);
 
+	if (station->hidden_network_scan_id) {
+		scan_cancel(netdev_get_wdev_id(station->netdev),
+				station->hidden_network_scan_id);
+		dbus_pending_reply(&station->hidden_pending,
+				dbus_error_aborted(station->hidden_pending));
+
+		return l_dbus_message_new_method_return(message);
+	}
+
 	if (!station_is_busy(station))
 		return l_dbus_message_new_method_return(message);
 
-- 
2.25.1

[PATCH 2/5] station: Add current hidden network to networks_sorted

[Andrew Zaborowski]

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

Update station->networks_sorted with new networks added inside
station_hidden_network_scan_results() because we rely on
station->networks being in sync with station->networks_sorted.
Otherwise station_set_scan_results may crash.

Optimally a generic scan shouldn't happen while we're connecting to a
new hidden network because the generic scan doesn't know about the
hidden network's SSID and will not include the SSID in the probe request
so it can at most expire the BSS we're trying to connect to.  However we
can't easily tell whether we're in the middle of a Connect() or
ConnectHiddenNetwork() call as mentioned in the previous commit, so we
have to accept that we may trigger a new scan while waiting for secrets
from the agent.  Thus the BSS discovered in the active scan for the
hidden SSID may expire if the agent request takes too long but we this
change at least we don't crash.
---
 src/station.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/station.c b/src/station.c
index 6ac9e53b..b70d3416 100644
--- a/src/station.c
+++ b/src/station.c
@@ -2572,11 +2572,16 @@ static bool station_hidden_network_scan_results(int err,
 					memcmp(bss->ssid, ssid, ssid_len))
 			goto next;
 
-		if (station_add_seen_bss(station, bss)) {
-			l_queue_push_tail(station->bss_list, bss);
+		network = station_add_seen_bss(station, bss);
+		if (!network)
+			goto next;
 
-			continue;
-		}
+		l_queue_push_tail(station->bss_list, bss);
+		network_rank_update(network, false);
+		l_queue_remove(station->networks_sorted, network);
+		l_queue_insert(station->networks_sorted, network,
+				network_rank_compare, NULL);
+		continue;
 
 next:
 		scan_bss_free(bss);
-- 
2.25.1

[PATCH 3/5] network: Check if network busy before new connection

[Andrew Zaborowski]

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

Check if we have an ongoing agent call before starting a new connection
attempt and potentially overwriting network->agent_request.
---
 src/network.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/network.c b/src/network.c
index 3e856aa1..388bc17a 100644
--- a/src/network.c
+++ b/src/network.c
@@ -1146,6 +1146,9 @@ static struct l_dbus_message *network_connect(struct l_dbus *dbus,
 		 */
 		return l_dbus_message_new_method_return(message);
 
+	if (network->agent_request)
+		return dbus_error_busy(message);
+
 	/*
 	 * Select the best BSS to use at this time.  If we have to query the
 	 * agent this may not be the final choice because BSS visibility can
@@ -1198,6 +1201,11 @@ void network_connect_new_hidden_network(struct network *network,
 
 	l_debug("");
 
+	if (network->agent_request) {
+		error = dbus_error_busy(*message);
+		goto reply_error;
+	}
+
 	/*
 	 * This is not a Known Network.  If connection succeeds, either
 	 * network_sync_psk or network_connected will save this network
-- 
2.25.1

[PATCH 4/5] station: Stop autoconnect for the hidden SSID scan

[Andrew Zaborowski]

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

---
 src/station.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/src/station.c b/src/station.c
index b70d3416..befc4e25 100644
--- a/src/station.c
+++ b/src/station.c
@@ -2557,12 +2557,12 @@ static bool station_hidden_network_scan_results(int err,
 
 	if (err) {
 		dbus_pending_reply(&msg, dbus_error_failed(msg));
-		return false;
+		goto error;
 	}
 
 	if (!l_dbus_message_get_arguments(msg, "s", &ssid)) {
 		dbus_pending_reply(&msg, dbus_error_invalid_args(msg));
-		return false;
+		goto error;
 	}
 
 	ssid_len = strlen(ssid);
@@ -2587,19 +2587,17 @@ next:
 		scan_bss_free(bss);
 	}
 
-	l_queue_destroy(bss_list, NULL);
-
 	network_psk = station_network_find(station, ssid, SECURITY_PSK);
 	network_open = station_network_find(station, ssid, SECURITY_NONE);
 
 	if (!network_psk && !network_open) {
 		dbus_pending_reply(&msg, dbus_error_not_found(msg));
-		return true;
+		goto error;
 	}
 
 	if (network_psk && network_open) {
 		dbus_pending_reply(&msg, dbus_error_service_set_overlap(msg));
-		return true;
+		goto error;
 	}
 
 	network = network_psk ? : network_open;
@@ -2607,6 +2605,12 @@ next:
 	network_connect_new_hidden_network(network, &msg);
 	l_dbus_message_unref(msg);
 
+	l_queue_destroy(bss_list, NULL);
+	return true;
+
+error:
+	l_queue_destroy(bss_list, (l_queue_destroy_func_t) scan_bss_free);
+	station_set_autoconnect(station, true);
 	return true;
 }
 
@@ -2659,6 +2663,7 @@ static struct l_dbus_message *station_dbus_connect_hidden_network(
 		return dbus_error_failed(message);
 
 	station->hidden_pending = l_dbus_message_ref(message);
+	station_set_autoconnect(station, false);
 
 	return NULL;
 }
-- 
2.25.1

[PATCH 5/5] network: Stop autoconnect during agent request or ANQP

[Andrew Zaborowski]

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

Stop autoconnect whenever we're saving a pending DBus message and resume
when we return the error response.

One event that may be unhandled here is when we cancel the agent request
in network_unregister which may happen when the station interface goes
DOWN or when the network goes out of range and expires.
---
 src/network.c | 14 ++++++++++++--
 src/station.c |  1 +
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/src/network.c b/src/network.c
index 388bc17a..08f855d2 100644
--- a/src/network.c
+++ b/src/network.c
@@ -827,6 +827,7 @@ static void passphrase_callback(enum agent_result result,
 
 err:
 	network_settings_close(network);
+	station_set_autoconnect(station, true);
 }
 
 static struct l_dbus_message *network_connect_psk(struct network *network,
@@ -862,6 +863,8 @@ static struct l_dbus_message *network_connect_psk(struct network *network,
 
 		if (!network->agent_request)
 			return dbus_error_no_agent(message);
+
+		station_set_autoconnect(station, false);
 	} else
 		station_connect_network(station, network, bss, message);
 
@@ -992,8 +995,10 @@ static bool eap_send_agent_req(struct network *network,
 		break;
 	}
 
-	if (network->agent_request)
+	if (network->agent_request) {
+		station_set_autoconnect(network->station, false);
 		return true;
+	}
 
 	eap_secret_request_free(req);
 	return false;
@@ -1071,6 +1076,7 @@ static void eap_secret_done(enum agent_result result,
 	dbus_pending_reply(&message, dbus_error_no_agent(message));
 err:
 	network_settings_close(network);
+	station_set_autoconnect(network->station, true);
 }
 
 static struct l_dbus_message *network_connect_8021x(struct network *network,
@@ -1180,6 +1186,7 @@ static struct l_dbus_message *network_connect(struct l_dbus *dbus,
 						l_dbus_message_ref(message);
 			l_debug("Pending ANQP request, delaying connect to %s",
 						network->ssid);
+			station_set_autoconnect(station, false);
 			return NULL;
 		}
 
@@ -1530,6 +1537,7 @@ static void anqp_watch_changed(enum station_anqp_state state,
 			reply = dbus_error_not_configured(
 						network->connect_after_anqp);
 			dbus_pending_reply(&network->connect_after_anqp, reply);
+			station_set_autoconnect(network->station, true);
 			return;
 		}
 
@@ -1537,8 +1545,10 @@ static void anqp_watch_changed(enum station_anqp_state state,
 					network_bss_select(network, true),
 					network->connect_after_anqp);
 
-		if (reply)
+		if (reply) {
 			l_dbus_send(dbus_get_bus(), reply);
+			station_set_autoconnect(network->station, true);
+		}
 
 		l_dbus_message_unref(network->connect_after_anqp);
 		network->connect_after_anqp = NULL;
diff --git a/src/station.c b/src/station.c
index befc4e25..120a11ea 100644
--- a/src/station.c
+++ b/src/station.c
@@ -2520,6 +2520,7 @@ void station_connect_network(struct station *station, struct network *network,
 
 error:
 	l_dbus_send(dbus, dbus_error_from_errno(err, message));
+	station_set_autoconnect(station, true);
 }
 
 static void station_hidden_network_scan_triggered(int err, void *user_data)
-- 
2.25.1

Re: [PATCH 1/5] station: Make Disconnect() cancel ConnectHiddenNetwork()

[Denis Kenzior]

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

Hi Andrew,

On 10/8/20 3:49 AM, Andrew Zaborowski wrote:
> ConnectHiddenNetwork can be seen a triggering this sequence:
> 1. the active scan,
> 2. the optional agent request,
> 3. the Authentication/Association/4-Way Handshake/netconfig,
> 4. connected state
> 
> Currently Disconnect() interrupts 3 and 4, allow it to also interrupt
> state 1.  It's difficult to tell whether we're in state 2 from within
> station.c.

Makes sense.  If you want to take care of 2, then we'd need to invoke 
station_network_foreach and abort the agent requests on each network.

> ---
>   src/station.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 

Applied, thanks.

Regards,
-Denis

Re: [PATCH 2/5] station: Add current hidden network to networks_sorted

[Denis Kenzior]

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

Hi Andrew,

On 10/8/20 3:49 AM, Andrew Zaborowski wrote:
> Update station->networks_sorted with new networks added inside
> station_hidden_network_scan_results() because we rely on
> station->networks being in sync with station->networks_sorted.
> Otherwise station_set_scan_results may crash.

Have you encountered a crash?  Or just suspect one exists?  A crash log would be 
nice.

> 
> Optimally a generic scan shouldn't happen while we're connecting to a
> new hidden network because the generic scan doesn't know about the
> hidden network's SSID and will not include the SSID in the probe request
> so it can at most expire the BSS we're trying to connect to.  However we
> can't easily tell whether we're in the middle of a Connect() or
> ConnectHiddenNetwork() call as mentioned in the previous commit, so we
> have to accept that we may trigger a new scan while waiting for secrets

Right.

> from the agent.  Thus the BSS discovered in the active scan for the
> hidden SSID may expire if the agent request takes too long but we this
> change at least we don't crash.

So looks like you're concerned about network_bss_list_clear() not being called 
on the temporary network we're connecting to (because 
station_hidden_network_scan_results() never adds it to the networks_sorted list) 
and those bsses being expired by station_bss_list_remove_expired_bsses()...? 
But then the crash would likely happen in network_connect somewhere...

But actually the nastier issue is if the new scan result list contains the same 
BSSes but in hidden form...

> ---
>   src/station.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/src/station.c b/src/station.c
> index 6ac9e53b..b70d3416 100644
> --- a/src/station.c
> +++ b/src/station.c
> @@ -2572,11 +2572,16 @@ static bool station_hidden_network_scan_results(int err,
>   					memcmp(bss->ssid, ssid, ssid_len))
>   			goto next;
>   
> -		if (station_add_seen_bss(station, bss)) {
> -			l_queue_push_tail(station->bss_list, bss);
> +		network = station_add_seen_bss(station, bss);
> +		if (!network)
> +			goto next;
>   
> -			continue;
> -		}
> +		l_queue_push_tail(station->bss_list, bss);
> +		network_rank_update(network, false);
> +		l_queue_remove(station->networks_sorted, network);
> +		l_queue_insert(station->networks_sorted, network,
> +				network_rank_compare, NULL);
> +		continue;

This looks okay.  Not sure if we really care about showing this network as part 
of the sorted list until we're attempting to connect, so maybe just using 
l_hashmap_foreach(station->networks, network_bss_list_clear()) would be better?

But it seems to me this still doesn't solve the issue of the hidden network 
object being removed by process_network() if the scan is performed?

Regards,
-Denis

>   
>   next:
>   		scan_bss_free(bss);
> 

Re: [PATCH 3/5] network: Check if network busy before new connection

[Denis Kenzior]

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

Hi Andrew,

On 10/8/20 3:49 AM, Andrew Zaborowski wrote:
> Check if we have an ongoing agent call before starting a new connection
> attempt and potentially overwriting network->agent_request.
> ---
>   src/network.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 

Applied, thanks.

Regards,
-Denis

Re: [PATCH 5/5] network: Stop autoconnect during agent request or ANQP

[Denis Kenzior]

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

Hi Andrew,

On 10/8/20 3:49 AM, Andrew Zaborowski wrote:
> Stop autoconnect whenever we're saving a pending DBus message and resume
> when we return the error response.

So I don't think that we can do it this way.  The problem is that multiple 
Network.Connects may exist in the 'Obtain secrets' stage.  So even if you pause 
autoconnect for the first, the 2 .. n would still end up  pending in the 
autoconnect stage...

We probably need to discuss the whys of what you're trying to accomplish here...

Regards,
-Denis

Re: [PATCH 2/5] station: Add current hidden network to networks_sorted

[Andrew Zaborowski]

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

Hi Denis,

On Thu, 8 Oct 2020 at 17:19, Denis Kenzior <denkenz@gmail.com> wrote:
> On 10/8/20 3:49 AM, Andrew Zaborowski wrote:
> > Update station->networks_sorted with new networks added inside
> > station_hidden_network_scan_results() because we rely on
> > station->networks being in sync with station->networks_sorted.
> > Otherwise station_set_scan_results may crash.
>
> Have you encountered a crash?  Or just suspect one exists?  A crash log would be
> nice.

I was running under valgrind and it just reported invalid accesses but
didn't crash that time.

>
> >
> > Optimally a generic scan shouldn't happen while we're connecting to a
> > new hidden network because the generic scan doesn't know about the
> > hidden network's SSID and will not include the SSID in the probe request
> > so it can at most expire the BSS we're trying to connect to.  However we
> > can't easily tell whether we're in the middle of a Connect() or
> > ConnectHiddenNetwork() call as mentioned in the previous commit, so we
> > have to accept that we may trigger a new scan while waiting for secrets
>
> Right.
>
> > from the agent.  Thus the BSS discovered in the active scan for the
> > hidden SSID may expire if the agent request takes too long but we this
> > change at least we don't crash.
>
> So looks like you're concerned about network_bss_list_clear() not being called
> on the temporary network we're connecting to (because
> station_hidden_network_scan_results() never adds it to the networks_sorted list)

Right, so we don't call network_bss_list_clear() on the hidden network
but we do free the old BSS because the new bss list has the same BSS
in it (in my case it did include the SSID, it was identical).  Next we
try to insert the new scan_bss into the network and
scan_bss_rank_compare() triggers an invalid access.

> and those bsses being expired by station_bss_list_remove_expired_bsses()...?
> But then the crash would likely happen in network_connect somewhere...
>
> But actually the nastier issue is if the new scan result list contains the same
> BSSes but in hidden form...

True, in that case I guess the old BSS gets freed and the network gets
removed.  We probably need to treat it as a special case in
station_set_scan_results.

>
> > ---
> >   src/station.c | 13 +++++++++----
> >   1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/station.c b/src/station.c
> > index 6ac9e53b..b70d3416 100644
> > --- a/src/station.c
> > +++ b/src/station.c
> > @@ -2572,11 +2572,16 @@ static bool station_hidden_network_scan_results(int err,
> >                                       memcmp(bss->ssid, ssid, ssid_len))
> >                       goto next;
> >
> > -             if (station_add_seen_bss(station, bss)) {
> > -                     l_queue_push_tail(station->bss_list, bss);
> > +             network = station_add_seen_bss(station, bss);
> > +             if (!network)
> > +                     goto next;
> >
> > -                     continue;
> > -             }
> > +             l_queue_push_tail(station->bss_list, bss);
> > +             network_rank_update(network, false);
> > +             l_queue_remove(station->networks_sorted, network);
> > +             l_queue_insert(station->networks_sorted, network,
> > +                             network_rank_compare, NULL);
> > +             continue;
>
> This looks okay.  Not sure if we really care about showing this network as part
> of the sorted list until we're attempting to connect, so maybe just using
> l_hashmap_foreach(station->networks, network_bss_list_clear()) would be better?

Inside station_set_scan_results?  That would work too, but I think it
may be better to be consistent and report this network like other
networks in case it confuses clients.

>
> But it seems to me this still doesn't solve the issue of the hidden network
> object being removed by process_network() if the scan is performed?

Right, I didn't realize the whole network would get removed.  If it
was just the BSSes then the next time network.c tries to select a BSS
it would just abort connection, but if the network goes away we crash.
So I didn't bother fixing every scenario in this patchset.

I was thinking ConnectHiddenNetwork should set connected_bss,
connected_network from the beginning, and station_is_busy() should
report true.  network.c and wsc.c should look at station_is_busy()
before accepting a connection, and should also notify station that
they have a pending Connect() / PushButton() / StartPIN().

Best regards