wpa_supplicant: Use nl80211_send_eapol_data for station

Supplicant is using generic L2 send function for EAPOL
messages which doesn't give back status whether frame has been
acked or not. It can lead to wrong wpa states when EAPOL 4/4
is lost i.e. client is in connected state but keys aren't
established on AP side.
Fix that by using nl80211_send_eapol_data as for AP side
and check in conneced state that 4/4 EAPOL has been acked.

Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@neratec.com>
---
 src/drivers/driver.h         | 12 ++++++++++++
 src/drivers/driver_nl80211.c | 11 +++++++++++
 wpa_supplicant/driver_i.h    | 10 ++++++++++
 wpa_supplicant/events.c      | 17 ++++++++++++++++-
 wpa_supplicant/wpas_glue.c   |  6 ++++++
 5 files changed, 55 insertions(+), 1 deletion(-)

On 06/09/2017 02:21 AM, Wojciech Dubowik wrote:
> Supplicant is using generic L2 send function for EAPOL
> messages which doesn't give back status whether frame has been
> acked or not. It can lead to wrong wpa states when EAPOL 4/4
> is lost i.e. client is in connected state but keys aren't
> established on AP side.
> Fix that by using nl80211_send_eapol_data as for AP side
> and check in conneced state that 4/4 EAPOL has been acked.

This seems like it could help my case where keys are set before the 4/4 is
transmitted.  Maybe only set the keys once we get the txstatus back?

Thanks,
Ben

