DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CF96D2081C
Subject: Re: [RFC PATCH v2 2/2] mac80211: Add support for Extended Key ID
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless@vger.kernel.org
References: <20181111110235.14213-1-alexander@wetzel-home.de>
 <20181111110235.14213-3-alexander@wetzel-home.de>
 <24665c3e0e7e970eb8cd63dad57e2b0b3097cfcc.camel@sipsolutions.net>
 <d20b43fc-408c-d3fb-75b2-9c2bffd37264@wetzel-home.de>
 <61c9ed7fc16517991cda3c9d930b53c24306234e.camel@sipsolutions.net>
From:   Alexander Wetzel <alexander@wetzel-home.de>
Message-ID: <cb55102c-456c-3aea-44c2-cd9d0ebc3c3e@wetzel-home.de>
Date:   Thu, 6 Dec 2018 17:27:03 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.3.1
MIME-Version: 1.0
In-Reply-To: <61c9ed7fc16517991cda3c9d930b53c24306234e.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

> 
>>>> -	/* 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...
> 
> Sure. But ... not sure how that's related?
> 
>> 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.
> 
> Exactly.
> 
>> And when a driver supports Extended Key ID we
>> don't care about if the driver is able to rekey PTK0 correctly.
> 
> Strictly speaking, that's false, since you don't know if wpa_s actually
> used it, and the peer STA allowed it.
> 
> It's also not what you implemented, you implemented checking if
> NL80211_KEY_RX_ONLY was ever used.
> 
> However, what I'm trying to say is that I'm not sure this makes sense?
> 
> It seems to me it would be safer, and easier (no station flag), to just
> check
> 
>   if ("we're replacing the current TX key")
> 
> and trigger the workarounds in that case. No?
> 
> Yes, parts of the issue also manifest themselves on the RX side, but if
> you're not replacing the current key then you were using extended key ID
> support?
> 
Ah, now I get it:-)
Will try that out also.


>>>> +++ 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.
> 
> Hmm, ok. So that probably wants a big comment saying that it relies on
> key idx 2/3 being invalid. I'm not sure I like the NUM_DEFAULT_KEYS-1,
> better perhaps to do something like
> 
> 	/* comment saying why */
> 	BUILD_BUG_ON(ARRAY_SIZE(sta->ptks) > 2);
> 	sta->ptk_idx = 2;
> 
> or so?
> 
>> ieee80211_tx_h_select_key starts encrypting packets as soon as
>> sta->ptk[tx->sta->ptk_idx] is not null.
> 
> Right, so I guess this makes sense.
> 
> johannes
>Thank you very much for all the helpful tips and suggestions!

Alexander