| Message ID | 20220720160302.231516-1-ajay.kathat@microchip.com |
|---|---|
| State | New |
| 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 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;