Re: Question on setting key right after the EAPOL 4/4 is sent.

[Arend van Spriel <arend.vanspriel@broadcom.com>]

On 09-06-17 22:18, Ben Greear wrote:
> On 06/09/2017 01:01 PM, Johannes Berg wrote:
>> On Fri, 2017-06-09 at 06:46 -0700, Ben Greear wrote:
>>
>>>> However, the solution is far simpler! Once you have nl80211 PAE
>>>> transport, you can easily even set the key before transmitting the
>>>> packet and simply indicate that this particular packet should _not_
>>>> be encrypted regardless of key presence.
>>>
>>> My ath10k firmware cannot deal with a case like this:
>>>
>>> pkt is enqueued before key is set
>>> key is set
>>> pkt is transmitted (incorrectly)
>>>
>>> This is because of how the tid's header-length variables are set up
>>> and modified when the keys are set, and I don't see any good way to
>>> fix this.
>>
>> That seems awful, and anyway will not work with the mentioned non-IEEE
>> protocols that require not encrypting the rekeying frames even when
>> keys have been set up.
>>
>> I don't know what to tell you here, I think it'd be best if you fix
>> that.
> 
> The case that fails is basically any packet that is currently
> enqueued in the firmware when the key is set is transmitted incorrectly
> or not at all.
> And, maybe this is only for DATA tids.
> 
> Otherwise, the no-encrypt logic should work fine.  So, it is just the race
> with the EAPOL 4/4 and set-key that causes issues as far as I can tell.
> 
> It looks like the EAPOL 4/4 and set-key race is fixable without too much
> effort,
> so I think I will focus on that for now instead of further hacking special
> case logic into the firmware.
> 
>>> Stock ath10k firmware goes to great lengths to parse EAPOL frames and
>>> try to work around it in that manner, but that breaks .11r (or used
>>> to, I haven't tried stock firmware lately) and adds more complexity
>>> to the code.
>>
>> It just has to be a single flag saying "don't encrypt this frame" -
>> nothing super complicated about that?
>>
>> In ath10k it looks like HTT_DATA_TX_DESC_FLAGS0_NO_ENCRYPT gets set for
>> this, seems easy enough?
>>
>>>  From a patch someone sent to hostapd list last night, it seems we
>>> could get the tx-status for the EAPOL 4/4, and in that case, we
>>> *know* the pkt has been transmitted, so we can then set the key
>>> safely it would seem?
>>
>> I think so, and I don't remember why we dismissed this solution. Could
>> be that we just decided solving the bridging issue at the same time,
>> while not introducing more latency, was better.
>>
>> Also, the other way can possibly solve some PTK rekeying issues, so
>> overall the solution to go all the way seems better.
> 
> I guess I don't fully understand the PAE thing, but if you all like it,
> then sounds good to me.
> 
> For kernels not supporting this new feature, it seems that having
> supplicant
> wait for tx-status would be a good interim fix?

For what it is worth. In brcmfmac we block setting the PTK when EAPOL
frame is in transit exactly for this long-standing issue. So in short
this is what we do: in .start_xmit() we (atomically) increase pend_1x
count when ether proto is PAE and decrease it when transmit completes
(through brcmf_tx_finalize()). As long as pend_1x count is non-zero we
block setting the PTK.

Regards,
Arend