| Message ID | 20231113175401.343239-3-prestwoj@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [v3,1/4] dpp: remove duplicate connected network check | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| prestwoj/iwd-ci-gitlint | success | GitLint |
| prestwoj/iwd-alpine-ci-incremental_build | fail | Make FAIL (patch 2): src/dpp.c: In function 'dpp_handle_config_response_frame': src/dpp.c:1082:33: error: 'ssid_len' may be used uninitialized in this function [-Werror=maybe-uninitialized] 1082 | params.ssid_len = ssid_len; | ~~~~~~~~~~~~~~~~^~~~~~~~~~ cc1: all warnings being treated as errors make[1]: *** [Makefile:2539: src/dpp.o] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1713: all] Error 2 |
Hi James, On 11/13/23 11:54, James Prestwood wrote: > The post-DPP connection was never done quite right due to station's > state being unknown. The state is now tracked in DPP by a previous > patch but the scan path in DPP is still wrong. > > It relies on station autoconnect logic which has the potential to > connect to a different network than what was configured with DPP. > Its unlikely but still could happen in theory. In addition the scan > was not selectively filtering results by the SSID that DPP > configured. > > This fixes the above problems by first filtering the scan by the > SSID. Then setting the scan results into station without triggering > autoconnect. And finally using network_autoconnect() directly > instead of relying on station to choose the SSID. > --- > src/dpp.c | 44 +++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 41 insertions(+), 3 deletions(-) > All 4 applied, but one comment: > diff --git a/src/dpp.c b/src/dpp.c > index 06ae2929..a95f93e2 100644 > --- a/src/dpp.c > +++ b/src/dpp.c > @@ -853,13 +853,42 @@ static bool dpp_scan_results(int err, struct l_queue *bss_list, > { > struct dpp_sm *dpp = userdata; > struct station *station = station_find(netdev_get_ifindex(dpp->netdev)); > + struct scan_bss *bss; > + char ssid[33]; > + struct network *network; > > if (err < 0) > - return false; > + goto reset; > + > + if (!bss_list || l_queue_length(bss_list) == 0) > + goto reset; > + > + /* > + * The station watch _should_ detect this and reset, which cancels the > + * scan. But just in case... > + */ > + if (L_WARN_ON(station_get_connected_network(station))) > + goto reset; > + > + /* Purely for grabbing the SSID */ > + bss = l_queue_peek_head(bss_list); > > - station_set_scan_results(station, bss_list, freqs, true); > + memcpy(ssid, bss->ssid, bss->ssid_len); > + ssid[bss->ssid_len] = '\0'; Are you sure this SSID is UTF8? station_set_scan_results will filter any SSIDs that are not. > + > + station_set_scan_results(station, bss_list, freqs, false); > + > + network = station_network_find(station, ssid, SECURITY_PSK); And so this can fail. > + > + dpp_reset(dpp); > + > + bss = network_bss_select(network, true); > + network_autoconnect(network, bss); > > return true; > + > +reset: > + return false; > } > > static void dpp_scan_destroy(void *userdata) Regards, -Denis
Hi Denis, On 11/16/23 07:18, Denis Kenzior wrote: > Hi James, > > On 11/13/23 11:54, James Prestwood wrote: >> The post-DPP connection was never done quite right due to station's >> state being unknown. The state is now tracked in DPP by a previous >> patch but the scan path in DPP is still wrong. >> >> It relies on station autoconnect logic which has the potential to >> connect to a different network than what was configured with DPP. >> Its unlikely but still could happen in theory. In addition the scan >> was not selectively filtering results by the SSID that DPP >> configured. >> >> This fixes the above problems by first filtering the scan by the >> SSID. Then setting the scan results into station without triggering >> autoconnect. And finally using network_autoconnect() directly >> instead of relying on station to choose the SSID. >> --- >> src/dpp.c | 44 +++++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 41 insertions(+), 3 deletions(-) >> > > All 4 applied, but one comment: > >> diff --git a/src/dpp.c b/src/dpp.c >> index 06ae2929..a95f93e2 100644 >> --- a/src/dpp.c >> +++ b/src/dpp.c >> @@ -853,13 +853,42 @@ static bool dpp_scan_results(int err, struct >> l_queue *bss_list, >> { >> struct dpp_sm *dpp = userdata; >> struct station *station = >> station_find(netdev_get_ifindex(dpp->netdev)); >> + struct scan_bss *bss; >> + char ssid[33]; >> + struct network *network; >> if (err < 0) >> - return false; >> + goto reset; >> + >> + if (!bss_list || l_queue_length(bss_list) == 0) >> + goto reset; >> + >> + /* >> + * The station watch _should_ detect this and reset, which >> cancels the >> + * scan. But just in case... >> + */ >> + if (L_WARN_ON(station_get_connected_network(station))) >> + goto reset; >> + >> + /* Purely for grabbing the SSID */ >> + bss = l_queue_peek_head(bss_list); >> - station_set_scan_results(station, bss_list, freqs, true); >> + memcpy(ssid, bss->ssid, bss->ssid_len); >> + ssid[bss->ssid_len] = '\0'; > > Are you sure this SSID is UTF8? station_set_scan_results will filter > any SSIDs that are not. Good catch, I hadn't considered that. BUT in theory this shouldn't happen since utf8 is verified when parsing the configuration object. Either way, I still sent a patch because an extra check doesn't hurt, and its results coming from the kernel so I'd rather avoid a crash if something ever changed in the future. Thanks, James
diff --git a/src/dpp.c b/src/dpp.c index 06ae2929..a95f93e2 100644 --- a/src/dpp.c +++ b/src/dpp.c @@ -853,13 +853,42 @@ static bool dpp_scan_results(int err, struct l_queue *bss_list, { struct dpp_sm *dpp = userdata; struct station *station = station_find(netdev_get_ifindex(dpp->netdev)); + struct scan_bss *bss; + char ssid[33]; + struct network *network; if (err < 0) - return false; + goto reset; + + if (!bss_list || l_queue_length(bss_list) == 0) + goto reset; + + /* + * The station watch _should_ detect this and reset, which cancels the + * scan. But just in case... + */ + if (L_WARN_ON(station_get_connected_network(station))) + goto reset; + + /* Purely for grabbing the SSID */ + bss = l_queue_peek_head(bss_list); - station_set_scan_results(station, bss_list, freqs, true); + memcpy(ssid, bss->ssid, bss->ssid_len); + ssid[bss->ssid_len] = '\0'; + + station_set_scan_results(station, bss_list, freqs, false); + + network = station_network_find(station, ssid, SECURITY_PSK); + + dpp_reset(dpp); + + bss = network_bss_select(network, true); + network_autoconnect(network, bss); return true; + +reset: + return false; } static void dpp_scan_destroy(void *userdata) @@ -898,6 +927,7 @@ static void dpp_handle_config_response_frame(const struct mmpdu_header *frame, struct network *network = NULL; struct scan_bss *bss = NULL; char ssid[33]; + size_t ssid_len; if (dpp->state != DPP_STATE_CONFIGURING) return; @@ -1027,6 +1057,7 @@ static void dpp_handle_config_response_frame(const struct mmpdu_header *frame, */ if (station) { memcpy(ssid, config->ssid, config->ssid_len); + ssid_len = config->ssid_len; ssid[config->ssid_len] = '\0'; network = station_network_find(station, ssid, SECURITY_PSK); @@ -1045,7 +1076,14 @@ static void dpp_handle_config_response_frame(const struct mmpdu_header *frame, __station_connect_network(station, network, bss, STATION_STATE_CONNECTING); else if (station) { - dpp->connect_scan_id = scan_active(dpp->wdev_id, NULL, 0, + struct scan_parameters params = {0}; + + params.ssid = (void *) ssid; + params.ssid_len = ssid_len; + + l_debug("Scanning for %s", ssid); + + dpp->connect_scan_id = scan_active_full(dpp->wdev_id, ¶ms, dpp_scan_triggered, dpp_scan_results, dpp, dpp_scan_destroy);