On Fri, Mar 4, 2016 at 9:32 PM, Jouni Malinen <j@xxxxx> 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.
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
