| Message ID | 20201214172118.18100-2-jouni@codeaurora.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 56c5485c9e444c2e85e11694b6c44f1338fc20fd |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ath9k: Safer key deletion to avoid unexpected behavior | expand |
Jouni Malinen <jouni@codeaurora.org> wrote: > 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 <jouni@codeaurora.org> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> 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
On Thursday 17 December 2020 06:51:48 Kalle Valo wrote: > Jouni Malinen <jouni@codeaurora.org> wrote: > > > 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 <jouni@codeaurora.org> > > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> > > 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?
Pali Rohár <pali@kernel.org> writes: > On Thursday 17 December 2020 06:51:48 Kalle Valo wrote: >> Jouni Malinen <jouni@codeaurora.org> wrote: >> >> > 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 <jouni@codeaurora.org> >> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> >> >> 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? Yeah, but you were just a little late as I already applied them.
On Thursday 17 December 2020 18:06:27 Kalle Valo wrote: > Pali Rohár <pali@kernel.org> writes: > > > On Thursday 17 December 2020 06:51:48 Kalle Valo wrote: > >> Jouni Malinen <jouni@codeaurora.org> wrote: > >> > >> > 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 <jouni@codeaurora.org> > >> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> > >> > >> 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? > > Yeah, but you were just a little late as I already applied them. Ok, would you then send these patches to stable manually? > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Pali Rohár <pali@kernel.org> writes: > On Thursday 17 December 2020 18:06:27 Kalle Valo wrote: >> Pali Rohár <pali@kernel.org> writes: >> >> > On Thursday 17 December 2020 06:51:48 Kalle Valo wrote: >> >> Jouni Malinen <jouni@codeaurora.org> wrote: >> >> >> >> > 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 <jouni@codeaurora.org> >> >> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> >> >> >> >> 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? >> >> 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.
diff --git a/drivers/net/wireless/ath/key.c b/drivers/net/wireless/ath/key.c index 1816b4e7dc26..59618bb41f6c 100644 --- a/drivers/net/wireless/ath/key.c +++ b/drivers/net/wireless/ath/key.c @@ -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;
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 <jouni@codeaurora.org> --- drivers/net/wireless/ath/key.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)