On 27/08/2019 14:34, Johannes Berg wrote: > Hi Brendan, > >> >> + if (nla_put(msg, NL80211_ATTR_FRAME, len, buf)) >> >> + goto fail; >> > One of the reasons to have this path was to enable/disable encryption on >> > this particular frame, IIRC. Shouldn't >> > NL80211_ATTR_CONTROL_PORT_NO_ENCRYPT get set here at least in some >> > cases? >> >> Well actually my goal is strictly to _disable_ encryption for this frame. > Right, I think you mentioned that. > >> In our >> driver we currently have a double-awkward workaround for the key 802.11 key >> installation flaw: we explicitly disable encyption for the initial handshake, to >> avoid the MAC inadvertently encrypting handshake packets before the peer has >> installed the PTK... but for rekeying handshakes we then have to _stop_ >> explicitly disabling encryption because if we send unencapsulated packets during >> an RSNA they will have no packet number. Handling that properly would require >> more workarounds in the GCMP replay detection code on the RX side. But now that >> we are encrypting handshake packets we are back to having the race condition: >> now we can end up inadvertently encrypting frames with a key that the peer has >> not yet installed. > It's not clear to me what this change gives you though. At least in > theory, nothing stops you from detecting that it's an EAPOL packet, and > then stopping the queues and all in a similar fashion to what you'd do > when you get the EAPOL packet through nl80211. We don't flush the queues when we get the EAPOL packet, we flush the queue when we get the .add_key cfg80211 call, so we can be sure that the EAPOL packet has already been encapsulated by the MAC before we install the new key. We can already do this ring flush in .add_key, but we don't really know for certain that the handshake packet has reached the driver's ring yet (at least, I do not understand the kernel's network stack well enough to convince myself that it must have). With .tx_control_port, we know for sure that it has. We could do some insane hacks with existing information, like "we've seen an EAPOL frame of type XYZ, so we know an .add_key call should be on the way (or maybe we've already had it, because the frame got delayed somehow in the kernel network stack?) so we can somehow order the actioning of the .add_key vs the transmission of the EAPOL" but I hope you'll agree with my characterisation as "insane" :D > One of the things we did want to have here was the ability to *control* > whether the frames were encrypted, there are some protocols (WAPI?) that > require encryption, while the normal case (WPA/RSN) require _no_ > encryption. Hence my comment regarding being able to control it in the > supplicant, although I guess since it doesn't actually support any paths > that want encryption it doesn't matter for now (and for upstream). > >> The goal of this patch is ultimately to let the driver know that when it is >> asked to install a new PTK to the MAC, the handshake frames have already entered >> its TX ring. So if we flush the rings before installing a key, we avoid the race >> condition. > So if I understand correctly: you want the EAPOL over nl80211 so you > don't have a race between sending the EAPOL and installing the key? I > guess that makes more sense now. Yes, or more exactly so it's _possible_ for the driver to avoid the race (by flushing the TX rings before installing the key in cfg80211 .add_key, because it now knows for sure any handshake frames have already gone into the ring). >> On the other hand, this now makes me worry that there could be other drivers out >> there that implement .tx_control_port, but do not take advantage of it to avoid >> the key installation race... in that case this patch could break handshakes for >> those drivers. On the other-other hand I don't know why you'd implement >> .tx_control_port except for this very reason. > I'm not sure how it'd break anything, it'd just behave as before? But > this is new, nobody really implements it yet. So I don't think we have > to worry about that even if I'm not sure I understand your concern. Well my fear was that there could be a driver that relies on something like: if (skb->protocol == htons(ETH_P_PAE)) /* explicitly disable encryption for this frame */ in its .ndo_start_xmit, to avoid the race condition vs key installation. But then it implements .tx_control_port and respects the noencrypt flag (but doesn't flush its TX queues in .add_key). So this hostap patch would end up changing that driver's behaviour because it would set noencrypt to 0 and the driver would no longer be explicitly disabling encryption for the frame. But yes, I think you're right that we don't need to worry about it, I don't think there's any reason to believe such a driver exists. Hope that's a bit more clear... Brendan _______________________________________________ Hostap mailing list Hostap@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/hostap
