Re: [PATCH 1/2] scan: fix invalid read when canceling an ongoing scan

Re: [PATCH 1/2] scan: fix invalid read when canceling an ongoing scan

[Daniel Bond]

Hi,

There was reported an issue reported in the Arch Linux issue tracker,
which I also experienced on my hardware after upgrading iwd to version
2.20. What happens is that iwd constantly segfaults with messages like
this:

[  279.974994] Code: 00 00 00 00 f3 0f 1e fa 55 48 89 e5 41 57 41 56
41 55 41 54 53 48 83 ec 18 48 89 4d c8 4c 89 45 c0 48 85 ff 0f 84 f3
 00 00 00 <80> 7e 20 00 49 89 f5 0f 85 e6 00 00 00 48 83 fa 01 48 89 fb 49 89
[  280.685109] iwd[1648]: segfault at 32 ip 0000788413f05de6 sp
00007ffeb3d023d0 error 4 in libell.so.0.0.2[1ade6,788413efb000+57000]
lik
ely on CPU 1 (core 1, socket 0)
[  280.685125] Code: 00 00 00 00 f3 0f 1e fa 55 48 89 e5 41 57 41 56
41 55 41 54 53 48 83 ec 18 48 89 4d c8 4c 89 45 c0 48 85 ff 0f 84 f3
 00 00 00 <80> 7e 20 00 49 89 f5 0f 85 e6 00 00 00 48 83 fa 01 48 89 fb 49 89
[  290.573368] iwd[1686]: segfault at 32 ip 00007c0b35d49de6 sp
00007fff86c509d0 error 4 in libell.so.0.0.2[1ade6,7c0b35d3f000+57000]
lik
ely on CPU 2 (core 0, socket 0)

The issue seems to be resolved by checking that results->sr is set:

- if (!results->sr->canceled)
+ if (results->sr && !results->sr->canceled)

More information reported in the arch linux gitlab issuetracker, under
gitlab archlinux org /archlinux/packaging/packages/iwd/-/issues/5 .

In advance, thanks.

Br, Daniel Bond

Re: [PATCH 1/2] scan: fix invalid read when canceling an ongoing scan

[James Prestwood]

Hi Daniel,

On 9/1/24 11:34 PM, Daniel Bond wrote:
> Hi,
>
> There was reported an issue reported in the Arch Linux issue tracker,
> which I also experienced on my hardware after upgrading iwd to version
> 2.20. What happens is that iwd constantly segfaults with messages like
> this:
>
> [  279.974994] Code: 00 00 00 00 f3 0f 1e fa 55 48 89 e5 41 57 41 56
> 41 55 41 54 53 48 83 ec 18 48 89 4d c8 4c 89 45 c0 48 85 ff 0f 84 f3
>   00 00 00 <80> 7e 20 00 49 89 f5 0f 85 e6 00 00 00 48 83 fa 01 48 89 fb 49 89
> [  280.685109] iwd[1648]: segfault at 32 ip 0000788413f05de6 sp
> 00007ffeb3d023d0 error 4 in libell.so.0.0.2[1ade6,788413efb000+57000]
> lik
> ely on CPU 1 (core 1, socket 0)
> [  280.685125] Code: 00 00 00 00 f3 0f 1e fa 55 48 89 e5 41 57 41 56
> 41 55 41 54 53 48 83 ec 18 48 89 4d c8 4c 89 45 c0 48 85 ff 0f 84 f3
>   00 00 00 <80> 7e 20 00 49 89 f5 0f 85 e6 00 00 00 48 83 fa 01 48 89 fb 49 89
> [  290.573368] iwd[1686]: segfault at 32 ip 00007c0b35d49de6 sp
> 00007fff86c509d0 error 4 in libell.so.0.0.2[1ade6,7c0b35d3f000+57000]
> lik
> ely on CPU 2 (core 0, socket 0)
>
> The issue seems to be resolved by checking that results->sr is set:
>
> - if (!results->sr->canceled)
> + if (results->sr && !results->sr->canceled)
The patch on the arch issue report looks good to me. Can we go ahead and 
just send that patch with git send-email?
>
> More information reported in the arch linux gitlab issuetracker, under
> gitlab archlinux org /archlinux/packaging/packages/iwd/-/issues/5 .
>
> In advance, thanks.
>
> Br, Daniel Bond

Re: [PATCH 1/2] scan: fix invalid read when canceling an ongoing scan

[Daniel Bond]

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

Sounds good. I'm not sure where to send it with git send-email, but
I've attached the patch against current HEAD of iwd. If you want me to
do something else just point me in the right direction.

Thanks!

Br, Daniel

