From 1060886b115e152124fec0e0106605dd3031e6d5 Mon Sep 17 00:00:00 2001 From: Arkadiusz Bokowy Date: Wed, 29 Mar 2023 21:48:37 +0200 Subject: [PATCH] Check WPA supplicant state before saving config (#22895) * Check WPA supplicant state before saving config * Add save config log message * Fix segfault when WiFi is not enabled but ble-wifi pairing is used * Fix typo in variable name associattion -> association * C++ initialization for GDBusWpaSupplicant struct * Add missing mWpaSupplicantMutex locks --------- Co-authored-by: Andrei Litvin --- .../Linux/ConnectivityManagerImpl.cpp | 48 +++++++++++++------ src/platform/Linux/ConnectivityManagerImpl.h | 26 +++++----- .../webos/ConnectivityManagerImpl.cpp | 30 +++++++----- src/platform/webos/ConnectivityManagerImpl.h | 22 +++++---- 4 files changed, 77 insertions(+), 49 deletions(-) diff --git a/src/platform/Linux/ConnectivityManagerImpl.cpp b/src/platform/Linux/ConnectivityManagerImpl.cpp index c04adfaf105780..57bba172fe3590 100644 --- a/src/platform/Linux/ConnectivityManagerImpl.cpp +++ b/src/platform/Linux/ConnectivityManagerImpl.cpp @@ -147,7 +147,8 @@ void ConnectivityManagerImpl::_OnPlatformEvent(const ChipDeviceEvent * event) } #if CHIP_DEVICE_CONFIG_ENABLE_WPA -bool ConnectivityManagerImpl::mAssociattionStarted = false; + +bool ConnectivityManagerImpl::mAssociationStarted = false; BitFlags::ConnectivityFlags> ConnectivityManagerImpl::mConnectivityFlag; struct GDBusWpaSupplicant ConnectivityManagerImpl::mWpaSupplicant; @@ -157,6 +158,7 @@ ConnectivityManager::WiFiStationMode ConnectivityManagerImpl::_GetWiFiStationMod { if (mWiFiStationMode != kWiFiStationMode_ApplicationControlled) { + std::lock_guard lock(mWpaSupplicantMutex); mWiFiStationMode = (mWpaSupplicant.iface != nullptr) ? kWiFiStationMode_Enabled : kWiFiStationMode_Disabled; } @@ -397,7 +399,7 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte { if (g_strcmp0(value_str, "\'associating\'") == 0) { - mAssociattionStarted = true; + mAssociationStarted = true; } else if (g_strcmp0(value_str, "\'disconnected\'") == 0) { @@ -409,7 +411,7 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte delegate->OnConnectionStatusChanged(static_cast(ConnectionStatusEnum::kConnected)); } - if (mAssociattionStarted) + if (mAssociationStarted) { uint8_t associationFailureCause = static_cast(AssociationFailureCauseEnum::kUnknown); uint16_t status = WLAN_STATUS_UNSPECIFIED_FAILURE; @@ -447,7 +449,7 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte DeviceLayer::SystemLayer().ScheduleLambda([]() { ConnectivityMgrImpl().UpdateNetworkStatus(); }); - mAssociattionStarted = false; + mAssociationStarted = false; } else if (g_strcmp0(value_str, "\'associated\'") == 0) { @@ -460,7 +462,7 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte } else if (g_strcmp0(value_str, "\'completed\'") == 0) { - if (mAssociattionStarted) + if (mAssociationStarted) { DeviceLayer::SystemLayer().ScheduleLambda([]() { if (mpConnectCallback != nullptr) @@ -471,7 +473,7 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte ConnectivityMgrImpl().PostNetworkConnect(); }); } - mAssociattionStarted = false; + mAssociationStarted = false; } } @@ -723,14 +725,10 @@ void ConnectivityManagerImpl::_OnWpaProxyReady(GObject * source_object, GAsyncRe void ConnectivityManagerImpl::StartWiFiManagement() { + std::lock_guard lock(mWpaSupplicantMutex); + mConnectivityFlag.ClearAll(); - mWpaSupplicant.state = GDBusWpaSupplicant::INIT; - mWpaSupplicant.scanState = GDBusWpaSupplicant::WIFI_SCANNING_IDLE; - mWpaSupplicant.proxy = nullptr; - mWpaSupplicant.iface = nullptr; - mWpaSupplicant.bss = nullptr; - mWpaSupplicant.interfacePath = nullptr; - mWpaSupplicant.networkPath = nullptr; + mWpaSupplicant = GDBusWpaSupplicant{}; ChipLogProgress(DeviceLayer, "wpa_supplicant: Start WiFi management"); @@ -752,6 +750,8 @@ void ConnectivityManagerImpl::DriveAPState() CHIP_ERROR err = CHIP_NO_ERROR; WiFiAPState targetState; + std::lock_guard lock(mWpaSupplicantMutex); + // If the AP interface is not under application control... if (mWiFiAPMode != kWiFiAPMode_ApplicationControlled) { @@ -867,6 +867,8 @@ CHIP_ERROR ConnectivityManagerImpl::ConfigureWiFiAP() uint16_t discriminator = 0; char ssid[32]; + std::lock_guard lock(mWpaSupplicantMutex); + channel = ConnectivityUtils::MapChannelToFrequency(kWiFi_BAND_2_4_GHZ, CHIP_DEVICE_CONFIG_WIFI_AP_CHANNEL); if (GetCommissionableDataProvider()->GetSetupDiscriminator(discriminator) != CHIP_NO_ERROR) @@ -961,8 +963,11 @@ ConnectivityManagerImpl::ConnectWiFiNetworkAsync(ByteSpan ssid, ByteSpan credent char ssidStr[kMaxWiFiSSIDLength + 1u] = { 0 }; char keyStr[kMaxWiFiKeyLength + 1u] = { 0 }; + std::lock_guard lock(mWpaSupplicantMutex); + VerifyOrReturnError(ssid.size() <= kMaxWiFiSSIDLength, CHIP_ERROR_INVALID_ARGUMENT); VerifyOrReturnError(credentials.size() <= kMaxWiFiKeyLength, CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(mWpaSupplicant.iface != nullptr, CHIP_ERROR_INCORRECT_STATE); // There is another ongoing connect request, reject the new one. VerifyOrReturnError(mpConnectCallback == nullptr, CHIP_ERROR_INCORRECT_STATE); @@ -1046,6 +1051,9 @@ void ConnectivityManagerImpl::_ConnectWiFiNetworkAsyncCallback(GObject * source_ ConnectivityManagerImpl * this_ = reinterpret_cast(user_data); std::unique_ptr attachRes; std::unique_ptr err; + + std::lock_guard lock(mWpaSupplicantMutex); + { gboolean result = wpa_fi_w1_wpa_supplicant1_interface_call_select_network_finish(mWpaSupplicant.iface, res, &MakeUniquePointerReceiver(err).Get()); @@ -1132,7 +1140,15 @@ CHIP_ERROR ConnectivityManagerImpl::CommitConfig() gboolean result; std::unique_ptr err; - ChipLogProgress(DeviceLayer, "wpa_supplicant: connected to network"); + std::lock_guard lock(mWpaSupplicantMutex); + + if (mWpaSupplicant.state != GDBusWpaSupplicant::WPA_INTERFACE_CONNECTED) + { + ChipLogError(DeviceLayer, "wpa_supplicant: CommitConfig: interface proxy not connected"); + return CHIP_ERROR_INCORRECT_STATE; + } + + ChipLogProgress(DeviceLayer, "wpa_supplicant: save config"); result = wpa_fi_w1_wpa_supplicant1_interface_call_save_config_sync(mWpaSupplicant.iface, nullptr, &MakeUniquePointerReceiver(err).Get()); @@ -1196,7 +1212,7 @@ CHIP_ERROR ConnectivityManagerImpl::GetWiFiSecurityType(SecurityTypeEnum & secur if (mWpaSupplicant.state != GDBusWpaSupplicant::WPA_INTERFACE_CONNECTED) { - ChipLogError(DeviceLayer, "wpa_supplicant: _GetWiFiSecurityType: interface proxy not connected"); + ChipLogError(DeviceLayer, "wpa_supplicant: GetWiFiSecurityType: interface proxy not connected"); return CHIP_ERROR_INCORRECT_STATE; } @@ -1605,6 +1621,8 @@ bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissi void ConnectivityManagerImpl::_OnWpaInterfaceScanDone(GObject * source_object, GAsyncResult * res, gpointer user_data) { + std::lock_guard lock(mWpaSupplicantMutex); + ChipLogProgress(DeviceLayer, "wpa_supplicant: network scan done"); gchar ** bsss = wpa_fi_w1_wpa_supplicant1_interface_dup_bsss(mWpaSupplicant.iface); gchar ** oldBsss = bsss; diff --git a/src/platform/Linux/ConnectivityManagerImpl.h b/src/platform/Linux/ConnectivityManagerImpl.h index e3fac9628a2238..b3a87ea1f6735b 100644 --- a/src/platform/Linux/ConnectivityManagerImpl.h +++ b/src/platform/Linux/ConnectivityManagerImpl.h @@ -65,7 +65,7 @@ namespace DeviceLayer { #if CHIP_DEVICE_CONFIG_ENABLE_WPA struct GDBusWpaSupplicant { - enum + enum WpaState { INIT, WPA_CONNECTING, @@ -74,19 +74,21 @@ struct GDBusWpaSupplicant WPA_NO_INTERFACE_PATH, WPA_GOT_INTERFACE_PATH, WPA_INTERFACE_CONNECTED, - } state; + }; - enum + enum WpaScanning { WIFI_SCANNING_IDLE, WIFI_SCANNING, - } scanState; + }; - WpaFiW1Wpa_supplicant1 * proxy; - WpaFiW1Wpa_supplicant1Interface * iface; - WpaFiW1Wpa_supplicant1BSS * bss; - gchar * interfacePath; - gchar * networkPath; + WpaState state = INIT; + WpaScanning scanState = WIFI_SCANNING_IDLE; + WpaFiW1Wpa_supplicant1 * proxy = nullptr; + WpaFiW1Wpa_supplicant1Interface * iface = nullptr; + WpaFiW1Wpa_supplicant1BSS * bss = nullptr; + gchar * interfacePath = nullptr; + gchar * networkPath = nullptr; }; #endif @@ -203,9 +205,11 @@ class ConnectivityManagerImpl final : public ConnectivityManager, static bool _GetBssInfo(const gchar * bssPath, NetworkCommissioning::WiFiScanResponse & result); - static bool mAssociattionStarted; + static bool mAssociationStarted; static BitFlags mConnectivityFlag; - static struct GDBusWpaSupplicant mWpaSupplicant; + static GDBusWpaSupplicant mWpaSupplicant; + // Access to mWpaSupplicant has to be protected by a mutex because it is accessed from + // the CHIP event loop thread and dedicated D-Bus thread started by platform manager. static std::mutex mWpaSupplicantMutex; NetworkCommissioning::Internal::BaseDriver::NetworkStatusChangeCallback * mpStatusChangeCallback = nullptr; diff --git a/src/platform/webos/ConnectivityManagerImpl.cpp b/src/platform/webos/ConnectivityManagerImpl.cpp index 6cbefb8751eeae..675ac7078d23f0 100644 --- a/src/platform/webos/ConnectivityManagerImpl.cpp +++ b/src/platform/webos/ConnectivityManagerImpl.cpp @@ -143,7 +143,8 @@ void ConnectivityManagerImpl::_OnPlatformEvent(const ChipDeviceEvent * event) } #if CHIP_DEVICE_CONFIG_ENABLE_WPA -bool ConnectivityManagerImpl::mAssociattionStarted = false; + +bool ConnectivityManagerImpl::mAssociationStarted = false; BitFlags::ConnectivityFlags> ConnectivityManagerImpl::mConnectivityFlag; struct GDBusWpaSupplicant ConnectivityManagerImpl::mWpaSupplicant; @@ -393,7 +394,7 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte { if (g_strcmp0(value_str, "\'associating\'") == 0) { - mAssociattionStarted = true; + mAssociationStarted = true; } else if (g_strcmp0(value_str, "\'disconnected\'") == 0) { @@ -405,7 +406,7 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte delegate->OnConnectionStatusChanged(static_cast(ConnectionStatusEnum::kConnected)); } - if (mAssociattionStarted) + if (mAssociationStarted) { uint8_t associationFailureCause = static_cast(AssociationFailureCauseEnum::kUnknown); uint16_t status = WLAN_STATUS_UNSPECIFIED_FAILURE; @@ -435,7 +436,7 @@ void ConnectivityManagerImpl::_OnWpaPropertiesChanged(WpaFiW1Wpa_supplicant1Inte DeviceLayer::SystemLayer().ScheduleLambda([]() { ConnectivityMgrImpl().UpdateNetworkStatus(); }); - mAssociattionStarted = false; + mAssociationStarted = false; } else if (g_strcmp0(value_str, "\'associated\'") == 0) { @@ -697,13 +698,7 @@ void ConnectivityManagerImpl::_OnWpaProxyReady(GObject * source_object, GAsyncRe void ConnectivityManagerImpl::StartWiFiManagement() { mConnectivityFlag.ClearAll(); - mWpaSupplicant.state = GDBusWpaSupplicant::INIT; - mWpaSupplicant.scanState = GDBusWpaSupplicant::WIFI_SCANNING_IDLE; - mWpaSupplicant.proxy = nullptr; - mWpaSupplicant.iface = nullptr; - mWpaSupplicant.bss = nullptr; - mWpaSupplicant.interfacePath = nullptr; - mWpaSupplicant.networkPath = nullptr; + mWpaSupplicant = GDBusWpaSupplicant{}; ChipLogProgress(DeviceLayer, "wpa_supplicant: Start WiFi management"); @@ -936,6 +931,7 @@ ConnectivityManagerImpl::ConnectWiFiNetworkAsync(ByteSpan ssid, ByteSpan credent VerifyOrReturnError(ssid.size() <= kMaxWiFiSSIDLength, CHIP_ERROR_INVALID_ARGUMENT); VerifyOrReturnError(credentials.size() <= kMaxWiFiKeyLength, CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(mWpaSupplicant.iface != nullptr, CHIP_ERROR_INCORRECT_STATE); // There is another ongoing connect request, reject the new one. VerifyOrReturnError(mpConnectCallback == nullptr, CHIP_ERROR_INCORRECT_STATE); @@ -1094,7 +1090,15 @@ CHIP_ERROR ConnectivityManagerImpl::CommitConfig() gboolean result; std::unique_ptr err; - ChipLogProgress(DeviceLayer, "wpa_supplicant: connected to network"); + std::lock_guard lock(mWpaSupplicantMutex); + + if (mWpaSupplicant.state != GDBusWpaSupplicant::WPA_INTERFACE_CONNECTED) + { + ChipLogError(DeviceLayer, "wpa_supplicant: CommitConfig: interface proxy not connected"); + return CHIP_ERROR_INCORRECT_STATE; + } + + ChipLogProgress(DeviceLayer, "wpa_supplicant: save config"); result = wpa_fi_w1_wpa_supplicant1_interface_call_save_config_sync(mWpaSupplicant.iface, nullptr, &MakeUniquePointerReceiver(err).Get()); @@ -1158,7 +1162,7 @@ CHIP_ERROR ConnectivityManagerImpl::GetWiFiSecurityType(SecurityTypeEnum & secur if (mWpaSupplicant.state != GDBusWpaSupplicant::WPA_INTERFACE_CONNECTED) { - ChipLogError(DeviceLayer, "wpa_supplicant: _GetWiFiSecurityType: interface proxy not connected"); + ChipLogError(DeviceLayer, "wpa_supplicant: GetWiFiSecurityType: interface proxy not connected"); return CHIP_ERROR_INCORRECT_STATE; } diff --git a/src/platform/webos/ConnectivityManagerImpl.h b/src/platform/webos/ConnectivityManagerImpl.h index 6d23f83d8683f0..8146377d3efeb6 100644 --- a/src/platform/webos/ConnectivityManagerImpl.h +++ b/src/platform/webos/ConnectivityManagerImpl.h @@ -65,7 +65,7 @@ namespace DeviceLayer { #if CHIP_DEVICE_CONFIG_ENABLE_WPA struct GDBusWpaSupplicant { - enum + enum WpaState { INIT, WPA_CONNECTING, @@ -74,19 +74,21 @@ struct GDBusWpaSupplicant WPA_NO_INTERFACE_PATH, WPA_GOT_INTERFACE_PATH, WPA_INTERFACE_CONNECTED, - } state; + }; - enum + enum WpaScanning { WIFI_SCANNING_IDLE, WIFI_SCANNING, - } scanState; + }; - WpaFiW1Wpa_supplicant1 * proxy; - WpaFiW1Wpa_supplicant1Interface * iface; - WpaFiW1Wpa_supplicant1BSS * bss; - gchar * interfacePath; - gchar * networkPath; + WpaState state = INIT; + WpaScanning scanState = WIFI_SCANNING_IDLE; + WpaFiW1Wpa_supplicant1 * proxy = nullptr; + WpaFiW1Wpa_supplicant1Interface * iface = nullptr; + WpaFiW1Wpa_supplicant1BSS * bss = nullptr; + gchar * interfacePath = nullptr; + gchar * networkPath = nullptr; }; #endif @@ -203,7 +205,7 @@ class ConnectivityManagerImpl final : public ConnectivityManager, static bool _GetBssInfo(const gchar * bssPath, NetworkCommissioning::WiFiScanResponse & result); - static bool mAssociattionStarted; + static bool mAssociationStarted; static BitFlags mConnectivityFlag; static struct GDBusWpaSupplicant mWpaSupplicant; static std::mutex mWpaSupplicantMutex;