Hi Stefan, On Wed, 14 Aug 2019, Stefan Sperling wrote:
> On Tue, Aug 13, 2019 at 10:47:24AM -0300, Martin Pieuchot wrote: > > if the crash is because of a missing key, put one :) > > Yes, you've convinced me. With this patch we install the CCMP key > to both firmware and net80211. Until firmware confirms that the > key has been installed, we do software encrypt/decrypt. I agree we should handle a missing key but suggest an alternative approach below. I'm unconvinced the root cause is a race on loading the key: I tried to induce an infinite race, by skipping the command to upload the key to hardware, but couldn't reproduce the crash. I suspect the guard for handling hardware decryption in iwm_rx_rx_mpdu() is too weak, and a frame governed by the offloaded key is slipping through. The immediate cause of the crash is that, unless the driver sets IEEE80211_RXI_HWDEC on all frames governed by a key, the stack will attempt software decryption without the necessary context and crash. i.e. software decryption is the default and the stack assumes this is always possible. But it isn't: the stack has delegated decryption to the driver. So, evade this class of errors and check that software decrypt can handle a key, i.e that ieee8021_set_key() has been called, and drop the frame if not. (see below; compiled but untested.) Thoughts? best, Richard. iwm0 at pci1 dev 0 function 0 "Intel Dual Band Wireless AC 7265" rev 0x69, msi iwm0: hw rev 0x210, fw ver 16.242414.0, address xx:xx:xx:xx:xx:xx Index: net80211/ieee80211_crypto.c =================================================================== RCS file: /cvs/src/sys/net80211/ieee80211_crypto.c,v retrieving revision 1.74 diff -u -p -u -p -r1.74 ieee80211_crypto.c --- net80211/ieee80211_crypto.c 24 Sep 2018 20:14:59 -0000 1.74 +++ net80211/ieee80211_crypto.c 15 Aug 2019 01:12:28 -0000 @@ -157,6 +157,10 @@ ieee80211_set_key(struct ieee80211com *i /* should not get there */ error = EINVAL; } + + if (error == 0) + k->k_flags |= IEEE80211_KEY_SWCRYPTO; + return error; } @@ -280,6 +284,11 @@ ieee80211_decrypt(struct ieee80211com *i } k = &ic->ic_nw_keys[kid]; } + + if ((k->k_flags & IEEE80211_KEY_SWCRYPTO) == 0) { + goto err; + } + switch (k->k_cipher) { case IEEE80211_CIPHER_WEP40: case IEEE80211_CIPHER_WEP104: @@ -296,9 +305,15 @@ ieee80211_decrypt(struct ieee80211com *i break; default: /* key not defined */ - m_freem(m0); - m0 = NULL; + goto err; } + + return m0; + +err: + m0 = NULL; + m_freem(m0); + return m0; } Index: net80211/ieee80211_crypto.h =================================================================== RCS file: /cvs/src/sys/net80211/ieee80211_crypto.h,v retrieving revision 1.25 diff -u -p -u -p -r1.25 ieee80211_crypto.h --- net80211/ieee80211_crypto.h 18 Aug 2017 17:30:12 -0000 1.25 +++ net80211/ieee80211_crypto.h 15 Aug 2019 01:12:28 -0000 @@ -78,6 +78,7 @@ struct ieee80211_key { #define IEEE80211_KEY_GROUP 0x00000001 /* group data key */ #define IEEE80211_KEY_TX 0x00000002 /* Tx+Rx */ #define IEEE80211_KEY_IGTK 0x00000004 /* integrity group key */ +#define IEEE80211_KEY_SWCRYPTO 0x00000080 /* loaded for software crypto */ u_int k_len; u_int64_t k_rsc[IEEE80211_NUM_TID]; ----------------------------------------------- illustration of disabling key upload below here ----------------------------------------------- Index: dev/pci/if_iwm.c =================================================================== RCS file: /cvs/src/sys/dev/pci/if_iwm.c,v retrieving revision 1.244 diff -u -p -u -p -r1.244 if_iwm.c --- dev/pci/if_iwm.c 8 Aug 2019 13:56:56 -0000 1.244 +++ dev/pci/if_iwm.c 14 Aug 2019 20:32:47 -0000 @@ -6088,7 +6088,7 @@ int iwm_set_key(struct ieee80211com *ic, struct ieee80211_node *ni, struct ieee80211_key *k) { - struct iwm_softc *sc = ic->ic_softc; + // struct iwm_softc *sc = ic->ic_softc; struct iwm_add_sta_key_cmd cmd = { 0 }; if ((k->k_flags & IEEE80211_KEY_GROUP) || @@ -6108,8 +6108,10 @@ iwm_set_key(struct ieee80211com *ic, str cmd.key_offset = 0; cmd.sta_id = IWM_STATION_ID; - return iwm_send_cmd_pdu(sc, IWM_ADD_STA_KEY, IWM_CMD_ASYNC, - sizeof(cmd), &cmd); + // return iwm_send_cmd_pdu(sc, IWM_ADD_STA_KEY, IWM_CMD_ASYNC, + // sizeof(cmd), &cmd); + + return 0; } void > > Should the firmware fail to install the key for some reason, we will > just keep using our software fallback until the interface is reset. > > This also fixes the race with incoming frames during WPA handshake, > i.e. while (ni->ni_flags & IEEE80211_NODE_RXPROT) == 0. > Such frames can be passed to net80211 and will be dropped there. > > Lightly tested on a 7260 device. > > Ok? > > diff refs/heads/master refs/heads/ccmp-crash2 > blob - 038e6a63dfff113b525bb1e9a30c935996535569 > blob + 8afcce065eb0d85f4469a3f378cacba1513bc215 > --- sys/dev/pci/if_iwm.c > +++ sys/dev/pci/if_iwm.c > @@ -3595,10 +3595,17 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_pac > rxi.rxi_rssi = rssi; > rxi.rxi_tstamp = device_timestamp; > > - /* Handle hardware decryption. */ > + /* > + * Handle hardware decryption. > + * > + * If the WPA handshake has completed but firmware is not yet ready > + * to decrypt frames, fall back to software decryption in net80211. > + * net80211 will drop encrypted frames until the handshake completes. > + */ > if (((wh->i_fc[0] & IEEE80211_FC0_TYPE_MASK) != IEEE80211_FC0_TYPE_CTL) > && (wh->i_fc[1] & IEEE80211_FC1_PROTECTED) && > !IEEE80211_IS_MULTICAST(wh->i_addr1) && > + (sc->sc_flags & IWM_FLAG_CCMP_READY) && > (ni->ni_flags & IEEE80211_NODE_RXPROT) && > ni->ni_pairwise_key.k_cipher == IEEE80211_CIPHER_CCMP) { > if ((rx_pkt_status & IWM_RX_MPDU_RES_STATUS_SEC_ENC_MSK) != > @@ -4402,7 +4409,8 @@ iwm_tx(struct iwm_softc *sc, struct mbuf *m, struct ie > > if (wh->i_fc[1] & IEEE80211_FC1_PROTECTED) { > k = ieee80211_get_txkey(ic, wh, ni); > - if (k->k_cipher != IEEE80211_CIPHER_CCMP) { > + if (k->k_cipher != IEEE80211_CIPHER_CCMP || > + (sc->sc_flags & IWM_FLAG_CCMP_READY) == 0) { > if ((m = ieee80211_encrypt(ic, m, k)) == NULL) > return ENOBUFS; > > @@ -4418,7 +4426,8 @@ iwm_tx(struct iwm_softc *sc, struct mbuf *m, struct ie > /* Copy 802.11 header in TX command. */ > memcpy(((uint8_t *)tx) + sizeof(*tx), wh, hdrlen); > > - if (k != NULL && k->k_cipher == IEEE80211_CIPHER_CCMP) { > + if (k != NULL && k->k_cipher == IEEE80211_CIPHER_CCMP && > + (sc->sc_flags & IWM_FLAG_CCMP_READY)) { > /* Trim 802.11 header and prepend CCMP IV. */ > m_adj(m, hdrlen - IEEE80211_CCMP_HDRLEN); > ivp = mtod(m, u_int8_t *); > @@ -6090,6 +6099,7 @@ iwm_set_key(struct ieee80211com *ic, struct ieee80211_ > { > struct iwm_softc *sc = ic->ic_softc; > struct iwm_add_sta_key_cmd cmd = { 0 }; > + int err; > > if ((k->k_flags & IEEE80211_KEY_GROUP) || > k->k_cipher != IEEE80211_CIPHER_CCMP) { > @@ -6097,6 +6107,8 @@ iwm_set_key(struct ieee80211com *ic, struct ieee80211_ > return (ieee80211_set_key(ic, ni, k)); > } > > + sc->sc_flags &= ~(IWM_FLAG_CCMP_READY | IWM_FLAG_CCMP_KEY_SENT); > + > cmd.key_flags = htole16(IWM_STA_KEY_FLG_CCM | > IWM_STA_KEY_FLG_WEP_KEY_MAP | > ((k->k_id << IWM_STA_KEY_FLG_KEYID_POS) & > @@ -6108,8 +6120,15 @@ iwm_set_key(struct ieee80211com *ic, struct ieee80211_ > cmd.key_offset = 0; > cmd.sta_id = IWM_STATION_ID; > > - return iwm_send_cmd_pdu(sc, IWM_ADD_STA_KEY, IWM_CMD_ASYNC, > - sizeof(cmd), &cmd); > + /* Allow for software CCMP encryption/decryption fallback. */ > + err = ieee80211_set_key(ic, ni, k); > + if (!err) { > + err = iwm_send_cmd_pdu(sc, IWM_ADD_STA_KEY, IWM_CMD_ASYNC, > + sizeof(cmd), &cmd); > + if (!err) > + sc->sc_flags |= IWM_FLAG_CCMP_KEY_SENT; > + } > + return err; > } > > void > @@ -6135,6 +6154,10 @@ iwm_delete_key(struct ieee80211com *ic, struct ieee802 > cmd.sta_id = IWM_STATION_ID; > > iwm_send_cmd_pdu(sc, IWM_ADD_STA_KEY, IWM_CMD_ASYNC, sizeof(cmd), &cmd); > + sc->sc_flags &= ~(IWM_FLAG_CCMP_KEY_SENT | IWM_FLAG_CCMP_READY); > + > + /* Delete software fallback CCMP key. */ > + ieee80211_delete_key(ic, ni, k); > } > > void > @@ -6846,6 +6869,7 @@ iwm_stop(struct ifnet *ifp) > sc->sc_flags &= ~IWM_FLAG_TE_ACTIVE; > sc->sc_flags &= ~IWM_FLAG_HW_ERR; > sc->sc_flags &= ~IWM_FLAG_SHUTDOWN; > + sc->sc_flags &= ~(IWM_FLAG_CCMP_KEY_SENT | IWM_FLAG_CCMP_READY); > > sc->sc_newstate(ic, IEEE80211_S_INIT, -1); > > @@ -7331,6 +7355,12 @@ iwm_notif_intr(struct iwm_softc *sc) > break; > > case IWM_ADD_STA_KEY: > + if (sc->sc_flags & IWM_FLAG_CCMP_KEY_SENT) { > + sc->sc_flags |= IWM_FLAG_CCMP_READY; > + sc->sc_flags &= ~IWM_FLAG_CCMP_KEY_SENT; > + } > + break; > + > case IWM_PHY_CONFIGURATION_CMD: > case IWM_TX_ANT_CONFIGURATION_CMD: > case IWM_ADD_STA: > blob - e6593fbc840833a36d5a792042faaa85e3ac9719 > blob + 747616074f868d8679f9f01232dec4ce09474f93 > --- sys/dev/pci/if_iwmvar.h > +++ sys/dev/pci/if_iwmvar.h > @@ -289,6 +289,8 @@ struct iwm_rx_ring { > #define IWM_FLAG_HW_ERR 0x80 /* hardware error occurred */ > #define IWM_FLAG_SHUTDOWN 0x100 /* shutting down; new tasks forbidden */ > #define IWM_FLAG_BGSCAN 0x200 /* background scan in progress > */ > +#define IWM_FLAG_CCMP_KEY_SENT 0x400 /* CCMP key sent to firmware */ > +#define IWM_FLAG_CCMP_READY 0x800 /* CCMP key installed in firmware */ > > struct iwm_ucode_status { > uint32_t uc_error_event_table; > >