On Tue, 3 Sept 2024 at 13:44, James Prestwood <prestwoj@gmail.com> wrote:
>
> Hi Daniel,
>
> On 9/1/24 11:34 PM, Daniel Bond wrote:
> > Hi,
> >
> > There was reported an issue reported in the Arch Linux issue tracker,
> > which I also experienced on my hardware after upgrading iwd to version
> > 2.20. What happens is that iwd constantly segfaults with messages like
> > this:
> >
> > [  279.974994] Code: 00 00 00 00 f3 0f 1e fa 55 48 89 e5 41 57 41 56
> > 41 55 41 54 53 48 83 ec 18 48 89 4d c8 4c 89 45 c0 48 85 ff 0f 84 f3
> >   00 00 00 <80> 7e 20 00 49 89 f5 0f 85 e6 00 00 00 48 83 fa 01 48 89 fb 49 89
> > [  280.685109] iwd[1648]: segfault at 32 ip 0000788413f05de6 sp
> > 00007ffeb3d023d0 error 4 in libell.so.0.0.2[1ade6,788413efb000+57000]
> > lik
> > ely on CPU 1 (core 1, socket 0)
> > [  280.685125] Code: 00 00 00 00 f3 0f 1e fa 55 48 89 e5 41 57 41 56
> > 41 55 41 54 53 48 83 ec 18 48 89 4d c8 4c 89 45 c0 48 85 ff 0f 84 f3
> >   00 00 00 <80> 7e 20 00 49 89 f5 0f 85 e6 00 00 00 48 83 fa 01 48 89 fb 49 89
> > [  290.573368] iwd[1686]: segfault at 32 ip 00007c0b35d49de6 sp
> > 00007fff86c509d0 error 4 in libell.so.0.0.2[1ade6,7c0b35d3f000+57000]
> > lik
> > ely on CPU 2 (core 0, socket 0)
> >
> > The issue seems to be resolved by checking that results->sr is set:
> >
> > - if (!results->sr->canceled)
> > + if (results->sr && !results->sr->canceled)
> The patch on the arch issue report looks good to me. Can we go ahead and
> just send that patch with git send-email?
> >
> > More information reported in the arch linux gitlab issuetracker, under
> > gitlab archlinux org /archlinux/packaging/packages/iwd/-/issues/5 .
> >
> > In advance, thanks.
> >
> > Br, Daniel Bond

