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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e9501c13be127b8b9d0c769a27d8d38636875f0f.camel@sipsolutions.net>
Date: Wed, 14 Feb 2024 10:29:14 +0100
From: Johannes Berg <johannes@...solutions.net>
To: Alexis Lothoré <alexis.lothore@...tlin.com>, Kalle
	Valo <kvalo@...nel.org>, kernel test robot <lkp@...el.com>
Cc: Ajay Singh <ajay.kathat@...rochip.com>, 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,

> > 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??)


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)
||


> - take the initial quick but not-so-nice double cast fix and call it a day

but that doesn't actually work for anything other than
WLAN_AKM_SUITE_SAE...

johannes