From: Alexander Wetzel <alexander@wetzel-home.de>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: linux-wireless@vger.kernel.org
Subject: Re: [RFC PATCH v2 2/2] mac80211: Add support for Extended Key ID
Date: Wed, 5 Dec 2018 22:58:16 +0100 [thread overview]
Message-ID: <d20b43fc-408c-d3fb-75b2-9c2bffd37264@wetzel-home.de> (raw)
In-Reply-To: <24665c3e0e7e970eb8cd63dad57e2b0b3097cfcc.camel@sipsolutions.net>
>> + * @IEEE80211_KEY_FLAG_RX_ONLY: Set by mac80211 to indicate that the key
>> + * must not be used for TX (yet).
>
> I'm not sure that's relevant, since you have one key pointer for TX?
>
>> + * @IEEE80211_KEY_FLAG_SET_TX: Set by mac80211 to indicate that a previously
>> + * installed key with IEEE80211_KEY_FLAG_RX_ONLY should take over TX also.
>
> That also doesn't seem relevant ...
>
> Oh, all of this is for HW offloads?
>
> I _think_ I would prefer to have new key ops instead. Now you'd have
>
> SET_KEY / <empty flags>
> SET_KEY / RX_ONLY
> SET_KEY / SET_TX
>
> but I think maybe
>
> SET_KEY
> SET_KEY_RX_ONLY
> KEY_ENABLE_TX
>
> would make more sense?
Fine for me and should make it more understandable. So I'll try that.
>
>> + if (pairwise && params->flag == NL80211_KEY_SET_TX) {
>> + mutex_lock(&local->sta_mtx);
>> + sta = sta_info_get_bss(sdata, mac_addr);
>> +
>> + if (!sta ||
>> + !(key = rcu_dereference(sta->ptk[key_idx])) ||
>
> indentation here is off by one
>
Thanks.
>> + !(key->conf.flags | IEEE80211_KEY_FLAG_RX_ONLY)) {
>
> that makes no sense, should be & I guess
>
yes, I think that was one of the bugs I fixed the last weeks:-)
>> - /* PTK only using key ID 0 needs special handling on rekey */
>> - if (new_key && sta && ptk0rekey) {
>> + /* PTK rekey without Extended Key ID needs special handling */
>> + if (new_key && pairwise && sta &&
>> + !test_sta_flag(sta, WLAN_STA_EXT_KEY_ID)) {
>> local = old_key->local;
>> sdata = old_key->sdata;
>
> This seems wrong, even if you have ext key ID support and everything,
> but you do 0 -> 0 rekeying, then you still need all the special handling
> (in fact also then if you go 1->1!). So it seems you'd instead want to
> see if you're going from a TX key to a TX key with the same key ID, and
> then you don't need this flag at all.
>
The intention for Extended Key ID is, to have a comparable short time
frame where both key IDs can be used. When replacing e.g. key ID 0 again
it should be idle for a long time. I guess if someone starts re-keying
in 1s intervals it may become an issue, but then anyone re-keying that
often can't be helped...
With Extended Key IDs it's impossible to directly switch from a TX key
with one key ID to another one with the same id.
1) Association
2) key ID 0 installed RX only
3) key Id 0 set_tx
4) rekey timeout passes
5) key ID 1 installed RX only
6) key ID 1 set_tx (also making key ID 0 RX only)
7) rekey timeout passes
8) key ID 0 replaced with new RX only key
9) key ID 0 set_tx
10) rekey timeout passes
...
So nobody will use the key being replaced, we don't have to protect
against PN poisoning. And when a driver supports Extended Key ID we
don't care about if the driver is able to rekey PTK0 correctly.
>> +++ b/net/mac80211/sta_info.c
>> @@ -350,6 +350,7 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
>> sta->sta.max_rx_aggregation_subframes =
>> local->hw.max_rx_aggregation_subframes;
>>
>> + sta->ptk_idx = NUM_DEFAULT_KEYS - 1;
>
> That makes no sense? Why should it be 3? That's invalid anyway?
Yes, that's the whole reason for that change:-) Setting it to 2 would
also be fine, as long as it's not 0 or 1.
ieee80211_tx_h_select_key starts encrypting packets as soon as
sta->ptk[tx->sta->ptk_idx] is not null.
So installing the first key as RX only will also activate the key for
TX. the AP will therefore encrypt EAPOL #3 of the initial connect...
To avoid expensive run time checks I simply switched the default setting
to make sure sta->ptk[tx->sta->ptk_idx] will be NULL at the initial key
install.
Alexander
next prev parent reply other threads:[~2018-12-05 22:05 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-11 11:02 [RFC PATCH v2 0/2] Extended Key ID support for linux Alexander Wetzel
2018-11-11 11:02 ` [RFC PATCH v2 1/2] nl80211/cfg80211: Add support for Extended Key ID Alexander Wetzel
2018-12-05 14:51 ` Johannes Berg
2018-12-05 20:54 ` Alexander Wetzel
2018-12-06 7:25 ` Johannes Berg
2018-12-06 16:21 ` Alexander Wetzel
2018-11-11 11:02 ` [RFC PATCH v2 2/2] mac80211: " Alexander Wetzel
2018-12-05 14:58 ` Johannes Berg
2018-12-05 21:58 ` Alexander Wetzel [this message]
2018-12-06 7:32 ` Johannes Berg
2018-12-06 16:27 ` Alexander Wetzel
2018-12-05 14:42 ` [RFC PATCH v2 0/2] Extended Key ID support for linux Johannes Berg
2018-12-05 19:06 ` Alexander Wetzel
2018-12-07 10:01 ` Jouni Malinen
2018-12-08 13:58 ` Alexander Wetzel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d20b43fc-408c-d3fb-75b2-9c2bffd37264@wetzel-home.de \
--to=alexander@wetzel-home.de \
--cc=johannes@sipsolutions.net \
--cc=linux-wireless@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.