| Message ID | 20240717-mwifiex-wpa-psk-sha256-v2-2-eb53d5082b62@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | mwifiex: add support for WPA-PSK-SHA256 | expand |
Hi Sascha, On Wed, Jul 17, 2024 at 10:30:08AM +0200, Sascha Hauer wrote: > This adds support for the WPA-PSK AKM suite with SHA256 as hashing > method (WPA-PSK-SHA256). Tested with a wpa_supplicant provided AP > using key_mgmt=WPA-PSK-SHA256. > > Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/net/wireless/marvell/mwifiex/fw.h | 1 + > drivers/net/wireless/marvell/mwifiex/uap_cmd.c | 3 +++ > 2 files changed, 4 insertions(+) > > diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h > index 3adc447b715f6..1c76754b616ff 100644 > --- a/drivers/net/wireless/marvell/mwifiex/fw.h > +++ b/drivers/net/wireless/marvell/mwifiex/fw.h > @@ -415,6 +415,7 @@ 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 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 7f822660fd955..c055fdc7114ba 100644 > --- a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c > +++ b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c > @@ -60,6 +60,9 @@ int mwifiex_set_secure_params(struct mwifiex_private *priv, > 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; I feel like this relates to previous questions you've had [1], and while I think the answer at the time made sense to me (basically, EAP and PSK are mutually exclusive), it makes less sense to me here that PSK-SHA256 is mutually exclusive with PSK. And in particular, IIUC, this means that the ordering in a wpa_supplicant.conf line like key_mgmt=WPA-PSK WPA-PSK-SHA256 matters -- only the latter will actually be in use. Is that intended? Is this really a single-value field, and not a multiple-option bitfield? Or if these are really mutually exclusive, then maybe we're on the wrong track here: https://patchwork.kernel.org/project/linux-wireless/patch/20240530130156.1651174-1-s.hauer@pengutronix.de/ wifi: mwifiex: increase max_num_akm_suites In any case, something feels off here, because the nl80211 API doesn't say anything about the ordering of AKM suites being relevant. Brian > default: > break; > } > > -- > 2.39.2 > [1] Subject: Re: [EXT] Re: [PATCH v10 2/2] wifi: mwifiex: add host mlme for AP mode https://lore.kernel.org/all/Zmvjw3aG9j8kW0Ld@pengutronix.de/ https://lore.kernel.org/all/PA4PR04MB9638B7F0F4E49F79057C15FBD1CD2@PA4PR04MB9638.eurprd04.prod.outlook.com/
On Thu, Jul 18, 2024 at 03:55:18PM -0700, Brian Norris wrote: > Hi Sascha, > > On Wed, Jul 17, 2024 at 10:30:08AM +0200, Sascha Hauer wrote: > > This adds support for the WPA-PSK AKM suite with SHA256 as hashing > > method (WPA-PSK-SHA256). Tested with a wpa_supplicant provided AP > > using key_mgmt=WPA-PSK-SHA256. > > > > Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > drivers/net/wireless/marvell/mwifiex/fw.h | 1 + > > drivers/net/wireless/marvell/mwifiex/uap_cmd.c | 3 +++ > > 2 files changed, 4 insertions(+) > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h > > index 3adc447b715f6..1c76754b616ff 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/fw.h > > +++ b/drivers/net/wireless/marvell/mwifiex/fw.h > > @@ -415,6 +415,7 @@ 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 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 7f822660fd955..c055fdc7114ba 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c > > +++ b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c > > @@ -60,6 +60,9 @@ int mwifiex_set_secure_params(struct mwifiex_private *priv, > > 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; > > I feel like this relates to previous questions you've had [1], and while > I think the answer at the time made sense to me (basically, EAP and PSK > are mutually exclusive), it makes less sense to me here that PSK-SHA256 > is mutually exclusive with PSK. And in particular, IIUC, this means that > the ordering in a wpa_supplicant.conf line like > > key_mgmt=WPA-PSK WPA-PSK-SHA256 > > matters -- only the latter will actually be in use. > > Is that intended? Is this really a single-value field, and not a > multiple-option bitfield? It seems that when only the KEY_MGMT_PSK_SHA256 is set, then KEY_MGMT_PSK also works. Likewise, when only KEY_MGMT_SAE is set, then also KEY_MGMT_PSK_SHA256 and KEY_MGMT_PSK work. I gave it a test and also was surprised to see that we only have to set the "most advanced" bit which then includes the "less advanced" features automatically. I could change setting the key_mgmt bits to |= as it feels more natural and raises less eyebrows, but in my testing it didn't make a difference. BTW wpa_supplicant parses the key_mgmt options into a bitfield which is then evaluated elsewhere, so the order the AKM suites are passed to the kernel is always the same, regardless of the order they appear in the config. Sascha
[ +CC David, in case he has thoughts ] On Fri, Jul 19, 2024 at 08:04:59AM +0200, Sascha Hauer wrote: > On Thu, Jul 18, 2024 at 03:55:18PM -0700, Brian Norris wrote: > > On Wed, Jul 17, 2024 at 10:30:08AM +0200, Sascha Hauer wrote: > > > This adds support for the WPA-PSK AKM suite with SHA256 as hashing > > > method (WPA-PSK-SHA256). Tested with a wpa_supplicant provided AP > > > using key_mgmt=WPA-PSK-SHA256. > > > > > > Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > > --- > > > drivers/net/wireless/marvell/mwifiex/fw.h | 1 + > > > drivers/net/wireless/marvell/mwifiex/uap_cmd.c | 3 +++ > > > 2 files changed, 4 insertions(+) > > > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h > > > index 3adc447b715f6..1c76754b616ff 100644 > > > --- a/drivers/net/wireless/marvell/mwifiex/fw.h > > > +++ b/drivers/net/wireless/marvell/mwifiex/fw.h > > > @@ -415,6 +415,7 @@ 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 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 7f822660fd955..c055fdc7114ba 100644 > > > --- a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c > > > +++ b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c > > > @@ -60,6 +60,9 @@ int mwifiex_set_secure_params(struct mwifiex_private *priv, > > > 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; > > > > I feel like this relates to previous questions you've had [1], and while > > I think the answer at the time made sense to me (basically, EAP and PSK > > are mutually exclusive), it makes less sense to me here that PSK-SHA256 > > is mutually exclusive with PSK. And in particular, IIUC, this means that > > the ordering in a wpa_supplicant.conf line like > > > > key_mgmt=WPA-PSK WPA-PSK-SHA256 > > > > matters -- only the latter will actually be in use. > > > > Is that intended? Is this really a single-value field, and not a > > multiple-option bitfield? > > It seems that when only the KEY_MGMT_PSK_SHA256 is set, then > KEY_MGMT_PSK also works. Likewise, when only KEY_MGMT_SAE is set, then > also KEY_MGMT_PSK_SHA256 and KEY_MGMT_PSK work. > I gave it a test and also was surprised to see that we only have to set > the "most advanced" bit which then includes the "less advanced" features > automatically. Huh, that's interesting. So these KEY_MGMT* flags don't really mean what they say. It might be nice to have some additional commentary in the driver in that case. > I could change setting the key_mgmt bits to |= as it feels more natural > and raises less eyebrows, but in my testing it didn't make a difference. That would make sense to me, but I think that's in conflict with what David Lin said here: https://lore.kernel.org/all/PA4PR04MB9638B7F0F4E49F79057C15FBD1CD2@PA4PR04MB9638.eurprd04.prod.outlook.com/ "Firmware can only support one of WLAN_AKM_SUITE_8021X, WLAN_AKM_SUITE_PSK, or WLAN_AKM_SUITE_SAE." If that's true, then it seems like we need some kind of priority conditions here (e.g., if PSK is provided, but then we see PSK_SHA256, let PSK_SHA256 override -- but not vice versa). That might be pretty ugly though. > BTW wpa_supplicant parses the key_mgmt options into a bitfield which is > then evaluated elsewhere, so the order the AKM suites are passed to the > kernel is always the same, regardless of the order they appear in the > config. I hear you, but that's not really how we define kernel APIs -- by the particular implementation of a single commonly-used user space. Brian
On Fri, Jul 19, 2024 at 12:05:01PM -0700, Brian Norris wrote: > [ +CC David, in case he has thoughts ] > > On Fri, Jul 19, 2024 at 08:04:59AM +0200, Sascha Hauer wrote: > > On Thu, Jul 18, 2024 at 03:55:18PM -0700, Brian Norris wrote: > > > On Wed, Jul 17, 2024 at 10:30:08AM +0200, Sascha Hauer wrote: > > > > This adds support for the WPA-PSK AKM suite with SHA256 as hashing > > > > method (WPA-PSK-SHA256). Tested with a wpa_supplicant provided AP > > > > using key_mgmt=WPA-PSK-SHA256. > > > > > > > > Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > > > --- > > > > drivers/net/wireless/marvell/mwifiex/fw.h | 1 + > > > > drivers/net/wireless/marvell/mwifiex/uap_cmd.c | 3 +++ > > > > 2 files changed, 4 insertions(+) > > > > > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h > > > > index 3adc447b715f6..1c76754b616ff 100644 > > > > --- a/drivers/net/wireless/marvell/mwifiex/fw.h > > > > +++ b/drivers/net/wireless/marvell/mwifiex/fw.h > > > > @@ -415,6 +415,7 @@ 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 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 7f822660fd955..c055fdc7114ba 100644 > > > > --- a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c > > > > +++ b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c > > > > @@ -60,6 +60,9 @@ int mwifiex_set_secure_params(struct mwifiex_private *priv, > > > > 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; > > > > > > I feel like this relates to previous questions you've had [1], and while > > > I think the answer at the time made sense to me (basically, EAP and PSK > > > are mutually exclusive), it makes less sense to me here that PSK-SHA256 > > > is mutually exclusive with PSK. And in particular, IIUC, this means that > > > the ordering in a wpa_supplicant.conf line like > > > > > > key_mgmt=WPA-PSK WPA-PSK-SHA256 > > > > > > matters -- only the latter will actually be in use. > > > > > > Is that intended? Is this really a single-value field, and not a > > > multiple-option bitfield? > > > > It seems that when only the KEY_MGMT_PSK_SHA256 is set, then > > KEY_MGMT_PSK also works. Likewise, when only KEY_MGMT_SAE is set, then > > also KEY_MGMT_PSK_SHA256 and KEY_MGMT_PSK work. > > I gave it a test and also was surprised to see that we only have to set > > the "most advanced" bit which then includes the "less advanced" features > > automatically. > > Huh, that's interesting. So these KEY_MGMT* flags don't really mean what > they say. It might be nice to have some additional commentary in the > driver in that case. > > > I could change setting the key_mgmt bits to |= as it feels more natural > > and raises less eyebrows, but in my testing it didn't make a difference. Thinking about this again we really do need to use '|=' and not '=' to make the result independent of the ordering of the AKM suites array entries. > > That would make sense to me, but I think that's in conflict with what > David Lin said here: > > https://lore.kernel.org/all/PA4PR04MB9638B7F0F4E49F79057C15FBD1CD2@PA4PR04MB9638.eurprd04.prod.outlook.com/ > > "Firmware can only support one of WLAN_AKM_SUITE_8021X, > WLAN_AKM_SUITE_PSK, or WLAN_AKM_SUITE_SAE." I don't really know how this sentence was meant. It clearly works when both WLAN_AKM_SUITE_PSK and WLAN_AKM_SUITE_SAE are advertised. Of course in the only one of both is selected by the station. > > If that's true, then it seems like we need some kind of priority > conditions here (e.g., if PSK is provided, but then we see PSK_SHA256, > let PSK_SHA256 override -- but not vice versa). That might be pretty > ugly though. > > > BTW wpa_supplicant parses the key_mgmt options into a bitfield which is > > then evaluated elsewhere, so the order the AKM suites are passed to the > > kernel is always the same, regardless of the order they appear in the > > config. > > I hear you, but that's not really how we define kernel APIs -- by the > particular implementation of a single commonly-used user space. I know. I wrote this just as a note that we can't use wpa_supplicant out of the box to actually test how a different order of AKM suites in the array behaves. Sascha
Hi Brian and Sascha, > From: Sascha Hauer <s.hauer@pengutronix.de> > Sent: Monday, July 22, 2024 3:49 PM > To: Brian Norris <briannorris@chromium.org> > Cc: Francesco Dolcini <francesco@dolcini.it>; Kalle Valo <kvalo@kernel.org>; > linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; Francesco > Dolcini <francesco.dolcini@toradex.com>; David Lin <yu-hao.lin@nxp.com> > Subject: [EXT] Re: [PATCH v2 2/2] wifi: mwifiex: add support for > WPA-PSK-SHA256 > > Caution: This is an external email. Please take care when clicking links or > opening attachments. When in doubt, report the message using the 'Report > this email' button > > > On Fri, Jul 19, 2024 at 12:05:01PM -0700, Brian Norris wrote: > > [ +CC David, in case he has thoughts ] > > > > On Fri, Jul 19, 2024 at 08:04:59AM +0200, Sascha Hauer wrote: > > > On Thu, Jul 18, 2024 at 03:55:18PM -0700, Brian Norris wrote: > > > > On Wed, Jul 17, 2024 at 10:30:08AM +0200, Sascha Hauer wrote: > > > > > This adds support for the WPA-PSK AKM suite with SHA256 as > > > > > hashing method (WPA-PSK-SHA256). Tested with a wpa_supplicant > > > > > provided AP using key_mgmt=WPA-PSK-SHA256. > > > > > > > > > > Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > > > > --- > > > > > drivers/net/wireless/marvell/mwifiex/fw.h | 1 + > > > > > drivers/net/wireless/marvell/mwifiex/uap_cmd.c | 3 +++ > > > > > 2 files changed, 4 insertions(+) > > > > > > > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h > > > > > b/drivers/net/wireless/marvell/mwifiex/fw.h > > > > > index 3adc447b715f6..1c76754b616ff 100644 > > > > > --- a/drivers/net/wireless/marvell/mwifiex/fw.h > > > > > +++ b/drivers/net/wireless/marvell/mwifiex/fw.h > > > > > @@ -415,6 +415,7 @@ 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 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 7f822660fd955..c055fdc7114ba 100644 > > > > > --- a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c > > > > > +++ b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c > > > > > @@ -60,6 +60,9 @@ int mwifiex_set_secure_params(struct > mwifiex_private *priv, > > > > > 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; > > > > > > > > I feel like this relates to previous questions you've had [1], and > > > > while I think the answer at the time made sense to me (basically, > > > > EAP and PSK are mutually exclusive), it makes less sense to me > > > > here that PSK-SHA256 is mutually exclusive with PSK. And in > > > > particular, IIUC, this means that the ordering in a > > > > wpa_supplicant.conf line like > > > > > > > > key_mgmt=WPA-PSK WPA-PSK-SHA256 > > > > > > > > matters -- only the latter will actually be in use. > > > > > > > > Is that intended? Is this really a single-value field, and not a > > > > multiple-option bitfield? > > > > > > It seems that when only the KEY_MGMT_PSK_SHA256 is set, then > > > KEY_MGMT_PSK also works. Likewise, when only KEY_MGMT_SAE is set, > > > then also KEY_MGMT_PSK_SHA256 and KEY_MGMT_PSK work. > > > I gave it a test and also was surprised to see that we only have to > > > set the "most advanced" bit which then includes the "less advanced" > > > features automatically. > > > > Huh, that's interesting. So these KEY_MGMT* flags don't really mean > > what they say. It might be nice to have some additional commentary in > > the driver in that case. > > > > > I could change setting the key_mgmt bits to |= as it feels more > > > natural and raises less eyebrows, but in my testing it didn't make a > difference. > > Thinking about this again we really do need to use '|=' and not '=' > to make the result independent of the ordering of the AKM suites array entries. > Yes, for our private driver. It uses '|=" and can work for firmware of IW416 and IW61x. For nxpwifi, it will follow mwifiex first and will be updated to use "|=" later. > > > > That would make sense to me, but I think that's in conflict with what > > David Lin said here: > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore > > .kernel.org%2Fall%2FPA4PR04MB9638B7F0F4E49F79057C15FBD1CD2%40PA > 4PR04MB > > > 9638.eurprd04.prod.outlook.com%2F&data=05%7C02%7Cyu-hao.lin%40nxp.co > m% > > > 7C0bdf1446db20472a267008dcaa22c655%7C686ea1d3bc2b4c6fa92cd99c5c30 > 1635% > > > 7C0%7C0%7C638572313533588836%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi > MC4wLjAwM > > > DAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sd > ata=p > > oKXdL5f%2Bwp7uXaPqvl38IF9OS5XtRfir3xIyEoAV0E%3D&reserved=0 > > > > "Firmware can only support one of WLAN_AKM_SUITE_8021X, > > WLAN_AKM_SUITE_PSK, or WLAN_AKM_SUITE_SAE." > > I don't really know how this sentence was meant. It clearly works when both > WLAN_AKM_SUITE_PSK and WLAN_AKM_SUITE_SAE are advertised. Of course > in the only one of both is selected by the station. > Mwifiex supports a lot of legacy devices, I don't know if modifications of the coding for the data of TLV_TYPE_UAP_AKMP will affect existed devices or not. Maybe you can follow the patch for host mlme to add a flag like ''host_mlme_enabled'' to enable this kind of change for specific device. David
On Mon, Jul 22, 2024 at 09:48:59AM +0200, Sascha Hauer wrote: > On Fri, Jul 19, 2024 at 12:05:01PM -0700, Brian Norris wrote: > > On Fri, Jul 19, 2024 at 08:04:59AM +0200, Sascha Hauer wrote: > > > I could change setting the key_mgmt bits to |= as it feels more natural > > > and raises less eyebrows, but in my testing it didn't make a difference. > Thinking about this again we really do need to use '|=' and not '=' > to make the result independent of the ordering of the AKM suites array > entries. Yes, absolutely. I missed this while looking at your changes. Francesco
Hi David and Sascha, On Mon, Jul 22, 2024 at 1:46 AM David Lin <yu-hao.lin@nxp.com> wrote: > > From: Sascha Hauer <s.hauer@pengutronix.de> > > Thinking about this again we really do need to use '|=' and not '=' > > to make the result independent of the ordering of the AKM suites array entries. > > > > Yes, for our private driver. It uses '|=" and can work for firmware of IW416 and IW61x. > For nxpwifi, it will follow mwifiex first and will be updated to use "|=" later. Thanks for the reply, David. Treating this as a bitfield sounds good to me, then. > > > That would make sense to me, but I think that's in conflict with what > > > David Lin said here: ... > > > "Firmware can only support one of WLAN_AKM_SUITE_8021X, > > > WLAN_AKM_SUITE_PSK, or WLAN_AKM_SUITE_SAE." > > > > I don't really know how this sentence was meant. It clearly works when both > > WLAN_AKM_SUITE_PSK and WLAN_AKM_SUITE_SAE are advertised. Of course > > in the only one of both is selected by the station. Yeah, I was a little confused too. I don't have many AP systems to test though -- all my use cases are STA. > Mwifiex supports a lot of legacy devices, I don't know if modifications of the coding > for the data of TLV_TYPE_UAP_AKMP will affect existed devices or not. Maybe you > can follow the patch for host mlme to add a flag like ''host_mlme_enabled'' to enable > this kind of change for specific device. I don't love adding new flags for small changes just out of extreme caution. If we find a good reason to, that's an option, but in this case, it feels like the behavior is poorly defined and possibly inconsistent or broken with the current code. Specifically, if anyone was specifying PSK+EAP from user space, we didn't really guarantee behavior. If users were really using that previously and are broken by such a change ... well, we can figure out a way forward by introducing such a flag. Brian
On Mon, Jul 22, 2024 at 12:30:53PM -0700, Brian Norris wrote: > Hi David and Sascha, > > On Mon, Jul 22, 2024 at 1:46 AM David Lin <yu-hao.lin@nxp.com> wrote: > > > From: Sascha Hauer <s.hauer@pengutronix.de> > > > > Thinking about this again we really do need to use '|=' and not '=' > > > to make the result independent of the ordering of the AKM suites array entries. > > > > > > > Yes, for our private driver. It uses '|=" and can work for firmware of IW416 and IW61x. > > For nxpwifi, it will follow mwifiex first and will be updated to use "|=" later. > > Thanks for the reply, David. Treating this as a bitfield sounds good > to me, then. > > > > > That would make sense to me, but I think that's in conflict with what > > > > David Lin said here: > ... > > > > "Firmware can only support one of WLAN_AKM_SUITE_8021X, > > > > WLAN_AKM_SUITE_PSK, or WLAN_AKM_SUITE_SAE." > > > > > > I don't really know how this sentence was meant. It clearly works when both > > > WLAN_AKM_SUITE_PSK and WLAN_AKM_SUITE_SAE are advertised. Of course > > > in the only one of both is selected by the station. > > Yeah, I was a little confused too. I don't have many AP systems to > test though -- all my use cases are STA. > > > Mwifiex supports a lot of legacy devices, I don't know if modifications of the coding > > for the data of TLV_TYPE_UAP_AKMP will affect existed devices or not. Maybe you > > can follow the patch for host mlme to add a flag like ''host_mlme_enabled'' to enable > > this kind of change for specific device. > > I don't love adding new flags for small changes just out of extreme > caution. If we find a good reason to, that's an option, but in this > case, it feels like the behavior is poorly defined and possibly > inconsistent or broken with the current code. Specifically, if anyone > was specifying PSK+EAP from user space, we didn't really guarantee > behavior. If users were really using that previously and are broken by > such a change ... well, we can figure out a way forward by introducing > such a flag. +1, thanks Brian. I'll send an updated series. Sascha
diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h index 3adc447b715f6..1c76754b616ff 100644 --- a/drivers/net/wireless/marvell/mwifiex/fw.h +++ b/drivers/net/wireless/marvell/mwifiex/fw.h @@ -415,6 +415,7 @@ 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 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 7f822660fd955..c055fdc7114ba 100644 --- a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c +++ b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c @@ -60,6 +60,9 @@ int mwifiex_set_secure_params(struct mwifiex_private *priv, 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; default: break; }