>
> Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@neratec.com>
> ---
>  src/drivers/driver.h         | 12 ++++++++++++
>  src/drivers/driver_nl80211.c | 11 +++++++++++
>  wpa_supplicant/driver_i.h    | 10 ++++++++++
>  wpa_supplicant/events.c      | 17 ++++++++++++++++-
>  wpa_supplicant/wpas_glue.c   |  6 ++++++
>  5 files changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/src/drivers/driver.h b/src/drivers/driver.h
> index 9587d06..b73679d 100644
> --- a/src/drivers/driver.h
> +++ b/src/drivers/driver.h
> @@ -2665,6 +2665,18 @@ struct wpa_driver_ops {
>  			       const u8 *own_addr, u32 flags);
>
>  	/**
> +	 * send_eapol - Send an EAPOL packet (STA only)
> +	 * @priv: private driver interface data
> +	 * @addr: Destination MAC address
> +	 * @data: EAPOL packet starting with IEEE 802.1X header
> +	 * @data_len: Length of the EAPOL packet in octets
> +	 *
> +	 * Returns: 0 on success, -1 on failure
> +	 */
> +	int (*send_eapol)(void *priv, const u8 *addr, const u8 *data,
> +			       size_t data_len);
> +
> +	/**
>  	 * sta_deauth - Deauthenticate a station (AP only)
>  	 * @priv: Private driver interface data
>  	 * @own_addr: Source address and BSSID for the Deauthentication frame
> diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
> index 1b7be39..60185dd 100644
> --- a/src/drivers/driver_nl80211.c
> +++ b/src/drivers/driver_nl80211.c
> @@ -4844,6 +4844,16 @@ static int wpa_driver_nl80211_hapd_send_eapol(
>  	return res;
>  }
>
> +static int wpa_driver_nl80211_send_eapol(
> +	void *priv, const u8 *addr, const u8 *data,
> +	size_t data_len)
> +{
> +	struct i802_bss *bss = priv;
> +
> +	return nl80211_send_eapol_data(bss, addr, data, data_len);
> +}
> +
> +
>
>  static int wpa_driver_nl80211_sta_set_flags(void *priv, const u8 *addr,
>  					    unsigned int total_flags,
> @@ -10186,6 +10196,7 @@ const struct wpa_driver_ops wpa_driver_nl80211_ops = {
>  	.sta_add = wpa_driver_nl80211_sta_add,
>  	.sta_remove = driver_nl80211_sta_remove,
>  	.hapd_send_eapol = wpa_driver_nl80211_hapd_send_eapol,
> +	.send_eapol = wpa_driver_nl80211_send_eapol,
>  	.sta_set_flags = wpa_driver_nl80211_sta_set_flags,
>  	.hapd_init = i802_init,
>  	.hapd_deinit = i802_deinit,
> diff --git a/wpa_supplicant/driver_i.h b/wpa_supplicant/driver_i.h
> index fa2296b..e0a177c 100644
> --- a/wpa_supplicant/driver_i.h
> +++ b/wpa_supplicant/driver_i.h
> @@ -349,6 +349,16 @@ static inline int wpa_drv_hapd_send_eapol(struct wpa_supplicant *wpa_s,
>  	return -1;
>  }
>
> +static inline int wpa_drv_send_eapol(struct wpa_supplicant *wpa_s,
> +					  const u8 *addr, const u8 *data,
> +					  size_t data_len)
> +{
> +	if (wpa_s->driver->hapd_send_eapol)
> +		return wpa_s->driver->send_eapol(wpa_s->drv_priv, addr,
> +						      data, data_len);
> +	return -1;
> +}
> +
>  static inline int wpa_drv_sta_set_flags(struct wpa_supplicant *wpa_s,
>  					const u8 *addr, int total_flags,
>  					int flags_or, int flags_and)
> diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c
> index 3a2ec64..af9bf2a 100644
> --- a/wpa_supplicant/events.c
> +++ b/wpa_supplicant/events.c
> @@ -3966,13 +3966,28 @@ void wpa_supplicant_event(void *ctx, enum wpa_event_type event,
>  		}
>  #endif /* CONFIG_AP */
>  		break;
> -#ifdef CONFIG_AP
>  	case EVENT_EAPOL_TX_STATUS:
> +#ifdef CONFIG_AP
>  		ap_eapol_tx_status(wpa_s, data->eapol_tx_status.dst,
>  				   data->eapol_tx_status.data,
>  				   data->eapol_tx_status.data_len,
>  				   data->eapol_tx_status.ack);
> +#else
> +		wpa_dbg(wpa_s, MSG_DEBUG,
> +				"EAPOL_TX_STATUS: ACK(%d)",
> +				   data->eapol_tx_status.ack);
> +		if (!data->eapol_tx_status.ack &&
> +		   wpa_s->wpa_state == WPA_COMPLETED) {
> +			wpa_dbg(wpa_s, MSG_DEBUG,
> +				"EAPOL 4/4 Not acked, disconnecting");
> +			wpa_s->own_disconnect_req = 1;
> +			wpa_supplicant_deauthenticate(
> +				wpa_s, WLAN_REASON_4WAY_HANDSHAKE_TIMEOUT);
> +
> +		}
> +#endif
>  		break;
> +#ifdef CONFIG_AP
>  	case EVENT_DRIVER_CLIENT_POLL_OK:
>  		ap_client_poll_ok(wpa_s, data->client_poll.addr);
>  		break;
> diff --git a/wpa_supplicant/wpas_glue.c b/wpa_supplicant/wpas_glue.c
> index ae246f9..da81ac0 100644
> --- a/wpa_supplicant/wpas_glue.c
> +++ b/wpa_supplicant/wpas_glue.c
> @@ -97,6 +97,7 @@ static u8 * wpa_alloc_eapol(const struct wpa_supplicant *wpa_s, u8 type,
>  static int wpa_ether_send(struct wpa_supplicant *wpa_s, const u8 *dest,
>  			  u16 proto, const u8 *buf, size_t len)
>  {
> +	int ret;
>  #ifdef CONFIG_TESTING_OPTIONS
>  	if (wpa_s->ext_eapol_frame_io && proto == ETH_P_EAPOL) {
>  		size_t hex_len = 2 * len + 1;
> @@ -111,6 +112,11 @@ static int wpa_ether_send(struct wpa_supplicant *wpa_s, const u8 *dest,
>  		return 0;
>  	}
>  #endif /* CONFIG_TESTING_OPTIONS */
> +	ret = wpa_drv_send_eapol(wpa_s, dest, buf, len);
> +	if (ret < 0)
> +		wpa_printf(MSG_DEBUG, " (%d)", ret);
> +	else
> +		return ret;
>
>  	if (wpa_s->l2) {
>  		return l2_packet_send(wpa_s->l2, dest, proto, buf, len);
>

HI,

Couple of comments below (in addition to changing this to use the nl80211 transport as discussed).

> -----Original Message-----
> From: Hostap [mailto:hostap-bounces@lists.infradead.org] On Behalf Of
> Wojciech Dubowik
> Sent: Friday, June 09, 2017 12:22
> To: hostap@lists.infradead.org
> Cc: Wojciech Dubowik <Wojciech.Dubowik@neratec.com>
> Subject: [PATCH] wpa_supplicant: Use nl80211_send_eapol_data for station
> 
> Supplicant is using generic L2 send function for EAPOL messages which
> doesn't give back status whether frame has been acked or not. It can lead to
> wrong wpa states when EAPOL 4/4 is lost i.e. client is in connected state but
> keys aren't established on AP side.
> Fix that by using nl80211_send_eapol_data as for AP side and check in
> conneced state that 4/4 EAPOL has been acked.
> 
> Signed-off-by: Wojciech Dubowik <Wojciech.Dubowik@neratec.com>
> ---
>  src/drivers/driver.h         | 12 ++++++++++++
>  src/drivers/driver_nl80211.c | 11 +++++++++++
>  wpa_supplicant/driver_i.h    | 10 ++++++++++
>  wpa_supplicant/events.c      | 17 ++++++++++++++++-
>  wpa_supplicant/wpas_glue.c   |  6 ++++++
>  5 files changed, 55 insertions(+), 1 deletion(-)

...

> 
> diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c index
> 3a2ec64..af9bf2a 100644
> --- a/wpa_supplicant/events.c
> +++ b/wpa_supplicant/events.c
> @@ -3966,13 +3966,28 @@ void wpa_supplicant_event(void *ctx, enum
> wpa_event_type event,
>  		}
>  #endif /* CONFIG_AP */
>  		break;
> -#ifdef CONFIG_AP
>  	case EVENT_EAPOL_TX_STATUS:
> +#ifdef CONFIG_AP
>  		ap_eapol_tx_status(wpa_s, data->eapol_tx_status.dst,
>  				   data->eapol_tx_status.data,
>  				   data->eapol_tx_status.data_len,
>  				   data->eapol_tx_status.ack);
> +#else

This would not work in case that CONFIG_AP is set (wpa_supplicant support for AP/P2P GO etc. interfaces). Instead check wpa_s->ap_iface to see if this is an AP interface or not.

> +		wpa_dbg(wpa_s, MSG_DEBUG,
> +				"EAPOL_TX_STATUS: ACK(%d)",
> +				   data->eapol_tx_status.ack);
> +		if (!data->eapol_tx_status.ack &&
> +		   wpa_s->wpa_state == WPA_COMPLETED) {
> +			wpa_dbg(wpa_s, MSG_DEBUG,
> +				"EAPOL 4/4 Not acked, disconnecting");
> +			wpa_s->own_disconnect_req = 1;
> +			wpa_supplicant_deauthenticate(
> +				wpa_s,
> WLAN_REASON_4WAY_HANDSHAKE_TIMEOUT);
> +

It is possible that the Authenticator can recover from loss of message 4 by retrying to send message 3 again (as done in hostap), so it might be better to have the wpa PAE state machine handle such a case more gracefully.

> +		}
> +#endif
>  		break;
> +#ifdef CONFIG_AP
>  	case EVENT_DRIVER_CLIENT_POLL_OK:
>  		ap_client_poll_ok(wpa_s, data->client_poll.addr);
>  		break;
> diff --git a/wpa_supplicant/wpas_glue.c b/wpa_supplicant/wpas_glue.c
> index ae246f9..da81ac0 100644
> --- a/wpa_supplicant/wpas_glue.c
> +++ b/wpa_supplicant/wpas_glue.c
> @@ -97,6 +97,7 @@ static u8 * wpa_alloc_eapol(const struct wpa_supplicant
> *wpa_s, u8 type,  static int wpa_ether_send(struct wpa_supplicant *wpa_s,
> const u8 *dest,
>  			  u16 proto, const u8 *buf, size_t len)  {
> +	int ret;
>  #ifdef CONFIG_TESTING_OPTIONS
>  	if (wpa_s->ext_eapol_frame_io && proto == ETH_P_EAPOL) {
>  		size_t hex_len = 2 * len + 1;
> @@ -111,6 +112,11 @@ static int wpa_ether_send(struct wpa_supplicant
> *wpa_s, const u8 *dest,
>  		return 0;
>  	}
>  #endif /* CONFIG_TESTING_OPTIONS */
> +	ret = wpa_drv_send_eapol(wpa_s, dest, buf, len);

FWIW, I do not think that drv_send_eapol and l2 should be mixed. I think that it would be better to decide which API to use during init.

Regards,

Ilan.