| Message ID | CAKXXJEw72EYHJpdwoWhudmOmFF=w+u_JG3vrp=-TCfR3zhANZw@mail.gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
Hi The patch was mangled by your email client, but I'm not sure if it is correct anyway. On Tue, Aug 01, 2017 at 06:43:33PM -0400, Michael Skeffington wrote: > Mac80211 doesnt check MMIC failure until after falling through the > check for whether the packet is decrypted. Therefore, this driver > never causes MMIC countermeasures to be initiated. The relevant mac80211 code look like this: ieee80211_rx_result ieee80211_rx_h_michael_mic_verify(struct ieee80211_rx_data *rx) { <snip> /* * No way to verify the MIC if the hardware stripped it or * the IV with the key index. In this case we have solely rely * on the driver to set RX_FLAG_MMIC_ERROR in the event of a * MIC failure report. */ if (status->flag & (RX_FLAG_MMIC_STRIPPED | RX_FLAG_IV_STRIPPED)) { if (status->flag & RX_FLAG_MMIC_ERROR) goto mic_fail_no_key; if (!(status->flag & RX_FLAG_IV_STRIPPED) && rx->key && rx->key->conf.cipher == WLAN_CIPHER_SUITE_TKIP) goto update_iv; return RX_CONTINUE; } /* * Some hardware seems to generate Michael MIC failure reports; even * though, the frame was not encrypted with TKIP and therefore has no * MIC. Ignore the flag them to avoid triggering countermeasures. */ if (!rx->key || rx->key->conf.cipher != WLAN_CIPHER_SUITE_TKIP || !(status->flag & RX_FLAG_DECRYPTED)) return RX_CONTINUE; if (rx->sdata->vif.type == NL80211_IFTYPE_AP && rx->key->conf.keyidx) { /* * APs with pairwise keys should never receive Michael MIC * errors for non-zero keyidx because these are reserved for * group keys and only the AP is sending real multicast * frames in the BSS. */ return RX_DROP_UNUSABLE; } if (status->flag & RX_FLAG_MMIC_ERROR) goto mic_fail; <snip> So we indeed check RX_FLAG_DECRYPTED and then RX_FLAG_MMIC_ERROR at some point. However before that check, we also have: if (status->flag & (RX_FLAG_MMIC_STRIPPED | RX_FLAG_IV_STRIPPED)) { if (status->flag & RX_FLAG_MMIC_ERROR) goto mic_fail_no_key; and we always set RX_FLAG_MMIC_STRIPPED and RX_FLAG_IV_STRIPPED flags in rt2800 driver. Hence I do not think patch fixes any problem. Perhaps what should be done is change mic_fail_no_key to mic_fail label in mac80211 to increase rx->key->u.tkip.mic_failures++ statistic. Thanks Stanislaw
On Wed, 2017-08-02 at 09:01 +0200, Stanislaw Gruszka wrote: > The relevant mac80211 code look like this: > > ieee80211_rx_result > ieee80211_rx_h_michael_mic_verify(struct ieee80211_rx_data *rx) I believe that ieee80211_rx_h_decrypt() will drop the frames you're looking at, and I do think the original patch is correct. If MMIC validation was (and could be) done, then the frame must have been decrypted properly. johannes
Johannes Berg <johannes@sipsolutions.net> writes: > On Wed, 2017-08-02 at 09:01 +0200, Stanislaw Gruszka wrote: > >> The relevant mac80211 code look like this: >> >> ieee80211_rx_result >> ieee80211_rx_h_michael_mic_verify(struct ieee80211_rx_data *rx) > > I believe that ieee80211_rx_h_decrypt() will drop the frames you're > looking at, and I do think the original patch is correct. If MMIC > validation was (and could be) done, then the frame must have been > decrypted properly. Just to avoid any confusion, with original patch you mean this one? rt2x00: Fix MMIC countermeasures. https://patchwork.kernel.org/patch/9875647/
I traced through this code during MMIC failure and ieee80211_rx_h_decrypt() drops the frame before getting to ieee80211_rx_h_michael_mic_verify(). Johannes suggested this change to me in response to a previous thread and I am offering this patch after having conducted the proper testing on it. On Wed, Aug 2, 2017 at 9:43 AM, Kalle Valo <kvalo@codeaurora.org> wrote: > Johannes Berg <johannes@sipsolutions.net> writes: > >> On Wed, 2017-08-02 at 09:01 +0200, Stanislaw Gruszka wrote: >> >>> The relevant mac80211 code look like this: >>> >>> ieee80211_rx_result >>> ieee80211_rx_h_michael_mic_verify(struct ieee80211_rx_data *rx) >> >> I believe that ieee80211_rx_h_decrypt() will drop the frames you're >> looking at, and I do think the original patch is correct. If MMIC >> validation was (and could be) done, then the frame must have been >> decrypted properly. > > Just to avoid any confusion, with original patch you mean this one? > > rt2x00: Fix MMIC countermeasures. > https://patchwork.kernel.org/patch/9875647/ > > -- > Kalle Valo
On Wed, Aug 02, 2017 at 10:36:52AM -0400, Michael Skeffington wrote: > I traced through this code during MMIC failure and > ieee80211_rx_h_decrypt() drops the frame before getting to > ieee80211_rx_h_michael_mic_verify(). Johannes suggested this change > to me in response to a previous thread and I am offering this patch > after having conducted the proper testing on it. Ok, please repost the patch without mangling it i.e. using git-send-email. Thanks Stanislaw
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c b/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c index ee5276e233fa..ace91a2db756 100644 --- a/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c +++ b/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c @@ -136,10 +136,19 @@ void rt2800mmio_fill_rxdone(struct queue_entry *entry, */ rxdesc->flags |= RX_FLAG_MMIC_STRIPPED; - if (rxdesc->cipher_status == RX_CRYPTO_SUCCESS) + if (rxdesc->cipher_status == RX_CRYPTO_SUCCESS) { rxdesc->flags |= RX_FLAG_DECRYPTED; - else if (rxdesc->cipher_status == RX_CRYPTO_FAIL_MIC) + } else if (rxdesc->cipher_status == RX_CRYPTO_FAIL_MIC) { + /* + * In order to check the Michael Mic, the packet must have + * been decrypted. Mac80211 doesnt check the MMIC failure + * flag to initiate MMIC countermeasures if the decoded flag + * has not been set. + */ + rxdesc->flags |= RX_FLAG_DECRYPTED; + rxdesc->flags |= RX_FLAG_MMIC_ERROR; + } } if (rt2x00_get_field32(word, RXD_W3_MY_BSS)) diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800usb.c b/drivers/net/wireless/ralink/rt2x00/rt2800usb.c index 685b8e0cd67d..7e5f397c37f9 100644 --- a/drivers/net/wireless/ralink/rt2x00/rt2800usb.c +++ b/drivers/net/wireless/ralink/rt2x00/rt2800usb.c @@ -697,11 +697,20 @@ static void rt2800usb_fill_rxdone(struct queue_entry *entry, * stripped it from the frame. Signal this to mac80211. */ rxdesc->flags |= RX_FLAG_MMIC_STRIPPED; - - if (rxdesc->cipher_status == RX_CRYPTO_SUCCESS) + + if (rxdesc->cipher_status == RX_CRYPTO_SUCCESS) { + rxdesc->flags |= RX_FLAG_DECRYPTED; + } else if (rxdesc->cipher_status == RX_CRYPTO_FAIL_MIC) { + /* + * In order to check the Michael Mic, the packet must have + * been decrypted. Mac80211 doesnt check the MMIC failure + * flag to initiate MMIC countermeasures if the decoded flag + * has not been set. + */ rxdesc->flags |= RX_FLAG_DECRYPTED; - else if (rxdesc->cipher_status == RX_CRYPTO_FAIL_MIC) + rxdesc->flags |= RX_FLAG_MMIC_ERROR;
Mac80211 doesnt check MMIC failure until after falling through the check for whether the packet is decrypted. Therefore, this driver never causes MMIC countermeasures to be initiated. This change may (or may not) be relevant for rt2500usb, rt73usb, and rt61pci as well but I don't have any of those devices to test with. Signed-off-by: Michael Skeffington <mike@hellotwist.com> --- + } } if (rt2x00_get_field32(word, RXD_W0_MY_BSS))