Comments on WPS patches for hostapd/wpa_supplicant

[Jouni Malinen]

Here are some semi-random comments on the latest version of patches you
sent for hostapd/wpa_supplicant.


Which madwifi driver version includes definitions for struct
ieee80211req_wscie and IEEE80211_IOCTL_GETWSCIE?


Was wpa_supplicant/wps_enrollee.c forgotten from the tarball? 
wps_enrollee build fails since there is no rule to create
wps_enrollee.o which would likely mean that the matching .c file was
missing.


Adding "-W -Werror" to the gcc compiler is recommended. I'm using this
and require the code to compile without warnings before committing
into into my git tree.


OpenSSL build did not seem to work, i.e., had to use internal crypto
for both hostapd and wps_enrollee builds.


Passing struct sta_info into hostapd WPA state machines is not
acceptable. The sta_info structure is internal to hostapd's 802.11
management code and it must not be used in WPA code. This is new
cleanup in hostapd 0.6.x and I do not want to move back to the old
version. In other words, the following change won't go in and will
need to be done differently (i.e., just pass in the needed
information):

 struct wpa_state_machine *
-wpa_auth_sta_init(struct wpa_authenticator *wpa_auth, const u8 *addr)
+wpa_auth_sta_init(struct wpa_authenticator *wpa_auth, struct sta_info
*sta)

It looks like this was used for wpa.c to be able to look for
sta->flags and four different WPS flags. Does WPA code really need to
know this level of details? Wouldn't simple 1-bit flag be enough? If
this changes dynamically, that should be done using a function call to
update the WPA authenticator state for the STA.


src/eap_server/eap_identity.c:

+#ifdef EAP_WPS
+#include <eap_wps.h>
+#endif

+#ifdef EAP_WPS
+        /* Examine to see if it relates to WPS.
+         * If so, there is a side effect to the station state.
+         */
+        eap_server_wps_identity_process(pos, sm->sta);
+#endif  /* EAP_WPS */

Does this need to be in eap_identity.c? I would prefer to see a
callback from eap.c. In addition, struct sta_info should not be
touched from EAP methods, so cleaner solution would be needed to
handle this side effect anyway. I'm not completely sure what this is
used for, but the part that changes struct sta_info needs to be moved
away from the EAP method and handled, e.g., by adding a new struct
eapol_callbacks function for this.


-- 
Jouni Malinen                                            PGP id EFC895FA