From: Alexander Wetzel <alexander@wetzel-home.de>
To: Johannes Berg <johannes@sipsolutions.net>
Cc: linux-wireless@vger.kernel.org
Subject: Re: [RFC PATCH v3 03/12] mac80211: IEEE 802.11 Extended Key ID support
Date: Thu, 21 Feb 2019 20:47:23 +0100 [thread overview]
Message-ID: <171d2db4-7639-4738-2a6d-c899f0247aee@wetzel-home.de> (raw)
In-Reply-To: <e27bffe8-4c75-34d9-b326-39ffb05b73eb@wetzel-home.de>
>>>
>>> - Enforce cipher does not change when replacing a key.
>>
>> is that actually required somehow?
>
> The code is silently assuming a rekey is using the same cipher. Someone
> e.g. switching from WEP to CCMP with a rekey would pass all sanity
> checks and allow to use the code in a way never intended or tested.
> With the current handling the userspace e.g. should be able to install a
> WEB key using keyid 3 and then rekey it with a CCMP key, claiming keyid
> 0 bit mac80211 will copy the keyid from the old and use keyid 3.
> Something not possible otherwise.
>
> That said I do not see how this could be exploited, I simply try to
> enforce all assumptions to be on the safe side. (I did not dig deeper
> into potential exploits after finding out how keyids are used during
> rekey.)
>
>
>>> + * @EXT_SET_KEY: a new key must be set but is only valid for decryption
>>> + * @EXT_KEY_RX_TX: a key installed with @EXT_SET_KEY is becoming the
>>> + * designated Rx/Tx key for the station
>>
>> Not sure I like the EXT_SET_KEY. There's also no "designated Rx key", is
>> there? It's always selected by key ID.
>>
>> How about SET_KEY_RXONLY and SET_KEY_TX or something like that?
>>
>
> First, you are of course right about the designated Rx key. I'll update
> that.
>
> Second, I spend quite some time finding good names for the calls and one
> of the last tweaks to this patch was replacing SET_KEY_RX_ONLY to
> EXT_SET_KEY...
>
> So here the reasoning for why I named them as they are and why I prefer
> the names used in the patch.
> First, many drivers will handle SET_KEY and SET_KEY_RX_ONLY with the
> same code and not differentiate between those at all. Using EXT_as a
> prefix for the "normal" command is therefore a nice way to imply the
> command can only be used with Extended Key ID and still link it to the
> original command.
> But more important for me was the clash between what the command spells
> and what it does in the COMPAT mode: SET_KEY_RX_ONLY would then be used
> to install a TX only key which never can be used by the card for Rx.
> So I decided to rename it to EXT_SET_KEY, just indicating that this
> command adds a new key to the card for Extended Key ID and drop the
> confusing reference to Rx.
>
> I also had a comparable problem with SET_KEY_TX:
> Most cards will already have done what must be done to use a key for Tx
> with the first command. Only ath10k (assuming it could support Extended
> Key ID at all) would really switch Tx with this command.
> All other drivers seem to lookup the hw key ID and use whatever key is
> referenced there. In fact I think most NATIVE drivers won't have to do
> anything here and just can return 0.
> Now COMPAT drivers (normally) will normally need a new special command
> to enable Rx crypto offload for the new key. So we either would have to
> add a new command for COMPAT drivers or accept that SET_KEY_TX is used
> for that, again putting quite some stain an the name.
>
> Using EXT_SET_KEY instead just implies that we are adding a new key for
> Extended Key IDs and it's our first contact with this key.
> EXT_KEY_RX_TX is then the second contact and has to do whatever is
> necessary to switch the added but not yet fully activated key to fully
> activated. That COPMPAT drivers will enable Rx with it while native
> drivers do nothing or really switch Tx with the command.
>
> Long story short: Using SET_KEY_RXONLY and SET_KEY_TX is not wrong, but
> I would rate them more confusing.
>
>>> +static int ieee80211_set_tx_key(struct ieee80211_sub_if_data *sdata,
>>> + const u8 *mac_addr, u8 key_idx)
>>> +{
>>> + struct ieee80211_local *local = sdata->local;
>>> + struct ieee80211_key *key;
>>> + struct sta_info *sta;
>>> + int ret;
>>> +
>>> + if (!wiphy_ext_feature_isset(local->hw.wiphy,
>>> + NL80211_EXT_FEATURE_EXT_KEY_ID))
>>> + return -EINVAL;
>>
>> You set this, wouldn't it make more sense to check EXT_KEY_ID_NATIVE?
>>
>> Or maybe this is because of the next patch?
>
> Exactly. Assuming we merge NATIVE and drop COMPAT that would move down
> to the driver.
My first answer for that on makes no sense, so let me try again:
Exactly. With COMPAT we would have to check for EXT_KEY_ID_NATIVE and
EXT_KEY_ID_COMPAT. Since we have already done that and set
NL80211_EXT_FEATURE_EXT_KEY_ID I check that here.
Assuming we merge NATIVE and drop COMPAT it may make sense to keep the
check here and drop EXT_KEY_ID_NATIVE altogether.
The drivers than can set NL80211_EXT_FEATURE_EXT_KEY_ID directly.
>
>>
>>> + sta = sta_info_get_bss(sdata, mac_addr);
>>> +
>>> + if (!sta)
>>> + return -EINVAL;
>>> +
>>> + if (sta->ptk_idx == key_idx)
>>> + return 0;
>>> +
>>> + mutex_lock(&local->key_mtx);
>>> + key = key_mtx_dereference(local, sta->ptk[key_idx]);
>>> +
>>> + if (key && key->flags & KEY_FLAG_RX_ONLY)
>>
>> do you even need the flag? Isn't it equivalent to checking
>> sta->ptk_idx != key->idx
>> or so?
>>
>> Less data to maintain would be better.
>
> I have to look at that again. It will change some assumptions for sure
> but still could work out with some slight differences. I'll have to look
> deeper into that since I remember two moments where I was sure needing
> the flag. That may well be outdated, but at a first glance it would at
> least open the door to first install two key in legacy mode and then
> switch between them. (Which should be no problem, of course)
> I'll follow up on that separately, but that may take some time. When it
> works you'll get a new RFC.
>
>>
>>> + bool ext_native = ieee80211_hw_check(&local->hw,
>>> EXT_KEY_ID_NATIVE);
>>
>> you sort of only need this in the next patch, but I guess it doesn't
>> matter that much
>>
> Ups, yes. Forgot that when I split it in two patches some weeks ago,
>
>>> +int ieee80211_key_activate_tx(struct ieee80211_key *key)
>>> +{
>>> + struct ieee80211_sub_if_data *sdata = key->sdata;
>>> + struct sta_info *sta = key->sta;
>>> + struct ieee80211_local *local = key->local;
>>> + struct ieee80211_key *old;
>>> + int ret;
>>> +
>>> + assert_key_lock(local);
>>> +
>>> + key->flags &= ~KEY_FLAG_RX_ONLY;
>>> +
>>> + if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) ||
>>> + key->conf.flags & (IEEE80211_KEY_FLAG_GENERATE_MMIC |
>>> + IEEE80211_KEY_FLAG_PUT_MIC_SPACE |
>>> + IEEE80211_KEY_FLAG_RESERVE_TAILROOM))
>>> + increment_tailroom_need_count(sdata);
>>> +
>>> + if (key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
>>> + ret = drv_set_key(local, EXT_KEY_RX_TX, sdata,
>>> + &sta->sta, &key->conf);
>>> + if (ret) {
>>> + sdata_err(sdata,
>>> + "failed to activate key for Tx (%d, %pM)\n",
>>> + key->conf.keyidx, sta->sta.addr);
>>> + return ret;
>>
>> You've already cleared the RX_ONLY flag, which gets you inconsistent
>> data now.
>>
>
> I don't think so, it looks ok for me. But the delay_tailroom logic took
> a surprisingly large chunk of time and I should explain how the updated
> logic is intended to work. Maybe I've messed it up somehow and just do
> not see it:
>
> I decided to handle that exact, no shortcuts. Only keys which can be
> used for Tx will be counted for delay tailroom. So when installing a Rx
> only key tailroom_needed will never be increased.
> Prior to enabling Tx with a key the code above drops the RX_ONLY flag
> and then evaluates if we have to call increment_tailroom_need_count. The
> key is then activated for Tx a few lines below, the old key is set to
> RX_ONLY and - assuming the old key also increased the tailroom needed
> counter - decrements it for the old key.
> (And I'm not sure if I should get that working without a dedicated key
> flag, but let's wait and see how that would look like and then discuss it.)
>
> The obvious simplification would be to just skip both steps and neither
> increment nor decrement tailroom needed. Problem with that is, that the
> HW offload decides during key install if the driver can support HW
> offload or not. So it could be that e.g. the old key did use HW
> encryption but the new will not. Handling that would further complicated
> by the fact that a key install also could failand then would have to
> clean up that again.
> So I just update tailroom_needed as soon as possible, allowing to abort
> any time and not worry about all that.
>
>>> + }
>>> + }
>>> +
>>> + old = key_mtx_dereference(local, sta->ptk[sta->ptk_idx]);
>>> + sta->ptk_idx = key->conf.keyidx;
>>
>> but you set this only here.
>>
>>> - /* Stop TX till we are on the new key */
>>> + /* Stop Tx till we are on the new key */
>>
>> Uh, I had to read that three times ... please don't make changes like
>> that? :)
>
> Noted, will drop all of those changes.
>
>>
>>> old_key->flags |= KEY_FLAG_TAINTED;
>>> ieee80211_clear_fast_xmit(sta);
>>> - /* Aggregation sessions during rekey are complicated due to the
>>> + /* Aggregation sessions during rekey are complicated by the
>>
>> similarly here, please don't make drive-by comment wording issues (also,
>> I'm not sure I agree - the old version just treats "complicated" as an
>> adjective, you treat it as a verb, but ultimately doesn't really matter?
>>
>>> #define NUM_DEFAULT_KEYS 4
>>> #define NUM_DEFAULT_MGMT_KEYS 2
>>> +#define INVALID_PTK_KEYIDX 2 /* Existing key slot never used by PTK
>>> keys */
>>
>> We could also use something obviously wrong like 0xff?
>
> No, not without some undesired modifications. We actually fetch the
> referenced key and the key slot must exist and be NULL. We (mostly)
> discussed that in the previous RFC, I just decided to use a define
> instead the numeric value. (Mostly due the fact that A-MPDU also needs
> an "invalid" ID and using the same looks like a good idea.
>
> Here how we use this #define in sta_info.c
>
> /* Extended Key ID can install keys for keyid 0 and 1 as Rx only.
> * Tx starts uses a key as soon as a key is installed in the slot
> * ptk_idx references to. To avoid using the initial Rx key prematurely
> * for Tx we initialize ptk_idx to a value never used, making sure the
> * referenced key is always NULL till ptk_idx is set to a valid value.
> */
> BUILD_BUG_ON(ARRAY_SIZE(sta->ptk) <= INVALID_PTK_KEYIDX);
> sta->ptk_idx = INVALID_PTK_KEYIDX;
> sta->ptk_idx_next = INVALID_PTK_KEYIDX;
>
>>
>>> +++ b/net/mac80211/tx.c
>>> @@ -3000,23 +3000,15 @@ void ieee80211_check_fast_xmit(struct
>>> sta_info *sta)
>>> switch (build.key->conf.cipher) {
>>> case WLAN_CIPHER_SUITE_CCMP:
>>> case WLAN_CIPHER_SUITE_CCMP_256:
>>> - /* add fixed key ID */
>>> - if (gen_iv) {
>>> - (build.hdr + build.hdr_len)[3] =
>>> - 0x20 | (build.key->conf.keyidx << 6);
>>> + if (gen_iv)
>>> build.pn_offs = build.hdr_len;
>>> - }
>>> if (gen_iv || iv_spc)
>>> build.hdr_len += IEEE80211_CCMP_HDR_LEN;
>>> break;
>>> case WLAN_CIPHER_SUITE_GCMP:
>>> case WLAN_CIPHER_SUITE_GCMP_256:
>>> - /* add fixed key ID */
>>> - if (gen_iv) {
>>> - (build.hdr + build.hdr_len)[3] =
>>> - 0x20 | (build.key->conf.keyidx << 6);
>>> + if (gen_iv)
>>> build.pn_offs = build.hdr_len;
>>> - }
>>> if (gen_iv || iv_spc)
>>> build.hdr_len += IEEE80211_GCMP_HDR_LEN;
>>> break;
>>> @@ -3383,6 +3375,7 @@ static void ieee80211_xmit_fast_finish(struct
>>> ieee80211_sub_if_data *sdata,
>>> pn = atomic64_inc_return(&key->conf.tx_pn);
>>> crypto_hdr[0] = pn;
>>> crypto_hdr[1] = pn >> 8;
>>> + crypto_hdr[3] = 0x20 | (key->conf.keyidx << 6);
>>> crypto_hdr[4] = pn >> 16;
>>> crypto_hdr[5] = pn >> 24;
>>> crypto_hdr[6] = pn >> 32;
>>
>> This shouldn't be needed, you do update the fast TX cache when changing
>> the key?
>
> That's only right for the push path but can send out wrong packets when
> the driver is using the pull path:
>
> 1) ieee80211_xmit_fast() will use fast_tx structure to fill in the
> "cached" keyid and queue the packet. (let's say 0)
>
> 2) ieee80211_check_fast_xmit is called due to a rekey (and keyid change
> from 0 -> 1)
>
> 3) ieee80211_tx_dequeue() will then dequeue the prepared skb from 1),
> refresh the key information but keep keyid 0 in the skb and instruct the
> driver to encrypt it for keyid 1 -> WRONG
>
> 4) The remote sta tries to decrypt the packet using the key 0, as
> referenced by the keyid. Which will of course not work.
>
> With Extended Key ID (and some debugging) I added a simple rule: When
> you assign the pn you also set the matching keyid.
>
> Alexander
next prev parent reply other threads:[~2019-02-21 19:55 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-10 21:06 [RFC PATCH v3 00/12] Draft for Extended Key ID support Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 01/12] mac80211: Optimize tailroom_needed update checks Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 02/12] nl80211/cfg80211: Extended Key ID support Alexander Wetzel
2019-02-15 10:52 ` Johannes Berg
2019-02-17 19:19 ` Alexander Wetzel
2019-02-22 8:30 ` Johannes Berg
2019-02-22 23:04 ` Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 03/12] mac80211: IEEE 802.11 " Alexander Wetzel
2019-02-15 11:06 ` Johannes Berg
2019-02-19 20:58 ` Alexander Wetzel
2019-02-21 19:47 ` Alexander Wetzel [this message]
2019-02-22 8:41 ` Johannes Berg
2019-02-23 21:02 ` Alexander Wetzel
2019-03-01 20:43 ` Alexander Wetzel
2019-02-23 17:26 ` Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 04/12] mac80211: Compatibility " Alexander Wetzel
2019-02-15 11:09 ` Johannes Berg
2019-02-21 20:07 ` Alexander Wetzel
2019-02-22 8:53 ` Johannes Berg
2019-02-23 22:50 ` Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 05/12] mac80211: Mark A-MPDU keyid borders for drivers Alexander Wetzel
2019-02-15 11:50 ` Johannes Berg
2019-02-21 21:20 ` Alexander Wetzel
2019-02-22 8:51 ` Johannes Berg
2019-02-23 21:47 ` Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 06/12] mac80211_hwsim: Ext Key ID support (NATIVE) Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 07/12] iwlwifi: Extended " Alexander Wetzel
2019-02-15 11:52 ` Johannes Berg
2019-02-22 20:50 ` Alexander Wetzel
2019-02-22 21:06 ` Johannes Berg
2019-02-24 13:04 ` Alexander Wetzel
2019-04-08 20:10 ` Johannes Berg
2019-04-10 20:46 ` Alexander Wetzel
2019-04-12 9:51 ` Johannes Berg
2019-04-14 16:12 ` Alexander Wetzel
2019-04-15 8:44 ` Johannes Berg
2019-04-15 20:09 ` Alexander Wetzel
2019-04-16 9:31 ` Johannes Berg
2019-04-16 18:28 ` Alexander Wetzel
2019-04-16 19:11 ` Johannes Berg
2019-04-16 21:32 ` Alexander Wetzel
2019-04-12 11:19 ` Johannes Berg
2019-02-10 21:06 ` [RFC PATCH v3 08/12] iwlwifi: dvm - EXT_KEY_ID A-MPDU API update Alexander Wetzel
2019-02-15 11:54 ` Johannes Berg
2019-02-22 21:15 ` Alexander Wetzel
2019-02-22 21:20 ` Johannes Berg
2019-02-10 21:06 ` [RFC PATCH v3 09/12] ath: Basic Extended Key ID support (COMPAT+NATIVE) Alexander Wetzel
2019-02-13 11:05 ` Kalle Valo
2019-02-13 23:15 ` Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 10/12] ath5k: ath_key_config() API compatibility update Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 11/12] ath9k: Extended Key ID support (COMPAT) Alexander Wetzel
2019-02-10 21:06 ` [RFC PATCH v3 12/12] ath9k: EXT_KEY_ID A-MPDU API update Alexander Wetzel
2019-02-15 11:10 ` [RFC PATCH v3 00/12] Draft for Extended Key ID support Johannes Berg
2019-02-21 20:44 ` 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=171d2db4-7639-4738-2a6d-c899f0247aee@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).