[PATCH] rt2x00: Fix MMIC countermeasures.

[PATCH] rt2x00: Fix MMIC countermeasures.

[Michael Skeffington]

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>

---

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

        if (rt2x00_get_field32(word, RXD_W0_MY_BSS))

Re: [PATCH] rt2x00: Fix MMIC countermeasures.

[Stanislaw Gruszka]

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

Re: [PATCH] rt2x00: Fix MMIC countermeasures.

[Johannes Berg]

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

Re: [PATCH] rt2x00: Fix MMIC countermeasures.

[Kalle Valo]

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

Re: [PATCH] rt2x00: Fix MMIC countermeasures.

[Michael Skeffington]

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

Re: [PATCH] rt2x00: Fix MMIC countermeasures.

[Stanislaw Gruszka]

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