Date: Thu, 15 Feb 2024 04:10:31 +0000
From: <Ajay.Kathat@...rochip.com>
To: <johannes@...solutions.net>, <alexis.lothore@...tlin.com>,
	<kvalo@...nel.org>, <lkp@...el.com>
CC: <oe-kbuild-all@...ts.linux.dev>, <linux-kernel@...r.kernel.org>,
	<linux-wireless@...r.kernel.org>
Subject: Re: drivers/net/wireless/microchip/wilc1000/cfg80211.c:361:42:
 sparse: sparse: incorrect type in assignment (different base types)

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@microchip.com/
>>>
>>> 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

Powered by blists - more mailing lists