| Message ID | 20230502185955.436081-1-prestwoj@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | station: fix potential NULL access with forced roam | expand |
Hi James, On 5/2/23 13:59, James Prestwood wrote: > If forcing a roam using developer mode IWD could become > unconnected during the scan and result in a segfault when > the network_get_security() is called. > --- > src/station.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > Do you have a backtrace? > diff --git a/src/station.c b/src/station.c > index be571083..9104ebf6 100644 > --- a/src/station.c > +++ b/src/station.c > @@ -2482,7 +2482,7 @@ static bool station_roam_scan_notify(int err, struct l_queue *bss_list, This seems suspicious. Forced roam should be going via station_force_roam_scan_notify(), not via this callback. > uint16_t mdid; > enum security orig_security, security; > > - if (err) { > + if (err || !network) { > station_roam_failed(station); > return false; > } And even then, isn't station->roam_scan_id canceled, so the callback shouldn't even be called in the first place? Regards, -Denis
Hi Denis, On 5/7/23 4:10 PM, Denis Kenzior wrote: > Hi James, > > On 5/2/23 13:59, James Prestwood wrote: >> If forcing a roam using developer mode IWD could become >> unconnected during the scan and result in a segfault when >> the network_get_security() is called. >> --- >> src/station.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> > > Do you have a backtrace? > >> diff --git a/src/station.c b/src/station.c >> index be571083..9104ebf6 100644 >> --- a/src/station.c >> +++ b/src/station.c >> @@ -2482,7 +2482,7 @@ static bool station_roam_scan_notify(int err, >> struct l_queue *bss_list, > > This seems suspicious. Forced roam should be going via > station_force_roam_scan_notify(), not via this callback. Yes true. I did see this crash here but maybe it was related to local changes. I was roaming back and forth but your right, a forced roam shouldn't ever hit this callback. I'll get back to you on this. > >> uint16_t mdid; >> enum security orig_security, security; >> - if (err) { >> + if (err || !network) { >> station_roam_failed(station); >> return false; >> } > > And even then, isn't station->roam_scan_id canceled, so the callback > shouldn't even be called in the first place? > > Regards, > -Denis
diff --git a/src/station.c b/src/station.c index be571083..9104ebf6 100644 --- a/src/station.c +++ b/src/station.c @@ -2482,7 +2482,7 @@ static bool station_roam_scan_notify(int err, struct l_queue *bss_list, uint16_t mdid; enum security orig_security, security; - if (err) { + if (err || !network) { station_roam_failed(station); return false; }