From:   Alexander Wetzel <alexander@wetzel-home.de>
Subject: Re: [RFC PATCH v3 03/12] mac80211: IEEE 802.11 Extended Key ID
 support
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless@vger.kernel.org
References: <20190210210620.31181-1-alexander@wetzel-home.de>
 <20190210210620.31181-4-alexander@wetzel-home.de>
 <f4d8db23bc3804fdc98f2ce7a702215bd82fd4e3.camel@sipsolutions.net>
Message-ID: <e27bffe8-4c75-34d9-b326-39ffb05b73eb@wetzel-home.de>
Date:   Tue, 19 Feb 2019 21:58:54 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.5.0
MIME-Version: 1.0
In-Reply-To: <f4d8db23bc3804fdc98f2ce7a702215bd82fd4e3.camel@sipsolutions.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

>>
>>   - 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.

> 
>> +	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