summaryrefslogtreecommitdiff
path: root/drivers/net
diff options
context:
space:
mode:
authorJohannes Berg <johannes.berg@intel.com>2023-04-18 12:28:06 +0300
committerJohannes Berg <johannes.berg@intel.com>2023-04-20 11:45:54 +0200
commit7b41a99ce2d494d01e8fcb25e7113f143a0f2155 (patch)
tree431d02ec2470b94ab8a743a8e7c53110a862d30c /drivers/net
parent3d90d2f4a018fe8cfd65068bc6350b6222be4852 (diff)
wifi: iwlwifi: mvm: fix MIC removal confusion
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> Link: https://lore.kernel.org/r/20230418122405.81345b6ab0cd.Ibe0348defb6cce11c99929a1f049e60b5cfc150c@changeid Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Diffstat (limited to 'drivers/net')
-rw-r--r--drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c28
1 files 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: