[ +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@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.
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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
"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
