On 13/08/19(Tue) 13:52, Stefan Sperling wrote:
> This should hopefully prevent a crash reported to me by Jesper Wallin,
> where net80211 crashes when it attempts to decrypt a CCMP-encrypted
> frame which iwm passed up without decrypting it first.
How does the stack crashes? Shouldn't we drop this packet in the stack
as well?
> By code inspection I have determined that this problem could happen
> in case a CCMP frame is received peer before the WPA handshake has
> completed (e.g. during re-association). Note how the hardware decryption
> code path depends on IEEE80211_NODE_RXPROT being set, which is set only
> once the handshake has been completed.
> We have to be careful here to avoid breaking non-CCMP ciphers (WEP, TKIP).
So it's like working around the firwmare by defining two new state
related to the CCMP key and dropping packets that should not reach
us at this point?
> It also attempts to close a small race where we're handing the key to
> firmware and eventually get an interrupt which confirms key installation,
> in parallel to sending/receiving unicast data frames.
> Until the key has been confirmed by firmware, don't transmit data frames
> and drop received encrypted frames.
Makes sense.
The actual solution also has a race if you change the key before
acknowledging the firmware IWM_ADD_STA_KEY, but I suppose this is
acceptable.
> This part of the diff has poor error handling; if firmware never confirms
> they key then iwm will get stuck and the user will have to recover.
Well up/down should work, right?
> But that should be better than the current situation where we'd be
> sending unencrypted frames or perhaps crash in net80211.
>
> These fixes could be separated but a combined diff is easier to test.
What should we test?
> Once we've got this working for iwm(4), the iwn(4) driver will need
> a similar fix.
>
> diff refs/heads/master refs/heads/ccmp-crash
> blob - 038e6a63dfff113b525bb1e9a30c935996535569
> blob + 0571a3a042c0097002241e2accc026c55af205ed
> --- sys/dev/pci/if_iwm.c
> +++ sys/dev/pci/if_iwm.c
> @@ -3601,6 +3601,13 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_pac
> !IEEE80211_IS_MULTICAST(wh->i_addr1) &&
> (ni->ni_flags & IEEE80211_NODE_RXPROT) &&
> ni->ni_pairwise_key.k_cipher == IEEE80211_CIPHER_CCMP) {
> + if ((sc->sc_flags & IWM_FLAG_CCMP_READY) == 0) {
> + /* Firmware has not installed the key yet. */
> + ic->ic_stats.is_ccmp_dec_errs++;
> + ifp->if_ierrors++;
> + m_freem(m);
> + goto done;
> + }
> if ((rx_pkt_status & IWM_RX_MPDU_RES_STATUS_SEC_ENC_MSK) !=
> IWM_RX_MPDU_RES_STATUS_SEC_CCM_ENC) {
> ic->ic_stats.is_ccmp_dec_errs++;
> @@ -3625,6 +3632,22 @@ iwm_rx_rx_mpdu(struct iwm_softc *sc, struct iwm_rx_pac
> goto done;
> }
> rxi.rxi_flags |= IEEE80211_RXI_HWDEC;
> + } else if ((wh->i_fc[1] & IEEE80211_FC1_PROTECTED) &&
> + !IEEE80211_IS_MULTICAST(wh->i_addr1) &&
Bikesheding: these 5 hardware decryption checks are executed for
non-multicast and if the FC1_PROTECTED bit is set right? Should
that be a single if statement?
> + (ic->ic_flags & IEEE80211_F_RSNON) &&
> + (ic->ic_rsnprotos & IEEE80211_PROTO_RSN) &&
> + (ic->ic_rsnciphers & IEEE80211_CIPHER_CCMP) &&
> + (ni->ni_rsnciphers & IEEE80211_CIPHER_CCMP) &&
> + (ni->ni_flags & IEEE80211_NODE_RXPROT) == 0) {
> + /*
> + * We're doing CCMP and have received an encrypted unicast
> + * frame before 4-way handshake has completed. Don't pass it
> + * up the stack; net80211 would crash trying to decrypt it.
> + */
> + ic->ic_stats.is_ccmp_dec_errs++;
> + ifp->if_ierrors++;
> + m_freem(m);
> + goto done;
> }
>
> #if NBPFILTER > 0
> @@ -6090,6 +6113,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 +6121,8 @@ iwm_set_key(struct ieee80211com *ic, struct ieee80211_
> return (ieee80211_set_key(ic, ni, k));
> }
>
> + sc->sc_flags &= ~IWM_FLAG_CCMP_READY;
> +
> 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 +6134,11 @@ 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,
> + 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 +6164,7 @@ 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);
> }
>
> void
> @@ -6765,6 +6795,14 @@ iwm_start(struct ifnet *ifp)
> (ic->ic_xflags & IEEE80211_F_TX_MGMT_ONLY))
> break;
>
> + if (sc->sc_flags & IWM_FLAG_CCMP_KEY_SENT) {
> + /*
> + * Can't send data frames before firmware is
> + * ready to encrypt them.
> + */
> + break;
> + }
> +
> IFQ_DEQUEUE(&ifp->if_snd, m);
> if (!m)
> break;
> @@ -6846,6 +6884,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 +7370,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;
>
