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;
>
>
