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

[Brian Norris <briannorris@chromium.org>]

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