| Message ID | 20220720160302.231516-1-ajay.kathat@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [1/8] wifi: wilc1000: fix incorrect type assignment sparse warning | expand |
<Ajay.Kathat@microchip.com> writes: > From: Ajay Singh <ajay.kathat@microchip.com> > > Sparse reported below warning >>> drivers/net/wireless/microchip/wilc1000/cfg80211.c:361:42: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int key_mgmt_suite @@ got restricted __be32 [usertype] @@ > > 'key_mgmt_suite' value is expected in big-endian format. After receiving > mgmt_suite value from wpa_s convert to cpu endianness because the same > value is return back using cfg80211_external_auth_request(). Use > be32_to_cpu() conversion API to avoid the sparse warning. > > Reported-by: kernel test robot <lkp@intel.com> > Fixes: c5b331d4f550fb78 ("wifi: wilc1000: add WPA3 SAE support") > Signed-off-by: Ajay Singh <ajay.kathat@microchip.com> > --- > drivers/net/wireless/microchip/wilc1000/cfg80211.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c > index 5c2c7f1dbffd..19862d932f1f 100644 > --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c > +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c > @@ -359,7 +359,7 @@ static int connect(struct wiphy *wiphy, struct net_device *dev, > memcpy(vif->auth.ssid.ssid, sme->ssid, sme->ssid_len); > vif->auth.ssid.ssid_len = sme->ssid_len; > } > - vif->auth.key_mgmt_suite = cpu_to_be32(sme->crypto.akm_suites[0]); > + vif->auth.key_mgmt_suite = be32_to_cpu((__force __be32)sme->crypto.akm_suites[0]); No time to investigate in detail but the cast is just ugly. Isn't there a better way to fix this?
Hi Kalle, On 27/07/22 18:30, Kalle Valo wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > <Ajay.Kathat@microchip.com> writes: > >> From: Ajay Singh <ajay.kathat@microchip.com> >> >> Sparse reported below warning >>>> drivers/net/wireless/microchip/wilc1000/cfg80211.c:361:42: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int key_mgmt_suite @@ got restricted __be32 [usertype] @@ >> 'key_mgmt_suite' value is expected in big-endian format. After receiving >> mgmt_suite value from wpa_s convert to cpu endianness because the same >> value is return back using cfg80211_external_auth_request(). Use >> be32_to_cpu() conversion API to avoid the sparse warning. >> >> Reported-by: kernel test robot <lkp@intel.com> >> Fixes: c5b331d4f550fb78 ("wifi: wilc1000: add WPA3 SAE support") >> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com> >> --- >> drivers/net/wireless/microchip/wilc1000/cfg80211.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c >> index 5c2c7f1dbffd..19862d932f1f 100644 >> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c >> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c >> @@ -359,7 +359,7 @@ static int connect(struct wiphy *wiphy, struct net_device *dev, >> memcpy(vif->auth.ssid.ssid, sme->ssid, sme->ssid_len); >> vif->auth.ssid.ssid_len = sme->ssid_len; >> } >> - vif->auth.key_mgmt_suite = cpu_to_be32(sme->crypto.akm_suites[0]); >> + vif->auth.key_mgmt_suite = be32_to_cpu((__force __be32)sme->crypto.akm_suites[0]); > No time to investigate in detail but the cast is just ugly. Isn't there > a better way to fix this? I think, there is an another way to handle this issue. 'key_mgmt_suite' element in 'cfg80211_external_auth_params' struct should be converted to '__be32' type(like below code snippet) because wpa_s expects the value in big-endian format . After this change, the type case can be avoided. Though I am not sure if these changes can have impact on other driver. diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index cc8a9880b9d6..92df70afe445 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -3509,7 +3509,7 @@ struct cfg80211_external_auth_params { enum nl80211_external_auth_action action; u8 bssid[ETH_ALEN] __aligned(2); struct cfg80211_ssid ssid; - unsigned int key_mgmt_suite; + __be32 key_mgmt_suite; u16 status; const u8 *pmkid; }; Regards, Ajay
<Ajay.Kathat@microchip.com> writes: > Hi Kalle, > > On 27/07/22 18:30, Kalle Valo wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you >> know the content is safe >> >> <Ajay.Kathat@microchip.com> writes: >> >>> From: Ajay Singh <ajay.kathat@microchip.com> >>> >>> Sparse reported below warning >>>>> drivers/net/wireless/microchip/wilc1000/cfg80211.c:361:42: sparse: sparse: incorrect type in assignment (different base types) @@ expected unsigned int key_mgmt_suite @@ got restricted __be32 [usertype] @@ >>> 'key_mgmt_suite' value is expected in big-endian format. After receiving >>> mgmt_suite value from wpa_s convert to cpu endianness because the same >>> value is return back using cfg80211_external_auth_request(). Use >>> be32_to_cpu() conversion API to avoid the sparse warning. >>> >>> Reported-by: kernel test robot <lkp@intel.com> >>> Fixes: c5b331d4f550fb78 ("wifi: wilc1000: add WPA3 SAE support") >>> Signed-off-by: Ajay Singh <ajay.kathat@microchip.com> >>> --- >>> drivers/net/wireless/microchip/wilc1000/cfg80211.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c >>> index 5c2c7f1dbffd..19862d932f1f 100644 >>> --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c >>> +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c >>> @@ -359,7 +359,7 @@ static int connect(struct wiphy *wiphy, struct net_device *dev, >>> memcpy(vif->auth.ssid.ssid, sme->ssid, sme->ssid_len); >>> vif->auth.ssid.ssid_len = sme->ssid_len; >>> } >>> - vif->auth.key_mgmt_suite = cpu_to_be32(sme->crypto.akm_suites[0]); >>> + vif->auth.key_mgmt_suite = be32_to_cpu((__force __be32)sme->crypto.akm_suites[0]); >> No time to investigate in detail but the cast is just ugly. Isn't there >> a better way to fix this? > > > I think, there is an another way to handle this issue. 'key_mgmt_suite' > element in 'cfg80211_external_auth_params' struct should be converted to > '__be32' type(like below code snippet) because wpa_s expects the value > in big-endian format . After this change, the type case can be avoided. > Though I am not sure if these changes can have impact on other driver. > > > diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h > index cc8a9880b9d6..92df70afe445 100644 > --- a/include/net/cfg80211.h > +++ b/include/net/cfg80211.h > @@ -3509,7 +3509,7 @@ struct cfg80211_external_auth_params { > enum nl80211_external_auth_action action; > u8 bssid[ETH_ALEN] __aligned(2); > struct cfg80211_ssid ssid; > - unsigned int key_mgmt_suite; > + __be32 key_mgmt_suite; > u16 status; > const u8 *pmkid; > }; So just that I understand correctly: struct cfg80211_crypto_settings::akm_suites is in cpu endian but struct struct cfg80211_external_auth_params::key_mgmt_suite is in big endian? But your original patch uses be32_to_cpu() so I must be confused. It would be good to document this in cfg80211.h, the documentation there doesn't mention anything about endian.
On Wed, 2022-07-27 at 17:32 +0000, Ajay.Kathat@microchip.com wrote: > > I think, there is an another way to handle this issue. 'key_mgmt_suite' > element in 'cfg80211_external_auth_params' struct should be converted to > '__be32' type(like below code snippet) because wpa_s expects the value > in big-endian format . After this change, the type case can be avoided. > Though I am not sure if these changes can have impact on other driver. > Ugh. I think maybe it would be better to fix wpa_supplicant? Thing is, we use the NL80211_ATTR_AKM_SUITES attribute here for it, and even wpa_supplicant mostly uses that in host endian: num_suites = wpa_key_mgmt_to_suites(params->key_mgmt_suites, suites, ARRAY_SIZE(suites)); ... nla_put(msg, NL80211_ATTR_AKM_SUITES, num_suites * sizeof(u32), suites)) with static int wpa_key_mgmt_to_suites(unsigned int key_mgmt_suites, u32 suites[], int max_suites) { int num_suites = 0; #define __AKM(a, b) \ if (num_suites < max_suites && \ (key_mgmt_suites & (WPA_KEY_MGMT_ ## a))) \ suites[num_suites++] = (RSN_AUTH_KEY_MGMT_ ## b) __AKM(IEEE8021X, UNSPEC_802_1X); and also case WPA_KEY_MGMT_FT_FILS_SHA384: mgmt = RSN_AUTH_KEY_MGMT_FT_FILS_SHA384; break; case WPA_KEY_MGMT_PSK: default: mgmt = RSN_AUTH_KEY_MGMT_PSK_OVER_802_1X; break; } wpa_printf(MSG_DEBUG, " * akm=0x%x", mgmt); if (nla_put_u32(msg, NL80211_ATTR_AKM_SUITES, mgmt)) return -1; Now those are all userspace->kernel direction, but also: wiphy_info_akm_suites(info, tb[NL80211_ATTR_AKM_SUITES]); which eventually uses static unsigned int get_akm_suites_info(struct nlattr *tb) { int i, num; unsigned int key_mgmt = 0; u32 *akms; if (!tb) return 0; num = nla_len(tb) / sizeof(u32); akms = nla_data(tb); for (i = 0; i < num; i++) { switch (akms[i]) { case RSN_AUTH_KEY_MGMT_UNSPEC_802_1X: so again it's in native endianness. So IMHO commit 5ff39c1380d9dea794c5102c0b6d11d1b1e23ad0 Author: Sunil Dutt <usdutt@codeaurora.org> Date: Thu Feb 1 17:01:28 2018 +0530 SAE: Support external authentication offload for driver-SME cases is the problem there in that it assumed big endian for a value that's clearly not meant to be big endian. And what garbage out-of-tree drivers do we don't know ... Even in the kernel, we have static int qtnf_event_handle_external_auth(struct qtnf_vif *vif, const struct qlink_event_external_auth *ev, u16 len) { struct cfg80211_external_auth_params auth = {0}; [...] auth.key_mgmt_suite = le32_to_cpu(ev->akm_suite); [...] ret = cfg80211_external_auth_request(vif->netdev, &auth, GFP_KERNEL); but maybe that was never tested? johannes
On Thu, Oct 13, 2022 at 09:39:56AM +0200, Johannes Berg wrote: > On Wed, 2022-07-27 at 17:32 +0000, Ajay.Kathat@microchip.com wrote: > > I think, there is an another way to handle this issue. 'key_mgmt_suite' > > element in 'cfg80211_external_auth_params' struct should be converted to > > '__be32' type(like below code snippet) because wpa_s expects the value > > in big-endian format . After this change, the type case can be avoided. > > Though I am not sure if these changes can have impact on other driver. > > > > Ugh. I think maybe it would be better to fix wpa_supplicant? Assuming you are referring to what sme_external_auth_trigger() does, yes, the use of RSN_SELECTOR_GET() there on an unsigned int variable is clearly wrong. As a workaround, it could be modified to accept both the big endian and host byte order since the risk of conflicting with a vendor specific AKM suite here would be very minimal.. Furthermore, it looks like this has missed the new RSN_AUTH_KEY_MGMT_SAE_EXT_KEY selector update as well. > Thing is, we use the NL80211_ATTR_AKM_SUITES attribute here for it, and > even wpa_supplicant mostly uses that in host endian: By the way, that use of a u32 attribute (or an array of such) in an interface is quite confusing for suite selectors (both AKM and cipher) since a suite selector is really a four octet binary string that starts with three octet OUI that is followed with a single octet integer value. nl80211.h does not seem to provide any documentation on what the exact value is supposed to be. I can understand use of u32 and native byte order as an implementation internal thing, but it should not really be used in an interface since it is just waiting for this type of issues to show up. It's always confusing to review the driver_nl80211* changes for this area since I have to convince myself each time that this really is a native byte order u32 value.. That sme_external_auth_trigger() case is outside driver_nl80211* so it did not get the same detail of review unfortunately.
On Thu, 2022-10-13 at 12:40 +0300, Jouni Malinen wrote: > On Thu, Oct 13, 2022 at 09:39:56AM +0200, Johannes Berg wrote: > > On Wed, 2022-07-27 at 17:32 +0000, Ajay.Kathat@microchip.com wrote: > > > I think, there is an another way to handle this issue. 'key_mgmt_suite' > > > element in 'cfg80211_external_auth_params' struct should be converted to > > > '__be32' type(like below code snippet) because wpa_s expects the value > > > in big-endian format . After this change, the type case can be avoided. > > > Though I am not sure if these changes can have impact on other driver. > > > > > > > Ugh. I think maybe it would be better to fix wpa_supplicant? > > Assuming you are referring to what sme_external_auth_trigger() does, > yes, the use of RSN_SELECTOR_GET() there on an unsigned int variable is > clearly wrong. Right, that's what I meant. > As a workaround, it could be modified to accept both the > big endian and host byte order since the risk of conflicting with a > vendor specific AKM suite here would be very minimal.. True. > > Thing is, we use the NL80211_ATTR_AKM_SUITES attribute here for it, and > > even wpa_supplicant mostly uses that in host endian: > > By the way, that use of a u32 attribute (or an array of such) in an > interface is quite confusing for suite selectors (both AKM and cipher) > since a suite selector is really a four octet binary string that starts > with three octet OUI that is followed with a single octet integer > value. nl80211.h does not seem to provide any documentation on what the > exact value is supposed to be. I guess we should fix the documentation then, but basically, for a selector of O-U-I:D we use (O << 24) | (U << 16) | (I << 8) | D for the ID. If we had consistently used be32 then that'd actually at least match the four octet binary string and it'd be irrelevant, but ... we clearly didn't. > I can understand use of u32 and native byte order as an implementation > internal thing, but it should not really be used in an interface since > it is just waiting for this type of issues to show up. Yeah, can't really disagree with that, though I think it's a bit late now, unfortunately. johannes
diff --git a/drivers/net/wireless/microchip/wilc1000/cfg80211.c b/drivers/net/wireless/microchip/wilc1000/cfg80211.c index 5c2c7f1dbffd..19862d932f1f 100644 --- a/drivers/net/wireless/microchip/wilc1000/cfg80211.c +++ b/drivers/net/wireless/microchip/wilc1000/cfg80211.c @@ -359,7 +359,7 @@ static int connect(struct wiphy *wiphy, struct net_device *dev, memcpy(vif->auth.ssid.ssid, sme->ssid, sme->ssid_len); vif->auth.ssid.ssid_len = sme->ssid_len; } - vif->auth.key_mgmt_suite = cpu_to_be32(sme->crypto.akm_suites[0]); + vif->auth.key_mgmt_suite = be32_to_cpu((__force __be32)sme->crypto.akm_suites[0]); ether_addr_copy(vif->auth.bssid, sme->bssid); break;