| Message ID | 1456937426-8128-1-git-send-email-eliad@wizery.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Wed, Mar 02, 2016 at 06:50:26PM +0200, Eliad Peller wrote: > There is a race condition in which wpa_supplicant might > receive the EAPOL-Start frame (from the just-associated > station) before the tx completion of the assoc response. What is the point of that wpa_supplicant reference here? Isn't this supposed to apply to AP side functionality? In other words, this applies to both hostapd and wpa_supplicant (if using AP mode). > This in turn will cause the EAPOL-Start frame to get > dropped, and potentially failing the connection. What kind of connection fails due to one EAPOL-Start frame being dropped? > Solve this by saving EAPOLs from authenticated-but-not- > associated stations, and handling them during the assoc > response tx completion processing. I'm probably fine with this change in general, but couple of comments on the implementation: > diff --git a/src/ap/ieee802_11.c b/src/ap/ieee802_11.c > @@ -2782,6 +2782,25 @@ static void handle_assoc_cb(struct hostapd_data *hapd, > + if (sta->pending_eapol_rx) { > + struct os_reltime now, age; > + os_get_reltime(&now); > + os_reltime_sub(&now, &sta->pending_eapol_rx_time, &age); > + if (age.sec == 0 && age.usec < 200000 && > + os_memcmp(sta->pending_eapol_rx_src, > + mgmt->da, ETH_ALEN) == 0) { What is the point of comparing sta->pending_eapol_rx_src to mgmt->da? This is after sta = ap_get_sta(hapd, mgmt->da) and there is not really any possibility for these to be different.. > diff --git a/src/ap/ieee802_1x.c b/src/ap/ieee802_1x.c > @@ -891,6 +891,18 @@ void ieee802_1x_receive(struct hostapd_data *hapd, const u8 *sa, const u8 *buf, > + if (sta && (sta->flags & WLAN_STA_AUTH)) { > + wpa_printf(MSG_DEBUG, "Saving EAPOL for later use"); > + wpabuf_free(sta->pending_eapol_rx); > + sta->pending_eapol_rx = wpabuf_alloc_copy(buf, len); > + if (sta->pending_eapol_rx) { > + os_get_reltime(&sta->pending_eapol_rx_time); > + os_memcpy(sta->pending_eapol_rx_src, sa, > + ETH_ALEN); Similarly here.. Why is sa copied into sta->pending_eapol_rx_src when sta->addr already contains the MAC address of the STA which is sa here after sta = ap_get_sta(hapd, sa). > diff --git a/src/ap/sta_info.h b/src/ap/sta_info.h > @@ -113,6 +113,10 @@ struct sta_info { > + struct wpabuf *pending_eapol_rx; > + struct os_reltime pending_eapol_rx_time; > + u8 pending_eapol_rx_src[ETH_ALEN]; pending_eapol_rx_src should go away. One allocation could be used to store both the pending frame and pending_eapol_rx_time to reduce the size of struct sta_info due to this type of short-lived data.
On Fri, Mar 4, 2016 at 9:32 PM, Jouni Malinen <j@w1.fi> wrote: > On Wed, Mar 02, 2016 at 06:50:26PM +0200, Eliad Peller wrote: >> There is a race condition in which wpa_supplicant might >> receive the EAPOL-Start frame (from the just-associated >> station) before the tx completion of the assoc response. > > What is the point of that wpa_supplicant reference here? Isn't this > supposed to apply to AP side functionality? In other words, this applies > to both hostapd and wpa_supplicant (if using AP mode). > i encountered it on during P2P tests, so i (mistakenly) referenced wpa_supplicant. i'll fix it. >> This in turn will cause the EAPOL-Start frame to get >> dropped, and potentially failing the connection. > > What kind of connection fails due to one EAPOL-Start frame being > dropped? > i guess the problem comes because the EAPOL wasn't simply dropped over the air, but by the remote peer (after it was acked). >> Solve this by saving EAPOLs from authenticated-but-not- >> associated stations, and handling them during the assoc >> response tx completion processing. > > I'm probably fine with this change in general, but couple of comments on > the implementation: > >> diff --git a/src/ap/ieee802_11.c b/src/ap/ieee802_11.c >> @@ -2782,6 +2782,25 @@ static void handle_assoc_cb(struct hostapd_data *hapd, >> + if (sta->pending_eapol_rx) { >> + struct os_reltime now, age; >> + os_get_reltime(&now); >> + os_reltime_sub(&now, &sta->pending_eapol_rx_time, &age); >> + if (age.sec == 0 && age.usec < 200000 && >> + os_memcmp(sta->pending_eapol_rx_src, >> + mgmt->da, ETH_ALEN) == 0) { > > What is the point of comparing sta->pending_eapol_rx_src to mgmt->da? > This is after sta = ap_get_sta(hapd, mgmt->da) and there is not really > any possibility for these to be different.. > you're right. this is a leftover from previous version where only a single eapol was saved (in the hapd struct). >> diff --git a/src/ap/ieee802_1x.c b/src/ap/ieee802_1x.c >> @@ -891,6 +891,18 @@ void ieee802_1x_receive(struct hostapd_data *hapd, const u8 *sa, const u8 *buf, >> + if (sta && (sta->flags & WLAN_STA_AUTH)) { >> + wpa_printf(MSG_DEBUG, "Saving EAPOL for later use"); >> + wpabuf_free(sta->pending_eapol_rx); >> + sta->pending_eapol_rx = wpabuf_alloc_copy(buf, len); >> + if (sta->pending_eapol_rx) { >> + os_get_reltime(&sta->pending_eapol_rx_time); >> + os_memcpy(sta->pending_eapol_rx_src, sa, >> + ETH_ALEN); > > Similarly here.. Why is sa copied into sta->pending_eapol_rx_src when > sta->addr already contains the MAC address of the STA which is sa here > after sta = ap_get_sta(hapd, sa). > ditto. >> diff --git a/src/ap/sta_info.h b/src/ap/sta_info.h >> @@ -113,6 +113,10 @@ struct sta_info { >> + struct wpabuf *pending_eapol_rx; >> + struct os_reltime pending_eapol_rx_time; >> + u8 pending_eapol_rx_src[ETH_ALEN]; > > pending_eapol_rx_src should go away. One allocation could be used to > store both the pending frame and pending_eapol_rx_time to reduce the > size of struct sta_info due to this type of short-lived data. > thanks for the review. i'll update the patch. Eliad.
diff --git a/src/ap/ieee802_11.c b/src/ap/ieee802_11.c index b36e68d..d221825 100644 --- a/src/ap/ieee802_11.c +++ b/src/ap/ieee802_11.c @@ -2782,6 +2782,25 @@ static void handle_assoc_cb(struct hostapd_data *hapd, wpa_auth_sm_event(sta->wpa_sm, WPA_ASSOC); hapd->new_assoc_sta_cb(hapd, sta, !new_assoc); ieee802_1x_notify_port_enabled(sta->eapol_sm, 1); + + if (sta->pending_eapol_rx) { + struct os_reltime now, age; + os_get_reltime(&now); + os_reltime_sub(&now, &sta->pending_eapol_rx_time, &age); + if (age.sec == 0 && age.usec < 200000 && + os_memcmp(sta->pending_eapol_rx_src, + mgmt->da, ETH_ALEN) == 0) { + wpa_printf(MSG_DEBUG, "Process pending EAPOL " + "frame that was received just before " + "association notification"); + ieee802_1x_receive( + hapd, sta->pending_eapol_rx_src, + wpabuf_head(sta->pending_eapol_rx), + wpabuf_len(sta->pending_eapol_rx)); + } + wpabuf_free(sta->pending_eapol_rx); + sta->pending_eapol_rx = NULL; + } } diff --git a/src/ap/ieee802_1x.c b/src/ap/ieee802_1x.c index c774d5c..e34d0d3 100644 --- a/src/ap/ieee802_1x.c +++ b/src/ap/ieee802_1x.c @@ -891,6 +891,18 @@ void ieee802_1x_receive(struct hostapd_data *hapd, const u8 *sa, const u8 *buf, !(hapd->iface->drv_flags & WPA_DRIVER_FLAGS_WIRED))) { wpa_printf(MSG_DEBUG, "IEEE 802.1X data frame from not " "associated/Pre-authenticating STA"); + + if (sta && (sta->flags & WLAN_STA_AUTH)) { + wpa_printf(MSG_DEBUG, "Saving EAPOL for later use"); + wpabuf_free(sta->pending_eapol_rx); + sta->pending_eapol_rx = wpabuf_alloc_copy(buf, len); + if (sta->pending_eapol_rx) { + os_get_reltime(&sta->pending_eapol_rx_time); + os_memcpy(sta->pending_eapol_rx_src, sa, + ETH_ALEN); + } + } + return; } @@ -1183,6 +1195,9 @@ void ieee802_1x_free_station(struct hostapd_data *hapd, struct sta_info *sta) eloop_cancel_timeout(ieee802_1x_wnm_notif_send, hapd, sta); #endif /* CONFIG_HS20 */ + wpabuf_free(sta->pending_eapol_rx); + sta->pending_eapol_rx = NULL; + if (sm == NULL) return; diff --git a/src/ap/sta_info.h b/src/ap/sta_info.h index e223341..ecba920 100644 --- a/src/ap/sta_info.h +++ b/src/ap/sta_info.h @@ -113,6 +113,10 @@ struct sta_info { /* IEEE 802.1X related data */ struct eapol_state_machine *eapol_sm; + struct wpabuf *pending_eapol_rx; + struct os_reltime pending_eapol_rx_time; + u8 pending_eapol_rx_src[ETH_ALEN]; + u64 acct_session_id; struct os_reltime acct_session_start; int acct_session_started;
There is a race condition in which wpa_supplicant might receive the EAPOL-Start frame (from the just-associated station) before the tx completion of the assoc response. This in turn will cause the EAPOL-Start frame to get dropped, and potentially failing the connection. Solve this by saving EAPOLs from authenticated-but-not- associated stations, and handling them during the assoc response tx completion processing. Signed-off-by: Eliad Peller <eliad@wizery.com> --- src/ap/ieee802_11.c | 19 +++++++++++++++++++ src/ap/ieee802_1x.c | 15 +++++++++++++++ src/ap/sta_info.h | 4 ++++ 3 files changed, 38 insertions(+)