On Tue, Aug 13, 2019 at 09:40:22AM -0300, Martin Pieuchot wrote: > 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?
Jesper only sent me a screen shot and no public bug report :( The trace looks roughly like this, where AES_Encrypt_ECB crashes because ieee80211_ccmp_set_key() is never called since iwm_set_key overrides it. AES_Encrypt_ECB() ieee80211_ccmp_phase1() ieee80211_ccmp_decrypt() ieee80211_input() ieee80211_ba_move_window() ieee80211_input() iwm_rx_rx_mpdu() iwm_notif_intr() iwm_intr() What's a bit confusing here is that the interrupt was for an uencrypted frame (a Block Ack Request) which moved the BA window forward. When this happens we flush frames currently waiting in the Rx BA reordering buffer. The encrypted frame which caused the crash was on that queue; It must have passed through iwm_rx_rx_mpdu() earlier before the WPA handshake was complete, and ended up waiting in the Rx BA queue because its sequence number was off. > Shouldn't we drop this packet in the stack as well? net80211 will always see a 'protected' bit in the frame header In ieee80211_input, if rxi has an HW_DEC flag set, the code assumes this frame needs no further processing for crypto. Otherwise it tries to decrypt. So if the driver passes up a frame without decrypting it and without setting HW_DEC, we crash. There's nothing that would allow net80211 to detect an incorrect absence of HW_DEC. The driver overrides the set_key function and corresponding frames are expected to arrive with the HW_DEC flag set in net80211. (Please don't ask me to change this design right now for a better fix, it wasn't my idea...) > > 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? Yes. > > 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. The stack-side key has already been changed at that point. It is derived from the WPA handshake. The firmware has to accept it or we can't proceed. > > 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? Yes it should. > > 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? Just check for regressions. I have lightly tested it on one machine and it works there. There's not a lot of testing needed, but if a few people could run the diff to check if I've missed something, that would be nice.
