Re: [PATCH] nl80211: add key management offload feature

[Jouni Malinen <j@w1.fi>]

On Tue, Sep 27, 2016 at 01:24:24PM +0200, Arend Van Spriel wrote:
> On 27-9-2016 12:56, Amitkumar Karwar wrote:
> > From: lihz <lihz@marvell.com>
> 
> minor thing. Could you use another prefix iso 'nl80211:'. That has
> different expectation for me at least, ie. changes in nl80211 api, but
> this patch is for hostap repo so 'hostap:' or 'wpa_supp:' would be
> better fit here.

Well.. That's for the context of linux-wireless. As far as the actual
commit in hostap.git and the hostap mailing list (now with the correct
address) is concerned, "nl80211:" is the correct prefix to use in the
commit message.

> > diff --git a/src/common/defs.h b/src/common/defs.h
> > @@ -148,7 +148,9 @@ enum wpa_alg {
> > -	WPA_ALG_BIP_CMAC_256
> > +	WPA_ALG_BIP_CMAC_256,
> > +	WPA_ALG_PMK_R0,
> > +	WPA_ALG_PMK_R0_NAME,

I guess I could kind of understand WPA_ALG_PMK_R0 as a new "algorithm"
since this is also used to configure keys, but PMK-R0-Name is going
pretty far in that regard. It most certainly is not a key..

> >  #define WLAN_CIPHER_SUITE_SMS4		0x00147201
> > +#define WLAN_CIPHER_SUITE_PMK		0x00147202
> > +#define WLAN_CIPHER_SUITE_PMK_R0	0x00147203
> > +#define WLAN_CIPHER_SUITE_PMK_R0_NAME	0x00147204

As noted previously, it is not acceptable to assign new AKMs from
someone else's OUI. Once there is consensus on what values are needed, I
can assign the needed values from the 00:13:74 OUI.

> > diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
> > @@ -2675,21 +2675,34 @@ static int wpa_driver_nl80211_set_key(const char *ifname, struct i802_bss *bss,
> > -#ifdef CONFIG_DRIVER_NL80211_QCA
> > -	if (alg == WPA_ALG_PMK &&
> > -	    (drv->capa.flags & WPA_DRIVER_FLAGS_KEY_MGMT_OFFLOAD)) {
> > -		wpa_printf(MSG_DEBUG, "%s: calling issue_key_mgmt_set_key",
> > -			   __func__);
> > -		ret = issue_key_mgmt_set_key(drv, key, key_len);
> > -		return ret;
> > +
> > +	if ((alg == WPA_ALG_PMK || alg == WPA_ALG_PMK_R0 ||
> > +	     alg == WPA_ALG_PMK_R0_NAME) &&

I understand PMK as a new key that is being configured. For FT, I'm not
completely sure about PMK-R0 as a separate algorithm and especially not
about using this interface for setting PMK-R0-Name which is tightly
coupled name with a specific PMK-R0 and not something that one would
configure separately.

> > diff --git a/src/drivers/driver_nl80211_event.c b/src/drivers/driver_nl80211_event.c
> > @@ -2065,18 +2065,6 @@ static void do_process_drv_event(struct i802_bss *bss, int cmd,
> >  	wpa_printf(MSG_DEBUG, "nl80211: Drv Event %d (%s) received for %s",
> >  		   cmd, nl80211_command_to_string(cmd), bss->ifname);
> >  
> > -	if (cmd == NL80211_CMD_ROAM &&
> > -	    (drv->capa.flags & WPA_DRIVER_FLAGS_KEY_MGMT_OFFLOAD)) {
> > -		/*
> > -		 * Device will use roam+auth vendor event to indicate
> > -		 * roaming, so ignore the regular roam event.
> > -		 */
> > -		wpa_printf(MSG_DEBUG,
> > -			   "nl80211: Ignore roam event (cmd=%d), device will use vendor event roam+auth",
> > -			   cmd);
> > -		return;
> > -	}

It is not going to be acceptable to break the existing mechanism that
uses QCA vendor specific commands/events. In other words, the new
extensions need to be done in a backwards compatible manner that allow
both to work based on driver capabilities.

> > diff --git a/src/rsn_supp/wpa_ft.c b/src/rsn_supp/wpa_ft.c
> > @@ -37,6 +37,10 @@ int wpa_derive_ptk_ft(struct wpa_sm *sm, const unsigned char *src_addr,
> > +	wpa_sm_set_key(sm, WPA_ALG_PMK_R0, NULL, 0, 1, NULL,
> > +		       0, sm->pmk_r0, PMK_LEN);
> > +	wpa_sm_set_key(sm, WPA_ALG_PMK_R0_NAME, NULL, 0, 1, NULL,
> > +		       0, sm->pmk_r0_name, WPA_PMK_NAME_LEN);

This looks quite bad. I don't think I can really support two separate
nl80211 commands to set a PMK-R0 and the matching PMK-R0-Name, i.e.,
this should really be a single (atomic) operation.

-- 
Jouni Malinen                                            PGP id EFC895FA