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