Hi On 2/14/24 02:29, Johannes Berg wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi, > >>> For reference here's the old discussion: >>> >>> https://patchwork.kernel.org/project/linux-wireless/patch/20220720160302.231516-1-ajay.kathat@xxxxxxxxxxxxx/ >>> >>> Any volunteers to help fix this? I would prefers fixes for issues like >>> this compared to questionable random cleanups we always get. >> >> I'm bumping this old thread because it looks like the sparse warning is still >> present in WILC driver, and I would gladly help getting rid of it, but since >> there's already been a fair amount of discussions around it, I am not sure what >> is expected/what should be done. Here is my understanding so far: >> - Ajay has proposed a patch ([1]) which indeed fixes the warning but involves >> many casts >> - Johannes and Jouni then gave details about the original issue leading to those >> casts ([2]): wpa_supplicant somehow forces the AKM suites values to be be32 at >> some point, while it should be treated in host endianness >> - as pointed by Ajay, the corresponding fix has been made since then by Jouni in >> wpa_supplicant ([3]). The fix make sure to handle key_mgmt_suite in host >> endianness AND to keep compatibility with current drivers having the be32 fix. - > > Am I confused, or is the change [3] in the other direction? > > From what I see, the code there (now changed again, btw) is about > reading the value *from the driver*. > > The driver change is about getting the value *from the supplicant*. > > And the _outgoing_ code (sending it to the driver) from the supplicant > has - as far as I can tell - been putting it into the attribute in host > byte order forever? See commit cfaab58007b4 ("nl80211: Connect API > support"). > > > Aha! So, I'm not _completely_ confused, however, the only use of this > value in this driver is sending it back to the supplicant! Which seems > entirely wrong, since the supplicant assumes basically anything will be > handled? > > (But - the firmware also has a parameter key_mgmt_suites [in struct > wilc_external_auth_param] which is never even set in > wilc_set_external_auth_param??) > > yeah, *key_mgmt_suites* is not passed to the firmware. It is added to *wilc_external_auth_param* but it was not necessary since SAE AUTH is offloaded to user space so it takes care of complete AUTH exchange and only confirm once it is done. key_mgmt_suites, which is received in connect(), is needs to be maintained in driver to pass back using cfg80211_external_auth_request(). > Also note that the supplicant will *only* read RSN_AUTH_KEY_MGMT_SAE in > big endian, so you've already lost here pretty much? > >> - It could have allowed to simply get rid of the all casts on AKM suites in >> wilc driver ([4]), but then new kernel/drivers would break with existing >> userspace, so it is not an option > > I am wondering if it works at all ... > >> Now, I see multiple options to fix the sparse warning: >> - apply the same fix as for wpa_supplicant ([3]) in wilc driver (so basically, >> become compatible with both endianness) > > But this cannot be done! On input to the driver, the value is in host > byte order always. The question is on output - and there you cannot > detect it. > >> - apply the same fix as for wpa_supplicant ([3]), not in wilc but in nl80211 >> (may need to update not only wilc but any driver having trailing be32 cast on >> AKM suites) > > That might even work? Well, not the same fix, since again input vs. > output, but something like this: > > --- a/net/wireless/nl80211.c > +++ b/net/wireless/nl80211.c > @@ -20136,9 +20136,27 @@ int cfg80211_external_auth_request(struct net_device *dev, > if (!hdr) > goto nla_put_failure; > > + /* > + * Some drivers and due to that userspace (wpa_supplicant) were > + * in the past interpreting this value as a big-endian value, > + * at a time where only WLAN_AKM_SUITE_SAE was used. This is now > + * fixed, but for the benefit of older wpa_supplicant versions, > + * send this particular value in big-endian. Note that newer > + * wpa_supplicant will also detect this particular value in big > + * endian still, so it all continues to work. > + */ > + if (params->key_mgmt_suite == WLAN_AKM_SUITE_SAE) { > + if (nla_put_be32(msg, NL80211_ATTR_AKM_SUITES, > + cpu_to_be32(WLAN_AKM_SUITE_SAE)) > + goto nla_put_failure; > + } else { > + if (nla_put_u32(msg, NL80211_ATTR_AKM_SUITES, > + params->key_mgmt_suite))) > + goto nla_put_failure; > + } > + > if (nla_put_u32(msg, NL80211_ATTR_WIPHY, rdev->wiphy_idx) || > nla_put_u32(msg, NL80211_ATTR_IFINDEX, dev->ifindex) || > - nla_put_u32(msg, NL80211_ATTR_AKM_SUITES, params->key_mgmt_suite) || > nla_put_u32(msg, NL80211_ATTR_EXTERNAL_AUTH_ACTION, > params->action) || > nla_put(msg, NL80211_ATTR_BSSID, ETH_ALEN, params->bssid) > || IMO this patch is better solution since it covers for both old and new wpa_s(3) and even the fixes in new wpa_s are not required with this change. After this change, the driver can be modified to use the host byte order that will also resolve the sparse warning. Regards, Ajay
