Skip to contentSkip to conversation

ath9k: Safer key deletion to avoid unexpected behavior

PATCH In review Jouni Malinen 3 participants 11 messages Dec 14, 2020 – Jan 11, 2021

Subsystems (3)
Qualcomm Atheros ATH9K wireless driver Atheros ATH5K wireless driver Atheros ath generic utilities
lore
Share & tools
lore

Jouni Malinen

author
#0 lore raw

cover ath9k: Safer key deletion to avoid unexpected behavior

While earlier mac80211 commits like a0761a301746 ("mac80211: drop data frames without key on encrypted links"), ce2e1ca70307 ("mac80211: Check port authorization in the ieee80211_tx_dequeue() case"), and b16798f5b907 ("mac80211: mark station unauthorized before key removal") addressed the most visible and easiest to trigger cases where pending frames from TX queues could potentially end up getting transmitted without proper encryption, some similar cases could still remain in the hardware TX queue. In such cases, clearing of the hardware key cache entry could happen while there were remaining frames in the TX queues with reference to the deleted key. That could end up transmitting such frames without encryption. These issues are related to CVE-2020-3702.

It is not clear whether this can be triggered in the station mode after the mac80211 changes, but it has been possible to come up with a test setup where this could be triggered in the AP mode where the TX queue is shared with multiple destinations and cannot be flushed when a single station disconnects. Address such cases by not clearing the hardware key cache entry immediately, but instead, temporarily leaving the key in place and disabling RX processing for the key cache entry. Such a postponed key deletion is completed once there are no remaining frames referencing the particular key cache entry. This is done before handling the next key cache operation to avoid having to perform heavy operations during normal TX operations.

Jouni Malinen (5):
ath: Use safer key clearing with key cache entries
ath9k: Clear key cache explicitly on disabling hardware
ath: Export ath_hw_keysetmac()
ath: Modify ath_key_delete() to not need full key entry
ath9k: Postpone key cache entry deletion for TXQ frames reference it

 drivers/net/wireless/ath/ath.h                |  3 +-
 drivers/net/wireless/ath/ath5k/mac80211-ops.c |  2 +-
 drivers/net/wireless/ath/ath9k/htc_drv_main.c |  2 +-
 drivers/net/wireless/ath/ath9k/hw.h           |  1 +
 drivers/net/wireless/ath/ath9k/main.c         | 95 ++++++++++++++++++-
 drivers/net/wireless/ath/key.c                | 41 ++++----
 6 files changed, 122 insertions(+), 22 deletions(-)
Signature · 1 line
2.20.1
author
#1 lore raw

PATCH 1/5 ath: Use safer key clearing with key cache entries

It is possible for there to be pending frames in TXQs with a reference to the key cache entry that is being deleted. If such a key cache entry is cleared, those pending frame in TXQ might get transmitted without proper encryption. It is safer to leave the previously used key into the key cache in such cases. Instead, only clear the MAC address to prevent RX processing from using this key cache entry.

This is needed in particularly in AP mode where the TXQs cannot be flushed on station disconnection. This change alone may not be able to address all cases where the key cache entry might get reused for other purposes immediately (the key cache entry should be released for reuse only once the TXQs do not have any remaining references to them), but this makes it less likely to get unprotected frames and the more complete changes may end up being significantly more complex.

Signed-off-by Jouni Malinen <[email protected]>
---
 drivers/net/wireless/ath/key.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
