| Message ID | 20240523081428.2852276-1-s.hauer@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | wifi: mwifiex: fix parsing of more than two AKM suites | expand |
Hi Sascha, kernel test robot noticed the following build warnings: [auto build test WARNING on wireless-next/main] [also build test WARNING on wireless/main linus/master v6.9 next-20240523] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Sascha-Hauer/wifi-mwifiex-fix-parsing-of-more-than-two-AKM-suites/20240523-161947 base: https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git main patch link: https://lore.kernel.org/r/20240523081428.2852276-1-s.hauer%40pengutronix.de patch subject: [PATCH] wifi: mwifiex: fix parsing of more than two AKM suites config: x86_64-randconfig-122-20240525 (https://download.01.org/0day-ci/archive/20240525/202405250631.g417GL0g-lkp@intel.com/config) compiler: gcc-11 (Ubuntu 11.4.0-4ubuntu1) 11.4.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240525/202405250631.g417GL0g-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202405250631.g417GL0g-lkp@intel.com/ sparse warnings: (new ones prefixed by >>) >> drivers/net/wireless/marvell/mwifiex/uap_cmd.c:54:17: sparse: sparse: cast to restricted __le16 drivers/net/wireless/marvell/mwifiex/uap_cmd.c:72:17: sparse: sparse: cast to restricted __le16 vim +54 drivers/net/wireless/marvell/mwifiex/uap_cmd.c 27 28 static void woal_check_rsn_ie(const struct IEEEtypes_Rsn_t *rsn_ie, int len, 29 struct mwifiex_uap_bss_param *bss_config, u8 *pairwise_cipher) 30 { 31 int left, count, i; 32 struct wpa_suite_ucast *key_mgmt; 33 34 left = len; 35 if (left < (int)sizeof(struct IEEEtypes_Rsn_t)) 36 return; 37 38 bss_config->wpa_cfg.group_cipher = 0; 39 *pairwise_cipher = 0; 40 bss_config->key_mgmt = 0; 41 42 /* check the group cipher */ 43 switch (be32_to_cpu(rsn_ie->group_cipher)) { 44 case WLAN_CIPHER_SUITE_TKIP: 45 bss_config->wpa_cfg.group_cipher = CIPHER_TKIP; 46 break; 47 case WLAN_CIPHER_SUITE_CCMP: 48 bss_config->wpa_cfg.group_cipher = CIPHER_AES_CCMP; 49 break; 50 default: 51 break; 52 } 53 > 54 count = le16_to_cpu(rsn_ie->pairwise_cipher.count); 55 for (i = 0; i < count; i++) { 56 switch (be32_to_cpu(rsn_ie->pairwise_cipher.suite[i])) { 57 case WLAN_CIPHER_SUITE_TKIP: 58 *pairwise_cipher |= CIPHER_TKIP; 59 break; 60 case WLAN_CIPHER_SUITE_CCMP: 61 *pairwise_cipher |= CIPHER_AES_CCMP; 62 break; 63 default: 64 break; 65 } 66 } 67 left -= sizeof(struct IEEEtypes_Rsn_t) + (count - 1) * sizeof(__be32); 68 if (left < (int)sizeof(struct wpa_suite_ucast)) 69 return; 70 71 key_mgmt = ((void *)rsn_ie + sizeof(struct IEEEtypes_Rsn_t) + (count - 1) * sizeof(__be32)); 72 count = le16_to_cpu(key_mgmt->count); 73 if (left < (int)(sizeof(struct wpa_suite_ucast) + 74 (count - 1) * sizeof(__be32))) 75 return; 76 77 for (i = 0; i < count; i++) { 78 switch (be32_to_cpu(key_mgmt->suite[i])) { 79 case WLAN_AKM_SUITE_8021X: 80 bss_config->key_mgmt |= KEY_MGMT_EAP; 81 break; 82 case WLAN_AKM_SUITE_PSK: 83 bss_config->key_mgmt |= KEY_MGMT_PSK; 84 break; 85 case WLAN_AKM_SUITE_PSK_SHA256: 86 bss_config->key_mgmt |= KEY_MGMT_PSK_SHA256; 87 break; 88 case WLAN_AKM_SUITE_SAE: 89 bss_config->key_mgmt |= KEY_MGMT_SAE; 90 break; 91 case WLAN_AKM_SUITE_OWE: 92 bss_config->key_mgmt |= KEY_MGMT_OWE; 93 break; 94 } 95 } 96 } 97
Sascha Hauer <s.hauer@pengutronix.de> writes: > params->crypto.n_akm_suites seems to be limited to two AKM suites. Once > there are more they will be passed as extra elements of type WLAN_EID_RSN > or WLAN_EID_VENDOR_SPECIFIC. > > This takes some snippets from the downstream vendor driver to parse > these elements and to set the correct protocol and key_mgmt bits to > enable the desired key managements algorithms in the hardware. > > This patch is not a request for inclusion, more a heads up that there's > something missing and the question if the approach taken is the right > one or if there are other preferred ways to fix this issue. Please mark patches like this as "[PATCH RFC]", that way we maintainers know to drop them automatically. > --- a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c > +++ b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c > @@ -9,6 +9,112 @@ > #include "11ac.h" > #include "11n.h" > > +struct wpa_suite_ucast { > + /* count */ > + u16 count; > + /** wpa_suite list */ > + __be32 suite[1]; > +} __packed; The comments here are not adding any extra information, please remove. In general having a some kind of prefix in the struct name would be nice. I don't know what mwifiex uses (if any) but, for example, in ath12k we use 'ath12k_'. > +struct IEEEtypes_Rsn_t { Lower case, no '_t' and also improve the naming. > + /** Rsn : version */ > + u16 version; > + /** Rsn : group cipher */ > + __be32 group_cipher; > + /** Rsn : pairwise cipher */ > + struct wpa_suite_ucast pairwise_cipher; > +} __packed; Useless comments.
On Sat, May 25, 2024 at 12:15:22PM +0300, Kalle Valo wrote: > Sascha Hauer <s.hauer@pengutronix.de> writes: > > > params->crypto.n_akm_suites seems to be limited to two AKM suites. Once > > there are more they will be passed as extra elements of type WLAN_EID_RSN > > or WLAN_EID_VENDOR_SPECIFIC. > > > > This takes some snippets from the downstream vendor driver to parse > > these elements and to set the correct protocol and key_mgmt bits to > > enable the desired key managements algorithms in the hardware. > > > > This patch is not a request for inclusion, more a heads up that there's > > something missing and the question if the approach taken is the right > > one or if there are other preferred ways to fix this issue. > > Please mark patches like this as "[PATCH RFC]", that way we maintainers > know to drop them automatically. Yes, will do. I should have known that. The question I had with this patch is more like whether the approach is fine. I wonder why I have to parse the WLAN_EID_RSN element in driver specific code and why I do not find similar code in other drivers which should suffer from the same problem. Looking deeper at it the Kernel by default only supports two AKM suites. wiphy->max_num_akm_suites is initialized to NL80211_MAX_NR_AKM_SUITES (which is 2). Whenever wpa_supplicant/hostapd specify more AKM suites the suites exceeding 2 are encoded in the WLAN_EID_RSN element and I would expect other drivers to handle this as well. I realized that the Kernel can handle up to 10 AKM suites by setting wiphy->max_num_akm_suites to CFG80211_MAX_NUM_AKM_SUITES and that seems to do the trick as well, at least when I patch wpa_supplicant to actually use this increased number. Sascha
diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h index 3adc447b715f6..d576b2d71a6b9 100644 --- a/drivers/net/wireless/marvell/mwifiex/fw.h +++ b/drivers/net/wireless/marvell/mwifiex/fw.h @@ -415,6 +415,9 @@ enum MWIFIEX_802_11_PRIVACY_FILTER { #define KEY_MGMT_NONE 0x04 #define KEY_MGMT_PSK 0x02 #define KEY_MGMT_EAP 0x01 +#define KEY_MGMT_PSK_SHA256 0x100 +#define KEY_MGMT_OWE 0x200 +#define KEY_MGMT_SAE 0x400 #define CIPHER_TKIP 0x04 #define CIPHER_AES_CCMP 0x08 #define VALID_CIPHER_BITMAP 0x0c diff --git a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c index 491e366119096..4b21626e2dd7f 100644 --- a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c +++ b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c @@ -9,6 +9,112 @@ #include "11ac.h" #include "11n.h" +struct wpa_suite_ucast { + /* count */ + u16 count; + /** wpa_suite list */ + __be32 suite[1]; +} __packed; + +struct IEEEtypes_Rsn_t { + /** Rsn : version */ + u16 version; + /** Rsn : group cipher */ + __be32 group_cipher; + /** Rsn : pairwise cipher */ + struct wpa_suite_ucast pairwise_cipher; +} __packed; + +static void woal_check_rsn_ie(const struct IEEEtypes_Rsn_t *rsn_ie, int len, + struct mwifiex_uap_bss_param *bss_config, u8 *pairwise_cipher) +{ + int left, count, i; + struct wpa_suite_ucast *key_mgmt; + + left = len; + if (left < (int)sizeof(struct IEEEtypes_Rsn_t)) + return; + + bss_config->wpa_cfg.group_cipher = 0; + *pairwise_cipher = 0; + bss_config->key_mgmt = 0; + + /* check the group cipher */ + switch (be32_to_cpu(rsn_ie->group_cipher)) { + case WLAN_CIPHER_SUITE_TKIP: + bss_config->wpa_cfg.group_cipher = CIPHER_TKIP; + break; + case WLAN_CIPHER_SUITE_CCMP: + bss_config->wpa_cfg.group_cipher = CIPHER_AES_CCMP; + break; + default: + break; + } + + count = le16_to_cpu(rsn_ie->pairwise_cipher.count); + for (i = 0; i < count; i++) { + switch (be32_to_cpu(rsn_ie->pairwise_cipher.suite[i])) { + case WLAN_CIPHER_SUITE_TKIP: + *pairwise_cipher |= CIPHER_TKIP; + break; + case WLAN_CIPHER_SUITE_CCMP: + *pairwise_cipher |= CIPHER_AES_CCMP; + break; + default: + break; + } + } + left -= sizeof(struct IEEEtypes_Rsn_t) + (count - 1) * sizeof(__be32); + if (left < (int)sizeof(struct wpa_suite_ucast)) + return; + + key_mgmt = ((void *)rsn_ie + sizeof(struct IEEEtypes_Rsn_t) + (count - 1) * sizeof(__be32)); + count = le16_to_cpu(key_mgmt->count); + if (left < (int)(sizeof(struct wpa_suite_ucast) + + (count - 1) * sizeof(__be32))) + return; + + for (i = 0; i < count; i++) { + switch (be32_to_cpu(key_mgmt->suite[i])) { + case WLAN_AKM_SUITE_8021X: + bss_config->key_mgmt |= KEY_MGMT_EAP; + break; + case WLAN_AKM_SUITE_PSK: + bss_config->key_mgmt |= KEY_MGMT_PSK; + break; + case WLAN_AKM_SUITE_PSK_SHA256: + bss_config->key_mgmt |= KEY_MGMT_PSK_SHA256; + break; + case WLAN_AKM_SUITE_SAE: + bss_config->key_mgmt |= KEY_MGMT_SAE; + break; + case WLAN_AKM_SUITE_OWE: + bss_config->key_mgmt |= KEY_MGMT_OWE; + break; + } + } +} + +static void woal_find_wpa_ies(const void *ie, int len, struct mwifiex_uap_bss_param *bss_config) +{ + const struct element *e; + + e = cfg80211_find_elem(WLAN_EID_RSN, ie, len); + if (e) { + woal_check_rsn_ie((void *)e->data, e->datalen, bss_config, + &bss_config->wpa_cfg.pairwise_cipher_wpa2); + + bss_config->protocol |= PROTOCOL_WPA2; + } + + e = cfg80211_find_vendor_elem(WLAN_EID_VENDOR_SPECIFIC, 0x1, ie, len); + if (e) { + woal_check_rsn_ie((void *)e->data, e->datalen, bss_config, + &bss_config->wpa_cfg.pairwise_cipher_wpa); + bss_config->protocol |= PROTOCOL_WPA; + } +} + /* This function parses security related parameters from cfg80211_ap_settings * and sets into FW understandable bss_config structure. */ @@ -17,6 +123,11 @@ int mwifiex_set_secure_params(struct mwifiex_private *priv, struct cfg80211_ap_settings *params) { int i; struct mwifiex_wep_key wep_key; + const u8 *ie = NULL; + int ie_len; + + ie = params->beacon.tail; + ie_len = params->beacon.tail_len; if (!params->privacy) { bss_config->protocol = PROTOCOL_NO_SECURITY; @@ -46,36 +157,34 @@ int mwifiex_set_secure_params(struct mwifiex_private *priv, bss_config->key_mgmt_operation |= KEY_MGMT_ON_HOST; + if (params->crypto.wpa_versions & NL80211_WPA_VERSION_1) + bss_config->protocol |= PROTOCOL_WPA; + if (params->crypto.wpa_versions & NL80211_WPA_VERSION_2) + bss_config->protocol |= PROTOCOL_WPA2; + + woal_find_wpa_ies(ie, ie_len, bss_config); + for (i = 0; i < params->crypto.n_akm_suites; i++) { + mwifiex_dbg(priv->adapter, MSG, "suite%d: 0x%08x\n", i, params->crypto.akm_suites[i]); + switch (params->crypto.akm_suites[i]) { case WLAN_AKM_SUITE_8021X: - if (params->crypto.wpa_versions & - NL80211_WPA_VERSION_1) { - bss_config->protocol = PROTOCOL_WPA; - bss_config->key_mgmt = KEY_MGMT_EAP; - } - if (params->crypto.wpa_versions & - NL80211_WPA_VERSION_2) { - bss_config->protocol |= PROTOCOL_WPA2; - bss_config->key_mgmt = KEY_MGMT_EAP; - } + bss_config->key_mgmt |= KEY_MGMT_EAP; break; case WLAN_AKM_SUITE_PSK: - if (params->crypto.wpa_versions & - NL80211_WPA_VERSION_1) { - bss_config->protocol = PROTOCOL_WPA; - bss_config->key_mgmt = KEY_MGMT_PSK; - } - if (params->crypto.wpa_versions & - NL80211_WPA_VERSION_2) { - bss_config->protocol |= PROTOCOL_WPA2; - bss_config->key_mgmt = KEY_MGMT_PSK; - } + bss_config->key_mgmt |= KEY_MGMT_PSK; + break; + case WLAN_AKM_SUITE_PSK_SHA256: + bss_config->key_mgmt |= KEY_MGMT_PSK_SHA256; break; default: break; } } + + mwifiex_dbg(priv->adapter, MSG, "protocol: 0x%08x key_mgmt: 0x%08x\n", + bss_config->protocol, bss_config->key_mgmt); + for (i = 0; i < params->crypto.n_ciphers_pairwise; i++) { switch (params->crypto.ciphers_pairwise[i]) { case WLAN_CIPHER_SUITE_WEP40:
params->crypto.n_akm_suites seems to be limited to two AKM suites. Once there are more they will be passed as extra elements of type WLAN_EID_RSN or WLAN_EID_VENDOR_SPECIFIC. This takes some snippets from the downstream vendor driver to parse these elements and to set the correct protocol and key_mgmt bits to enable the desired key managements algorithms in the hardware. This patch is not a request for inclusion, more a heads up that there's something missing and the question if the approach taken is the right one or if there are other preferred ways to fix this issue. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/net/wireless/marvell/mwifiex/fw.h | 3 + .../net/wireless/marvell/mwifiex/uap_cmd.c | 149 +++++++++++++++--- 2 files changed, 132 insertions(+), 20 deletions(-)