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

[Johannes Berg]

Getting back to this ... as I was preparing my patch.

> @@ -3687,6 +3692,9 @@ enum nl80211_key_attributes {
>   NL80211_KEY_DEFAULT_MGMT,
>   NL80211_KEY_TYPE,
>   NL80211_KEY_DEFAULT_TYPES,
> + NL80211_KEY_REPLAY_CTR,
> + NL80211_KEY_KCK,
> + NL80211_KEY_KEK,

You made those key attributes, but ...
 
>    nla_put(msg, NL80211_ATTR_RESP_IE, resp_ie_len,
> resp_ie)))
>   goto nla_put_failure;
>  
> + if (wiphy_ext_feature_isset(>wiphy,
> + NL80211_EXT_FEATURE_KEY_MGMT_OFF
> LOAD) &&
> + (nla_put_u8(msg, NL80211_ATTR_AUTHORIZED, authorized) ||
> + (key_replay_ctr && nla_put(msg, NL80211_KEY_REPLAY_CTR,
> +  NL80211_REPLAY_CTR_LEN, key_replay_ctr)) ||
> + (key_kck &&
> +  nla_put(msg, NL80211_KEY_KCK, NL80211_KCK_LEN,
> key_kck)) ||
> + (key_kek &&
> +  nla_put(msg, NL80211_KEY_KEK, NL80211_KEK_LEN,
> key_kek
> + goto nla_put_failure;

Used them at a top level here! That can't possibly have worked.

Anyway, I checked and we can transport these without adding new
attributes, but adding the NL80211_ATTR_REKEY_DATA attribute with its
nested KEK, KCK and REPLAY_CTR.

That leaves the authorized attribute, I guess nesting a whole bunch of
station info etc. doesn't make a lot of sense.

I also fail to see how the data is actually configured down, since you
just pass it through. I'll send our patch for configuring the PMK/PSK
via the PMKSA cache separately in a few minutes.

johannes

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

[Johannes Berg]

On Fri, 2016-10-14 at 16:52 +0300, Jouni Malinen wrote:
> On Tue, Sep 27, 2016 at 02:36:15PM +0200, Johannes Berg wrote:
> > 
> > > 
> > > + * @NL80211_ATTR_AUTHORIZED: flag attribute, if set indicates
> > > that the
> > > + *  connection is authorized.
> > > @@ -2267,6 +2270,8 @@ enum nl80211_attrs {
> > > + NL80211_ATTR_AUTHORIZED,
> > 
> > This already exists, no?
> > 
> > NL80211_STA_FLAG_AUTHORIZED should be more or less equivalent, if
> > you do it per station (or just for the AP in case of managed
> > connection)
> 
> It does indeed have a very similar meaning to the proposed
> NL80211_ATTR_AUTHORIZED. However, NL80211_STA_FLAG_AUTHORIZED is
> something that gets nested in NL80211_ATTR_STA or used with the
> mask/set struct in NL80211_ATTR_STA_FLAGS2. I'm not sure either of
> those would really be very clean ways to use with a connection/roam
> event..

Oh, right, this is used in the event, not for control...

I guess that makes sense then, although it should be a flag attribute
instead of a u8?

We could still put a nested NL80211_ATTR_STA, but I agree that seems
awkward.

> PMKSA caching cases, FT protocol, and 4-way handshake following EAP
> might be doable in this manner and that might indeed be the cleanest
> approach there. 

Ok

> However, there will also be need to cover possibility
> for offloading FILS at some point in the future.. 

Yeah, I hadn't considered FILS.

> For FILS with shared key, the configuration will actually include ERP
> keys that are not bound to any specific Authenticator/AP/BSSID and do
> not really have a PMKSA cache entry.. At latest at that point, I'd
> think we'll end up needing something else and that something else
> could be defined already now to cover all these cases instead of
> trying to force the current cases to go through
> NL80211_CMD_SET_PMKSA.

Could be done, I guess. But don't we then have to be careful to
actually bind the non-FILS keys to the right Authenticator/AP/BSSID,
and then we have to invent a way to bind it? Does that make sense?

> > In particular, with key management offloaded, it's not clear to me
> > what exactly the roles of the device and host are here. I'm
> > considering that the device would handle the 4-way and 2-way
> > handshakes, but then you wouldn't need the KEK/KCK/ReplayCounter in
> > the host, so there wouldn't be much point in giving them to it.
> > But if the device doesn't do that, what exactly *does* it do?
> 
> One of the key uses is to allow the Wi-Fi firmware to complete
> roaming (say, 4-way handshake based on existing PMKSA) when the host
> processor is not awake (i.e., wpa_supplicant is not running at all). 

Ah. So this might not be used when the processor *is* awake? That's a
key point I was missing, perhaps, since we're building something where
it's simply always done by the device.

Why would you want to do it in the processor when you have the ability
to do it in the firmware?

> However, this does not mean that we would necessarily offload all
> EAPOL-Key processing. GTK rekeying and the initial 4-way handshake
> for the first connection to an ESS might be performed by
> wpa_supplicant especially in cases where the host is awake anyway.
> That's why those PTK-related values need to be kept in sync between
> the driver/firmware and host (wpa_supplicant).

Interesting, ok. Whatever the reason, I guess we have to support it
being done that way.

I guess we'll have to hash out the exact details.

I think we can publish a proposal soon that uses the PMKSA cache, but
lacks all the event data since we never see EAPOL-key messages on the
host in that model.

This model here with the temporal key etc. stuff is clearly unworkable.

I'm not sure I've made up my mind on introducing a new mechanism that
covers FILS vs. PMKSA (and then later an only-FILS-style mechanism) -
that's the issue with binding to a BSSID above.

johannes

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

[Jouni Malinen]

On Tue, Sep 27, 2016 at 02:36:15PM +0200, Johannes Berg wrote:
> > + * @NL80211_ATTR_AUTHORIZED: flag attribute, if set indicates that the
> > + *  connection is authorized.
> > @@ -2267,6 +2270,8 @@ enum nl80211_attrs {
> > +   NL80211_ATTR_AUTHORIZED,
> 
> This already exists, no?
> 
> NL80211_STA_FLAG_AUTHORIZED should be more or less equivalent, if you
> do it per station (or just for the AP in case of managed connection)

It does indeed have a very similar meaning to the proposed
NL80211_ATTR_AUTHORIZED. However, NL80211_STA_FLAG_AUTHORIZED is
something that gets nested in NL80211_ATTR_STA or used with the mask/set
struct in NL80211_ATTR_STA_FLAGS2. I'm not sure either of those would
really be very clean ways to use with a connection/roam event..

> > @@ -3687,6 +3692,9 @@ enum nl80211_key_attributes {
> > +   NL80211_KEY_REPLAY_CTR,
> > +   NL80211_KEY_KCK,
> > +   NL80211_KEY_KEK,
> 
> I don't think we should conflate the (P)MK and *TK concepts in nl80211,
> they're both keys, but they're completely separate in terms of expected
> usage.
> 
> 
> Ilan and I looked at this, considering 4-way-HS offload after 1X
> authentication, and think that the more natural API would be to add all
> the necessary data to the PMKSA cache entry. Thus, a PMKSA cache entry
> for a device that does 4-way-handshake offloading would include the PMK
> (or perhaps MSK?), and for FT it would also including the PMK-R0,
> PMKR0Name (and possibly the MDID, or can it be derived?)

PMKSA caching cases, FT protocol, and 4-way handshake following EAP
might be doable in this manner and that might indeed be the cleanest
approach there. However, there will also be need to cover possibility
for offloading FILS at some point in the future.. For FILS with shared
key, the configuration will actually include ERP keys that are not bound
to any specific Authenticator/AP/BSSID and do not really have a PMKSA
cache entry.. At latest at that point, I'd think we'll end up needing
something else and that something else could be defined already now to
cover all these cases instead of trying to force the current cases to go
through NL80211_CMD_SET_PMKSA.


> However, I'm wondering what exactly the offloads here do. Jouni, could
> you also chime in with the QCA (vendor command) design?

The QCA vendor command/event allows multiple different authentication
cases to be offloaded to the driver (well, firmware) and depending on
the driver/firmware version, there could be a bit different behavior
based on whether the particular exchange was offloaded. In other words,
there is automatic fallback to wpa_supplicant completing the exchange if
the driver does not report that it was completed.

> In particular, with key management offloaded, it's not clear to me what
> exactly the roles of the device and host are here. I'm considering that
> the device would handle the 4-way and 2-way handshakes, but then you
> wouldn't need the KEK/KCK/ReplayCounter in the host, so there wouldn't
> be much point in giving them to it.
> But if the device doesn't do that, what exactly *does* it do?

One of the key uses is to allow the Wi-Fi firmware to complete roaming
(say, 4-way handshake based on existing PMKSA) when the host processor
is not awake (i.e., wpa_supplicant is not running at all). However, this
does not mean that we would necessarily offload all EAPOL-Key
processing. GTK rekeying and the initial 4-way handshake for the first
connection to an ESS might be performed by wpa_supplicant especially in
cases where the host is awake anyway. That's why those PTK-related
values need to be kept in sync between the driver/firmware and host
(wpa_supplicant).

-- 
Jouni MalinenPGP id EFC895FA

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

[Johannes Berg]

>  #define WLAN_CIPHER_SUITE_SMS4   0x00147201
> +#define WLAN_CIPHER_SUITE_PMK   0x00147202
> +#define WLAN_CIPHER_SUITE_PMK_R00x00147203
> +#define WLAN_CIPHER_SUITE_PMK_R0_NAME   0x00147204

Err, what? No, things can't work that way. This is the Chinese
company's OUI, you can't just assign it to PMK stuff.

> + * @NL80211_ATTR_AUTHORIZED: flag attribute, if set indicates that the
> + *  connection is authorized.
> + *
>   * @NUM_NL80211_ATTR: total number of nl80211_attrs available
>   * @NL80211_ATTR_MAX: highest attribute number currently defined
>   * @__NL80211_ATTR_AFTER_LAST: internal use
> @@ -2267,6 +2270,8 @@ enum nl80211_attrs {
>  
>   NL80211_ATTR_MESH_PEER_AID,
>  
> + NL80211_ATTR_AUTHORIZED,

This already exists, no?

NL80211_STA_FLAG_AUTHORIZED should be more or less equivalent, if you
do it per station (or just for the AP in case of managed connection)

>   /* add attributes here, update the policy in nl80211.c */
>  
>   __NL80211_ATTR_AFTER_LAST,
> @@ -3687,6 +3692,9 @@ enum nl80211_key_attributes {
>   NL80211_KEY_DEFAULT_MGMT,
>   NL80211_KEY_TYPE,
>   NL80211_KEY_DEFAULT_TYPES,
> + NL80211_KEY_REPLAY_CTR,
> + NL80211_KEY_KCK,
> + NL80211_KEY_KEK,

I don't think we should conflate the (P)MK and *TK concepts in nl80211,
they're both keys, but they're completely separate in terms of expected
usage.


Ilan and I looked at this, considering 4-way-HS offload after 1X
authentication, and think that the more natural API would be to add all
the necessary data to the PMKSA cache entry. Thus, a PMKSA cache entry
for a device that does 4-way-handshake offloading would include the PMK
(or perhaps MSK?), and for FT it would also including the PMK-R0,
PMKR0Name (and possibly the MDID, or can it be derived?)


However, I'm wondering what exactly the offloads here do. Jouni, could
you also chime in with the QCA (vendor command) design?

In particular, with key management offloaded, it's not clear to me what
exactly the roles of the device and host are here. I'm considering that
the device would handle the 4-way and 2-way handshakes, but then you
wouldn't need the KEK/KCK/ReplayCounter in the host, so there wouldn't
be much point in giving them to it.
But if the device doesn't do that, what exactly *does* it do?


Thanks,
johannes

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

[Kalle Valo]

Amitkumar Karwar  writes:

> From: lihz 

A minor thing, but the from header in both of the patches don't have the
full name and the git log would look ugly. It should be something like
this:

From: Huazeng Li 

-- 
Kalle Valo

[PATCH] cfg80211: add key management offload feature

[Amitkumar Karwar]

From: lihz 

This patch adds key management offload feature. It needs to be
advertised through NL80211_EXT_FEATURE_KEY_MGMT_OFFLOAD flag.
Existing cfg80211_roamed API has been extended to report keys
for roaming offload.

Signed-off-by: Huazeng Li 
Signed-off-by: Amitkumar Karwar 
---
 drivers/net/wireless/ath/ath6kl/cfg80211.c |  3 ++-
 .../broadcom/brcm80211/brcmfmac/cfg80211.c |  3 ++-
 drivers/net/wireless/rndis_wlan.c  |  3 ++-
 drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c  |  2 +-
 drivers/staging/wlan-ng/cfg80211.c |  2 +-
 include/linux/ieee80211.h  |  3 +++
 include/net/cfg80211.h |  8 +--
 include/uapi/linux/nl80211.h   | 11 +
 net/wireless/core.h|  8 ++-
 net/wireless/nl80211.c | 19 ++--
 net/wireless/nl80211.h |  4 +++-
 net/wireless/sme.c | 26 +-
 net/wireless/util.c|  4 +++-
 13 files changed, 79 insertions(+), 17 deletions(-)
 mode change 100644 => 100755 net/wireless/nl80211.c

diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c 
b/drivers/net/wireless/ath/ath6kl/cfg80211.c
index b7fe0af..9511f73 100644
--- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
+++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
@@ -809,7 +809,8 @@ void ath6kl_cfg80211_connect_event(struct ath6kl_vif *vif, 
u16 channel,
} else if (vif->sme_state == SME_CONNECTED) {
/* inform roam event to cfg80211 */
cfg80211_roamed_bss(vif->ndev, bss, assoc_req_ie, assoc_req_len,
-   assoc_resp_ie, assoc_resp_len, GFP_KERNEL);
+   assoc_resp_ie, assoc_resp_len, GFP_KERNEL,
+   NULL, NULL, NULL, 0);
}
 }
 
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 748eaa6..5934b77 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -5450,7 +5450,8 @@ done:
kfree(buf);
cfg80211_roamed(ndev, notify_channel, (u8 *)profile->bssid,
conn_info->req_ie, conn_info->req_ie_len,
-   conn_info->resp_ie, conn_info->resp_ie_len, GFP_KERNEL);
+   conn_info->resp_ie, conn_info->resp_ie_len, GFP_KERNEL,
+   NULL, NULL, NULL, 0);
brcmf_dbg(CONN, "Report roaming result\n");
 
set_bit(BRCMF_VIF_STATUS_CONNECTED, >vif->sme_state);
diff --git a/drivers/net/wireless/rndis_wlan.c 
b/drivers/net/wireless/rndis_wlan.c
index 603c904..ad9535f 100644
--- a/drivers/net/wireless/rndis_wlan.c
+++ b/drivers/net/wireless/rndis_wlan.c
@@ -2838,7 +2838,8 @@ static void rndis_wlan_do_link_up_work(struct usbnet 
*usbdev)
cfg80211_roamed(usbdev->net,
get_current_channel(usbdev, NULL),
bssid, req_ie, req_ie_len,
-   resp_ie, resp_ie_len, GFP_KERNEL);
+   resp_ie, resp_ie_len, GFP_KERNEL,
+   NULL, NULL, NULL, 0);
} else if (priv->infra_mode == NDIS_80211_INFRA_ADHOC)
cfg80211_ibss_joined(usbdev->net, bssid,
 get_current_channel(usbdev, NULL),
diff --git a/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c 
b/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c
index d0ba377..e74216a 100644
--- a/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c
+++ b/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c
@@ -341,7 +341,7 @@ void rtw_cfg80211_indicate_connect(struct rtw_adapter 
*padapter)
sizeof(struct ieee80211_hdr_3addr) + 6,
pmlmepriv->assoc_rsp_len -
sizeof(struct ieee80211_hdr_3addr) - 6,
-   GFP_ATOMIC);
+   GFP_ATOMIC, NULL, NULL, NULL, 0);
} else {
cfg80211_connect_result(padapter->pnetdev,
cur_network->network.MacAddress,
diff --git a/drivers/staging/wlan-ng/cfg80211.c 
b/drivers/staging/wlan-ng/cfg80211.c
index f46dfe6..178d955 100644
--- a/drivers/staging/wlan-ng/cfg80211.c
+++ b/drivers/staging/wlan-ng/cfg80211.c
@@ -722,7 +722,7 @@ void prism2_disconnected(wlandevice_t *wlandev)
 void prism2_roamed(wlandevice_t *wlandev)
 {
cfg80211_roamed(wlandev->netdev, NULL, wlandev->bssid,
-   NULL, 0, NULL, 0, GFP_KERNEL);
+   NULL, 0, NULL, 0, GFP_KERNEL, NULL, NULL,