Hi all, Any chance I could get a review on this? I'm leaving my current job soon so it would be great to get it merged/rejected before I lose access to the test HW! Cheers, Brendan On 31/05/2019 13:54, Brendan Jackman wrote: > Linux kernel v4.17 added the ability to request sending control port > frames via NL80211 instead of a normal network socket. Doing this provides > the device driver with ordering information between the control port > frames and the installation of keys. This empowers it to avoid race > conditions between, for example, PTK replacement and the sending of frame > 4 of the 4-way rekeying handshake in an RSNA. > > This patch adds a TX_CONTROL_PORT flag to the hostap driver API to report > that it supports, for a given device, a new operation called > tx_control_port. This operation is exactly like an ethernet send except > for the extra ordering information it provides for device drivers. The > nl80211 driver is updated to support this operation when the device > reports the CONTROL_PORT_OVER_NL80211 extended feature. Finally the RSN > supplicant system is updated to use this new operation for sending > EAPoL-Key frames when the driver reports that it is available; otherwise > falling back to a normal ethernet TX. > > There may be other cases than these EAPoL-Key frames that would benefit > from using the new operation but I do not know of them. > > I have tested this on DMG (11ad/ay) devices with an out-of-tree Linux > driver that does not use mac80211. Without this patch I consistently see > PTK rekeying fail if message 4/4 shares a stream with other in-flight > traffic. With this patch, and the driver updated to flush the relevant TX > queue before overwriting a PTK (knowing, now, that if there was a message > 4/4 related to the key installation, it has already entered the driver > queue), rekeying is reliable. > > There is still data loss surrounding key installation - this problem is > alluded to in 802.11-2016 12.6.21, where extended Key ID support is > described as the eventual solution. This patch aims to at least prevent > rekeying from totally breaking the association, in a way that works on > kernels as far back as 4.17 (as per Alexander Wetzel extended Key ID > support should be possible on 5.2). > > See http://lists.infradead.org/pipermail/hostap/2019-May/040089.html for a > little more context. > > Cc: Chaitanya Tata <chaitanya.tata@xxxxxxxxxxxxxxxxx> > Cc: Antony King <antony.king@xxxxxxxxxxxxxxxxx> > Signed-off-by: Brendan Jackman <brendan.jackman@xxxxxxxxxxxxxxxxx> > --- > src/drivers/driver.h | 26 +++++++++++++ > src/drivers/driver_nl80211.c | 39 ++++++++++++++++++++ > src/drivers/driver_nl80211_capa.c | 4 ++ > src/rsn_supp/wpa.c | 2 +- > src/rsn_supp/wpa.h | 2 + > src/rsn_supp/wpa_i.h | 7 ++++ > wpa_supplicant/driver_i.h | 8 ++++ > wpa_supplicant/ibss_rsn.c | 18 +++++++++ > wpa_supplicant/wpas_glue.c | 61 ++++++++++++++++++++++++------- > 9 files changed, 153 insertions(+), 14 deletions(-) > > diff --git a/src/drivers/driver.h b/src/drivers/driver.h > index 496bd522e..7cf1582ce 100644 > --- a/src/drivers/driver.h > +++ b/src/drivers/driver.h > @@ -1625,6 +1625,8 @@ struct wpa_driver_capa { > #define WPA_DRIVER_FLAGS_FTM_RESPONDER 0x0100000000000000ULL > /** Driver support 4-way handshake offload for WPA-Personal */ > #define WPA_DRIVER_FLAGS_4WAY_HANDSHAKE_PSK 0x0200000000000000ULL > +/** Driver has working tx_control_port */ > +#define WPA_DRIVER_FLAGS_TX_CONTROL_PORT 0x0400000000000000ULL > u64 flags; > > #define FULL_AP_CLIENT_STATE_SUPP(drv_flags) \ > @@ -2280,6 +2282,30 @@ struct wpa_driver_ops { > const u8 *seq, size_t seq_len, > const u8 *key, size_t key_len); > > + /** > + * tx_control_port - Send a frame over the 802.1X controlled port > + * @priv: private driver interface data > + * @dest: Destination MAC address > + * @proto: Ethertype in host byte order > + * @buf: Frame payload starting from IEEE 802.1X header > + * @len: Frame payload length > + * > + * Returns 0 on success, else an error > + * > + * This is like a normal ethernet send except that the OS device driver > + * is aware (by other means than the ethertype) that this frame is > + * ~special~, and more importantly it gains an ordering between the > + * transmission of the frame and other driver operations such as key > + * installations. This can be used to work around known limitations in > + * 802.11 protocols such as race conditions between 802.1X rekeying > + * handshake message 4/4 and a PTK being overwritten. > + * > + * This function is only implemented for a given interface if the driver > + * instance reports WPA_DRIVER_FLAGS_TX_CONTROL_PORT. Otherwise API > + * users should fall back to sending the frame via a normal socket. > + */ > + int (*tx_control_port)(void *priv, const u8 *dest, > + u16 proto, const u8 *buf, size_t len); > /** > * init - Initialize driver interface > * @ctx: context to be used when calling wpa_supplicant functions, > diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c > index 3556b6d69..b1d0220e9 100644 > --- a/src/drivers/driver_nl80211.c > +++ b/src/drivers/driver_nl80211.c > @@ -3007,6 +3007,44 @@ static int nl80211_set_pmk(struct wpa_driver_nl80211_data *drv, > } > > > +static int driver_nl80211_tx_control_port(void *priv, const u8 *dest, > + u16 proto, const u8 *buf, size_t len) > +{ > + struct i802_bss *bss = priv; > + struct nl_msg *msg = NULL; > + int ifindex = if_nametoindex(bss->ifname); > + int ret = 0; > + > + wpa_printf(MSG_DEBUG, > + "nl80211: tx_control_port "MACSTR" proto=0x%04x len=%zu", > + MAC2STR(dest), proto, len); > + > + msg = nl80211_ifindex_msg(bss->drv, ifindex, 0, > + NL80211_CMD_CONTROL_PORT_FRAME); > + if (!msg) > + return -ENOBUFS; > + > + if (nla_put_u16(msg, NL80211_ATTR_CONTROL_PORT_ETHERTYPE, proto)) > + goto fail; > + if (nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, dest)) > + goto fail; > + if (nla_put(msg, NL80211_ATTR_FRAME, len, buf)) > + goto fail; > + > + ret = send_and_recv_msgs(bss->drv, msg, NULL, NULL); > + if (ret) > + wpa_printf(MSG_DEBUG, > + "nl80211: tx_control_port failed: ret=%d (%s)", > + ret, strerror(ret)); > + > + return ret; > + > +fail: > + nl80211_nlmsg_clear(msg); > + nlmsg_free(msg); > + return -ENOBUFS; > +} > + > static int wpa_driver_nl80211_set_key(const char *ifname, struct i802_bss *bss, > enum wpa_alg alg, const u8 *addr, > int key_idx, int set_tx, > @@ -10940,6 +10978,7 @@ const struct wpa_driver_ops wpa_driver_nl80211_ops = { > .get_bssid = wpa_driver_nl80211_get_bssid, > .get_ssid = wpa_driver_nl80211_get_ssid, > .set_key = driver_nl80211_set_key, > + .tx_control_port = driver_nl80211_tx_control_port, > .scan2 = driver_nl80211_scan2, > .sched_scan = wpa_driver_nl80211_sched_scan, > .stop_sched_scan = wpa_driver_nl80211_stop_sched_scan, > diff --git a/src/drivers/driver_nl80211_capa.c b/src/drivers/driver_nl80211_capa.c > index a90a55db8..f87621003 100644 > --- a/src/drivers/driver_nl80211_capa.c > +++ b/src/drivers/driver_nl80211_capa.c > @@ -433,6 +433,10 @@ static void wiphy_info_ext_feature_flags(struct wiphy_info_data *info, > if (ext_feature_isset(ext_features, len, > NL80211_EXT_FEATURE_ENABLE_FTM_RESPONDER)) > capa->flags |= WPA_DRIVER_FLAGS_FTM_RESPONDER; > + > + if (ext_feature_isset(ext_features, len, > + NL80211_EXT_FEATURE_CONTROL_PORT_OVER_NL80211)) > + capa->flags |= WPA_DRIVER_FLAGS_TX_CONTROL_PORT; > } > > > diff --git a/src/rsn_supp/wpa.c b/src/rsn_supp/wpa.c > index e0039fac0..aecabac0b 100644 > --- a/src/rsn_supp/wpa.c > +++ b/src/rsn_supp/wpa.c > @@ -158,7 +158,7 @@ int wpa_eapol_key_send(struct wpa_sm *sm, struct wpa_ptk *ptk, > } > > wpa_hexdump(MSG_MSGDUMP, "WPA: TX EAPOL-Key", msg, msg_len); > - ret = wpa_sm_ether_send(sm, dest, proto, msg, msg_len); > + ret = wpa_sm_tx_control_port(sm, dest, proto, msg, msg_len); > eapol_sm_notify_tx_eapol_key(sm->eapol); > out: > os_free(msg); > diff --git a/src/rsn_supp/wpa.h b/src/rsn_supp/wpa.h > index ae9cd6484..145d57988 100644 > --- a/src/rsn_supp/wpa.h > +++ b/src/rsn_supp/wpa.h > @@ -33,6 +33,8 @@ struct wpa_sm_ctx { > const u8 *key, size_t key_len); > void * (*get_network_ctx)(void *ctx); > int (*get_bssid)(void *ctx, u8 *bssid); > + int (*tx_control_port)(void *ctx, const u8 *dest, u16 proto, > + const u8 *buf, size_t len); > int (*ether_send)(void *ctx, const u8 *dest, u16 proto, const u8 *buf, > size_t len); > int (*get_beacon_ie)(void *ctx); > diff --git a/src/rsn_supp/wpa_i.h b/src/rsn_supp/wpa_i.h > index d86734b0d..908af3d25 100644 > --- a/src/rsn_supp/wpa_i.h > +++ b/src/rsn_supp/wpa_i.h > @@ -216,6 +216,13 @@ static inline int wpa_sm_get_bssid(struct wpa_sm *sm, u8 *bssid) > return sm->ctx->get_bssid(sm->ctx->ctx, bssid); > } > > +static inline int wpa_sm_tx_control_port(struct wpa_sm *sm, const u8 *dest, > + u16 proto, const u8 *buf, size_t len) > +{ > + WPA_ASSERT(sm->ctx->tx_control_port); > + return sm->ctx->tx_control_port(sm->ctx->ctx, dest, proto, buf, len); > +} > + > static inline int wpa_sm_ether_send(struct wpa_sm *sm, const u8 *dest, > u16 proto, const u8 *buf, size_t len) > { > diff --git a/wpa_supplicant/driver_i.h b/wpa_supplicant/driver_i.h > index f073b8a6d..2372344fa 100644 > --- a/wpa_supplicant/driver_i.h > +++ b/wpa_supplicant/driver_i.h > @@ -138,6 +138,14 @@ static inline int wpa_drv_get_ssid(struct wpa_supplicant *wpa_s, u8 *ssid) > return -1; > } > > +static inline int wpa_drv_tx_contol_port(struct wpa_supplicant *wpa_s, > + const u8 *dest, > + u16 proto, const u8 *buf, size_t len) > +{ > + return wpa_s->driver->tx_control_port(wpa_s->drv_priv, > + dest, proto, buf, len); > +} > + > static inline int wpa_drv_set_key(struct wpa_supplicant *wpa_s, > enum wpa_alg alg, const u8 *addr, > int key_idx, int set_tx, > diff --git a/wpa_supplicant/ibss_rsn.c b/wpa_supplicant/ibss_rsn.c > index 6934c4725..cdadadee1 100644 > --- a/wpa_supplicant/ibss_rsn.c > +++ b/wpa_supplicant/ibss_rsn.c > @@ -76,6 +76,23 @@ static int supp_ether_send(void *ctx, const u8 *dest, u16 proto, const u8 *buf, > } > > > +static int supp_tx_control_port(void *ctx, const u8 *dest, u16 proto, > + const u8 *buf, size_t len) > +{ > + struct ibss_rsn_peer *peer = ctx; > + struct wpa_supplicant *wpa_s = peer->ibss_rsn->wpa_s; > + > + wpa_printf(MSG_DEBUG, "SUPP: %s(dest=" MACSTR " proto=0x%04x " > + "len=%lu)", > + __func__, MAC2STR(dest), proto, (unsigned long) len); > + > + if (wpa_s->drv_flags & WPA_DRIVER_FLAGS_TX_CONTROL_PORT) > + return wpa_drv_tx_contol_port(wpa_s, dest, proto, buf, len); > + else > + return supp_ether_send(wpa_s, dest, proto, buf, len); > +} > + > + > static u8 * supp_alloc_eapol(void *ctx, u8 type, const void *data, > u16 data_len, size_t *msg_len, void **data_pos) > { > @@ -211,6 +228,7 @@ static int ibss_rsn_supp_init(struct ibss_rsn_peer *peer, const u8 *own_addr, > ctx->set_state = supp_set_state; > ctx->get_state = supp_get_state; > ctx->ether_send = supp_ether_send; > + ctx->tx_control_port = supp_tx_control_port; > ctx->get_beacon_ie = supp_get_beacon_ie; > ctx->alloc_eapol = supp_alloc_eapol; > ctx->set_key = supp_set_key; > diff --git a/wpa_supplicant/wpas_glue.c b/wpa_supplicant/wpas_glue.c > index e98bf1147..6f5c301f8 100644 > --- a/wpa_supplicant/wpas_glue.c > +++ b/wpa_supplicant/wpas_glue.c > @@ -85,17 +85,9 @@ static u8 * wpa_alloc_eapol(const struct wpa_supplicant *wpa_s, u8 type, > } > > > -/** > - * wpa_ether_send - Send Ethernet frame > - * @wpa_s: Pointer to wpa_supplicant data > - * @dest: Destination MAC address > - * @proto: Ethertype in host byte order > - * @buf: Frame payload starting from IEEE 802.1X header > - * @len: Frame payload length > - * Returns: >=0 on success, <0 on failure > - */ > -static int wpa_ether_send(struct wpa_supplicant *wpa_s, const u8 *dest, > - u16 proto, const u8 *buf, size_t len) > +static inline int ext_eapol_frame_io_notify_tx(struct wpa_supplicant *wpa_s, > + const u8 *dest, u16 proto, > + const u8 *buf, size_t len) > { > #ifdef CONFIG_TESTING_OPTIONS > if (wpa_s->ext_eapol_frame_io && proto == ETH_P_EAPOL) { > @@ -108,18 +100,60 @@ static int wpa_ether_send(struct wpa_supplicant *wpa_s, const u8 *dest, > wpa_msg(wpa_s, MSG_INFO, "EAPOL-TX " MACSTR " %s", > MAC2STR(dest), hex); > os_free(hex); > - return 0; > + return -1; > } > #endif /* CONFIG_TESTING_OPTIONS */ > > + return 0; > +} > + > +/** > + * wpa_ether_send - Send Ethernet frame > + * @wpa_s: Pointer to wpa_supplicant data > + * @dest: Destination MAC address > + * @proto: Ethertype in host byte order > + * @buf: Frame payload starting from IEEE 802.1X header > + * @len: Frame payload length > + * Returns: >=0 on success, <0 on failure > + */ > +static int wpa_ether_send(struct wpa_supplicant *wpa_s, const u8 *dest, > + u16 proto, const u8 *buf, size_t len) > +{ > + if (ext_eapol_frame_io_notify_tx(wpa_s, dest, proto, buf, len)) > + return 0; > + > if (wpa_s->l2) { > return l2_packet_send(wpa_s->l2, dest, proto, buf, len); > } > > return -1; > } > -#endif /* IEEE8021X_EAPOL || !CONFIG_NO_WPA */ > > +/** > + * wpa_supplicant_tx_control_port - Send Ethernet frame over 802.1X control port > + * @wpa_s: Pointer to wpa_supplicant data > + * @dest: Destination MAC address > + * @proto: Ethertype in host byte order > + * @buf: Frame payload starting from IEEE 802.1X header > + * @len: Frame payload length > + * Just like wpa_ether_send, but when this function is used the driver may be > + * able to handle control port frames specially. > + * Returns: >=0 on success, <0 on failure > + */ > +static int wpa_supplicant_tx_control_port(void *ctx, const u8 *dest, > + u16 proto, const u8 *buf, size_t len) > +{ > + struct wpa_supplicant *wpa_s = ctx; > + > + if (wpa_s->drv_flags & WPA_DRIVER_FLAGS_TX_CONTROL_PORT) { > + if (ext_eapol_frame_io_notify_tx(wpa_s, dest, proto, buf, len)) > + return 0; > + return wpa_drv_tx_contol_port(wpa_s, dest, proto, buf, len); > + } else { > + return wpa_ether_send(wpa_s, dest, proto, buf, len); > + } > +} > +#endif /* IEEE8021X_EAPOL || !CONFIG_NO_WPA */ > > #ifdef IEEE8021X_EAPOL > > @@ -1214,6 +1248,7 @@ int wpa_supplicant_init_wpa(struct wpa_supplicant *wpa_s) > ctx->get_network_ctx = wpa_supplicant_get_network_ctx; > ctx->get_bssid = wpa_supplicant_get_bssid; > ctx->ether_send = _wpa_ether_send; > + ctx->tx_control_port = wpa_supplicant_tx_control_port; > ctx->get_beacon_ie = wpa_supplicant_get_beacon_ie; > ctx->alloc_eapol = _wpa_alloc_eapol; > ctx->cancel_auth_timeout = _wpa_supplicant_cancel_auth_timeout; _______________________________________________ Hostap mailing list Hostap@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/hostap
