Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   gregory.greenman@intel.com
To:     johannes@sipsolutions.net
Cc:     linux-wireless@vger.kernel.org,
        Johannes Berg <johannes.berg@intel.com>,
        Gregory Greenman <gregory.greenman@intel.com>
Subject: [PATCH 03/11] wifi: iwlwifi: mvm: fix MIC removal confusion
Date:   Tue, 18 Apr 2023 12:28:06 +0300
Message-Id: <20230418122405.81345b6ab0cd.Ibe0348defb6cce11c99929a1f049e60b5cfc150c@changeid>
In-Reply-To: <20230418092814.1438734-1-gregory.greenman@intel.com>
References: <20230418092814.1438734-1-gregory.greenman@intel.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

From: Johannes Berg <johannes.berg@intel.com>

The RADA/firmware collaborate on MIC stripping in the following
way:
 - the firmware fills the IWL_RX_MPDU_MFLG1_MIC_CRC_LEN_MASK
   value for how many words need to be removed at the end of
   the frame, CRC and, if decryption was done, MIC
 - if the RADA is active, it will
   - remove that much from the end of the frame
   - zero the value in IWL_RX_MPDU_MFLG1_MIC_CRC_LEN_MASK

As a consequence, the only thing the driver should need to do
is to
 - unconditionally tell mac80211 that the MIC was removed
   if decryption was already done
 - remove as much as IWL_RX_MPDU_MFLG1_MIC_CRC_LEN_MASK says
   at the end of the frame, since either RADA did it and then
   the value is 0, or RADA was disabled and then the value is
   whatever should be removed to strip both CRC & MIC

However, all this code was historically grown and getting a
bit confused. Originally, we were indicating that the MIC was
not stripped, which is the version of the code upstreamed in
commit 780e87c29e77 ("iwlwifi: mvm: add 9000 series RX processing")
which indicated RX_FLAG_DECRYPTED in iwl_mvm_rx_crypto().

We later had a commit to change that to also indicate that the
MIC was stripped, adding RX_FLAG_MIC_STRIPPED. However, this was
then "fixed" later to only do that conditionally on RADA being
enabled, since otherwise RADA didn't strip the MIC bytes yet.
At the time, we were also always including the FCS if the RADA
was not enabled, so that was still broken wrt. the FCS if the
RADA isn't enabled - but that's a pretty rare case. Notably
though, it does happen for management frames, where we do need
to remove the MIC and CRC but the RADA is disabled.

Later, in commit 40a0b38d7a7f ("iwlwifi: mvm: Fix calculation of
frame length"), we changed this again, upstream this was just a
single commit, but internally it was split into first the correct
commit and then an additional fix that reduced the number of bytes
that are removed by crypt_len. Note that this is clearly wrong
since crypt_len indicates the length of the PN header (always 8),
not the length of the MIC (8 or 16 depending on algorithm).
However, this additional fix mostly canceled the other bugs,
apart from the confusion about the size of the MIC.

To fix this correctly, remove all those additional workarounds.
We really should always indicate to mac80211 the MIC was stripped
(it cannot use it anyway if decryption was already done), and also
always actually remove it and the CRC regardless of the RADA being
enabled or not. That's simple though, the value indicated in the
metadata is zeroed by the RADA if it's enabled and used the value,
so there's no need to check if it's enabled or not.

Notably then, this fixes the MIC size confusion, letting us receive
GCMP-256 encrypted management frames correctly that would otherwise
be reported to mac80211 8 bytes too short since the RADA is turned
off for them, crypt_len is 8, but the MIC size is 16, so when we do
the adjustment based on IWL_RX_MPDU_MFLG1_MIC_CRC_LEN_MASK (which
indicates 20 bytes to remove) we remove 12 bytes but indicate then
to mac80211 the MIC is still present, so mac80211 again removes the
MIC of 16 bytes, for an overall removal of 28 rather than 20 bytes.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Signed-off-by: Gregory Greenman <gregory.greenman@intel.com>
---
 drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c | 28 ++++---------------
 1 file changed, 5 insertions(+), 23 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
index 9dbf14fa0ca7..542f6658f2d4 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
@@ -106,28 +106,12 @@ static int iwl_mvm_create_skb(struct iwl_mvm *mvm, struct sk_buff *skb,
 
 	/*
 	 * For non monitor interface strip the bytes the RADA might not have
-	 * removed. As monitor interface cannot exist with other interfaces
-	 * this removal is safe.
+	 * removed (it might be disabled, e.g. for mgmt frames). As a monitor
+	 * interface cannot exist with other interfaces, this removal is safe
+	 * and sufficient, in monitor mode there's no decryption being done.
 	 */
-	if (mic_crc_len && !ieee80211_hw_check(mvm->hw, RX_INCLUDES_FCS)) {
-		u32 pkt_flags = le32_to_cpu(pkt->len_n_flags);
-
-		/*
-		 * If RADA was not enabled then decryption was not performed so
-		 * the MIC cannot be removed.
-		 */
-		if (!(pkt_flags & FH_RSCSR_RADA_EN)) {
-			if (WARN_ON(crypt_len > mic_crc_len))
-				return -EINVAL;
-
-			mic_crc_len -= crypt_len;
-		}
-
-		if (WARN_ON(mic_crc_len > len))
-			return -EINVAL;
-
+	if (len > mic_crc_len && !ieee80211_hw_check(mvm->hw, RX_INCLUDES_FCS))
 		len -= mic_crc_len;
-	}
 
 	/* If frame is small enough to fit in skb->head, pull it completely.
 	 * If not, only pull ieee80211_hdr (including crypto if present, and
@@ -411,9 +395,7 @@ static int iwl_mvm_rx_crypto(struct iwl_mvm *mvm, struct ieee80211_sta *sta,
 		if (!(status & IWL_RX_MPDU_STATUS_MIC_OK))
 			return -1;
 
-		stats->flag |= RX_FLAG_DECRYPTED;
-		if (pkt_flags & FH_RSCSR_RADA_EN)
-			stats->flag |= RX_FLAG_MIC_STRIPPED;
+		stats->flag |= RX_FLAG_DECRYPTED | RX_FLAG_MIC_STRIPPED;
 		*crypt_len = IEEE80211_CCMP_HDR_LEN;
 		return 0;
 	case IWL_RX_MPDU_STATUS_SEC_TKIP:
-- 
2.38.1