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@xxxxxxxxxxx>
> > Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> > ---
> >  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 |