[PATCH v2 0/2] mwifiex: add support for WPA-PSK-SHA256

[PATCH v2 0/2] mwifiex: add support for WPA-PSK-SHA256

[Sascha Hauer]

This series adds support for the WPA-PSK AKM suite with SHA256 as hashing
method (WPA-PSK-SHA256)

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
Changes in v2:
- set bss_config->protocol to zero to make the code clearer
- Link to v1: https://lore.kernel.org/r/20240716-mwifiex-wpa-psk-sha256-v1-0-05ae70f37bb3@pengutronix.de

---
Sascha Hauer (2):
      wifi: mwifiex: simplify WPA flags setting
      wifi: mwifiex: add support for WPA-PSK-SHA256

 drivers/net/wireless/marvell/mwifiex/fw.h      |  1 +
 drivers/net/wireless/marvell/mwifiex/uap_cmd.c | 31 +++++++++-----------------
 2 files changed, 12 insertions(+), 20 deletions(-)
---
base-commit: 0c3836482481200ead7b416ca80c68a29cfdaabd
change-id: 20240716-mwifiex-wpa-psk-sha256-2d2a9fc3b06b

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>

[PATCH v2 1/2] wifi: mwifiex: simplify WPA flags setting

[Sascha Hauer]

The WPA flags setting only depends on the wpa_versions bitfield and not
on the AKM suite, so move it out of the switch/case to simplify the code
a bit. Also set bss_config->protocol to zero explicitly. This is done
only to make the code clearer, bss_config has been zero alloced by the
caller, so should be zero already. No functional change intended.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/net/wireless/marvell/mwifiex/uap_cmd.c | 28 ++++++++------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
index 491e366119096..7f822660fd955 100644
--- a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
@@ -46,31 +46,19 @@ int mwifiex_set_secure_params(struct mwifiex_private *priv,
 
 	bss_config->key_mgmt_operation |= KEY_MGMT_ON_HOST;
 
+	bss_config->protocol = 0;
+	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;
+
 	for (i = 0; i < params->crypto.n_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;
 		default:
 			break;

-- 
2.39.2

Re: [PATCH v2 1/2] wifi: mwifiex: simplify WPA flags setting

[Francesco Dolcini]

On Wed, Jul 17, 2024 at 10:30:07AM +0200, Sascha Hauer wrote:
> The WPA flags setting only depends on the wpa_versions bitfield and not
> on the AKM suite, so move it out of the switch/case to simplify the code
> a bit. Also set bss_config->protocol to zero explicitly. This is done
> only to make the code clearer, bss_config has been zero alloced by the
> caller, so should be zero already. No functional change intended.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>

Re: [PATCH v2 1/2] wifi: mwifiex: simplify WPA flags setting

[Brian Norris]

On Wed, Jul 17, 2024 at 10:30:07AM +0200, Sascha Hauer wrote:
> The WPA flags setting only depends on the wpa_versions bitfield and not
> on the AKM suite, so move it out of the switch/case to simplify the code
> a bit. Also set bss_config->protocol to zero explicitly. This is done
> only to make the code clearer, bss_config has been zero alloced by the
> caller, so should be zero already. No functional change intended.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/net/wireless/marvell/mwifiex/uap_cmd.c | 28 ++++++++------------------
>  1 file changed, 8 insertions(+), 20 deletions(-)

Acked-by: Brian Norris <briannorris@chromium.org>

[PATCH v2 2/2] wifi: mwifiex: add support for WPA-PSK-SHA256

[Sascha Hauer]

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;
 		default:
 			break;
 		}

-- 
2.39.2

Re: [PATCH v2 2/2] wifi: mwifiex: add support for WPA-PSK-SHA256

[Brian Norris]

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/

Re: [PATCH v2 2/2] wifi: mwifiex: add support for WPA-PSK-SHA256

[Sascha Hauer]

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

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 2/2] wifi: mwifiex: add support for WPA-PSK-SHA256

[Brian Norris]

[ +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

Re: [PATCH v2 2/2] wifi: mwifiex: add support for WPA-PSK-SHA256

[Sascha Hauer]

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

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

RE: [EXT] Re: [PATCH v2 2/2] wifi: mwifiex: add support for WPA-PSK-SHA256

[David Lin]

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

Re: [EXT] Re: [PATCH v2 2/2] wifi: mwifiex: add support for WPA-PSK-SHA256

[Brian Norris]

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

Re: [EXT] Re: [PATCH v2 2/2] wifi: mwifiex: add support for WPA-PSK-SHA256

[Sascha Hauer]

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

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 2/2] wifi: mwifiex: add support for WPA-PSK-SHA256

[Francesco Dolcini]

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