diff mbox series

[1/2] wifi: nl80211: force WLAN_AKM_SUITE_SAE in big endian in NL80211_CMD_EXTERNAL_AUTH

Message ID 20240215-nl80211_fix_akm_suites_endianness-v1-1-57e902632f9d@bootlin.com
State New
Headers
Series wifi: nl80211/wilc1000: force WLAN_AKM_SUITE_SAE to big endian in NL80211_CMD_EXTERNAL_AUTH |

Commit Message

Alexis Lothoré Feb. 15, 2024, 2:13 p.m. UTC 
  User-space supplicant (observed at least on wpa_supplicant) historically
parses the NL80211_ATTR_AKM_SUITES from the NL80211_CMD_EXTERNAL_AUTH
message as big endian _only_ when its value is WLAN_AKM_SUITE_SAE, while
processing anything else in host endian. This behavior makes any driver
relying on SAE external auth to switch AKM suite to big endian if it is
WLAN_AKM_SUITE_SAE. A fix bringing compatibility with both endianness
has been brought into wpa_supplicant, however we must keep compatibility
with older versions, while trying to reduce the occurences of this manual
conversion in wireless drivers.

Add the be32 conversion specifically on WLAN_AKM_SUITE_SAE in nl80211 layer
to keep compatibility with older wpa_supplicant versions.

Suggested-by: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
Only a few minor modifications from Johannes' initial suggestion:
- fixed unbalanced parenthesis
- slightly reworded the inlined documentation
---
 net/wireless/nl80211.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

Comments

Kalle Valo Feb. 15, 2024, 4:58 p.m. UTC | #1 
Alexis Lothoré <alexis.lothore@bootlin.com> writes:

> User-space supplicant (observed at least on wpa_supplicant) historically
> parses the NL80211_ATTR_AKM_SUITES from the NL80211_CMD_EXTERNAL_AUTH
> message as big endian _only_ when its value is WLAN_AKM_SUITE_SAE, while
> processing anything else in host endian. This behavior makes any driver
> relying on SAE external auth to switch AKM suite to big endian if it is
> WLAN_AKM_SUITE_SAE. A fix bringing compatibility with both endianness
> has been brought into wpa_supplicant, however we must keep compatibility
> with older versions, while trying to reduce the occurences of this manual
> conversion in wireless drivers.
>
> Add the be32 conversion specifically on WLAN_AKM_SUITE_SAE in nl80211 layer
> to keep compatibility with older wpa_supplicant versions.
>
> Suggested-by: Johannes Berg <johannes@sipsolutions.net>
> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>

A pointer to the discussion would be nice to have:

Link: https://lore.kernel.org/all/09eeb7d4-c922-45ee-a1ac-59942153dbce@bootlin.com/

I assume Johannes can add that.

Alexis, thanks so much for working on this! This has been bugging me for
long but never found the time to investigate it.
Alexis Lothoré Feb. 16, 2024, 8:21 a.m. UTC | #2 
On 2/15/24 17:58, Kalle Valo wrote:
> Alexis Lothoré <alexis.lothore@bootlin.com> writes:
> 
>> User-space supplicant (observed at least on wpa_supplicant) historically
>> parses the NL80211_ATTR_AKM_SUITES from the NL80211_CMD_EXTERNAL_AUTH
>> message as big endian _only_ when its value is WLAN_AKM_SUITE_SAE, while
>> processing anything else in host endian. This behavior makes any driver
>> relying on SAE external auth to switch AKM suite to big endian if it is
>> WLAN_AKM_SUITE_SAE. A fix bringing compatibility with both endianness
>> has been brought into wpa_supplicant, however we must keep compatibility
>> with older versions, while trying to reduce the occurences of this manual
>> conversion in wireless drivers.
>>
>> Add the be32 conversion specifically on WLAN_AKM_SUITE_SAE in nl80211 layer
>> to keep compatibility with older wpa_supplicant versions.
>>
>> Suggested-by: Johannes Berg <johannes@sipsolutions.net>
>> Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
> 
> A pointer to the discussion would be nice to have:
> 
> Link: https://lore.kernel.org/all/09eeb7d4-c922-45ee-a1ac-59942153dbce@bootlin.com/
> 
> I assume Johannes can add that.

Ah yes, indeed. Johannes, please let me know if you prefer me to resend it with
the link in the commit message.

> Alexis, thanks so much for working on this! This has been bugging me for
> long but never found the time to investigate it.

I'm glad to help, especially since I have the corresponding hardware. This
warning was on my radar, and your last complaint about remaining sparse warnings
in the wireless tree eventually triggered the action :)
Kalle Valo Feb. 16, 2024, 9:34 a.m. UTC | #3 
Alexis Lothoré <alexis.lothore@bootlin.com> writes:

>> Alexis, thanks so much for working on this! This has been bugging me for
>> long but never found the time to investigate it.
>
> I'm glad to help, especially since I have the corresponding hardware. This
> warning was on my radar, and your last complaint about remaining sparse warnings
> in the wireless tree eventually triggered the action :)

Hah, at least there's one person who reads my complaints :)

I owe you a beer on this one, if we ever meet please remind me.
diff mbox series

Patch

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 5f18cbf7cc3d..046ce0513d31 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -20136,9 +20136,26 @@  int cfg80211_external_auth_request(struct net_device *dev,
 	if (!hdr)
 		goto nla_put_failure;
 
+	/* Some historical mistakes in drivers <-> userspace interface (notably
+	 * between drivers and wpa_supplicant) led to a big-endian conversion
+	 * being needed on NL80211_ATTR_AKM_SUITES _only_ when its value is
+	 * WLAN_AKM_SUITE_SAE. This is now fixed on userspace side, 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) ||