Hi Jouni, Thanks for the review suggestions. >SAE offload for STA mode was added in an earlier patch: >https://w1.fi/cgit/hostap/commit/?id=c3b8452e0e04038349fbfe757639ff3b377e98da >https://w1.fi/cgit/hostap/commit/?id=236c0cfbcdf7459be1727afb7eb540c4ee081cbc >So that part would need some small changes here. We will sync the SAE offload changes based on top of the branch. >As far as the AP mode case is concerned, does this work only if both SAE >and the following 4-way handshake are offloaded? Or would there be >possibility for offload SAE and performing 4-way handshake from hostapd >(with the PMK from the driver provided to user space)? It would be good f>or the commit message to be clear on this detail. This is applicable only for drivers supporting 4way HS offload. We will update the description accordingly. As you suggested we will split and have separate patch for 4way HS offload and SAE offload. >There is no "SAE passphrase". It is "SAE password". Please note that >hostapd supports multiple SAE passwords and selection of which one to >use based on either STA MAC address or SAE password identifier. Even if >the SAE-in-driver case would not support those options, the >implementation hostapd would need to check for these being enabled in >the configuration and if so, refuse to try to configure SAE offload that >won't work in a way that was configured. >By default, this should use hapd->conf->sae_passwords and fall back to >hapd->conf->ssid.wpa_passphrase only if no SAE-specific password is >configured. We will address as per your suggestion. >That is not supposed to done in wpa_auth_ie.c, i.e., the WPA >authenticator functionality is supposed to be generic that does not >depend on hostapd-specific implementation. >In other words, this direct hostapd_wpa_auth_is_sae_offload_enabled() >call is expected to be done using struct wpa_auth_callbacks instead to >avoid direct dependency on hostapd-specific wpa_auth_glue.c. Ack. We will add a callback to get the driver capability. >That should be called sae_password. 63 is not the correct length limit >for this. Why would this even need to be a fixed length array instead of >being const pointer to the password in some upper layer struct owned by >the caller? Ack we will address this. >This should be in a separate patch. Ack we will be updating separate patch for PSK offload. >This is already included in hostap.git. Ack, thanks. >And this would be in the AP mode SAE offload patch. Ack. >This should now really be documented since the previous uses of struct >port_authorized were for STA mode while this last one is for AP mode and >such a mix of parameters is quite confusing without clear documentation. We will update the documentation. >This is in incorrect function (i.e., this should be only for CONNECT, >not for ASSOCIATE). Anyway, the STA case is already covered, so no need >for this anymore. Ack. >Is this use for NL80211_CMD_PORT_AUTHORIZED for AP mode documented >somewhere? nl80211.h does not seem to describe such use case at all. The >documentation implies this even to be for STA mode only even if >interpreted more widely to cover AP mode, it would be only for 802.1X >FT. The commit below adds support for nl80211_send_port_authorized() to be used for both STA/GC and AP/GO. https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git/commit/?h=for-next&id=e4e7e3af73694380f0d9a742d13b80598a3393e9 Regards, Vinayak On Tue, Nov 7, 2023 at 2:56 PM Jouni Malinen <j@xxxxx> wrote: > > On Thu, Oct 12, 2023 at 02:59:29PM +0530, Vinayak Yadawad wrote: > > In the current change we enable support for SAE offload by > > 1. Adding support of 4-way HS offload for AP interface > > 2. Adding support for SAE authentication offload > > The changes basically involve handling of necessary parameter > > plumbing to the driver in the connect path for STA interface. > > Also the changes involve AP parameter plumbing in AP bringup > > path and Port authorized event handling. > > SAE offload for STA mode was added in an earlier patch: > https://w1.fi/cgit/hostap/commit/?id=c3b8452e0e04038349fbfe757639ff3b377e98da > https://w1.fi/cgit/hostap/commit/?id=236c0cfbcdf7459be1727afb7eb540c4ee081cbc > > So that part would need some small changes here. > > As far as the AP mode case is concerned, does this work only if both SAE > and the following 4-way handshake are offloaded? Or would there be > possibility for offload SAE and performing 4-way handshake from hostapd > (with the PMK from the driver provided to user space)? It would be good > for the commit message to be clear on this detail. > > > diff --git a/src/ap/beacon.c b/src/ap/beacon.c > > @@ -2011,6 +2012,36 @@ int ieee802_11_build_ap_params(struct hostapd_data *hapd, > > + /* If SAE offload is enabled, provide passphrase to lower layer for > > + * PMK generation > > + */ > > + if ((wpa_key_mgmt_sae(hapd->conf->wpa_key_mgmt)) && > > + (hapd->iface->drv_flags2 & WPA_DRIVER_FLAGS2_SAE_OFFLOAD_AP)) { > > + if (hapd->conf->ssid.wpa_passphrase) { > > + params->sae_passphrase_len = > > + os_strlen(hapd->conf->ssid.wpa_passphrase); > > + if (params->sae_passphrase_len) { > > + os_memcpy(params->sae_passphrase, > > + hapd->conf->ssid.wpa_passphrase, > > + params->sae_passphrase_len); > > + } > > + } > > + if (hapd->conf->sae_pwe) { > > + params->sae_pwe = hapd->conf->sae_pwe; > > + } > > + } > > There is no "SAE passphrase". It is "SAE password". Please note that > hostapd supports multiple SAE passwords and selection of which one to > use based on either STA MAC address or SAE password identifier. Even if > the SAE-in-driver case would not support those options, the > implementation hostapd would need to check for these being enabled in > the configuration and if so, refuse to try to configure SAE offload that > won't work in a way that was configured. > > By default, this should use hapd->conf->sae_passwords and fall back to > hapd->conf->ssid.wpa_passphrase only if no SAE-specific password is > configured. > > > + /* If key mgmt offload is enabled, configure PSK */ > > + if ((hapd->conf->wpa_key_mgmt & WPA_KEY_MGMT_PSK) && > > + (hapd->iface->drv_flags2 & WPA_DRIVER_FLAGS2_4WAY_HANDSHAKE_AP_PSK)) { > > + if (hapd->conf->ssid.wpa_psk && hapd->conf->ssid.wpa_psk_set) { > > + os_memcpy(params->psk, hapd->conf->ssid.wpa_psk->psk, PMK_LEN); > > + } else if (hapd->conf->ssid.wpa_passphrase) { > > + pbkdf2_sha1(hapd->conf->ssid.wpa_passphrase, hapd->conf->ssid.ssid, > > + hapd->conf->ssid.ssid_len, 4096, params->psk, PMK_LEN); > > + } > > + } > > The commit title identified this as adding SAE offload, but this seems > to be adding WPA2-Personal (PSK) offload.. It would be better to split > these into two separate patches to make the exact applicable changes > easier to understand. > > > diff --git a/src/ap/wpa_auth_ie.c b/src/ap/wpa_auth_ie.c > > +#include "wpa_auth_glue.h" > > That is not supposed to done in wpa_auth_ie.c, i.e., the WPA > authenticator functionality is supposed to be generic that does not > depend on hostapd-specific implementation. > > > #ifdef CONFIG_SAE > > - if (sm->wpa_key_mgmt == WPA_KEY_MGMT_SAE && data.num_pmkid && > > - !sm->pmksa) { > > + if (sm->wpa_key_mgmt == WPA_KEY_MGMT_SAE && > > + !(hostapd_wpa_auth_is_sae_offload_enabled(wpa_auth->cb_ctx)) && > > + data.num_pmkid && !sm->pmksa) { > > In other words, this direct hostapd_wpa_auth_is_sae_offload_enabled() > call is expected to be done using struct wpa_auth_callbacks instead to > avoid direct dependency on hostapd-specific wpa_auth_glue.c. > > > diff --git a/src/drivers/driver.h b/src/drivers/driver.h > > @@ -69,6 +69,8 @@ enum hostapd_chan_width_attr { > > +#define MAX_PASSPHRASE_LEN 63 > > > @@ -1785,6 +1787,21 @@ struct wpa_driver_ap_params { > > + /** > > + * sae_passphrase - sae passphrase for SAE offload > > + */ > > + u8 sae_passphrase[MAX_PASSPHRASE_LEN]; > > That should be called sae_password. 63 is not the correct length limit > for this. Why would this even need to be a fixed length array instead of > being const pointer to the password in some upper layer struct owned by > the caller? > > > @@ -2253,6 +2270,12 @@ struct wpa_driver_capa { > > +/** Driver support AP_PSK authentication offload */ > > +#define WPA_DRIVER_FLAGS2_4WAY_HANDSHAKE_AP_PSK 0x0000000000008000ULL > > This should be in a separate patch. > > > +/** Driver support SAE STA authentication offload */ > > +#define WPA_DRIVER_FLAGS2_SAE_OFFLOAD 0x0000000000010000ULL > > This is already included in hostap.git. > > > +/** Driver support SAE AP authentication offload */ > > +#define WPA_DRIVER_FLAGS2_SAE_OFFLOAD_AP 0x0000000000020000ULL > > u64 flags2; > > And this would be in the AP mode SAE offload patch. > > > @@ -6645,6 +6668,7 @@ union wpa_event_data { > > struct port_authorized { > > const u8 *td_bitmap; > > size_t td_bitmap_len; > > + u8 sta_addr[ETH_ALEN]; > > } port_authorized; > > This should now really be documented since the previous uses of struct > port_authorized were for STA mode while this last one is for AP mode and > such a mix of parameters is quite confusing without clear documentation. > > > diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c > > @@ -6926,6 +6937,17 @@ static int nl80211_connect_common(struct wpa_driver_nl80211_data *drv, > > + /* Add SAE passphrase in case of SAE offload */ > > + if (wpa_key_mgmt_sae(params->key_mgmt_suite) && > > + (drv->capa.flags2 & WPA_DRIVER_FLAGS2_SAE_OFFLOAD) && > > + params->passphrase) { > > + wpa_hexdump_key(MSG_DEBUG, " * SAE passphrase", > > + params->passphrase, os_strlen(params->passphrase)); > > + if (nla_put(msg, NL80211_ATTR_SAE_PASSWORD, > > + os_strlen(params->passphrase), params->passphrase)) > > + return -1; > > + } > > This is in incorrect function (i.e., this should be only for CONNECT, > not for ASSOCIATE). Anyway, the STA case is already covered, so no need > for this anymore. > > > diff --git a/src/drivers/driver_nl80211_event.c b/src/drivers/driver_nl80211_event.c > > @@ -3505,7 +3505,14 @@ static void nl80211_port_authorized(struct wpa_driver_nl80211_data *drv, > > } > > > > addr = nla_data(tb[NL80211_ATTR_MAC]); > > - if (os_memcmp(addr, drv->bssid, ETH_ALEN) != 0) { > > + if (is_ap_interface(drv->nlmode) && drv->device_ap_sme) { > > + /* Update STA assoc address */ > > + os_memcpy(event.port_authorized.sta_addr, addr, ETH_ALEN); > > + wpa_printf(MSG_DEBUG, > > + "nl80211: Port authorized for STA BSSID " MACSTR, > > + MAC2STR(addr)); > > + } else if (is_sta_interface(drv->nlmode) && > > + os_memcmp(addr, drv->bssid, ETH_ALEN) != 0) { > > wpa_printf(MSG_DEBUG, > > "nl80211: Ignore port authorized event for " MACSTR > > " (not the currently connected BSSID " MACSTR ")", > > Is this use for NL80211_CMD_PORT_AUTHORIZED for AP mode documented > somewhere? nl80211.h does not seem to describe such use case at all. The > documentation implies this even to be for STA mode only even if > interpreted more widely to cover AP mode, it would be only for 802.1X > FT. > > > diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c > > @@ -4269,6 +4269,21 @@ static void wpas_start_assoc_cb(struct wpa_radio_work *work, int deinit) > > params.key_mgmt_suite == WPA_KEY_MGMT_IEEE8021X_SUITE_B_192)) > > params.req_handshake_offload = 1; > > > > + params.sae_pwe = wpa_s->conf->sae_pwe; > > + if ((wpa_s->drv_flags & WPA_DRIVER_FLAGS_SAE) && > > + (wpa_key_mgmt_sae(params.key_mgmt_suite))) { > > + if (ssid->sae_password) { > > + wpa_printf(MSG_DEBUG, "sae enabled join..Using sae_password"); > > + params.passphrase = ssid->sae_password; > > + } else if (ssid->passphrase) { > > + wpa_printf(MSG_DEBUG, "sae enabled join..Using passphrase"); > > + params.passphrase = ssid->passphrase; > > + } else { > > + wpa_printf(MSG_ERROR, "sae enabled join.." > > + "But neither sae_password nor passphrase set"); > > + } > > + } > > No need for this change anymore. > > -- > Jouni Malinen PGP id EFC895FA
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
_______________________________________________ Hostap mailing list Hostap@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/hostap
