| Message ID | 1567931575-27984-5-git-send-email-merez@codeaurora.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 42fe1e519e9f1c6f554c0183f8c9cdd92036cbbf |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wil6210 patches | expand |
Maya Erez <merez@codeaurora.org> wrote: > Fix a race between cfg80211 add_key call and transmitting of 4/4 EAP > packet. In case the transmit is delayed until after the add key takes > place, message 4/4 will be encrypted with the new key, and the > receiver side (AP) will drop it due to MIC error. > > Wil6210 will monitor and look for the transmitted packet 4/4 eap key. > In case add_key takes place before the transmission completed, then > wil6210 will let the FW store the key and wil6210 will notify the FW > to use the PTK key only after 4/4 eap packet transmission was > completed. This is rather ugly but I guess still ok. Or what do people think? But for a proper fix you should look at: [PATCH v2] wpa_supplicant: Send EAPoL-Key frames over NL80211 where available http://lists.infradead.org/pipermail/hostap/2019-September/040516.html
On 9/10/2019 3:23 PM, Kalle Valo wrote: > Maya Erez <merez@codeaurora.org> wrote: > >> Fix a race between cfg80211 add_key call and transmitting of 4/4 EAP >> packet. In case the transmit is delayed until after the add key takes >> place, message 4/4 will be encrypted with the new key, and the >> receiver side (AP) will drop it due to MIC error. >> >> Wil6210 will monitor and look for the transmitted packet 4/4 eap key. >> In case add_key takes place before the transmission completed, then >> wil6210 will let the FW store the key and wil6210 will notify the FW >> to use the PTK key only after 4/4 eap packet transmission was >> completed. > > This is rather ugly but I guess still ok. Or what do people think? The idea is similar to what we have in brcmfmac although it looks like a lot more code. So there seems precedent for the approach. > But for a proper fix you should look at: > > [PATCH v2] wpa_supplicant: Send EAPoL-Key frames over NL80211 where available > > http://lists.infradead.org/pipermail/hostap/2019-September/040516.html However, I agree that Denis did a better job with this and we should aim to use it. It is on my largish TODO list for brcmfmac. Regards, Arend
Am 10.09.19 um 15:23 schrieb Kalle Valo: > Maya Erez <merez@codeaurora.org> wrote: > >> Fix a race between cfg80211 add_key call and transmitting of 4/4 EAP >> packet. In case the transmit is delayed until after the add key takes >> place, message 4/4 will be encrypted with the new key, and the >> receiver side (AP) will drop it due to MIC error. >> >> Wil6210 will monitor and look for the transmitted packet 4/4 eap key. >> In case add_key takes place before the transmission completed, then >> wil6210 will let the FW store the key and wil6210 will notify the FW >> to use the PTK key only after 4/4 eap packet transmission was >> completed. > > This is rather ugly but I guess still ok. Or what do people think? > I don't know anything about the driver here but in mac80211 the idea to avoid the race is to simply flush the queues prior deleting the outgoing key. Now wpa_supplicant is not yet bypassing qdisks, but adding the socket parameter PACKET_QDISC_BYPASS is basically a one-liner in wpa_supplicant and should allow a generic way for drivers to avoid the race with a simple queue flush... Alexander
Hi Alexander, > I don't know anything about the driver here but in mac80211 the idea to > avoid the race is to simply flush the queues prior deleting the outgoing > key. Maybe a silly question, but what does flushing the queue mean in this context? Is it waiting for all the packets to be sent or dropping them on the floor? > > Now wpa_supplicant is not yet bypassing qdisks, but adding the socket > parameter PACKET_QDISC_BYPASS is basically a one-liner in wpa_supplicant > and should allow a generic way for drivers to avoid the race with a > simple queue flush... Can you expand on this actually? What would the sequence of events be? Also, how would this be made to work with CONTROL_PORT over NL80211 ? Regards, -Denis
Hi Denis, >> I don't know anything about the driver here but in mac80211 the idea >> to avoid the race is to simply flush the queues prior deleting the >> outgoing key. > > Maybe a silly question, but what does flushing the queue mean in this > context? Is it waiting for all the packets to be sent or dropping them > on the floor? > It's stopping them to make sure nothing can be added and then sends out all MPDUs in the queues. >> >> Now wpa_supplicant is not yet bypassing qdisks, but adding the socket >> parameter PACKET_QDISC_BYPASS is basically a one-liner in >> wpa_supplicant and should allow a generic way for drivers to avoid the >> race with a simple queue flush... > > Can you expand on this actually? What would the sequence of events be? > 1) wpa_supplicant hands over eapol #4 to the kernel. When bypassing the QDISC the frame is directly added to a driver queue or directly send out. When the send call returns the driver has eapol 4 either in the queuem already send it or the send command has failed. 2) wpa_supplicant deletes the old key (NL80211_CMD_DEL_KEY) 3) The driver stops all hw queues and sends out all MPDUs queued up to that time 4) Driver makes sure no traffic can be send with no/wrong key or PN to STA 5) the driver really removes the key from the HW/installs the new and resumes normal operation I've just posted my hostpad patch to use PACKET_QDISC_BYPASS for eapol frames; It's probably too optimistic and need more code to retry a transmit to compensate for the missing QDISC buffers. > Also, how would this be made to work with CONTROL_PORT over NL80211 ? > Control port is an optional feature drivers can provide. wpa_supplicant should just use it when available or fall back to the "traditional" path when not. Now the driver don't has to flush all queues when using control port, as long as it makes sure the control port frame will be send out prior to deleting the key. But then the driver must know that eapol frames will really be handed over via control port; So I guess flushing all queues is still the simpler solution. So I guess it will change next to nothing... Alexander
On 9/12/2019 11:04 PM, Alexander Wetzel wrote: > Hi Denis, > >>> I don't know anything about the driver here but in mac80211 the idea >>> to avoid the race is to simply flush the queues prior deleting the >>> outgoing key. >> >> Maybe a silly question, but what does flushing the queue mean in this >> context? Is it waiting for all the packets to be sent or dropping >> them on the floor? >> > > It's stopping them to make sure nothing can be added and then sends out > all MPDUs in the queues. > >>> >>> Now wpa_supplicant is not yet bypassing qdisks, but adding the socket >>> parameter PACKET_QDISC_BYPASS is basically a one-liner in >>> wpa_supplicant and should allow a generic way for drivers to avoid >>> the race with a simple queue flush... >> >> Can you expand on this actually? What would the sequence of events be? >> > > 1) wpa_supplicant hands over eapol #4 to the kernel. > When bypassing the QDISC the frame is directly added to a driver > queue or directly send out. When the send call returns the driver > has eapol 4 either in the queuem already send it or the send command > has failed. > > 2) wpa_supplicant deletes the old key (NL80211_CMD_DEL_KEY) > > 3) The driver stops all hw queues and sends out all MPDUs queued up to > that time > > 4) Driver makes sure no traffic can be send with no/wrong key or PN to > STA > > 5) the driver really removes the key from the HW/installs the new and > resumes normal operation > > I've just posted my hostpad patch to use PACKET_QDISC_BYPASS for eapol > frames; It's probably too optimistic and need more code to retry a > transmit to compensate for the missing QDISC buffers. > >> Also, how would this be made to work with CONTROL_PORT over NL80211 ? >> > > Control port is an optional feature drivers can provide. wpa_supplicant > should just use it when available or fall back to the "traditional" path > when not. Now the driver don't has to flush all queues when using > control port, as long as it makes sure the control port frame will be > send out prior to deleting the key. > > But then the driver must know that eapol frames will really be handed > over via control port; So I guess flushing all queues is still the > simpler solution. So I guess it will change next to nothing... Well, in the steps you describe (maybe its just how you describe it) it relies on how the driver is handling it all. I mean step 4) seems more the goal of the whole approach. Basically, we now have two bypass methods dealing with the same/similar issue: 1) bypass the QDISC. 2) bypass network stack entirely with CONTROL_PORT. How does option 1) work for drivers that skip the QDISC for all traffic and rely on mac80211 to schedule the packets? Guess mac80211 can control that, right? Regards, Arend
Hi Arend, Alexander, > Basically, we now have two bypass methods dealing with the same/similar > issue: > > 1) bypass the QDISC. > 2) bypass network stack entirely with CONTROL_PORT. It also raises the question in my mind as to why we have two ways of doing the same thing? From the discussion so far it also sounds like each requires somewhat different / special handling in the driver. Wouldn't it make sense to deprecate one and encourage drivers to implement the other? CONTROL_PORT was added specifically to take care of the re-keying races and can be extended with additional attributes over time, as needed (perhaps for extended key id, etc). Also note that in our testing CONTROL_PORT is _way_ faster than PAE socket... Regards, -Denis
>> Hi Denis, >> >>>> I don't know anything about the driver here but in mac80211 the idea >>>> to avoid the race is to simply flush the queues prior deleting the >>>> outgoing key. >>> >>> Maybe a silly question, but what does flushing the queue mean in this >>> context? Is it waiting for all the packets to be sent or dropping >>> them on the floor? >>> >> >> It's stopping them to make sure nothing can be added and then sends >> out all MPDUs in the queues. >> >>>> >>>> Now wpa_supplicant is not yet bypassing qdisks, but adding the >>>> socket parameter PACKET_QDISC_BYPASS is basically a one-liner in >>>> wpa_supplicant and should allow a generic way for drivers to avoid >>>> the race with a simple queue flush... >>> >>> Can you expand on this actually? What would the sequence of events be? >>> >> >> 1) wpa_supplicant hands over eapol #4 to the kernel. >> When bypassing the QDISC the frame is directly added to a driver >> queue or directly send out. When the send call returns the driver >> has eapol 4 either in the queuem already send it or the send command >> has failed. >> >> 2) wpa_supplicant deletes the old key (NL80211_CMD_DEL_KEY) >> >> 3) The driver stops all hw queues and sends out all MPDUs queued up to >> that time >> >> 4) Driver makes sure no traffic can be send with no/wrong key or PN to >> STA >> >> 5) the driver really removes the key from the HW/installs the new and >> resumes normal operation >> >> I've just posted my hostpad patch to use PACKET_QDISC_BYPASS for eapol >> frames; It's probably too optimistic and need more code to retry a >> transmit to compensate for the missing QDISC buffers. >> >>> Also, how would this be made to work with CONTROL_PORT over NL80211 ? >>> >> >> Control port is an optional feature drivers can provide. >> wpa_supplicant should just use it when available or fall back to the >> "traditional" path when not. Now the driver don't has to flush all >> queues when using control port, as long as it makes sure the control >> port frame will be send out prior to deleting the key. >> >> But then the driver must know that eapol frames will really be handed >> over via control port; So I guess flushing all queues is still the >> simpler solution. So I guess it will change next to nothing... > > Well, in the steps you describe (maybe its just how you describe it) it > relies on how the driver is handling it all. I mean step 4) seems more > the goal of the whole approach. > Well, if you do not take care there are plenty of pitfalls a driver can fall into when trying to rekey, especially when having ongoing traffic. Most drivers will need some code to make sure they can safely delete the old key for a STA and install a new one without a full disassociation. Just what exactly is driver/hw depended. (I've detailed knowledge for iwlfifi and ath9k and good guess how ath10 is handling it. All other cards: No idea...) I've tested around ten different cards (Android, iPhone, notebooks, usb dongles) and found only two handling it correctly. The chances that someone has both an AP and a device handling that correctly is therefore not very good, but then my sample is still too small to be representative. Known broken devices are e.g. Samsung galaxy S5, Nexus 5x, HTC 10, my Samsumg Smart TV, iwlwifi cards (both windows and linux, just different) and for sure any device using ath9k driver with a kernel < 4.19. The only "good" devices I found were an iPhone (forgot the model) and a Microsoft Surface Pro (also forgot the exact model) I was focusing on cards I'm using: iwlwifi, ath9k and ath10k. Of those cards ath10k was ok, iwlwif was working around 50% of the rekeys and ath9k 100% broken (pretty sure it compromised even the security by sending out the some frames two times: With encryption and without. The details of that are best documented here, which fixed it for many - but probably not all - mac80211 cards: https://patchwork.kernel.org/patch/10583675/ The core idea here is, to tell hostpd/wpa_supplicant when a driver believes it can rekey correctly and without that confirmation refuses to reky but disconnect/reconects fast. But that is work in progress, delayed by first implementing the "ideal" rekey solution added in IEEE 802.11 - 2102 "Extended key ID". Problem is of course, that all card/drivers are handling things a bit different and what works for one may well be broken for another. ath10k is a good sample for that: Doing basically everything in HW it worked quite well, bypassing the pitfalls. The generic risks are: - PN out of sync with the key (ath9k's main fault) Especially risky are drivers using HW crypto but generating the PNs in SW. - A-MPDU sessions across rekeys. (Holding back MPDUs till all belonging to the session are received. And then bump the PN for the new key to the value the old key used. And then dropping all MPDUs for the new key as "replay") - Not stopping/blocking Tx depending on the outgoing key - repeating lost frames originally send with the old with the new key > Basically, we now have two bypass methods dealing with the same/similar > issue: > > 1) bypass the QDISC. > 2) bypass network stack entirely with CONTROL_PORT. > > How does option 1) work for drivers that skip the QDISC for all traffic Which drivers skip QDISC, and how? I'm not aware of a way "normal" network traffic can do that. A "normal" linux wlan driver will register as a network card and short of setting PACKET_QDISC_BYPASS on the socket or providing a non-standard API all network traffic will pass trough QDISC. (But that's mostly new area for me, just stitched the path together end2end some days ago.) Now assuming you start a load generator using PACKET_QDISC_BYPASS and try to rekey the connection: the NIC driver still will have the eapol#4 in one of it's queue. So stopping to add new skb's for the queues and send out everything which is in the driver queues would send out tons of other MPDUs, but one of them would be the eapol #4 one. The only "trick" here is, that the the sendto() call from wpa_supplicant up to the *driver* queues is atomic. With PACKET_QDISC_BYPASS set it - at least for my understanding after investigating the issue some hours. Once the sendto call returns and code execution in wpa_supplicant continues - heading for the key deletion - the eapol #4 MPDU is accessible for the driver and can be send out. > and rely on mac80211 to schedule the packets? Guess mac80211 can control > that, right? > Not sure i understand that part.. mac80211 is like the top half of a wlan driver: It handles some parts but the full driver consists of (mac80211 + low level driver). Alexander
Am 13.09.19 um 16:33 schrieb Denis Kenzior: > Hi Arend, Alexander, > >> Basically, we now have two bypass methods dealing with the >> same/similar issue: >> >> 1) bypass the QDISC. >> 2) bypass network stack entirely with CONTROL_PORT. > > It also raises the question in my mind as to why we have two ways of > doing the same thing? From the discussion so far it also sounds like > each requires somewhat different / special handling in the driver. > Wouldn't it make sense to deprecate one and encourage drivers to > implement the other? > My understanding is, that any control port frame must bypass any queues and just send out the frame directly, correct? Any packets send via it is directly jumping to the very front and is immediately send out. And that the intend of it is to replace the "old" path. So the best way forward here would be to 1) implement the patch here to work around the problem without control_port or the theoretical QDISC bypass 2) start implementing control port for the future. correct? > CONTROL_PORT was added specifically to take care of the re-keying races > and can be extended with additional attributes over time, as needed > (perhaps for extended key id, etc). Also note that in our testing > CONTROL_PORT is _way_ faster than PAE socket... > Extended Key ID is pretty robust when rekeying and the driver/card only has to take care to not mix KeyIDs within one A-MPDU. It's no problem encrypting eapol#4 with the new key. You can even encrypt it at the initial connect and it will work. Basically all races the "classical" rekey has to work around go away. For "normal" rekeys it's working pretty well with ath9k and iwlwifi even without control_port and just learned some weeks ago that QDISC could still cause issues... Alexander
Hi Alexander, > And that the intend of it is to replace the "old" path. Correct. > > So the best way forward here would be to > > 1) implement the patch here to work around the problem without > control_port or the theoretical QDISC bypass > 2) start implementing control port for the future. > > correct? > I don't know what the right answer is, but it seems strange to me that we developed a 'better way', upstreamed it several years ago, but are still trying to kludge around adding special flags to what is now considered a legacy approach. Also disconcerting that not a single fullmac driver has added support for this 'better way' yet. > >> CONTROL_PORT was added specifically to take care of the re-keying >> races and can be extended with additional attributes over time, as >> needed (perhaps for extended key id, etc). Also note that in our >> testing CONTROL_PORT is _way_ faster than PAE socket... >> > > Extended Key ID is pretty robust when rekeying and the driver/card only > has to take care to not mix KeyIDs within one A-MPDU. It's no problem > encrypting eapol#4 with the new key. You can even encrypt it at the > initial connect and it will work. Basically all races the "classical" > rekey has to work around go away. > > For "normal" rekeys it's working pretty well with ath9k and iwlwifi even > without control_port and just learned some weeks ago that QDISC could > still cause issues... Okay, if control port doesn't need to handle extended keys then even better. By the way, thanks for your earlier explanation (upthread). Regards, -Denis
diff --git a/drivers/net/wireless/ath/wil6210/cfg80211.c b/drivers/net/wireless/ath/wil6210/cfg80211.c index 1883690..c70854e 100644 --- a/drivers/net/wireless/ath/wil6210/cfg80211.c +++ b/drivers/net/wireless/ath/wil6210/cfg80211.c @@ -331,6 +331,8 @@ static int wil_rf_sector_set_selected(struct wiphy *wiphy, [WMI_KEY_USE_PAIRWISE] = "PTK", [WMI_KEY_USE_RX_GROUP] = "RX_GTK", [WMI_KEY_USE_TX_GROUP] = "TX_GTK", + [WMI_KEY_USE_STORE_PTK] = "STORE_PTK", + [WMI_KEY_USE_APPLY_PTK] = "APPLY_PTK", }; int wil_iftype_nl2wmi(enum nl80211_iftype type) @@ -542,7 +544,7 @@ static int wil_cfg80211_get_station(struct wiphy *wiphy, /* * Find @idx-th active STA for specific MID for station dump. */ -static int wil_find_cid_by_idx(struct wil6210_priv *wil, u8 mid, int idx) +int wil_find_cid_by_idx(struct wil6210_priv *wil, u8 mid, int idx) { int i; @@ -1554,6 +1556,7 @@ void wil_set_crypto_rx(u8 key_index, enum wmi_key_usage key_usage, return; switch (key_usage) { + case WMI_KEY_USE_STORE_PTK: case WMI_KEY_USE_PAIRWISE: for (tid = 0; tid < WIL_STA_TID_NUM; tid++) { cc = &cs->tid_crypto_rx[tid].key_id[key_index]; @@ -1651,6 +1654,16 @@ static int wil_cfg80211_add_key(struct wiphy *wiphy, return -EINVAL; } + spin_lock_bh(&wil->eap_lock); + if (pairwise && wdev->iftype == NL80211_IFTYPE_STATION && + (vif->ptk_rekey_state == WIL_REKEY_M3_RECEIVED || + vif->ptk_rekey_state == WIL_REKEY_WAIT_M4_SENT)) { + key_usage = WMI_KEY_USE_STORE_PTK; + vif->ptk_rekey_state = WIL_REKEY_WAIT_M4_SENT; + wil_dbg_misc(wil, "Store EAPOL key\n"); + } + spin_unlock_bh(&wil->eap_lock); + rc = wmi_add_cipher_key(vif, key_index, mac_addr, params->key_len, params->key, key_usage); if (!rc && !IS_ERR(cs)) { diff --git a/drivers/net/wireless/ath/wil6210/main.c b/drivers/net/wireless/ath/wil6210/main.c index 0f270b8..2c3a192 100644 --- a/drivers/net/wireless/ath/wil6210/main.c +++ b/drivers/net/wireless/ath/wil6210/main.c @@ -373,6 +373,7 @@ static void _wil6210_disconnect_complete(struct wil6210_vif *vif, } clear_bit(wil_vif_fwconnecting, vif->status); clear_bit(wil_vif_ft_roam, vif->status); + vif->ptk_rekey_state = WIL_REKEY_IDLE; break; case NL80211_IFTYPE_AP: @@ -736,6 +737,8 @@ int wil_priv_init(struct wil6210_priv *wil) INIT_LIST_HEAD(&wil->pending_wmi_ev); spin_lock_init(&wil->wmi_ev_lock); spin_lock_init(&wil->net_queue_lock); + spin_lock_init(&wil->eap_lock); + init_waitqueue_head(&wil->wq); init_rwsem(&wil->mem_lock); @@ -1667,6 +1670,7 @@ int wil_reset(struct wil6210_priv *wil, bool load_fw) cancel_work_sync(&vif->disconnect_worker); wil6210_disconnect(vif, NULL, WLAN_REASON_DEAUTH_LEAVING); + vif->ptk_rekey_state = WIL_REKEY_IDLE; } } wil_bcast_fini_all(wil); diff --git a/drivers/net/wireless/ath/wil6210/netdev.c b/drivers/net/wireless/ath/wil6210/netdev.c index 59f041d..a2eca54 100644 --- a/drivers/net/wireless/ath/wil6210/netdev.c +++ b/drivers/net/wireless/ath/wil6210/netdev.c @@ -218,6 +218,7 @@ static void wil_vif_deinit(struct wil6210_vif *vif) cancel_work_sync(&vif->p2p.delayed_listen_work); wil_probe_client_flush(vif); cancel_work_sync(&vif->probe_client_worker); + cancel_work_sync(&vif->enable_tx_key_worker); } void wil_vif_free(struct wil6210_vif *vif) @@ -284,6 +285,7 @@ static void wil_vif_init(struct wil6210_vif *vif) INIT_WORK(&vif->probe_client_worker, wil_probe_client_worker); INIT_WORK(&vif->disconnect_worker, wil_disconnect_worker); INIT_WORK(&vif->p2p.delayed_listen_work, wil_p2p_delayed_listen_work); + INIT_WORK(&vif->enable_tx_key_worker, wil_enable_tx_key_worker); INIT_LIST_HEAD(&vif->probe_client_pending); @@ -540,6 +542,7 @@ void wil_vif_remove(struct wil6210_priv *wil, u8 mid) cancel_work_sync(&vif->disconnect_worker); wil_probe_client_flush(vif); cancel_work_sync(&vif->probe_client_worker); + cancel_work_sync(&vif->enable_tx_key_worker); /* for VIFs, ndev will be freed by destructor after RTNL is unlocked. * the main interface will be freed in wil_if_free, we need to keep it * a bit longer so logging macros will work. diff --git a/drivers/net/wireless/ath/wil6210/txrx.c b/drivers/net/wireless/ath/wil6210/txrx.c index b6253fc..cb13652 100644 --- a/drivers/net/wireless/ath/wil6210/txrx.c +++ b/drivers/net/wireless/ath/wil6210/txrx.c @@ -725,6 +725,182 @@ static void wil_get_netif_rx_params(struct sk_buff *skb, int *cid, } /* + * Check if skb is ptk eapol key message + * + * returns a pointer to the start of the eapol key structure, NULL + * if frame is not PTK eapol key + */ +static struct wil_eapol_key *wil_is_ptk_eapol_key(struct wil6210_priv *wil, + struct sk_buff *skb) +{ + u8 *buf; + const struct wil_1x_hdr *hdr; + struct wil_eapol_key *key; + u16 key_info; + int len = skb->len; + + if (!skb_mac_header_was_set(skb)) { + wil_err(wil, "mac header was not set\n"); + return NULL; + } + + len -= skb_mac_offset(skb); + + if (len < sizeof(struct ethhdr) + sizeof(struct wil_1x_hdr) + + sizeof(struct wil_eapol_key)) + return NULL; + + buf = skb_mac_header(skb) + sizeof(struct ethhdr); + + hdr = (const struct wil_1x_hdr *)buf; + if (hdr->type != WIL_1X_TYPE_EAPOL_KEY) + return NULL; + + key = (struct wil_eapol_key *)(buf + sizeof(struct wil_1x_hdr)); + if (key->type != WIL_EAPOL_KEY_TYPE_WPA && + key->type != WIL_EAPOL_KEY_TYPE_RSN) + return NULL; + + key_info = be16_to_cpu(key->key_info); + if (!(key_info & WIL_KEY_INFO_KEY_TYPE)) /* check if pairwise */ + return NULL; + + return key; +} + +static bool wil_skb_is_eap_3(struct wil6210_priv *wil, struct sk_buff *skb) +{ + struct wil_eapol_key *key; + u16 key_info; + + key = wil_is_ptk_eapol_key(wil, skb); + if (!key) + return false; + + key_info = be16_to_cpu(key->key_info); + if (key_info & (WIL_KEY_INFO_MIC | + WIL_KEY_INFO_ENCR_KEY_DATA)) { + /* 3/4 of 4-Way Handshake */ + wil_dbg_misc(wil, "EAPOL key message 3\n"); + return true; + } + /* 1/4 of 4-Way Handshake */ + wil_dbg_misc(wil, "EAPOL key message 1\n"); + + return false; +} + +static bool wil_skb_is_eap_4(struct wil6210_priv *wil, struct sk_buff *skb) +{ + struct wil_eapol_key *key; + u32 *nonce, i; + + key = wil_is_ptk_eapol_key(wil, skb); + if (!key) + return false; + + nonce = (u32 *)key->key_nonce; + for (i = 0; i < WIL_EAP_NONCE_LEN / sizeof(u32); i++, nonce++) { + if (*nonce != 0) { + /* message 2/4 */ + wil_dbg_misc(wil, "EAPOL key message 2\n"); + return false; + } + } + wil_dbg_misc(wil, "EAPOL key message 4\n"); + + return true; +} + +void wil_enable_tx_key_worker(struct work_struct *work) +{ + struct wil6210_vif *vif = container_of(work, + struct wil6210_vif, enable_tx_key_worker); + struct wil6210_priv *wil = vif_to_wil(vif); + int rc, cid; + + rtnl_lock(); + if (vif->ptk_rekey_state != WIL_REKEY_WAIT_M4_SENT) { + wil_dbg_misc(wil, "Invalid rekey state = %d\n", + vif->ptk_rekey_state); + rtnl_unlock(); + return; + } + + cid = wil_find_cid_by_idx(wil, vif->mid, 0); + if (!wil_cid_valid(wil, cid)) { + wil_err(wil, "Invalid cid = %d\n", cid); + rtnl_unlock(); + return; + } + + wil_dbg_misc(wil, "Apply PTK key after eapol was sent out\n"); + rc = wmi_add_cipher_key(vif, 0, wil->sta[cid].addr, 0, NULL, + WMI_KEY_USE_APPLY_PTK); + + vif->ptk_rekey_state = WIL_REKEY_IDLE; + rtnl_unlock(); + + if (rc) + wil_err(wil, "Apply PTK key failed %d\n", rc); +} + +void wil_tx_complete_handle_eapol(struct wil6210_vif *vif, struct sk_buff *skb) +{ + struct wil6210_priv *wil = vif_to_wil(vif); + struct wireless_dev *wdev = vif_to_wdev(vif); + bool q = false; + + if (wdev->iftype != NL80211_IFTYPE_STATION || + !test_bit(WMI_FW_CAPABILITY_SPLIT_REKEY, wil->fw_capabilities)) + return; + + /* check if skb is an EAP message 4/4 */ + if (!wil_skb_is_eap_4(wil, skb)) + return; + + spin_lock_bh(&wil->eap_lock); + switch (vif->ptk_rekey_state) { + case WIL_REKEY_IDLE: + /* ignore idle state, can happen due to M4 retransmission */ + break; + case WIL_REKEY_M3_RECEIVED: + vif->ptk_rekey_state = WIL_REKEY_IDLE; + break; + case WIL_REKEY_WAIT_M4_SENT: + q = true; + break; + default: + wil_err(wil, "Unknown rekey state = %d", + vif->ptk_rekey_state); + } + spin_unlock_bh(&wil->eap_lock); + + if (q) { + q = queue_work(wil->wmi_wq, &vif->enable_tx_key_worker); + wil_dbg_misc(wil, "queue_work of enable_tx_key_worker -> %d\n", + q); + } +} + +static void wil_rx_handle_eapol(struct wil6210_vif *vif, struct sk_buff *skb) +{ + struct wil6210_priv *wil = vif_to_wil(vif); + struct wireless_dev *wdev = vif_to_wdev(vif); + + if (wdev->iftype != NL80211_IFTYPE_STATION || + !test_bit(WMI_FW_CAPABILITY_SPLIT_REKEY, wil->fw_capabilities)) + return; + + /* check if skb is a EAP message 3/4 */ + if (!wil_skb_is_eap_3(wil, skb)) + return; + + if (vif->ptk_rekey_state == WIL_REKEY_IDLE) + vif->ptk_rekey_state = WIL_REKEY_M3_RECEIVED; +} + +/* * Pass Rx packet to the netif. Update statistics. * Called in softirq context (NAPI poll). */ @@ -796,6 +972,10 @@ void wil_netif_rx(struct sk_buff *skb, struct net_device *ndev, int cid, if (skb) { /* deliver to local stack */ skb->protocol = eth_type_trans(skb, ndev); skb->dev = ndev; + + if (skb->protocol == cpu_to_be16(ETH_P_PAE)) + wil_rx_handle_eapol(vif, skb); + if (gro) rc = napi_gro_receive(&wil->napi_rx, skb); else @@ -2332,6 +2512,10 @@ int wil_tx_complete(struct wil6210_vif *vif, int ringid) if (stats) stats->tx_errors++; } + + if (skb->protocol == cpu_to_be16(ETH_P_PAE)) + wil_tx_complete_handle_eapol(vif, skb); + wil_consume_skb(skb, d->dma.error == 0); } memset(ctx, 0, sizeof(*ctx)); diff --git a/drivers/net/wireless/ath/wil6210/txrx.h b/drivers/net/wireless/ath/wil6210/txrx.h index fceb251..5120475b 100644 --- a/drivers/net/wireless/ath/wil6210/txrx.h +++ b/drivers/net/wireless/ath/wil6210/txrx.h @@ -423,6 +423,46 @@ struct vring_rx_mac { #define RX_DMA_STATUS_PHY_INFO BIT(6) #define RX_DMA_STATUS_FFM BIT(7) /* EtherType Flex Filter Match */ +/* IEEE 802.11, 8.5.2 EAPOL-Key frames */ +#define WIL_KEY_INFO_KEY_TYPE BIT(3) /* val of 1 = Pairwise, 0 = Group key */ + +#define WIL_KEY_INFO_MIC BIT(8) +#define WIL_KEY_INFO_ENCR_KEY_DATA BIT(12) /* for rsn only */ + +#define WIL_EAP_NONCE_LEN 32 +#define WIL_EAP_KEY_RSC_LEN 8 +#define WIL_EAP_REPLAY_COUNTER_LEN 8 +#define WIL_EAP_KEY_IV_LEN 16 +#define WIL_EAP_KEY_ID_LEN 8 + +enum { + WIL_1X_TYPE_EAP_PACKET = 0, + WIL_1X_TYPE_EAPOL_START = 1, + WIL_1X_TYPE_EAPOL_LOGOFF = 2, + WIL_1X_TYPE_EAPOL_KEY = 3, +}; + +#define WIL_EAPOL_KEY_TYPE_RSN 2 +#define WIL_EAPOL_KEY_TYPE_WPA 254 + +struct wil_1x_hdr { + u8 version; + u8 type; + __be16 length; + /* followed by data */ +} __packed; + +struct wil_eapol_key { + u8 type; + __be16 key_info; + __be16 key_length; + u8 replay_counter[WIL_EAP_REPLAY_COUNTER_LEN]; + u8 key_nonce[WIL_EAP_NONCE_LEN]; + u8 key_iv[WIL_EAP_KEY_IV_LEN]; + u8 key_rsc[WIL_EAP_KEY_RSC_LEN]; + u8 key_id[WIL_EAP_KEY_ID_LEN]; +} __packed; + struct vring_rx_dma { u32 d0; struct wil_ring_dma_addr addr; diff --git a/drivers/net/wireless/ath/wil6210/txrx_edma.c b/drivers/net/wireless/ath/wil6210/txrx_edma.c index 29a9a24..f313041 100644 --- a/drivers/net/wireless/ath/wil6210/txrx_edma.c +++ b/drivers/net/wireless/ath/wil6210/txrx_edma.c @@ -1257,6 +1257,10 @@ int wil_tx_sring_handler(struct wil6210_priv *wil, if (stats) stats->tx_errors++; } + + if (skb->protocol == cpu_to_be16(ETH_P_PAE)) + wil_tx_complete_handle_eapol(vif, skb); + wil_consume_skb(skb, msg.status == 0); } memset(ctx, 0, sizeof(*ctx)); diff --git a/drivers/net/wireless/ath/wil6210/wil6210.h b/drivers/net/wireless/ath/wil6210/wil6210.h index 6463310..dd0c87a 100644 --- a/drivers/net/wireless/ath/wil6210/wil6210.h +++ b/drivers/net/wireless/ath/wil6210/wil6210.h @@ -732,6 +732,12 @@ enum wil_sta_status { wil_sta_connected = 2, }; +enum wil_rekey_state { + WIL_REKEY_IDLE = 0, + WIL_REKEY_M3_RECEIVED = 1, + WIL_REKEY_WAIT_M4_SENT = 2, +}; + /** * struct wil_sta_info - data for peer * @@ -880,6 +886,10 @@ struct wil6210_vif { int net_queue_stopped; /* netif_tx_stop_all_queues invoked */ bool fw_stats_ready; /* per-cid statistics are ready inside sta_info */ u64 fw_stats_tsf; /* measurement timestamp */ + + /* PTK rekey race prevention, this is relevant to station mode only */ + enum wil_rekey_state ptk_rekey_state; + struct work_struct enable_tx_key_worker; }; /** @@ -980,6 +990,7 @@ struct wil6210_priv { */ spinlock_t wmi_ev_lock; spinlock_t net_queue_lock; /* guarding stop/wake netif queue */ + spinlock_t eap_lock; /* guarding access to eap rekey fields */ struct napi_struct napi_rx; struct napi_struct napi_tx; struct net_device napi_ndev; /* dummy net_device serving all VIFs */ @@ -1229,6 +1240,7 @@ int wil_ps_update(struct wil6210_priv *wil, void wil_refresh_fw_capabilities(struct wil6210_priv *wil); void wil_mbox_ring_le2cpus(struct wil6210_mbox_ring *r); int wil_find_cid(struct wil6210_priv *wil, u8 mid, const u8 *mac); +int wil_find_cid_by_idx(struct wil6210_priv *wil, u8 mid, int idx); void wil_set_ethtoolops(struct net_device *ndev); struct fw_map *wil_find_fw_mapping(const char *section); @@ -1354,6 +1366,7 @@ void wil6210_disconnect_complete(struct wil6210_vif *vif, const u8 *bssid, void wil_probe_client_flush(struct wil6210_vif *vif); void wil_probe_client_worker(struct work_struct *work); void wil_disconnect_worker(struct work_struct *work); +void wil_enable_tx_key_worker(struct work_struct *work); void wil_init_txrx_ops(struct wil6210_priv *wil); @@ -1373,6 +1386,8 @@ void wil_update_net_queues_bh(struct wil6210_priv *wil, struct wil6210_vif *vif, struct wil_ring *ring, bool check_stop); netdev_tx_t wil_start_xmit(struct sk_buff *skb, struct net_device *ndev); int wil_tx_complete(struct wil6210_vif *vif, int ringid); +void wil_tx_complete_handle_eapol(struct wil6210_vif *vif, + struct sk_buff *skb); void wil6210_unmask_irq_tx(struct wil6210_priv *wil); void wil6210_unmask_irq_tx_edma(struct wil6210_priv *wil); diff --git a/drivers/net/wireless/ath/wil6210/wmi.c b/drivers/net/wireless/ath/wil6210/wmi.c index 5760d14..73fe9bf 100644 --- a/drivers/net/wireless/ath/wil6210/wmi.c +++ b/drivers/net/wireless/ath/wil6210/wmi.c @@ -2438,10 +2438,17 @@ int wmi_add_cipher_key(struct wil6210_vif *vif, u8 key_index, .key_len = key_len, }; - if (!key || (key_len > sizeof(cmd.key))) + if (key_len > sizeof(cmd.key)) return -EINVAL; - memcpy(cmd.key, key, key_len); + /* key len = 0 is allowed only for usage of WMI_KEY_USE_APPLY */ + if ((key_len == 0 || !key) && + key_usage != WMI_KEY_USE_APPLY_PTK) + return -EINVAL; + + if (key) + memcpy(cmd.key, key, key_len); + if (mac_addr) memcpy(cmd.mac, mac_addr, WMI_MAC_LEN); diff --git a/drivers/net/wireless/ath/wil6210/wmi.h b/drivers/net/wireless/ath/wil6210/wmi.h index d75022b..a2f7034 100644 --- a/drivers/net/wireless/ath/wil6210/wmi.h +++ b/drivers/net/wireless/ath/wil6210/wmi.h @@ -109,6 +109,7 @@ enum wmi_fw_capability { WMI_FW_CAPABILITY_CHANNEL_4 = 26, WMI_FW_CAPABILITY_IPA = 27, WMI_FW_CAPABILITY_TEMPERATURE_ALL_RF = 30, + WMI_FW_CAPABILITY_SPLIT_REKEY = 31, WMI_FW_CAPABILITY_MAX, }; @@ -421,6 +422,8 @@ enum wmi_key_usage { WMI_KEY_USE_PAIRWISE = 0x00, WMI_KEY_USE_RX_GROUP = 0x01, WMI_KEY_USE_TX_GROUP = 0x02, + WMI_KEY_USE_STORE_PTK = 0x03, + WMI_KEY_USE_APPLY_PTK = 0x04, }; struct wmi_add_cipher_key_cmd {