From 8b11755bc778b2190bd456cd006ba05ea32c979e Mon Sep 17 00:00:00 2001 From: Shreyas Sheth Date: Thu, 26 Mar 2026 11:42:05 +0530 Subject: [PATCH] fix(wpa_supplicant): Address comments for concurrency between WiFi and WPA3 task --- .../esp_supplicant/src/esp_hostap.c | 43 ++++++-- .../esp_supplicant/src/esp_wpa3.c | 34 +++++-- components/wpa_supplicant/src/ap/sta_info.c | 18 ++++ components/wpa_supplicant/src/ap/wpa_auth.c | 99 ++++++++++++++++--- 4 files changed, 168 insertions(+), 26 deletions(-) diff --git a/components/wpa_supplicant/esp_supplicant/src/esp_hostap.c b/components/wpa_supplicant/esp_supplicant/src/esp_hostap.c index 3ff1d1d3ea9..15dbf5803e3 100644 --- a/components/wpa_supplicant/esp_supplicant/src/esp_hostap.c +++ b/components/wpa_supplicant/esp_supplicant/src/esp_hostap.c @@ -61,6 +61,9 @@ void *hostap_init(void) wifi_pmf_config_t pmf_cfg = {0}; uint8_t authmode; uint8_t sae_ext = 0; +#ifdef CONFIG_SAE + struct hostapd_sae_commit_queue *q, *tmp; +#endif sae_ext = esp_wifi_ap_get_sae_ext_config_internal(); @@ -223,11 +226,29 @@ void *hostap_init(void) fail: #ifdef CONFIG_SAE if (hapd->sta_list_lock) { - wpa3_hostap_auth_deinit(); - if (g_wpa3_hostap_auth_api_lock && - WPA3_HOSTAP_AUTH_API_LOCK() == pdTRUE) { - WPA3_HOSTAP_AUTH_API_UNLOCK(); + if (wpa3_hostap_auth_deinit()) { + if (g_wpa3_hostap_auth_api_lock) { + /* Block until WPA3 task gives the API lock after SIG_TASK_DEL teardown */ + WPA3_HOSTAP_AUTH_API_LOCK(); + WPA3_HOSTAP_AUTH_API_UNLOCK(); + } + } else { + wpa_printf(MSG_ERROR, + "hostap_init fail: failed to post SIG_TASK_DEL, skipping WPA3 API lock wait"); } + HOSTAPD_STA_LIST_LOCK(hapd); + /* + * hostap_init() failed before global_hapd was assigned, so the WPA3 + * hostap task has no registered hapd to drain this queue on exit; + * free queued commits here before releasing hapd. + */ + dl_list_for_each_safe(q, tmp, &hapd->sae_commit_queue, + struct hostapd_sae_commit_queue, list) { + dl_list_del(&q->list); + os_free(q); + } + HOSTAPD_STA_LIST_UNLOCK(hapd); + os_mutex_delete(hapd->sta_list_lock); hapd->sta_list_lock = NULL; } @@ -302,11 +323,15 @@ bool hostap_deinit(void *data) wifi_ap_wps_disable_internal(); #endif #ifdef CONFIG_SAE - wpa3_hostap_auth_deinit(); - /* Wait till lock is released by wpa3 task */ - if (g_wpa3_hostap_auth_api_lock && - WPA3_HOSTAP_AUTH_API_LOCK() == pdTRUE) { - WPA3_HOSTAP_AUTH_API_UNLOCK(); + if (wpa3_hostap_auth_deinit()) { + /* Block until WPA3 task gives the API lock after SIG_TASK_DEL teardown */ + if (g_wpa3_hostap_auth_api_lock) { + WPA3_HOSTAP_AUTH_API_LOCK(); + WPA3_HOSTAP_AUTH_API_UNLOCK(); + } + } else { + wpa_printf(MSG_ERROR, + "hostap_deinit: failed to post SIG_TASK_DEL, skipping WPA3 API lock wait"); } #endif /* CONFIG_SAE */ diff --git a/components/wpa_supplicant/esp_supplicant/src/esp_wpa3.c b/components/wpa_supplicant/esp_supplicant/src/esp_wpa3.c index db5f8ab5f41..26c4d3f069e 100644 --- a/components/wpa_supplicant/esp_supplicant/src/esp_wpa3.c +++ b/components/wpa_supplicant/esp_supplicant/src/esp_wpa3.c @@ -418,6 +418,7 @@ void esp_wifi_unregister_wpa3_cb(void) static TaskHandle_t g_wpa3_hostap_task_hdl = NULL; static QueueHandle_t g_wpa3_hostap_evt_queue = NULL; +/* Global API lock - created once, never deleted */ SemaphoreHandle_t g_wpa3_hostap_auth_api_lock = NULL; int wpa3_hostap_post_evt(uint32_t evt_id, uint32_t data) @@ -446,7 +447,8 @@ int wpa3_hostap_post_evt(uint32_t evt_id, uint32_t data) return ESP_FAIL; } } else if (evt.id == SIG_TASK_DEL) { - /* wait for portMAX_DELAY as sometimes queue might be full */ + /* Wi-Fi blocks until SIG_TASK_DEL is queued; only Wi-Fi calls wpa3_hostap_post_evt. */ + /* Hence there will be no deadlock for g_wpa3_hostap_auth_api_lock. */ if (os_queue_send_to_front(g_wpa3_hostap_evt_queue, &evt, portMAX_DELAY) != pdPASS) { WPA3_HOSTAP_AUTH_API_UNLOCK(); wpa_printf(MSG_DEBUG, "failed to add msg to queue front"); @@ -657,10 +659,30 @@ static void esp_wpa3_hostap_task(void *pvParameters) os_queue_delete(g_wpa3_hostap_evt_queue); g_wpa3_hostap_evt_queue = NULL; + struct hostapd_data *hapd = hostapd_get_hapd_data(); + if (hapd && hapd->sta_list_lock) { + struct hostapd_sae_commit_queue *q, *tmp; + HOSTAPD_STA_LIST_LOCK(hapd); + dl_list_for_each_safe(q, tmp, &hapd->sae_commit_queue, + struct hostapd_sae_commit_queue, list) { + dl_list_del(&q->list); + os_free(q); + } + HOSTAPD_STA_LIST_UNLOCK(hapd); + /* + * Safe to delete sta_list_lock after unlock: only the WPA3 hostap task and + * the Wi-Fi task take this lock; the Wi-Fi task is blocked while posting + * SIG_TASK_DEL, so it cannot acquire sta_list_lock again until teardown + * sequencing completes. + */ + os_mutex_delete(hapd->sta_list_lock); + hapd->sta_list_lock = NULL; + } + if (g_wpa3_hostap_auth_api_lock) { WPA3_HOSTAP_AUTH_API_UNLOCK(); } - /* At this point, task is deleted*/ + /* At this point, task is deleted */ os_task_delete(NULL); } @@ -678,6 +700,7 @@ int wpa3_hostap_auth_init(void *data) return ESP_FAIL; } + /* g_wpa3_hostap_auth_api_lock is global - created once, never deleted */ if (g_wpa3_hostap_auth_api_lock == NULL) { g_wpa3_hostap_auth_api_lock = os_semphr_create(1, 1); if (!g_wpa3_hostap_auth_api_lock) { @@ -696,6 +719,8 @@ int wpa3_hostap_auth_init(void *data) return ESP_FAIL; } + dl_list_init(&hapd->sae_commit_queue); + if (os_task_create(esp_wpa3_hostap_task, "esp_wpa3_hostap_task", WPA3_HOSTAP_HANDLE_AUTH_TASK_STACK_SIZE, NULL, WPA3_HOSTAP_HANDLE_AUTH_TASK_PRIORITY, @@ -708,8 +733,6 @@ int wpa3_hostap_auth_init(void *data) return ESP_FAIL; } - dl_list_init(&hapd->sae_commit_queue); - return ESP_OK; } @@ -718,9 +741,8 @@ bool wpa3_hostap_auth_deinit(void) if (wpa3_hostap_post_evt(SIG_TASK_DEL, 0) != 0) { wpa_printf(MSG_DEBUG, "failed to send task delete event"); return false; - } else { - return true; } + return true; } static int wpa3_hostap_handle_auth(u8 *buf, size_t len, u32 auth_transaction, u16 status, u8 *bssid) diff --git a/components/wpa_supplicant/src/ap/sta_info.c b/components/wpa_supplicant/src/ap/sta_info.c index 91e52b25f6e..d026bf31c03 100644 --- a/components/wpa_supplicant/src/ap/sta_info.c +++ b/components/wpa_supplicant/src/ap/sta_info.c @@ -52,6 +52,24 @@ int ap_for_each_sta(struct hostapd_data *hapd, } +/** + * ap_get_sta - Look up a station by MAC address + * @hapd: hostapd data structure + * @sta: MAC address to look up + * Returns: Pointer to sta_info structure, or NULL if not found + * + * This function acquires and releases HOSTAPD_STA_LIST_LOCK internally. + * The returned pointer is NOT protected after the lock is released. + * Use-after-free can occur if the station is freed by another task + * (via ap_free_sta) between the unlock here and the caller's use. + * + * For safe access to sta fields, callers should either: + * - Use ap_get_sta_internal() while holding HOSTAPD_STA_LIST_LOCK, OR + * - Take sta->lock immediately after (for SAE stations), OR + * - Copy needed data while lock is still held + * + * Note: This is particularly important for wpa3_task callers. + */ struct sta_info * ap_get_sta(struct hostapd_data *hapd, const u8 *sta) { struct sta_info *s; diff --git a/components/wpa_supplicant/src/ap/wpa_auth.c b/components/wpa_supplicant/src/ap/wpa_auth.c index b2006ed7038..6eccfc961a7 100644 --- a/components/wpa_supplicant/src/ap/wpa_auth.c +++ b/components/wpa_supplicant/src/ap/wpa_auth.c @@ -136,17 +136,26 @@ static inline const u8 * wpa_auth_get_psk(struct wpa_authenticator *wpa_auth, } #ifdef CONFIG_SAE - struct sta_info *sta = ap_get_sta(hapd, addr); + /* wpa_auth_get_psk runs on the Wi-Fi task only, so sae_pmk_copy is not shared with any other task. */ + static u8 sae_pmk_copy[PMK_LEN]; + HOSTAPD_STA_LIST_LOCK(hapd); + struct sta_info *sta = ap_get_sta_internal(hapd, addr); if (sta && sta->auth_alg == WLAN_AUTH_SAE) { - if (!sta->sae || prev_psk) + if (!sta->sae || prev_psk) { + HOSTAPD_STA_LIST_UNLOCK(hapd); return NULL; - return sta->sae->pmk; + } + os_memcpy(sae_pmk_copy, sta->sae->pmk, PMK_LEN); + HOSTAPD_STA_LIST_UNLOCK(hapd); + return sae_pmk_copy; } if (sta && wpa_auth_uses_sae(sta->wpa_sm)) { wpa_printf(MSG_DEBUG, "No PSK for STA trying to use SAE with PMKSA caching"); + HOSTAPD_STA_LIST_UNLOCK(hapd); return NULL; } + HOSTAPD_STA_LIST_UNLOCK(hapd); #endif /*CONFIG_SAE*/ return (u8*)hostapd_get_psk(hapd->conf, addr, prev_psk); @@ -211,10 +220,10 @@ wpa_auth_send_eapol(struct wpa_authenticator *wpa_auth, const u8 *addr, } /* - * CAUTION: cb is invoked while HOSTAPD_STA_LIST_LOCK is held. - * The mutex is non-recursive, so cb must NEVER call any function - * that acquires HOSTAPD_STA_LIST_LOCK (e.g. ap_get_sta, ap_free_sta) - * — doing so will deadlock. + * Modified to handle Wi-Fi vs WPA3 task concurrency only when CONFIG_SAE is enabled. + * Snapshot SM indices under HOSTAPD_STA_LIST_LOCK, then per entry try + * sta->lock (timeout 0) when present; if it is not acquired, skip + * cb for that station; otherwise run cb. */ int wpa_auth_for_each_sta(struct wpa_authenticator *wpa_auth, int (*cb)(struct wpa_state_machine *sm, void *ctx), @@ -222,18 +231,86 @@ int wpa_auth_for_each_sta(struct wpa_authenticator *wpa_auth, { struct hostapd_data *hapd = hostapd_get_hapd_data(); struct sta_info *sta; + u8 idx_snap[WPA_SM_MAX_INDEX]; + unsigned int n = 0; + unsigned int i; +#ifdef CONFIG_SAE + void *sta_lk = NULL; + u8 sta_mac[ETH_ALEN]; +#endif /* CONFIG_SAE */ + int cb_ret; + if (hapd == NULL) return 1; HOSTAPD_STA_LIST_LOCK(hapd); for (sta = hapd->sta_list; sta; sta = sta->next) { - if (sta->wpa_sm && cb(sta->wpa_sm, cb_ctx)) { - HOSTAPD_STA_LIST_UNLOCK(hapd); - return 1; - } + struct wpa_state_machine *sm = sta->wpa_sm; + + if (!sm || n >= WPA_SM_MAX_INDEX) + continue; + if (sm->index >= WPA_SM_MAX_INDEX) + continue; + if (!(BIT(sm->index) & s_sm_valid_bitmap)) + continue; + if (s_sm_table[sm->index] != sm) + continue; + idx_snap[n++] = (u8) sm->index; } HOSTAPD_STA_LIST_UNLOCK(hapd); + + for (i = 0; i < n; i++) { + struct wpa_state_machine *sm; + +#ifdef CONFIG_SAE + sta_lk = NULL; +#endif /* CONFIG_SAE */ + + HOSTAPD_STA_LIST_LOCK(hapd); + sm = wpa_auth_get_sm(idx_snap[i]); + if (!sm) { + HOSTAPD_STA_LIST_UNLOCK(hapd); + continue; + } +#ifdef CONFIG_SAE + sta = ap_get_sta_internal(hapd, sm->addr); + if (sta && sta->wpa_sm == sm && sta->lock) { + /* Take sta->lock with timeout 0. + * Skip cb for this STA if sta->lock is not taken (WPA3 task may be holding the lock). */ + if (!os_semphr_take(sta->lock, 0)) { + HOSTAPD_STA_LIST_UNLOCK(hapd); + continue; + } + sta_lk = sta->lock; + os_memcpy(sta_mac, sta->addr, ETH_ALEN); + } +#endif /* CONFIG_SAE */ + HOSTAPD_STA_LIST_UNLOCK(hapd); + + cb_ret = cb(sm, cb_ctx); + +#ifdef CONFIG_SAE + /* + * Give only when re-lookup finds sta->lock == sta_lk; otherwise skip + * os_semphr_give(sta_lk) (sta_lk is not dereferenced on that path). + * ap_free_sta() give+deletes sta->lock and clears the field before freeing sta. + * ESP softAP: wpa_ap_remove and this walk usually run on the same Wi-Fi task as + * cb(), so STA removal does not run concurrently with cb() in the typical model. + */ + if (sta_lk) { + HOSTAPD_STA_LIST_LOCK(hapd); + sta = ap_get_sta_internal(hapd, sta_mac); + if (sta && sta->lock == sta_lk) + os_semphr_give(sta_lk); + HOSTAPD_STA_LIST_UNLOCK(hapd); + sta_lk = NULL; + } +#endif /* CONFIG_SAE */ + + if (cb_ret) + return 1; + } return 0; }