| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Zp4O68Y6oss_pwMm@pengutronix.de>
Date: Mon, 22 Jul 2024 09:48:59 +0200
From: Sascha Hauer <s.hauer@...gutronix.de>
To: Brian Norris <briannorris@...omium.org>
Cc: Francesco Dolcini <francesco@...cini.it>, Kalle Valo <kvalo@...nel.org>,
linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
Francesco Dolcini <francesco.dolcini@...adex.com>,
David Lin <yu-hao.lin@....com>
Subject: Re: [PATCH v2 2/2] wifi: mwifiex: add support for WPA-PSK-SHA256
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@...adex.com>
> > > > Signed-off-by: Sascha Hauer <s.hauer@...gutronix.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 |
Powered by blists - more mailing lists