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

Ajay.Kathat
2024-02-15T04:11:08Z

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