drivers/net/wireless/ath/key.c +10 -1
index 1816b4e7dc26..59618bb41f6c 100644
@@ -583,7 +583,16 @@ EXPORT_SYMBOL(ath_key_config);
  */
 void ath_key_delete(struct ath_common *common, struct ieee80211_key_conf *key)
 {
-	ath_hw_keyreset(common, key->hw_key_idx);
+	/* Leave CCMP and TKIP (main key) configured to avoid disabling
+	 * encryption for potentially pending frames already in a TXQ with the
+	 * keyix pointing to this key entry. Instead, only clear the MAC address
+	 * to prevent RX processing from using this key cache entry.
+	 */
+	if (test_bit(key->hw_key_idx, common->ccmp_keymap) ||
+	    test_bit(key->hw_key_idx, common->tkip_keymap))
+		ath_hw_keysetmac(common, key->hw_key_idx, NULL);
+	else
+		ath_hw_keyreset(common, key->hw_key_idx);
 	if (key->hw_key_idx < IEEE80211_WEP_NKID)
 		return;
Signature · 1 line
2.20.1
#2 lore raw

Re: [PATCH 1/5] ath: Use safer key clearing with key cache entries

Quoted 15 lines · Jouni Malinen

It is possible for there to be pending frames in TXQs with a reference to the key cache entry that is being deleted. If such a key cache entry is cleared, those pending frame in TXQ might get transmitted without proper encryption. It is safer to leave the previously used key into the key cache in such cases. Instead, only clear the MAC address to prevent RX processing from using this key cache entry.

This is needed in particularly in AP mode where the TXQs cannot be flushed on station disconnection. This change alone may not be able to address all cases where the key cache entry might get reused for other purposes immediately (the key cache entry should be released for reuse only once the TXQs do not have any remaining references to them), but this makes it less likely to get unprotected frames and the more complete changes may end up being significantly more complex.

Quoted 2 lines
Signed-off-by Jouni Malinen <[email protected]>
Signed-off-by Kalle Valo <[email protected]>

5 patches applied to ath-next branch of ath.git, thanks.

56c5485c9e44 ath: Use safer key clearing with key cache entries 73488cb2fa3b ath9k: Clear key cache explicitly on disabling hardware d2d3e36498dd ath: Export ath_hw_keysetmac()
144cd24dbc36 ath: Modify ath_key_delete() to not need full key entry ca2848022c12 ath9k: Postpone key cache entry deletion for TXQ frames reference it

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
#3 lore raw

Re: [PATCH 1/5] ath: Use safer key clearing with key cache entries

Quoted 20 lines · On Thursday 17 December 2020 06:51:48 Kalle Valo wrote:
Quoted 15 lines · Jouni Malinen

It is possible for there to be pending frames in TXQs with a reference to the key cache entry that is being deleted. If such a key cache entry is cleared, those pending frame in TXQ might get transmitted without proper encryption. It is safer to leave the previously used key into the key cache in such cases. Instead, only clear the MAC address to prevent RX processing from using this key cache entry.

This is needed in particularly in AP mode where the TXQs cannot be flushed on station disconnection. This change alone may not be able to address all cases where the key cache entry might get reused for other purposes immediately (the key cache entry should be released for reuse only once the TXQs do not have any remaining references to them), but this makes it less likely to get unprotected frames and the more complete changes may end up being significantly more complex.

Quoted 2 lines
Signed-off-by Jouni Malinen <[email protected]>
Signed-off-by Kalle Valo <[email protected]>
Quoted 7 lines

5 patches applied to ath-next branch of ath.git, thanks.

56c5485c9e44 ath: Use safer key clearing with key cache entries 73488cb2fa3b ath9k: Clear key cache explicitly on disabling hardware d2d3e36498dd ath: Export ath_hw_keysetmac()
144cd24dbc36 ath: Modify ath_key_delete() to not need full key entry ca2848022c12 ath9k: Postpone key cache entry deletion for TXQ frames reference it

Hello! Should not these patches be suitable for backporting into stable kernels (via CC: stable@ commit message line) as they are related to security issue CVE-2020-3702?

#4 lore raw

Re: [PATCH 1/5] ath: Use safer key clearing with key cache entries

Quoted 29 lines · Pali Rohár
Quoted 20 lines · On Thursday 17 December 2020 06:51:48 Kalle Valo wrote:
Quoted 15 lines · Jouni Malinen

It is possible for there to be pending frames in TXQs with a reference to the key cache entry that is being deleted. If such a key cache entry is cleared, those pending frame in TXQ might get transmitted without proper encryption. It is safer to leave the previously used key into the key cache in such cases. Instead, only clear the MAC address to prevent RX processing from using this key cache entry.

This is needed in particularly in AP mode where the TXQs cannot be flushed on station disconnection. This change alone may not be able to address all cases where the key cache entry might get reused for other purposes immediately (the key cache entry should be released for reuse only once the TXQs do not have any remaining references to them), but this makes it less likely to get unprotected frames and the more complete changes may end up being significantly more complex.

Quoted 2 lines
Signed-off-by Jouni Malinen <[email protected]>
Signed-off-by Kalle Valo <[email protected]>
Quoted 7 lines

5 patches applied to ath-next branch of ath.git, thanks.

56c5485c9e44 ath: Use safer key clearing with key cache entries 73488cb2fa3b ath9k: Clear key cache explicitly on disabling hardware d2d3e36498dd ath: Export ath_hw_keysetmac()
144cd24dbc36 ath: Modify ath_key_delete() to not need full key entry ca2848022c12 ath9k: Postpone key cache entry deletion for TXQ frames reference it

Quoted 3 lines

Hello! Should not these patches be suitable for backporting into stable kernels (via CC: stable@ commit message line) as they are related to security issue CVE-2020-3702?

Yeah, but you were just a little late as I already applied them.

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
#5 lore raw

Re: [PATCH 1/5] ath: Use safer key clearing with key cache entries

Quoted 31 lines · On Thursday 17 December 2020 18:06:27 Kalle Valo wrote:
Quoted 29 lines · Pali Rohár
Quoted 27 lines · On Thursday 17 December 2020 06:51:48 Kalle Valo wrote:
Quoted 15 lines · Jouni Malinen

It is possible for there to be pending frames in TXQs with a reference to the key cache entry that is being deleted. If such a key cache entry is cleared, those pending frame in TXQ might get transmitted without proper encryption. It is safer to leave the previously used key into the key cache in such cases. Instead, only clear the MAC address to prevent RX processing from using this key cache entry.

This is needed in particularly in AP mode where the TXQs cannot be flushed on station disconnection. This change alone may not be able to address all cases where the key cache entry might get reused for other purposes immediately (the key cache entry should be released for reuse only once the TXQs do not have any remaining references to them), but this makes it less likely to get unprotected frames and the more complete changes may end up being significantly more complex.

Quoted 2 lines
Signed-off-by Jouni Malinen <[email protected]>
Signed-off-by Kalle Valo <[email protected]>

5 patches applied to ath-next branch of ath.git, thanks.

56c5485c9e44 ath: Use safer key clearing with key cache entries 73488cb2fa3b ath9k: Clear key cache explicitly on disabling hardware d2d3e36498dd ath: Export ath_hw_keysetmac()
144cd24dbc36 ath: Modify ath_key_delete() to not need full key entry ca2848022c12 ath9k: Postpone key cache entry deletion for TXQ frames reference it

Quoted 5 lines
Quoted 3 lines

Hello! Should not these patches be suitable for backporting into stable kernels (via CC: stable@ commit message line) as they are related to security issue CVE-2020-3702?

Yeah, but you were just a little late as I already applied them.

Ok, would you then send these patches to stable manually?

Quoted 4 lines
Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
#6 lore raw

Re: [PATCH 1/5] ath: Use safer key clearing with key cache entries

Quoted 32 lines · Pali Rohár
Quoted 31 lines · On Thursday 17 December 2020 18:06:27 Kalle Valo wrote:
Quoted 29 lines · Pali Rohár
Quoted 27 lines · On Thursday 17 December 2020 06:51:48 Kalle Valo wrote:
Quoted 15 lines · Jouni Malinen

It is possible for there to be pending frames in TXQs with a reference to the key cache entry that is being deleted. If such a key cache entry is cleared, those pending frame in TXQ might get transmitted without proper encryption. It is safer to leave the previously used key into the key cache in such cases. Instead, only clear the MAC address to prevent RX processing from using this key cache entry.

This is needed in particularly in AP mode where the TXQs cannot be flushed on station disconnection. This change alone may not be able to address all cases where the key cache entry might get reused for other purposes immediately (the key cache entry should be released for reuse only once the TXQs do not have any remaining references to them), but this makes it less likely to get unprotected frames and the more complete changes may end up being significantly more complex.

Quoted 2 lines
Signed-off-by Jouni Malinen <[email protected]>
Signed-off-by Kalle Valo <[email protected]>

5 patches applied to ath-next branch of ath.git, thanks.

56c5485c9e44 ath: Use safer key clearing with key cache entries 73488cb2fa3b ath9k: Clear key cache explicitly on disabling hardware d2d3e36498dd ath: Export ath_hw_keysetmac()
144cd24dbc36 ath: Modify ath_key_delete() to not need full key entry ca2848022c12 ath9k: Postpone key cache entry deletion for TXQ frames reference it

Quoted 7 lines
Quoted 5 lines
Quoted 3 lines

Hello! Should not these patches be suitable for backporting into stable kernels (via CC: stable@ commit message line) as they are related to security issue CVE-2020-3702?

Yeah, but you were just a little late as I already applied them.

Ok, would you then send these patches to stable manually?

Sorry, I have too many patches in queue to do that. But I don't think I need to submit them, my understanding is that anyone can submit patches to stable.

Signature · 3 lines
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
author
#7 lore raw

PATCH 2/5 ath9k: Clear key cache explicitly on disabling hardware

In reply to Jouni Malinen

Now that ath/key.c may not be explicitly clearing keys from the key cache, clear all key cache entries when disabling hardware to make sure no keys are left behind beyond this point.

Signed-off-by Jouni Malinen <[email protected]>
---
 drivers/net/wireless/ath/ath9k/main.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/net/wireless/ath/ath9k/main.c +5 -0
index caebe3fd6869..10b87aa1d289 100644
@@ -894,6 +894,11 @@ static void ath9k_stop(struct ieee80211_hw *hw)
 
 	spin_unlock_bh(&sc->sc_pcu_lock);
 
+	/* Clear key cache entries explicitly to get rid of any potentially
+	 * remaining keys.
+	 */
+	ath9k_cmn_init_crypto(sc->sc_ah);
+
 	ath9k_ps_restore(sc);
 
 	sc->ps_idle = prev_idle;
Signature · 1 line
2.20.1
author
#8 lore raw

PATCH 3/5 ath: Export ath_hw_keysetmac()

In reply to Jouni Malinen

ath9k is going to use this for safer management of key cache entries.

Signed-off-by Jouni Malinen <[email protected]>
---
 drivers/net/wireless/ath/ath.h | 1 +
 drivers/net/wireless/ath/key.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)
