diff mbox series

[1/8] wifi: wilc1000: fix incorrect type assignment sparse warning

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

Commit Message

Ajay Singh July 20, 2022, 4:03 p.m. UTC
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(-)

Comments

Kalle Valo July 27, 2022, 1 p.m. UTC | #1
<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?
Ajay Singh July 27, 2022, 5:32 p.m. UTC | #2
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
Kalle Valo Oct. 13, 2022, 6:13 a.m. UTC | #3
<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.
Jouni Malinen Oct. 13, 2022, 9:40 a.m. UTC | #4
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.
Johannes Berg Oct. 13, 2022, 10:10 a.m. UTC | #5
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 mbox series

Patch

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;