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
