Re: drivers/net/wireless/microchip/wilc1000/cfg80211.c:361:42: sparse: sparse: incorrect type in assignment (different base types)

[Ajay.Kathat]

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