[PATCH] station: Prevent a NULL pointer access

[PATCH] station: Prevent a NULL pointer access

[Torsten Schmitz]

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

There is an unchecked NULL pointer access in network_has_open_pair.
open_info can definitely be NULL, I have the coredumps to prove it.
Let's check owe_info too though.
---
 src/station.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/station.c b/src/station.c
index 19f2aaeb..75ec36df 100644
--- a/src/station.c
+++ b/src/station.c
@@ -693,11 +693,17 @@ static bool network_has_open_pair(struct network *network, struct scan_bss *owe)
 	const struct l_queue_entry *entry;
 	struct ie_owe_transition_info *owe_info = owe->owe_trans;
 
+	if (!owe_info)
+		return false;
+
 	for (entry = network_bss_list_get_entries(network); entry;
 				entry = entry->next) {
 		struct scan_bss *open = entry->data;
 		struct ie_owe_transition_info *open_info = open->owe_trans;
 
+		if (!open_info)
+			continue;
+
 		/*
 		 * Check if this is an Open/Hidden pair:
 		 *
-- 
2.33.1

Re: [PATCH] station: Prevent a NULL pointer access

[noreply.torsten]

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

This bug was found in the wild:

iwd[580]: segfault at 28 ip 000055d0d46059f9 sp 00007ffd21aea400 error 4 in iwd[55d0d45ed000+7f000]

Here is the full backtrace of the coredump, showing open_info was NULL.

(gdb) bt full
#0  0x000055d0d46059f9 in network_has_open_pair (owe=0x55d0d51b5410, network=0x55d0d51a7250) at src/station.c:707
        open = 0x55d0d51acfe0
        open_info = 0x0
        entry = 0x55d0d51b04e0
        owe_info = 0x55d0d51b54f0
        entry = <optimized out>
        owe_info = <optimized out>
        open = <optimized out>
        open_info = <optimized out>
#1  station_owe_transition_results (err=0, bss_list=<optimized out>, freqs=<optimized out>, userdata=<optimized out>) at src/station.c:746
        network = <optimized out>
        station = <optimized out>
        bss = 0x55d0d51b5410
        __func__ = "station_owe_transition_results"
#2  0x000055d0d46147d0 in scan_finished (sc=0x55d0d51a8b70, err=0, bss_list=0x55d0d51b0930, freqs=0x55d0d51b0970, sr=0x55d0d51a89c0) at src/scan.c:1677
        new_owner = false
#3  0x000055d0d46159b2 in get_scan_done (user=0x55d0d51b16a0) at src/scan.c:1707
        results = 0x55d0d51b16a0
        sc = 0x55d0d51a8b70
        __func__ = "get_scan_done"
#4  0x00007f89595efaf3 in ?? () from /usr/lib/libell.so.0
No symbol table info available.
#5  0x00007f89595ec19d in ?? () from /usr/lib/libell.so.0
No symbol table info available.
#6  0x00007f89595eb34d in l_main_iterate () from /usr/lib/libell.so.0
No symbol table info available.
#7  0x00007f89595eb41e in l_main_run () from /usr/lib/libell.so.0
No symbol table info available.
#8  0x00007f89595eb650 in l_main_run_with_signal () from /usr/lib/libell.so.0
No symbol table info available.
#9  0x000055d0d45ee32f in main (argc=<optimized out>, argv=<optimized out>) at src/main.c:532
        exit_status = 1
        dbus = 0x55d0d5195010
        config_dir = <optimized out>
        config_dirs = <optimized out>
        i = <optimized out>
        __func__ = "main"

Re: [PATCH] station: Prevent a NULL pointer access

[Denis Kenzior]

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

Hi Torsten,

On 11/5/21 9:36 PM, Torsten Schmitz wrote:
> There is an unchecked NULL pointer access in network_has_open_pair.
> open_info can definitely be NULL, I have the coredumps to prove it.
> Let's check owe_info too though.

Thanks for the report.  Could you provide a bit more info about the network 
setup you managed to trigger this on?  I think the underlying cause would be 
useful to put into the commit description.

> ---
>   src/station.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/src/station.c b/src/station.c
> index 19f2aaeb..75ec36df 100644
> --- a/src/station.c
> +++ b/src/station.c
> @@ -693,11 +693,17 @@ static bool network_has_open_pair(struct network *network, struct scan_bss *owe)
>   	const struct l_queue_entry *entry;
>   	struct ie_owe_transition_info *owe_info = owe->owe_trans;
>   
> +	if (!owe_info)
> +		return false;
> +

This should not be necessary since owe_info is checked right before invoking 
this function.  See station_owe_transition_results():

	if (!bss->rsne || !bss->owe_trans ||
		...
		goto free;

>   	for (entry = network_bss_list_get_entries(network); entry;
>   				entry = entry->next) {
>   		struct scan_bss *open = entry->data;
>   		struct ie_owe_transition_info *open_info = open->owe_trans;
>   
> +		if (!open_info)
> +			continue;
> +

This would imply that this network has a set of APs that advertise the same 
SSID, but do not advertise an OWE transition element, and some APs that do 
advertise it?  I.e. some legacy APs that do not have a hidden partner?  If so, 
then this detail is worth mentioning.

>   		/*
>   		 * Check if this is an Open/Hidden pair:
>   		 *
> 

Regards,
-Denis

Re: [PATCH] station: Prevent a NULL pointer access

[Torsten Schmitz]

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

> Could you provide a bit more info about the
> network setup you managed to trigger this on? 

We have a mishmash of older and newer APs,
some supporting mesh some don't all advertising the same SSID.
There is an open and closed network.
owe_info->ssid in the crash was the SSID of the open network.
I do not use the open network and connecting to it with iwd
fails.

> This would imply that this network has a set of APs that advertise the
> same SSID, but do not advertise an OWE transition element, 
> and some APs that do advertise it? 

Sounds plausible.

>I.e. some legacy APs that do not have a hidden partner?

iwd shows three hidden APs for that open network. Their addresses 
suggest they belong to the older APs without mesh support.
So I think it is the older APs that do have hidden partners
and advertise OWE transitions.

> > +	if (!owe_info)
> > +		return false;
> > +
> This should not be necessary since owe_info is checked right before
> invoking
> this function. 

Oh, that's right. 
These big if statements seem good at hiding stuff.
Perhaps "O2: Try to avoid complex if body" should apply
to the if conditions as well?