Am 20.09.20 um 08:37 schrieb Alexander Wetzel:
>>
>> + /* GCMP and 256 bit CCMP keys the key can't be copied into the
>> + * MPDU struct ieee80211_tx_info. We therefore must flush the
>> + * queues to ensure there are no MPDUs left which are
referring
>> + * to the outgoing key.
>> + */
>> + if (key->flags & IEEE80211_KEY_FLAG_PAIRWISE &&
>> + (key->cipher == WLAN_CIPHER_SUITE_GCMP ||
>> + key->cipher == WLAN_CIPHER_SUITE_GCMP_256 ||
>> + key->cipher == WLAN_CIPHER_SUITE_CCMP_256)) {
>> + ieee80211_stop_queues(hw);
>> + iwl_mvm_mac_flush(hw, vif, 0, true);
>> + ieee80211_wake_queues(hw);
>> + }
>
> Shouldn't the wake be after installing the new key? Otherwise new frames
> can race and be there again?
mac80211 is taking care of that and has already stopped queuing new
MPDUs which would use the key by setting KEY_FLAG_TAINTED on it. So for
a PTK0 rekey we are fine, mac80211 won't queue frames with the key under
deletion.
Ok, forget this patch, the mvm part is pointless.
The maximum we have to do is pausing the queues when we delete a key, no
flush required at all... I'll look into that again and send an updated
version:
The hw_key and the keyid is set (again) when we dequeue. So we either
get a valid key or ieee80211_tx_dequeue() drops the MPDU itself.
Therefore it's irrelevant if we lookup the key from a table or store it
in the MPDU info struct. Now we may still have a race within the driver
but simply pausing dequeuing during key deletion should do the trick.
But I think we should start setting KEY_FLAG_TAINTED for *any* PTK
deletion to make sure we are not sending out MPDUs with a zero key or
something like that. This is a special kind of a rekey case we missed so
far but should be trival cover that, too.
That is then basically taking care about the generic Kr00k vulnerability
some months ago.
Now I'm not aware of we have any mitigations in the code for that but
when we set KEY_FLAG_TAINTED for key deletions any driver which is able
to rekey PTK0 correctly can't be affected by Kr00k any longer.
Looked at the current code and turns out we have at least two
mitigations for that now:
1) commit ce2e1ca70307 "(mac80211: Check port authorization in the
ieee80211_tx_dequeue() case)" and
2) commit a0761a301746 "(mac80211: drop data frames without key on
encrypted links)"
I'll look into that in the next days and prepare a patch for mac8011 to
discuss that.
I've the patch basically ready, but there are already the two
mitigations mentioned above in place.
The only thing the new patch seems to improves is deleting an active PTK
key without de-authentication which seems to have no valid use cases.
And it makes KEY_FLAG_TAINTED more logical by also stopping Tx for an
outgoing key and not only when we replace it.
I'll probably still send it as an RFC patch to sort out if we still want
that or not.
Alexander
