[PATCH v2 1/2] scan: don't survey on external scans

[PATCH v2 1/2] scan: don't survey on external scans

[James Prestwood]

Since surveys end up making driver calls in the kernel its not
entirely known how they are implemented or how long they will
take. For this reason the survey will be skipped if getting the
results from an external scan.

Doing this also fixes a crash caused by external scans where the
scan request pointer is not checked and dereferenced:

0x00005ffa6a0376de in get_survey_done (user_data=0x5ffa783a3f90) at src/scan.c:2059
0x0000749646a29bbd in ?? () from /usr/lib/libell.so.0
0x0000749646a243cb in ?? () from /usr/lib/libell.so.0
0x0000749646a24655 in l_main_iterate () from /usr/lib/libell.so.0
0x0000749646a24ace in l_main_run () from /usr/lib/libell.so.0
0x0000749646a263a4 in l_main_run_with_signal () from /usr/lib/libell.so.0
0x00005ffa6a00d642 in main (argc=<optimized out>, argv=<optimized out>) at src/main.c:614

Reported-by: Daniel Bond <danielbondno@gmail.com>
---
 src/scan.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

v2:
 * Disabled scan surveys rather than allowing a NULL scan request

diff --git a/src/scan.c b/src/scan.c
index debdeb1f..1cec9785 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -2089,9 +2089,10 @@ static void scan_get_results(struct scan_context *sc, struct scan_request *sr,
 	results->bss_list = l_queue_new();
 	results->freqs = freqs;
 
-	if (scan_survey(results))
+	/* If there is no scan request (external scan), just get the results */
+	if (sr && scan_survey(results))
 		return;
-	else
+	else if (sr)
 		l_warn("failed to start a scan survey");
 
 	get_results(results);
-- 
2.34.1

[PATCH v2 2/2] scan: check pending requests after regdom update

[James Prestwood]

While there is proper handling for a regdom update during a
TRIGGER_SCAN scan, prior to NEW_SCAN_RESULTS there is no such
handling if the regdom update comes in during a GET_SCAN or
GET_SURVEY.

In both the 6ghz and non-6ghz code paths we have some issues:
  - For non-6ghz devices, or regdom updates that did not enable
    6ghz the wiphy state watch callback will automatically issues
    another GET_SURVEY/GET_SCAN without checking if there was
    already one pending. It does this using the current scan request
    which gets freed by the prior GET_SCAN/GET_SURVEY calls when
    they complete, causing invalid reads when the subsequent calls
    finish.
 - If 6ghz was enabled by the update we actually append another
   trigger command to the list and potentially run it if its the
   current request. This also will end up in the same situation as
   the request is freed by the pending GET_SURVEY/GET_SCAN calls.

For the non-6ghz case there is little to no harm in ignoring the
regdom update because its very unlikely it changed the allowed
frequencies.

For the 6ghz case we could potentially handle the new trigger scan
within get_scan_done, but thats beyond the scope of this change
and is likely quite intrusive.
---
 src/scan.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/src/scan.c b/src/scan.c
index 1cec9785..2ffbef6d 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -2121,6 +2121,22 @@ static void scan_wiphy_watch(struct wiphy *wiphy,
 	if (!sr)
 		return;
 
+	/*
+	 * If the regdom update finished with GET_SCAN/GET_SURVEY in flight
+	 * don't try and get the results again and allow those calls to finish.
+	 * For the non-6ghz case this has no downside as the results should not
+	 * differ.
+	 *
+	 * If 6ghz was enabled by this regdom update there is still not much we
+	 * can do since the scan itself is already completed. Appending to the
+	 * command list won't do anything.
+	 *
+	 * TODO: Handle the 6ghz case by checking for this case in get_scan_done
+	 *       and continuing to iterate the sr->cmds array.
+	 */
+	if (sc->get_scan_cmd_id || sc->get_survey_cmd_id)
+		return;
+
 	/*
 	 * This update did not allow 6GHz, or the original request was
 	 * not expecting 6GHz. The periodic scan should now be ended.
-- 
2.34.1

Re: [PATCH v2 1/2] scan: don't survey on external scans

[Denis Kenzior]

Hi James,

On 9/5/24 10:26 AM, James Prestwood wrote:
> Since surveys end up making driver calls in the kernel its not
> entirely known how they are implemented or how long they will
> take. For this reason the survey will be skipped if getting the
> results from an external scan.
> 
> Doing this also fixes a crash caused by external scans where the
> scan request pointer is not checked and dereferenced:
> 
> 0x00005ffa6a0376de in get_survey_done (user_data=0x5ffa783a3f90) at src/scan.c:2059
> 0x0000749646a29bbd in ?? () from /usr/lib/libell.so.0
> 0x0000749646a243cb in ?? () from /usr/lib/libell.so.0
> 0x0000749646a24655 in l_main_iterate () from /usr/lib/libell.so.0
> 0x0000749646a24ace in l_main_run () from /usr/lib/libell.so.0
> 0x0000749646a263a4 in l_main_run_with_signal () from /usr/lib/libell.so.0
> 0x00005ffa6a00d642 in main (argc=<optimized out>, argv=<optimized out>) at src/main.c:614
> 
> Reported-by: Daniel Bond <danielbondno@gmail.com>
> ---
>   src/scan.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> v2:
>   * Disabled scan surveys rather than allowing a NULL scan request
> 

Both applied, thanks.

Regards,
-Denis