drivers/net/wireless/ath/ath.h +1 -0
index 7a364eca46d6..9d18105c449f 100644
@@ -203,6 +203,7 @@ int ath_key_config(struct ath_common *common,
 			  struct ieee80211_sta *sta,
 			  struct ieee80211_key_conf *key);
 bool ath_hw_keyreset(struct ath_common *common, u16 entry);
+bool ath_hw_keysetmac(struct ath_common *common, u16 entry, const u8 *mac);
 void ath_hw_cycle_counters_update(struct ath_common *common);
 int32_t ath_hw_get_listen_time(struct ath_common *common);
drivers/net/wireless/ath/key.c +2 -2
index 59618bb41f6c..cb266cf3c77c 100644
@@ -84,8 +84,7 @@ bool ath_hw_keyreset(struct ath_common *common, u16 entry)
 }
 EXPORT_SYMBOL(ath_hw_keyreset);
 
-static bool ath_hw_keysetmac(struct ath_common *common,
-			     u16 entry, const u8 *mac)
+bool ath_hw_keysetmac(struct ath_common *common, u16 entry, const u8 *mac)
 {
 	u32 macHi, macLo;
 	u32 unicast_flag = AR_KEYTABLE_VALID;

@@ -125,6 +124,7 @@ static bool ath_hw_keysetmac(struct ath_common *common,
 
 	return true;
 }
+EXPORT_SYMBOL(ath_hw_keysetmac);
 
 static bool ath_hw_set_keycache_entry(struct ath_common *common, u16 entry,
 				      const struct ath_keyval *k,
Signature · 1 line
2.20.1
author
#9 lore raw

PATCH 4/5 ath: Modify ath_key_delete() to not need full key entry

In reply to Jouni Malinen

tkip_keymap can be used internally to avoid the reference to key->cipher and with this, only the key index value itself is needed. This allows ath_key_delete() call to be postponed to be handled after the upper layer STA and key entry have already been removed. This is needed to make ath9k key cache management safer.

Signed-off-by Jouni Malinen <[email protected]>
---
 drivers/net/wireless/ath/ath.h                |  2 +-
 drivers/net/wireless/ath/ath5k/mac80211-ops.c |  2 +-
 drivers/net/wireless/ath/ath9k/htc_drv_main.c |  2 +-
 drivers/net/wireless/ath/ath9k/main.c         |  5 ++-
 drivers/net/wireless/ath/key.c                | 34 +++++++++----------
 5 files changed, 22 insertions(+), 23 deletions(-)
drivers/net/wireless/ath/ath.h +1 -1
index 9d18105c449f..f083fb9038c3 100644
@@ -197,7 +197,7 @@ struct sk_buff *ath_rxbuf_alloc(struct ath_common *common,
 bool ath_is_mybeacon(struct ath_common *common, struct ieee80211_hdr *hdr);
 
 void ath_hw_setbssidmask(struct ath_common *common);
-void ath_key_delete(struct ath_common *common, struct ieee80211_key_conf *key);
+void ath_key_delete(struct ath_common *common, u8 hw_key_idx);
 int ath_key_config(struct ath_common *common,
 			  struct ieee80211_vif *vif,
 			  struct ieee80211_sta *sta,
drivers/net/wireless/ath/ath5k/mac80211-ops.c +1 -1
index 8f2719ff463c..532eeac9e83e 100644
@@ -522,7 +522,7 @@ ath5k_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
 		}
 		break;
 	case DISABLE_KEY:
-		ath_key_delete(common, key);
+		ath_key_delete(common, key->hw_key_idx);
 		break;
 	default:
 		ret = -EINVAL;
drivers/net/wireless/ath/ath9k/htc_drv_main.c +1 -1
index 2b7832b1c800..72ef319feeda 100644
@@ -1461,7 +1461,7 @@ static int ath9k_htc_set_key(struct ieee80211_hw *hw,
 		}
 		break;
 	case DISABLE_KEY:
-		ath_key_delete(common, key);
+		ath_key_delete(common, key->hw_key_idx);
 		break;
 	default:
 		ret = -EINVAL;
drivers/net/wireless/ath/ath9k/main.c +2 -3
index 10b87aa1d289..bcdf150060f2 100644
@@ -1543,12 +1543,11 @@ static void ath9k_del_ps_key(struct ath_softc *sc,
 {
 	struct ath_common *common = ath9k_hw_common(sc->sc_ah);
 	struct ath_node *an = (struct ath_node *) sta->drv_priv;
-	struct ieee80211_key_conf ps_key = { .hw_key_idx = an->ps_key };
 
 	if (!an->ps_key)
 	    return;
 
-	ath_key_delete(common, &ps_key);
+	ath_key_delete(common, an->ps_key);
 	an->ps_key = 0;
 	an->key_idx[0] = 0;
 }

@@ -1748,7 +1747,7 @@ static int ath9k_set_key(struct ieee80211_hw *hw,
 		}
 		break;
 	case DISABLE_KEY:
-		ath_key_delete(common, key);
+		ath_key_delete(common, key->hw_key_idx);
 		if (an) {
 			for (i = 0; i < ARRAY_SIZE(an->key_idx); i++) {
 				if (an->key_idx[i] != key->hw_key_idx)
drivers/net/wireless/ath/key.c +17 -17
index cb266cf3c77c..61b59a804e30 100644
@@ -581,38 +581,38 @@ EXPORT_SYMBOL(ath_key_config);
 /*
  * Delete Key.
  */
-void ath_key_delete(struct ath_common *common, struct ieee80211_key_conf *key)
+void ath_key_delete(struct ath_common *common, u8 hw_key_idx)
 {
 	/* Leave CCMP and TKIP (main key) configured to avoid disabling
 	 * encryption for potentially pending frames already in a TXQ with the
 	 * keyix pointing to this key entry. Instead, only clear the MAC address
 	 * to prevent RX processing from using this key cache entry.
 	 */
-	if (test_bit(key->hw_key_idx, common->ccmp_keymap) ||
-	    test_bit(key->hw_key_idx, common->tkip_keymap))
-		ath_hw_keysetmac(common, key->hw_key_idx, NULL);
+	if (test_bit(hw_key_idx, common->ccmp_keymap) ||
+	    test_bit(hw_key_idx, common->tkip_keymap))
+		ath_hw_keysetmac(common, hw_key_idx, NULL);
 	else
-		ath_hw_keyreset(common, key->hw_key_idx);
-	if (key->hw_key_idx < IEEE80211_WEP_NKID)
+		ath_hw_keyreset(common, hw_key_idx);
+	if (hw_key_idx < IEEE80211_WEP_NKID)
 		return;
 
-	clear_bit(key->hw_key_idx, common->keymap);
-	clear_bit(key->hw_key_idx, common->ccmp_keymap);
-	if (key->cipher != WLAN_CIPHER_SUITE_TKIP)
+	clear_bit(hw_key_idx, common->keymap);
+	clear_bit(hw_key_idx, common->ccmp_keymap);
+	if (!test_bit(hw_key_idx, common->tkip_keymap))
 		return;
 
-	clear_bit(key->hw_key_idx + 64, common->keymap);
+	clear_bit(hw_key_idx + 64, common->keymap);
 
-	clear_bit(key->hw_key_idx, common->tkip_keymap);
-	clear_bit(key->hw_key_idx + 64, common->tkip_keymap);
+	clear_bit(hw_key_idx, common->tkip_keymap);
+	clear_bit(hw_key_idx + 64, common->tkip_keymap);
 
 	if (!(common->crypt_caps & ATH_CRYPT_CAP_MIC_COMBINED)) {
-		ath_hw_keyreset(common, key->hw_key_idx + 32);
-		clear_bit(key->hw_key_idx + 32, common->keymap);
-		clear_bit(key->hw_key_idx + 64 + 32, common->keymap);
+		ath_hw_keyreset(common, hw_key_idx + 32);
+		clear_bit(hw_key_idx + 32, common->keymap);
+		clear_bit(hw_key_idx + 64 + 32, common->keymap);
 
-		clear_bit(key->hw_key_idx + 32, common->tkip_keymap);
-		clear_bit(key->hw_key_idx + 64 + 32, common->tkip_keymap);
+		clear_bit(hw_key_idx + 32, common->tkip_keymap);
+		clear_bit(hw_key_idx + 64 + 32, common->tkip_keymap);
 	}
 }
 EXPORT_SYMBOL(ath_key_delete);
Signature · 1 line
2.20.1
author
#10 lore raw

PATCH 5/5 ath9k: Postpone key cache entry deletion for TXQ frames reference it

In reply to Jouni Malinen

Do not delete a key cache entry that is still being referenced by pending frames in TXQs. This avoids reuse of the key cache entry while a frame might still be transmitted using it.

To avoid having to do any additional operations during the main TX path operations, track pending key cache entries in a new bitmap and check whether any pending entries can be deleted before every new key add/remove operation. Also clear any remaining entries when stopping the interface.

Signed-off-by Jouni Malinen <[email protected]>
---
 drivers/net/wireless/ath/ath9k/hw.h   |  1 +
 drivers/net/wireless/ath/ath9k/main.c | 87 ++++++++++++++++++++++++++-
 2 files changed, 87 insertions(+), 1 deletion(-)
drivers/net/wireless/ath/ath9k/hw.h +1 -0
index 023599e10dd5..b7b65b1c90e8 100644
@@ -820,6 +820,7 @@ struct ath_hw {
 	struct ath9k_pacal_info pacal_info;
 	struct ar5416Stats stats;
 	struct ath9k_tx_queue_info txq[ATH9K_NUM_TX_QUEUES];
+	DECLARE_BITMAP(pending_del_keymap, ATH_KEYMAX);
 
 	enum ath9k_int imask;
 	u32 imrs2_reg;
drivers/net/wireless/ath/ath9k/main.c +86 -1

117 lines not on this page · raw message

Signature · 1 line
2.20.1