[-- Attachment #2: 0001-scan-add-guard-to-check-that-results-sr-is-set-to-av.patch --]
[-- Type: text/x-patch, Size: 880 bytes --]

From acfde32f65c23b00a0893b64b8bc0ab8eeb39143 Mon Sep 17 00:00:00 2001
From: Daniel Bond <danielbondno@gmail.com>
Date: Tue, 3 Sep 2024 18:52:56 +0200
Subject: [PATCH] scan: add guard to check that results->sr is set to avoid
 segfault issue.

Patch created by loqs in Archlinux issue: https://gitlab.archlinux.org/archlinux/packaging/packages/iwd/-/issues/5

As mentioned in list: https://lore.kernel.org/iwd/4083bf62-20d2-46e6-bfae-c926c2acbfe9@gmail.com/
---
 src/scan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/scan.c b/src/scan.c
index debdeb1f..e5360d1b 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -2056,7 +2056,7 @@ static void get_survey_done(void *user_data)
 
 	sc->get_survey_cmd_id = 0;
 
-	if (!results->sr->canceled)
+	if (results->sr && !results->sr->canceled)
 		get_results(results);
 	else
 		get_scan_done(user_data);
-- 
2.46.0

Re: [PATCH 1/2] scan: fix invalid read when canceling an ongoing scan

[James Prestwood]

Hi Daniel,

On 9/3/24 10:03 AM, Daniel Bond wrote:
> Sounds good. I'm not sure where to send it with git send-email, but
> I've attached the patch against current HEAD of iwd. If you want me to
> do something else just point me in the right direction.

You can just send it right to this list, iwd@lists.linux.dev. But Denis 
can probably take it as an attachment too, if not he'll speak up.

Thanks,

James

[PATCH] scan: add guard to check that results->sr is set to avoid segfault issue.

[Daniel Bond]

Patch created by loqs in Archlinux issue: https://gitlab.archlinux.org/archlinux/packaging/packages/iwd/-/issues/5

As mentioned in list: https://lore.kernel.org/iwd/4083bf62-20d2-46e6-bfae-c926c2acbfe9@gmail.com/
---
 src/scan.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/scan.c b/src/scan.c
index debdeb1f..e5360d1b 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -2056,7 +2056,7 @@ static void get_survey_done(void *user_data)
 
 	sc->get_survey_cmd_id = 0;
 
-	if (!results->sr->canceled)
+	if (results->sr && !results->sr->canceled)
 		get_results(results);
 	else
 		get_scan_done(user_data);
-- 
2.46.0

Re: [PATCH 1/2] scan: fix invalid read when canceling an ongoing scan

[Denis Kenzior]

Hi James,

>>
>> The issue seems to be resolved by checking that results->sr is set:
>>
>> - if (!results->sr->canceled)
>> + if (results->sr && !results->sr->canceled)
> The patch on the arch issue report looks good to me. Can we go ahead and just 
> send that patch with git send-email?

Why is results->sr NULL?

Is it caused by the cancellation path introduced by:
64d68b4f080c ("scan: fix invalid read when canceling an ongoing scan")
?

Or is this caused by calling scan_get_results with a NULL sr like in 
scan_notify()?  If sr is NULL, perhaps we shouldn't be invoking the survey path 
in the first place?

Regards,
-Denis

Re: [PATCH 1/2] scan: fix invalid read when canceling an ongoing scan

[James Prestwood]

Hi Denis,

On 9/4/24 8:23 PM, Denis Kenzior wrote:
> Hi James,
>
>>>
>>> The issue seems to be resolved by checking that results->sr is set:
>>>
>>> - if (!results->sr->canceled)
>>> + if (results->sr && !results->sr->canceled)
>> The patch on the arch issue report looks good to me. Can we go ahead 
>> and just send that patch with git send-email?
>
> Why is results->sr NULL?
>
> Is it caused by the cancellation path introduced by:
> 64d68b4f080c ("scan: fix invalid read when canceling an ongoing scan")
> ?
>
> Or is this caused by calling scan_get_results with a NULL sr like in 
> scan_notify()?  If sr is NULL, perhaps we shouldn't be invoking the 
> survey path in the first place?
I'm looking more into this. Its related to an external scan which I can 
now reproduce if I just remove my network profile and let IWD periodic 
scan then issue an external one. Worse though is I see more invalid 
reads related to the survey code path (when periodic scanning) so there 
is more to it than I originally thought.
>
> Regards,
> -Denis

[PATCH 1/2] scan: fix invalid read when canceling an ongoing scan

[James Prestwood]

When the survey code was added it neglected to add the same
cancelation logic that existed for the GET_SCAN call, i.e. if
a scan was canceled and there was a pending GET_SURVEY to the
kernel that needs to be canceled, and the request cleaned up.

Fixes: 35808debae ("scan: use GET_SURVEY for SNR calculation in ranking")
---
 src/scan.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/scan.c b/src/scan.c
index 1982cf74..30b3361e 100644
--- a/src/scan.c
+++ b/src/scan.c
@@ -948,7 +948,7 @@ bool scan_cancel(uint64_t wdev_id, uint32_t id)
 	 * Takes care of the following cases:
 	 * 1. If TRIGGER_SCAN is in flight
 	 * 2. TRIGGER_SCAN sent but bounced with -EBUSY
-	 * 3. Scan request is done but GET_SCAN is still pending
+	 * 3. Scan request is done but GET_SCAN/GET_SURVEY is still pending
 	 *
 	 * For case 3, we can easily cancel the command and proceed with the
 	 * other pending requests.  For case 1 & 2, the subsequent pending
@@ -963,6 +963,9 @@ bool scan_cancel(uint64_t wdev_id, uint32_t id)
 		if (sc->start_cmd_id)
 			l_genl_family_cancel(nl80211, sc->start_cmd_id);
 
+		if (sc->get_survey_cmd_id)
+			l_genl_family_cancel(nl80211, sc->get_survey_cmd_id);
+
 		if (sc->get_scan_cmd_id)
 			l_genl_family_cancel(nl80211, sc->get_scan_cmd_id);
 
@@ -2105,7 +2108,10 @@ static void get_survey_done(void *user_data)
 
 	sc->get_survey_cmd_id = 0;
 
-	get_results(results);
+	if (!results->sr->canceled)
+		get_results(results);
+	else
+		get_scan_done(user_data);
 }
 
 static bool scan_survey(struct scan_results *results)
-- 
2.34.1

Re: [PATCH 1/2] scan: fix invalid read when canceling an ongoing scan

[Denis Kenzior]

Hi James,

On 7/24/24 7:14 AM, James Prestwood wrote:
> When the survey code was added it neglected to add the same
> cancelation logic that existed for the GET_SCAN call, i.e. if
> a scan was canceled and there was a pending GET_SURVEY to the
> kernel that needs to be canceled, and the request cleaned up.
> 
> Fixes: 35808debae ("scan: use GET_SURVEY for SNR calculation in ranking")
> ---
>   src/scan.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 

Both applied, thanks.

Regards,
-Denis