[v2,1/3] station: add checks to prevent multiple roam scans

Commit Message

James Prestwood Jan. 13, 2023, 9:24 p.m. UTC

Under the following conditions IWD can accidentally trigger a second
roam scan while one is already in progress:

 - A low RSSI condition is met. This starts the roam rearm timer.
 - A packet loss condition is met, which triggers a roam scan.
 - The roam rearm timer fires and starts another roam scan while
   also overwriting the first roam scan ID.
 - Then, if IWD gets disconnected the overwritten roam scan gets
   canceled, and the roam state is cleared which NULL's
   station->connected_network.
 - The initial roam scan results then come in with the assumption
   that IWD is still connected which results in a crash trying to
   reference station->connected_network.

This can be fixed by adding a station_cannot_roam check in the rearm
timer. If IWD is already doing a roam scan station->preparing_roam
should be set which will cause it to return true and stop any further
action.

Aborting (signal 11) [/usr/libexec/iwd]
iwd[426]: ++++++++ backtrace ++++++++
iwd[426]: #0  0x7f858d7b2090 in /lib/x86_64-linux-gnu/libc.so.6
iwd[426]: #1  0x443df7 in network_get_security() at ome/locus/workspace/iwd/src/network.c:287
iwd[426]: #2  0x421fbb in station_roam_scan_notify() at ome/locus/workspace/iwd/src/station.c:2516
iwd[426]: #3  0x43ebc1 in scan_finished() at ome/locus/workspace/iwd/src/scan.c:1861
iwd[426]: #4  0x43ecf2 in get_scan_done() at ome/locus/workspace/iwd/src/scan.c:1891
iwd[426]: #5  0x4cbfe9 in destroy_request() at ome/locus/workspace/iwd/ell/genl.c:676
iwd[426]: #6  0x4cc98b in process_unicast() at ome/locus/workspace/iwd/ell/genl.c:954
iwd[426]: #7  0x4ccd28 in received_data() at ome/locus/workspace/iwd/ell/genl.c:1052
iwd[426]: #8  0x4c79c9 in io_callback() at ome/locus/workspace/iwd/ell/io.c:120
iwd[426]: #9  0x4c62e3 in l_main_iterate() at ome/locus/workspace/iwd/ell/main.c:476
iwd[426]: #10 0x4c6426 in l_main_run() at ome/locus/workspace/iwd/ell/main.c:519
iwd[426]: #11 0x4c6752 in l_main_run_with_signal() at ome/locus/workspace/iwd/ell/main.c:645
iwd[426]: #12 0x405987 in main() at ome/locus/workspace/iwd/src/main.c:600
iwd[426]: #13 0x7f858d793083 in /lib/x86_64-linux-gnu/libc.so.6
iwd[426]: +++++++++++++++++++++++++++
---
 src/station.c | 47 +++++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

Comments

Denis Kenzior Jan. 13, 2023, 11:01 p.m. UTC | #1

Hi James,

On 1/13/23 15:24, James Prestwood wrote:
> Under the following conditions IWD can accidentally trigger a second
> roam scan while one is already in progress:
> 
>   - A low RSSI condition is met. This starts the roam rearm timer.
>   - A packet loss condition is met, which triggers a roam scan.
>   - The roam rearm timer fires and starts another roam scan while
>     also overwriting the first roam scan ID.
>   - Then, if IWD gets disconnected the overwritten roam scan gets
>     canceled, and the roam state is cleared which NULL's
>     station->connected_network.
>   - The initial roam scan results then come in with the assumption
>     that IWD is still connected which results in a crash trying to
>     reference station->connected_network.
> 
> This can be fixed by adding a station_cannot_roam check in the rearm
> timer. If IWD is already doing a roam scan station->preparing_roam
> should be set which will cause it to return true and stop any further
> action.
> 

All applied, thanks.

Regards,
-Denis

Patch

diff --git a/src/station.c b/src/station.c
index 4452e83e..1a69b98a 100644
--- a/src/station.c
+++ b/src/station.c
@@ -2701,6 +2701,28 @@  static void station_start_roam(struct station *station)
 		station_roam_failed(station);
 }
 
+static bool station_cannot_roam(struct station *station)
+{
+	const struct l_settings *config = iwd_get_config();
+	bool disabled;
+
+	/*
+	 * Disable roaming with hardware that can roam automatically. Note this
+	 * is now required for recent kernels which have CQM event support on
+	 * this type of hardware (e.g. brcmfmac).
+	 */
+	if (wiphy_supports_firmware_roam(station->wiphy))
+		return true;
+
+	if (!l_settings_get_bool(config, "Scan", "DisableRoamingScan",
+								&disabled))
+		disabled = false;
+
+	return disabled || station->preparing_roam ||
+				station->state == STATION_STATE_ROAMING ||
+				station->state == STATION_STATE_FT_ROAMING;
+}
+
 static void station_roam_trigger_cb(struct l_timeout *timeout, void *user_data)
 {
 	struct station *station = user_data;
@@ -2710,6 +2732,9 @@  static void station_roam_trigger_cb(struct l_timeout *timeout, void *user_data)
 	l_timeout_remove(station->roam_trigger_timeout);
 	station->roam_trigger_timeout = NULL;
 
+	if (station_cannot_roam(station))
+		return;
+
 	station_start_roam(station);
 }
 
@@ -2735,28 +2760,6 @@  static void station_roam_timeout_rearm(struct station *station, int seconds)
 								station, NULL);
 }
 
-static bool station_cannot_roam(struct station *station)
-{
-	const struct l_settings *config = iwd_get_config();
-	bool disabled;
-
-	/*
-	 * Disable roaming with hardware that can roam automatically. Note this
-	 * is now required for recent kernels which have CQM event support on
-	 * this type of hardware (e.g. brcmfmac).
-	 */
-	if (wiphy_supports_firmware_roam(station->wiphy))
-		return true;
-
-	if (!l_settings_get_bool(config, "Scan", "DisableRoamingScan",
-								&disabled))
-		disabled = false;
-
-	return disabled || station->preparing_roam ||
-				station->state == STATION_STATE_ROAMING ||
-				station->state == STATION_STATE_FT_ROAMING;
-}
-
 #define WNM_REQUEST_MODE_PREFERRED_CANDIDATE_LIST	(1 << 0)
 #define WNM_REQUEST_MODE_DISASSOCIATION_IMMINENT	(1 << 2)
 #define WNM_REQUEST_MODE_TERMINATION_IMMINENT		(1 << 3)