Hi Johannes,
Thanks a lot for the review :)
On 23/08/2019 17:58, Johannes Berg wrote:
> On Fri, 2019-05-31 at 06:54 +0000, Brendan Jackman wrote:
>> +static int driver_nl80211_tx_control_port(void *priv, const u8 *dest,
>> + u16 proto, const u8 *buf, size_t len)
>> +{
>> + struct i802_bss *bss = priv;
>> + struct nl_msg *msg = NULL;
>> + int ifindex = if_nametoindex(bss->ifname);
>> + int ret = 0;
>> +
>> + wpa_printf(MSG_DEBUG,
>> + "nl80211: tx_control_port "MACSTR" proto=0x%04x len=%zu",
>> + MAC2STR(dest), proto, len);
>> +
>> + msg = nl80211_ifindex_msg(bss->drv, ifindex, 0,
>> + NL80211_CMD_CONTROL_PORT_FRAME);
>> + if (!msg)
>> + return -ENOBUFS;
>> +
>> + if (nla_put_u16(msg, NL80211_ATTR_CONTROL_PORT_ETHERTYPE, proto))
>> + goto fail;
>> + if (nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, dest))
>> + goto fail;
>> + 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. 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. As I mentioned in the original thread, the _proper_ solution
for this is Alexander Wetzel's Extended Key ID support, which means we don't
have to overwrite the key during rekeying. But that is only available on
5.3+. This is an interim solution.
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.
Hope that's clear... been a while since I had all these details paged in!
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.
>> +static int supp_tx_control_port(void *ctx, const u8 *dest, u16 proto,
>> + const u8 *buf, size_t len)
>> +{
>> + struct ibss_rsn_peer *peer = ctx;
>> + struct wpa_supplicant *wpa_s = peer->ibss_rsn->wpa_s;
>> +
>> + wpa_printf(MSG_DEBUG, "SUPP: %s(dest=" MACSTR " proto=0x%04x "
>> + "len=%lu)",
>> + __func__, MAC2STR(dest), proto, (unsigned long) len);
>> +
>> + if (wpa_s->drv_flags & WPA_DRIVER_FLAGS_TX_CONTROL_PORT)
>> + return wpa_drv_tx_contol_port(wpa_s, dest, proto, buf, len);
>
> Typo - "control". Must be in at least two other places too, otherwise it
> wouldn't compile
Oops, well spotted - will fix for v2 (after above discussions is settled).
>
> Otherwise LGTM!
>
Thanks again,
Brendan
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
