On Thu, Feb 27, 2020 at 10:52:46PM +0100, Alexander Wetzel wrote:
> Stop using set_tx and cleanup/restructure the key install logic
> depending on it.
>
> The updated logic is also no longer incorrectly installing some pairwise
> keys as default keys and has additional sanity checks refusing
> unexpected keys.
Regarding the changes on setting the default pairwise key.. Isn't that
preventing some use cases? For example, in IEEE 802.1X with dynamic WEP
keys, the Authenticator could configure two different unicast keys with
different Key IDs and then swap which key is used for transmission. And
wouldn't this apply for the new PTK rekeying case with two Key ID values
as well?
In general, it would probably make sense to split patch into quite a few
smaller patches that each address a clear individual change on their own
at least for the clear independent fixes. This patch was a bit difficult
to review and I could not come to conclusion that all of it is fine even
though most places were clear.
> diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
> + } else {
> wpa_printf(MSG_DEBUG, " broadcast key");
> -
> - types = nla_nest_start(key_msg, NL80211_KEY_DEFAULT_TYPES);
> - if (!types ||
> - nla_put_flag(key_msg, NL80211_KEY_DEFAULT_TYPE_MULTICAST))
> - goto fail;
> - nla_nest_end(key_msg, types);
> }
So I take this removal here is because that attribute is not really used
in the NL80211_CMD_NEW_KEY case.
> @@ -3197,19 +3206,19 @@ static int wpa_driver_nl80211_set_key(struct i802_bss *bss,
> if (addr && is_broadcast_ether_addr(addr)) {
> struct nlattr *types;
>
> + wpa_printf(MSG_DEBUG, " group key");
> types = nla_nest_start(key_msg, NL80211_KEY_DEFAULT_TYPES);
> if (!types ||
> nla_put_flag(key_msg, NL80211_KEY_DEFAULT_TYPE_MULTICAST))
> goto fail;
> nla_nest_end(key_msg, types);
And this is where we have that attribute for NL80211_CMD_SET_KEY when
selecting which one of the broadcast keys to use for transmission.
> } else if (addr) {
> - struct nlattr *types;
> -
> - types = nla_nest_start(key_msg, NL80211_KEY_DEFAULT_TYPES);
> - if (!types ||
> - nla_put_flag(key_msg, NL80211_KEY_DEFAULT_TYPE_UNICAST))
> - goto fail;
> - nla_nest_end(key_msg, types);
> + wpa_printf(MSG_DEBUG,
> + "nl80211: Default group key can't use a unicast address");
> + ret = -EINVAL;
> + goto fail;
But this is the case that would be used for changing which of the
multiple unicast keys is used for transmission. Why would this not be an
acceptable operation?
> @@ -3226,8 +3235,6 @@ static int wpa_driver_nl80211_set_key(struct i802_bss *bss,
> }
>
> ret = send_and_recv_msgs(drv, msg, NULL, NULL);
> - if (ret == -ENOENT)
> - ret = 0;
I'd assume this is because the error cannot happen anymore with the
error cases removed, but that was not obvious from the full context of
this patch. Splitting the NL80211_CMD_SET_KEY changes to a separate
patch with the commit message highlighting that detail would be helpful
for understanding this.
--
Jouni Malinen PGP id EFC895FA
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
