LinuxLists.cc - [PATCH] wifi: brcmfmac: cfg80211: Use WSEC to set SAE password

2023-11-07 06:07:19

Subject: [PATCH] wifi: brcmfmac: cfg80211: Use WSEC to set SAE password

Using the WSEC command instead of sae_password seems to be the supported
mechanism on newer firmware, and also how the brcmdhd driver does it.

According to user reports [1], the sae_password codepath doesn't actually
work on machines with Cypress chips anyway, so no harm in removing it.

This makes WPA3 work with iwd, or with wpa_supplicant pending a support
patchset [2].

[1] https://rachelbythebay.com/w/2023/11/06/wpa3/
[2] http://lists.infradead.org/pipermail/hostap/2023-July/041653.html

Signed-off-by: Hector Martin <[email protected]>
---
.../broadcom/brcm80211/brcmfmac/cfg80211.c | 46 +++++++++-------------
.../broadcom/brcm80211/brcmfmac/fwil_types.h | 2 +-
2 files changed, 20 insertions(+), 28 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 2a90bb24ba77..138af70a33b8 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -1687,52 +1687,44 @@ static u16 brcmf_map_fw_linkdown_reason(const struct brcmf_event_msg *e)
return reason;
}

-static int brcmf_set_pmk(struct brcmf_if *ifp, const u8 *pmk_data, u16 pmk_len)
+static int brcmf_set_wsec(struct brcmf_if *ifp, const u8 *key, u16 key_len, u16 flags)
{
struct brcmf_pub *drvr = ifp->drvr;
struct brcmf_wsec_pmk_le pmk;
int err;

+ if (key_len > sizeof(pmk.key)) {
+ bphy_err(drvr, "key must be less than %zu bytes\n",
+ sizeof(pmk.key));
+ return -EINVAL;
+ }
+
memset(&pmk, 0, sizeof(pmk));

- /* pass pmk directly */
- pmk.key_len = cpu_to_le16(pmk_len);
- pmk.flags = cpu_to_le16(0);
- memcpy(pmk.key, pmk_data, pmk_len);
+ /* pass key material directly */
+ pmk.key_len = cpu_to_le16(key_len);
+ pmk.flags = cpu_to_le16(flags);
+ memcpy(pmk.key, key, key_len);

- /* store psk in firmware */
+ /* store key material in firmware */
err = brcmf_fil_cmd_data_set(ifp, BRCMF_C_SET_WSEC_PMK,
&pmk, sizeof(pmk));
if (err < 0)
bphy_err(drvr, "failed to change PSK in firmware (len=%u)\n",
- pmk_len);
+ key_len);

return err;
}

+static int brcmf_set_pmk(struct brcmf_if *ifp, const u8 *pmk_data, u16 pmk_len)
+{
+ return brcmf_set_wsec(ifp, pmk_data, pmk_len, 0);
+}
+
static int brcmf_set_sae_password(struct brcmf_if *ifp, const u8 *pwd_data,
u16 pwd_len)
{
- struct brcmf_pub *drvr = ifp->drvr;
- struct brcmf_wsec_sae_pwd_le sae_pwd;
- int err;
-
- if (pwd_len > BRCMF_WSEC_MAX_SAE_PASSWORD_LEN) {
- bphy_err(drvr, "sae_password must be less than %d\n",
- BRCMF_WSEC_MAX_SAE_PASSWORD_LEN);
- return -EINVAL;
- }
-
- sae_pwd.key_len = cpu_to_le16(pwd_len);
- memcpy(sae_pwd.key, pwd_data, pwd_len);
-
- err = brcmf_fil_iovar_data_set(ifp, "sae_password", &sae_pwd,
- sizeof(sae_pwd));
- if (err < 0)
- bphy_err(drvr, "failed to set SAE password in firmware (len=%u)\n",
- pwd_len);
-
- return err;
+ return brcmf_set_wsec(ifp, pwd_data, pwd_len, BRCMF_WSEC_PASSPHRASE);
}

static void brcmf_link_down(struct brcmf_cfg80211_vif *vif, u16 reason,
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
index 611d1a6aabb9..b68c46caabe8 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
@@ -584,7 +584,7 @@ struct brcmf_wsec_key_le {
struct brcmf_wsec_pmk_le {
__le16 key_len;
__le16 flags;
- u8 key[2 * BRCMF_WSEC_MAX_PSK_LEN + 1];
+ u8 key[BRCMF_WSEC_MAX_SAE_PASSWORD_LEN];
};

/**

---
base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa
change-id: 20231107-brcmfmac-wpa3-9e5f66e8be34

Best regards,
--
Hector Martin <[email protected]>

2023-12-17 11:25:27

by Kalle Valo

[permalink] [raw]

Subject: Re: [PATCH] wifi: brcmfmac: cfg80211: Use WSEC to set SAE password

Hector Martin <[email protected]> wrote:

> Using the WSEC command instead of sae_password seems to be the supported
> mechanism on newer firmware, and also how the brcmdhd driver does it.
>
> According to user reports [1], the sae_password codepath doesn't actually
> work on machines with Cypress chips anyway, so no harm in removing it.
>
> This makes WPA3 work with iwd, or with wpa_supplicant pending a support
> patchset [2].
>
> [1] https://rachelbythebay.com/w/2023/11/06/wpa3/
> [2] http://lists.infradead.org/pipermail/hostap/2023-July/041653.html
>
> Signed-off-by: Hector Martin <[email protected]>
> Reviewed-by: Neal Gompa <[email protected]>

Arend, what do you think?

We recently talked about people testing brcmfmac patches, has anyone else
tested this?

--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2023-12-19 08:52:40

On Thu, Dec 21, 2023 at 3:40 PM Marcel Holtmann <[email protected]> wrote:
>
> Hi Julian,
>
> >>>>>> Using the WSEC command instead of sae_password seems to be the supported
> >>>>>> mechanism on newer firmware, and also how the brcmdhd driver does it.
> >>>>>>
> >>>>>> According to user reports [1], the sae_password codepath doesn't actually
> >>>>>> work on machines with Cypress chips anyway, so no harm in removing it.
> >>>>>>
> >>>>>> This makes WPA3 work with iwd, or with wpa_supplicant pending a support
> >>>>>> patchset [2].
> >>>>>>
> >>>>>> [1] https://rachelbythebay.com/w/2023/11/06/wpa3/
> >>>>>> [2] http://lists.infradead.org/pipermail/hostap/2023-July/041653.html
> >>>>>>
> >>>>>> Signed-off-by: Hector Martin <[email protected]>
> >>>>>> Reviewed-by: Neal Gompa <[email protected]>
> >>>>>
> >>>>> Arend, what do you think?
> >>>>>
> >>>>> We recently talked about people testing brcmfmac patches, has anyone else
> >>>>> tested this?
> >>>>
> >>>> Not sure I already replied so maybe I am repeating myself. I would prefer
> >>>> to keep the Cypress sae_password path as well although it reportedly does
> >>>> not work. The vendor support in the driver can be used to accommodate for
> >>>> that. The other option would be to have people with Cypress chipset test
> >>>> this patch. If that works for both we can consider dropping the
> >>>> sae_password path.
> >>>>
> >>>> Regards,
> >>>> Arend
> >>>
> >>> So, if nobody from Cypress chimes in ever, and nobody cares nor tests
> >>> Cypress chipsets, are we keeping any and all existing Cypress code-paths
> >>> as bitrotting code forever and adding gratuitous conditionals every time
> >>> any functionality needs to change "just in case it breaks Cypress" even
> >>> though it has been tested compatible on Broadcom chipsets/firmware?
> >>>
> >>> Because that's not sustainable long term.
> >>
> >> You should look into WEXT just for the fun of it. If it were up to me
> >> and a bunch of other people that would have been gone decades ago. Maybe
> >> a bad example if the sae_password is indeed not working, but the Cypress
> >> chipset is used in RPi3 and RPi4 so there must be a couple of users.
> >
> > There are reports that WPA3 is broken on the Cypress chipsets the
> > Raspberry Pis are using and this patch fixes it:
> > https://rachelbythebay.com/w/2023/11/06/wpa3/
> >
> > Based on that, it appears that all known users of WPA3 capable
> > hardware with this driver require this fix.
>
> the Pis are all using an outdated firmware. In their distro they put the
> firmware already under the alternates systems, but it just lacks the SAE
> offload support that is required to make WPA3 work. The linux-firmware
> version does the trick nicely.
>
> I documented what I did to make this work on Pi5 (note that I normally
> use Fedora on Pi4 and thus never encountered this issue)
>
> https://holtmann.dev/enabling-wpa3-on-raspberry-pi/
>
> However you need to use iwd and not hope that you get a wpa_supplicant
> released version that will work.
>
> So whole game of wpa_supplicant is vendor specific to the company that
> provides the driver is also insane, but that is another story. Use iwd
> and you can most likely have WPA3 support if you have the right firmware.
>

wpa_supplicant is perfectly fine if the necessary patches are
backported, as Fedora has done:
https://src.fedoraproject.org/rpms/wpa_supplicant/c/99f4bf2096d3976cee01c499d7a30c1376f5f0f7

--
真実はいつも一つ！/ Always, there's only one truth!

2023-12-22 05:10:53

by Hector Martin

[permalink] [raw]

Subject: Re: [PATCH] wifi: brcmfmac: cfg80211: Use WSEC to set SAE password

On 2023/12/21 18:57, Arend van Spriel wrote:
> - [email protected]
>
> On 12/21/2023 1:49 AM, Hector Martin wrote:
>>
>>
>> On 2023/12/21 4:36, Arend van Spriel wrote:
>>> On 12/20/2023 7:14 PM, Hector Martin wrote:
>>>>
>>>>
>>>> On 2023/12/20 19:20, Kalle Valo wrote:
>>>>> Linus Torvalds <[email protected]> writes:
>>>>>
>>>>>>> Just recently a patch was posted to remove the Infineon list from
>>>>>>> MAINTAINERS because that company cares so little they have literally
>>>>>>> stopped accepting emails from us. Meanwhile they are telling their
>>>>>>> customers that they do not recommend upstream brcmfmac and they should
>>>>>>> use their downstream driver [1].
>>>>>>
>>>>>> Unquestionably broadcom is not helping maintain things, and I think it
>>>>>> should matter.
>>>>>>
>>>>>> As Hector says, they point to their random driver dumps on their site
>>>>>> that you can't even download unless you are a "Broadcom community
>>>>>> member" or whatever, and hey - any company that works that way should
>>>>>> be seen as pretty much hostile to any actual maintenance and proper
>>>>>> development.
>>>>>
>>>>> Sadly this is the normal in the wireless world. All vendors focus on the
>>>>> latest generation, currently it's Wi-Fi 7, and lose interest on older
>>>>> generations. And vendors lose focus on the upstream drivers even faster,
>>>>> usually after a customer project ends.
>>>>>
>>>>> So in practise what we try to do is keep the drivers working somehow on
>>>>> our own, even after the vendors are long gone. If we would deliberately
>>>>> allow breaking drivers because vendor/corporations don't support us, I
>>>>> suspect we would have sevaral broken drivers in upstream.
>>>>>
>>>>>> If Daniel and Hector are responsive to actual problem reports for the
>>>>>> changes they cause, I do think that should count a lot.
>>>>>
>>>>> Sure, but they could also respect to the review comments. I find Arend's
>>>>> proposal is reasonable and that's what I would implement in v2. We
>>>>> (linux-wireless) make abstractions to workaround firmware problems or
>>>>> interface conflicts all the time, just look at ath10k for example. I
>>>>> would not be surprised if we need to add even more abstractions to
>>>>> brcmfmac in the future. And Arend is the expert here, he has best
>>>>> knowledge of Broadcom devices and I trust him.
>>>>>
>>>>> Has anyone even investigated what it would need to implement Arend's
>>>>> proposal? At least I don't see any indication of that.
>>>>
>>>> Of course we can implement it (and we will as we actually got a report
>>>> of this patch breaking Cypress now, finally).
>>>>
>>>> The question was never whether it could be done, we're already doing a
>>>> bunch of abstractions to deal with just the Broadcom-only side of things
>>>> too. The point I was trying to make is that we need to *know* what
>>>> firmware abstractions we need and *why* they are needed. We can't just
>>>> say, for every change, "well, nobody knows if the existing code works or
>>>> not, so let's just add an abstraction just in case the change breaks
>>>> something". As far as anyone involved in the discussions until now could
>>>> tell, this code was just something some Cypress person dumped upstream,
>>>> and nobody involved was being responsive to any of our inquiries, so
>>>> there was no way to be certain it worked at all, whether it was
>>>> supported in public firmware, or anything else.
>>>>
>>>> *Now* that we know the existing code is actually functional and not just
>>>> dead/broken, and that the WSEC approach is conversely not functional on
>>>> the Cypress firmwares, it makes sense to introduce an abstraction.
>>>
>>> Just a quick look in the git history could have told you that it was not
>>> just dumped upstream and at least one person was using it and extended
>>> it for 802.11r support (fast-roaming):
>>>
>>>
>>> author Paweł Drewniak <[email protected]> 2021-08-24 23:13:30 +0100
>>> committer Kalle Valo <[email protected]> 2021-08-29 11:33:07 +0300
>>> commit 4b51de063d5310f1fb297388b7955926e63e45c9 (patch)
>>> tree ba2ccb5cbd055d482a8daa263f5e53531c07667f
>>> /drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>> parent 81f9ebd43659320a88cae8ed5124c50b4d47ab66 (diff)
>>> download wireless-4b51de063d5310f1fb297388b7955926e63e45c9.tar.gz
>>> brcmfmac: Add WPA3 Personal with FT to supported cipher suites
>>> This allows the driver to connect to BSSIDs supporting SAE with 802.11r.
>>> Tested on Raspberry Pi 4 Model B (STA) and UniFi 6LR/OpenWRT 21.02.0-rc2.
>>> AP was set to 'sae-mixed' (WPA2/3 Personal).
>>>
>>> Signed-off-by: Paweł Drewniak <[email protected]>
>>> Signed-off-by: Kalle Valo <[email protected]>
>>> Link: https://lore.kernel.org/r/[email protected]
>>
>> Sure, but we also had user reports of it *not* actually working (maybe
>> it regressed?). We didn't know whether it was functional with the
>> linux-firmware blobs in any way, I wanted confirmation of that. And we
>> also didn't know that the patch *would* break it at all; perhaps the
>> Cypress firmware had also grown support for the WSEC mechanism.
>>
>> That's why I wanted someone to actually confirm the code worked (in some
>> subset of cases) and the patch didn't, before starting to introduce
>> conditionals. There is, of course, also the element that the Cypress
>> side has been long unmaintained, and if nobody is testing/giving
>> feedback/complaining, perhaps it's better to err on the side of maybe
>> breaking something and see if that gets someone to come out of the
>> woodwork if it really breaks, rather than tiptoeing around the code
>> without knowing what's going on and without anyone actually testing things.
>
> That is because you distrust the intent that Cypress was really
> contributing. They were and I trusted them to not just throw in a
> feature like WPA3. When Infineon took over they went mute. Upon
> reviewing your patch (again) I also sent an email to them asking
> specifically about the status of the sae_password interface. I did not
> use the mailing list which indeed bounces these days (hence removed
> them) but the last living soul that I had contact with about a year ago
> whether they were still comitted to be involved. I guess out of
> politeness or embarrassment I got confirmation they were and never heard
> from him again. The query about the sae_password interface is still pending.

If only corporate acquisition politics didn't repeatedly throw a wrench
into this one... :/

This is where we are though, Infineon clearly doesn't care, so it's time
to move on.

>> It's not about this *specific* patch, it's about the general situation
>> of not being able to touch firmware interfaces "just in case Cypress
>> breaks" being unsustainable in the long term. I wasn't pushing back
>> because I think this particular one will be hard, I was pushing back
>> because I can read the tea leaves and see this is not going to end well
>> if it's the approach we start taking for everything. We *need* someone
>> to be testing patches on Cypress, we can't just "try not to touch it"
>> and cross our fingers. That just ends in disaster, we are not going to
>> succeed in not breaking it either way and it's going to make the driver
>> worse.
>
> I admire you ability of reading tea leaves. You saw the Grim I reckon.
> Admittedly your responses on every comment from my side (or Kalle for
> that matter) was polarizing every discussion. That is common way people
> treat each other nowadays especially online where a conversation is just
> a pile of text going shit. It does not bring out the best in me either,
> but it was draining every ounce of energy from me so better end it by
> stepping out.

The hilariously outdated kernel development model surely doesn't help
either (I've stated my opinion on this quite a few times if you've
followed around) ;)

This stuff gets *really* frustrating when you're trying to improve what
is, I hope we can all admit, an undermaintained driver (that is not to
say it's anyone's fault personally), and end up getting held back due to
everything from coding style nitpicks to people not having the time to
be responsive. It's just not helpful. It's important to know when to
step aside and let people actually get stuff done.

When Daniel started sending me brcmfmac patches downstream, I took a
look at a few of them, decided he knew what he was doing, and just
started pulling in his branches wholesale. Was it perfect? No, I had to
debug at least one regression at one point. But it took me less time to
do that than it would've to go through the commits with a fine toothed
comb, so it was clearly the right decision.

That is not to say that should be the standard upstream (we make a point
of moving fast and breaking things more downstream, since it's a proving
ground for what eventually will be upstreamed), but I think it does
demonstrate the kind of delegation ability that is sorely lacking in
many drivers and subsystems in the kernel these days. Maintainers become
entrenched in their position, long beyond the point where they have the
time/motivation/ability to drive the code forward, and end up in the way
of new people who are trying to make a difference. I think Linus knows
full well the kernel maintainer community is stagnating.

That doesn't mean people should step down entirely. But it does mean
they need to recognize when this is happening and, at least, proactively
try to bring new people in, instead of just continuing to play a
gatekeeping role. The role of maintainers should not be that of a wall
people have to climb over to get their changes in, it should be to guide
new contributors and help onboard people who can contribute, as peers
and eventually as future maintainers.

Kalle, in the other thread you said "this is not fun anymore, this is
more like a business with requirements and demands coming from
everywhere.". That's what it feels like to us when our changes get
rejected because the local vars aren't in reverse Christmas tree order,
or because our commit messages have "v2:" in them. It feels like some
manager is trying to justify their position by creating busywork for
everyone else. Nobody should actually care about any of those things,
and if they do, they need to step back and really ask themselves how
they ended up believing that. If the goal is to enforce a reasonable
shared coding style so things don't spiral into chaos, FFS, let's just
do what every other project does these days and adopt clang-format. Then
*all* of us can stop wasting time on these trivialities and go back to
getting stuff done. And really, nobody cares about commit messages as
long as the tags are right, the subject line is succinct, and the
important information is in there. Extra stuff never hurt anyone.

> I added the ground work for multi-vendor support, but have not decided
> on the approach to take. Abstract per firmware interface primitive or
> simply have a cfg80211.c and fwil_types.h per vendor OR implement a
> vendor-specific cfg80211 callback and override the default callback
> during the driver attach, ie. in brcmf_fwvid_wcc_attach(). The latter
> duplicates things, but lean towards that as it may be easier on the
> long-term. What do your tea leaves tell you ;-)

FWIW, I was hoping you'd stay on at least as a reviewer. Your
contributions are valuable. You obviously know the driver and hardware
much better than most people. I encourage you to, at least, post a v2 of
the MAINTAINERS patch with yourself as an R: line.

As far as the actual driver abstraction architecture, I'm going to leave
it to Daniel to decide what makes the most sense, since he's the one
introducing new mechanisms for that already. There's always room for
refactoring later though, depending on the direction things take with
the vendor split. BTW, clang-format also makes refactoring a lot less
painful ;)

- Hector

2023-12-22 05:28:28

by Hector Martin

[permalink] [raw]

Subject: Re: [PATCH] wifi: brcmfmac: cfg80211: Use WSEC to set SAE password

On 2023/12/21 23:04, Arend van Spriel wrote:
> From: Hector Martin <[email protected]>
>
> Using the WSEC command instead of sae_password seems to be the supported
> mechanism on newer firmware, and also how the brcmdhd driver does it.
>
> According to user reports [1], the sae_password codepath doesn't actually
> work on machines with Cypress chips anyway, so no harm in removing it.
>
> This makes WPA3 work with iwd, or with wpa_supplicant pending a support
> patchset [2].
>
> [1] https://rachelbythebay.com/w/2023/11/06/wpa3/
> [2] http://lists.infradead.org/pipermail/hostap/2023-July/041653.html
>
> Signed-off-by: Hector Martin <[email protected]>
> Reviewed-by: Neal Gompa <[email protected]>
> Signed-off-by: Paweł Drewniak <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>
> [[email protected]: use multi-vendor framework]
> Signed-off-by: Arend van Spriel <[email protected]>
> ---
> Here is how the multi-vendor code could support both Cypress and
> WCC mobility chips. As said it might be easier to just override
> entire cfg80211 callback operations.
>
> Regards,
> Arend
> ---
> .../broadcom/brcm80211/brcmfmac/cfg80211.c | 13 +++++++---
> .../broadcom/brcm80211/brcmfmac/cfg80211.h | 3 +++
> .../broadcom/brcm80211/brcmfmac/fwil.c | 1 +
> .../broadcom/brcm80211/brcmfmac/fwil_types.h | 2 +-
> .../broadcom/brcm80211/brcmfmac/fwvid.h | 14 ++++++++++
> .../broadcom/brcm80211/brcmfmac/wcc/core.c | 26 +++++++++++++++++++
> 6 files changed, 55 insertions(+), 4 deletions(-)
>
[snip]
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/wcc/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/wcc/core.c
> index 5573a47766ad..01025d5c137b 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/wcc/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/wcc/core.c
> @@ -7,6 +7,7 @@
> #include <core.h>
> #include <bus.h>
> #include <fwvid.h>
> +#include <fwil.h>
>
> #include "vops.h"
>
> @@ -21,7 +22,32 @@ static void brcmf_wcc_detach(struct brcmf_pub *drvr)
> pr_debug("%s: executing\n", __func__);
> }
>
> +static int brcmf_wcc_set_sae_pwd(struct brcmf_if *ifp,
> + struct cfg80211_crypto_settings *crypto)
> +{
> + struct brcmf_pub *drvr = ifp->drvr;
> + struct brcmf_wsec_pmk_le pmk;
> + int err;
> +
> + memset(&pmk, 0, sizeof(pmk));
> +
> + /* pass pmk directly */
> + pmk.key_len = cpu_to_le16(crypto->sae_pwd_len);
> + pmk.flags = cpu_to_le16(BRCMF_WSEC_PASSPHRASE);
> + memcpy(pmk.key, crypto->sae_pwd, crypto->sae_pwd_len);
> +
> + /* store psk in firmware */
> + err = brcmf_fil_cmd_data_set(ifp, BRCMF_C_SET_WSEC_PMK,
> + &pmk, sizeof(pmk));
> + if (err < 0)
> + bphy_err(drvr, "failed to change PSK in firmware (len=%u)\n",
> + crypto->sae_pwd_len);
> +
> + return err;
> +}
> +
> const struct brcmf_fwvid_ops brcmf_wcc_ops = {
> .attach = brcmf_wcc_attach,
> .detach = brcmf_wcc_detach,
> + .set_sae_password = brcmf_wcc_set_sae_pwd,
> };

If we're going to move this into per-vendor code, we should also move
the Cypress codepath repectively. Is there a reason why we can't just
rename and export brcmf_set_wsec (as in my original patch) instead of
duplicating the code here? Fundamentally this code already exists in
common code for WPA support, so why not reuse it for SAE for WCC?

- Hector

2023-12-22 05:35:46

by Hector Martin

[permalink] [raw]

Subject: Re: [PATCH] wifi: brcmfmac: cfg80211: Use WSEC to set SAE password

On 2023/12/21 23:11, Arend van Spriel wrote:
> On 12/21/2023 3:04 PM, Arend van Spriel wrote:
>> From: Hector Martin <[email protected]>
>>
>> Using the WSEC command instead of sae_password seems to be the supported
>> mechanism on newer firmware, and also how the brcmdhd driver does it.
>>
>> According to user reports [1], the sae_password codepath doesn't actually
>> work on machines with Cypress chips anyway, so no harm in removing it.
>>
>> This makes WPA3 work with iwd, or with wpa_supplicant pending a support
>> patchset [2].
>>
>> [1] https://rachelbythebay.com/w/2023/11/06/wpa3/
>> [2] http://lists.infradead.org/pipermail/hostap/2023-July/041653.html
>>
>> Signed-off-by: Hector Martin <[email protected]>
>> Reviewed-by: Neal Gompa <[email protected]>
>> Signed-off-by: Paweł Drewniak <[email protected]>
>> Signed-off-by: Kalle Valo <[email protected]>
>
> Not sure where these signed-off-by entries are coming from. I downloaded
> the patch from patchwork. Just did not want to tinker on it.

I have no idea either. If they're an unintended artifact then you
probably want to remove them. As far as I can tell only my S-o-b and
Neal's R-b were part of the conversation.

FWIW I always use `b4 am` to download patches, I've never seen it stick
in weird tags.

- Hector

2023-12-22 06:16:55

by Marcel Holtmann

[permalink] [raw]

Subject: Re: [PATCH] wifi: brcmfmac: cfg80211: Use WSEC to set SAE password

Hi Neal,

>>>>>>>> Using the WSEC command instead of sae_password seems to be the supported
>>>>>>>> mechanism on newer firmware, and also how the brcmdhd driver does it.
>>>>>>>>
>>>>>>>> According to user reports [1], the sae_password codepath doesn't actually
>>>>>>>> work on machines with Cypress chips anyway, so no harm in removing it.
>>>>>>>>
>>>>>>>> This makes WPA3 work with iwd, or with wpa_supplicant pending a support
>>>>>>>> patchset [2].
>>>>>>>>
>>>>>>>> [1] https://rachelbythebay.com/w/2023/11/06/wpa3/
>>>>>>>> [2] http://lists.infradead.org/pipermail/hostap/2023-July/041653.html
>>>>>>>>
>>>>>>>> Signed-off-by: Hector Martin <[email protected]>
>>>>>>>> Reviewed-by: Neal Gompa <[email protected]>
>>>>>>>
>>>>>>> Arend, what do you think?
>>>>>>>
>>>>>>> We recently talked about people testing brcmfmac patches, has anyone else
>>>>>>> tested this?
>>>>>>
>>>>>> Not sure I already replied so maybe I am repeating myself. I would prefer
>>>>>> to keep the Cypress sae_password path as well although it reportedly does
>>>>>> not work. The vendor support in the driver can be used to accommodate for
>>>>>> that. The other option would be to have people with Cypress chipset test
>>>>>> this patch. If that works for both we can consider dropping the
>>>>>> sae_password path.
>>>>>>
>>>>>> Regards,
>>>>>> Arend
>>>>>
>>>>> So, if nobody from Cypress chimes in ever, and nobody cares nor tests
>>>>> Cypress chipsets, are we keeping any and all existing Cypress code-paths
>>>>> as bitrotting code forever and adding gratuitous conditionals every time
>>>>> any functionality needs to change "just in case it breaks Cypress" even
>>>>> though it has been tested compatible on Broadcom chipsets/firmware?
>>>>>
>>>>> Because that's not sustainable long term.
>>>>
>>>> You should look into WEXT just for the fun of it. If it were up to me
>>>> and a bunch of other people that would have been gone decades ago. Maybe
>>>> a bad example if the sae_password is indeed not working, but the Cypress
>>>> chipset is used in RPi3 and RPi4 so there must be a couple of users.
>>>
>>> There are reports that WPA3 is broken on the Cypress chipsets the
>>> Raspberry Pis are using and this patch fixes it:
>>> https://rachelbythebay.com/w/2023/11/06/wpa3/
>>>
>>> Based on that, it appears that all known users of WPA3 capable
>>> hardware with this driver require this fix.
>>
>> the Pis are all using an outdated firmware. In their distro they put the
>> firmware already under the alternates systems, but it just lacks the SAE
>> offload support that is required to make WPA3 work. The linux-firmware
>> version does the trick nicely.
>>
>> I documented what I did to make this work on Pi5 (note that I normally
>> use Fedora on Pi4 and thus never encountered this issue)
>>
>> https://holtmann.dev/enabling-wpa3-on-raspberry-pi/
>>
>> However you need to use iwd and not hope that you get a wpa_supplicant
>> released version that will work.
>>
>> So whole game of wpa_supplicant is vendor specific to the company that
>> provides the driver is also insane, but that is another story. Use iwd
>> and you can most likely have WPA3 support if you have the right firmware.
>>
>
> wpa_supplicant is perfectly fine if the necessary patches are
> backported, as Fedora has done:
> https://src.fedoraproject.org/rpms/wpa_supplicant/c/99f4bf2096d3976cee01c499d7a30c1376f5f0f7

my point exactly. Tell me when the last hostap release was and how much
delta that has to HEAD. So the wpa_supplicant you have in Fedora is
essentially yet another fork of an upstream project. One of many.

Reality is there is limited interest to make WiFi great on Linux. And
if you really look honestly, then you realize it is pretty bad.

Regards

Marcel

2023-12-22 07:31:08

On 12/22/2023 1:03 AM, Neal Gompa wrote:
> On Thu, Dec 21, 2023 at 3:40 PM Marcel Holtmann <[email protected]> wrote:
>>
>> Hi Julian,
>>
>>>>>>>> Using the WSEC command instead of sae_password seems to be the supported
>>>>>>>> mechanism on newer firmware, and also how the brcmdhd driver does it.
>>>>>>>>
>>>>>>>> According to user reports [1], the sae_password codepath doesn't actually
>>>>>>>> work on machines with Cypress chips anyway, so no harm in removing it.
>>>>>>>>
>>>>>>>> This makes WPA3 work with iwd, or with wpa_supplicant pending a support
>>>>>>>> patchset [2].
>>>>>>>>
>>>>>>>> [1] https://rachelbythebay.com/w/2023/11/06/wpa3/
>>>>>>>> [2] http://lists.infradead.org/pipermail/hostap/2023-July/041653.html
>>>>>>>>
>>>>>>>> Signed-off-by: Hector Martin <[email protected]>
>>>>>>>> Reviewed-by: Neal Gompa <[email protected]>
>>>>>>>
>>>>>>> Arend, what do you think?
>>>>>>>
>>>>>>> We recently talked about people testing brcmfmac patches, has anyone else
>>>>>>> tested this?
>>>>>>
>>>>>> Not sure I already replied so maybe I am repeating myself. I would prefer
>>>>>> to keep the Cypress sae_password path as well although it reportedly does
>>>>>> not work. The vendor support in the driver can be used to accommodate for
>>>>>> that. The other option would be to have people with Cypress chipset test
>>>>>> this patch. If that works for both we can consider dropping the
>>>>>> sae_password path.
>>>>>>
>>>>>> Regards,
>>>>>> Arend
>>>>>
>>>>> So, if nobody from Cypress chimes in ever, and nobody cares nor tests
>>>>> Cypress chipsets, are we keeping any and all existing Cypress code-paths
>>>>> as bitrotting code forever and adding gratuitous conditionals every time
>>>>> any functionality needs to change "just in case it breaks Cypress" even
>>>>> though it has been tested compatible on Broadcom chipsets/firmware?
>>>>>
>>>>> Because that's not sustainable long term.
>>>>
>>>> You should look into WEXT just for the fun of it. If it were up to me
>>>> and a bunch of other people that would have been gone decades ago. Maybe
>>>> a bad example if the sae_password is indeed not working, but the Cypress
>>>> chipset is used in RPi3 and RPi4 so there must be a couple of users.
>>>
>>> There are reports that WPA3 is broken on the Cypress chipsets the
>>> Raspberry Pis are using and this patch fixes it:
>>> https://rachelbythebay.com/w/2023/11/06/wpa3/
>>>
>>> Based on that, it appears that all known users of WPA3 capable
>>> hardware with this driver require this fix.
>>
>> the Pis are all using an outdated firmware. In their distro they put the
>> firmware already under the alternates systems, but it just lacks the SAE
>> offload support that is required to make WPA3 work. The linux-firmware
>> version does the trick nicely.
>>
>> I documented what I did to make this work on Pi5 (note that I normally
>> use Fedora on Pi4 and thus never encountered this issue)
>>
>> https://holtmann.dev/enabling-wpa3-on-raspberry-pi/
>>
>> However you need to use iwd and not hope that you get a wpa_supplicant
>> released version that will work.
>>
>> So whole game of wpa_supplicant is vendor specific to the company that
>> provides the driver is also insane, but that is another story. Use iwd
>> and you can most likely have WPA3 support if you have the right firmware.
>>
>
> wpa_supplicant is perfectly fine if the necessary patches are
> backported, as Fedora has done:
> https://src.fedoraproject.org/rpms/wpa_supplicant/c/99f4bf2096d3976cee01c499d7a30c1376f5f0f7

The brcmfmac firmware has its own 802.11 stack implementation and as
such it has a SME running in firmware which means the driver only
implements the NL80211_CMD_CONNECT primitive. Now if the firmware also
has in-driver supplicant (*-idsup-*) supporting SAE (*-sae-*) it can be
offloaded. That is what Cypress went with at least for upstream. For
firmware without these in the firmware target string the driver needs to
implement support for NL80211_CMD_EXTERNAL_AUTH, which is what we opted
for in Broadcom BCA (or WCC-Access as we call it these days). So I don't
think it is a fair assessment to call the wpa_supplicant implementation
vendor specific.

Regards,
Arend

Attachments:

smime.p7s (4.12 kB)
S/MIME Cryptographic Signature

2024-01-07 09:51:42

by Arend van Spriel

[permalink] [raw]

Subject: Re: [PATCH] wifi: brcmfmac: cfg80211: Use WSEC to set SAE password

On 12/22/2023 6:10 AM, Hector Martin wrote:
>
>
> On 2023/12/21 18:57, Arend van Spriel wrote:
>> - [email protected]
>>
>> On 12/21/2023 1:49 AM, Hector Martin wrote:
>>>
>>>
>>> On 2023/12/21 4:36, Arend van Spriel wrote:
>>>> On 12/20/2023 7:14 PM, Hector Martin wrote:
>>>>>
>>>>>
>>>>> On 2023/12/20 19:20, Kalle Valo wrote:
>>>>>> Linus Torvalds <[email protected]> writes:
>>>>>>
>>>>>>>> Just recently a patch was posted to remove the Infineon list from
>>>>>>>> MAINTAINERS because that company cares so little they have literally
>>>>>>>> stopped accepting emails from us. Meanwhile they are telling their
>>>>>>>> customers that they do not recommend upstream brcmfmac and they should
>>>>>>>> use their downstream driver [1].
>>>>>>>
>>>>>>> Unquestionably broadcom is not helping maintain things, and I think it
>>>>>>> should matter.
>>>>>>>
>>>>>>> As Hector says, they point to their random driver dumps on their site
>>>>>>> that you can't even download unless you are a "Broadcom community
>>>>>>> member" or whatever, and hey - any company that works that way should
>>>>>>> be seen as pretty much hostile to any actual maintenance and proper
>>>>>>> development.
>>>>>>
>>>>>> Sadly this is the normal in the wireless world. All vendors focus on the
>>>>>> latest generation, currently it's Wi-Fi 7, and lose interest on older
>>>>>> generations. And vendors lose focus on the upstream drivers even faster,
>>>>>> usually after a customer project ends.
>>>>>>
>>>>>> So in practise what we try to do is keep the drivers working somehow on
>>>>>> our own, even after the vendors are long gone. If we would deliberately
>>>>>> allow breaking drivers because vendor/corporations don't support us, I
>>>>>> suspect we would have sevaral broken drivers in upstream.
>>>>>>
>>>>>>> If Daniel and Hector are responsive to actual problem reports for the
>>>>>>> changes they cause, I do think that should count a lot.
>>>>>>
>>>>>> Sure, but they could also respect to the review comments. I find Arend's
>>>>>> proposal is reasonable and that's what I would implement in v2. We
>>>>>> (linux-wireless) make abstractions to workaround firmware problems or
>>>>>> interface conflicts all the time, just look at ath10k for example. I
>>>>>> would not be surprised if we need to add even more abstractions to
>>>>>> brcmfmac in the future. And Arend is the expert here, he has best
>>>>>> knowledge of Broadcom devices and I trust him.
>>>>>>
>>>>>> Has anyone even investigated what it would need to implement Arend's
>>>>>> proposal? At least I don't see any indication of that.
>>>>>
>>>>> Of course we can implement it (and we will as we actually got a report
>>>>> of this patch breaking Cypress now, finally).
>>>>>
>>>>> The question was never whether it could be done, we're already doing a
>>>>> bunch of abstractions to deal with just the Broadcom-only side of things
>>>>> too. The point I was trying to make is that we need to *know* what
>>>>> firmware abstractions we need and *why* they are needed. We can't just
>>>>> say, for every change, "well, nobody knows if the existing code works or
>>>>> not, so let's just add an abstraction just in case the change breaks
>>>>> something". As far as anyone involved in the discussions until now could
>>>>> tell, this code was just something some Cypress person dumped upstream,
>>>>> and nobody involved was being responsive to any of our inquiries, so
>>>>> there was no way to be certain it worked at all, whether it was
>>>>> supported in public firmware, or anything else.
>>>>>
>>>>> *Now* that we know the existing code is actually functional and not just
>>>>> dead/broken, and that the WSEC approach is conversely not functional on
>>>>> the Cypress firmwares, it makes sense to introduce an abstraction.
>>>>
>>>> Just a quick look in the git history could have told you that it was not
>>>> just dumped upstream and at least one person was using it and extended
>>>> it for 802.11r support (fast-roaming):
>>>>
>>>>
>>>> author Paweł Drewniak <[email protected]> 2021-08-24 23:13:30 +0100
>>>> committer Kalle Valo <[email protected]> 2021-08-29 11:33:07 +0300
>>>> commit 4b51de063d5310f1fb297388b7955926e63e45c9 (patch)
>>>> tree ba2ccb5cbd055d482a8daa263f5e53531c07667f
>>>> /drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>> parent 81f9ebd43659320a88cae8ed5124c50b4d47ab66 (diff)
>>>> download wireless-4b51de063d5310f1fb297388b7955926e63e45c9.tar.gz
>>>> brcmfmac: Add WPA3 Personal with FT to supported cipher suites
>>>> This allows the driver to connect to BSSIDs supporting SAE with 802.11r.
>>>> Tested on Raspberry Pi 4 Model B (STA) and UniFi 6LR/OpenWRT 21.02.0-rc2.
>>>> AP was set to 'sae-mixed' (WPA2/3 Personal).
>>>>
>>>> Signed-off-by: Paweł Drewniak <[email protected]>
>>>> Signed-off-by: Kalle Valo <[email protected]>
>>>> Link: https://lore.kernel.org/r/[email protected]
>>>
>>> Sure, but we also had user reports of it *not* actually working (maybe
>>> it regressed?). We didn't know whether it was functional with the
>>> linux-firmware blobs in any way, I wanted confirmation of that. And we
>>> also didn't know that the patch *would* break it at all; perhaps the
>>> Cypress firmware had also grown support for the WSEC mechanism.
>>>
>>> That's why I wanted someone to actually confirm the code worked (in some
>>> subset of cases) and the patch didn't, before starting to introduce
>>> conditionals. There is, of course, also the element that the Cypress
>>> side has been long unmaintained, and if nobody is testing/giving
>>> feedback/complaining, perhaps it's better to err on the side of maybe
>>> breaking something and see if that gets someone to come out of the
>>> woodwork if it really breaks, rather than tiptoeing around the code
>>> without knowing what's going on and without anyone actually testing things.
>>
>> That is because you distrust the intent that Cypress was really
>> contributing. They were and I trusted them to not just throw in a
>> feature like WPA3. When Infineon took over they went mute. Upon
>> reviewing your patch (again) I also sent an email to them asking
>> specifically about the status of the sae_password interface. I did not
>> use the mailing list which indeed bounces these days (hence removed
>> them) but the last living soul that I had contact with about a year ago
>> whether they were still comitted to be involved. I guess out of
>> politeness or embarrassment I got confirmation they were and never heard
>> from him again. The query about the sae_password interface is still pending.
>
> If only corporate acquisition politics didn't repeatedly throw a wrench
> into this one... :/
>
> This is where we are though, Infineon clearly doesn't care, so it's time
> to move on.
>
>>> It's not about this *specific* patch, it's about the general situation
>>> of not being able to touch firmware interfaces "just in case Cypress
>>> breaks" being unsustainable in the long term. I wasn't pushing back
>>> because I think this particular one will be hard, I was pushing back
>>> because I can read the tea leaves and see this is not going to end well
>>> if it's the approach we start taking for everything. We *need* someone
>>> to be testing patches on Cypress, we can't just "try not to touch it"
>>> and cross our fingers. That just ends in disaster, we are not going to
>>> succeed in not breaking it either way and it's going to make the driver
>>> worse.
>>
>> I admire you ability of reading tea leaves. You saw the Grim I reckon.
>> Admittedly your responses on every comment from my side (or Kalle for
>> that matter) was polarizing every discussion. That is common way people
>> treat each other nowadays especially online where a conversation is just
>> a pile of text going shit. It does not bring out the best in me either,
>> but it was draining every ounce of energy from me so better end it by
>> stepping out.
>
> The hilariously outdated kernel development model surely doesn't help
> either (I've stated my opinion on this quite a few times if you've
> followed around) ;)

It is not a fair statement to call the kernel development process
outdated. It is a vast code base that needs agreed upon steps to keep it
rolling as it is. Attend a plumbers conference or collaboration summit
or better become a speaker and vent all your opinions there and have a
discussion with community members. They are held yearly and maybe over
the past years things have been introduced that give more churn than
value and that would be a great topic for discussion. However, it is
better left outside of the development workflow.

> This stuff gets *really* frustrating when you're trying to improve what
> is, I hope we can all admit, an undermaintained driver (that is not to
> say it's anyone's fault personally), and end up getting held back due to
> everything from coding style nitpicks to people not having the time to
> be responsive. It's just not helpful. It's important to know when to
> step aside and let people actually get stuff done.
>
> When Daniel started sending me brcmfmac patches downstream, I took a
> look at a few of them, decided he knew what he was doing, and just
> started pulling in his branches wholesale. Was it perfect? No, I had to
> debug at least one regression at one point. But it took me less time to
> do that than it would've to go through the commits with a fine toothed
> comb, so it was clearly the right decision.

With the patch that started it all I simply had another view based on
trusting my peers. Infineon has been pulling away from brcmfmac off the
bat, but Cypress was serious enough about the driver not to drop a heap
of dung on it. Based on that I felt regressions would be around the
corner if we took it as is.

> That is not to say that should be the standard upstream (we make a point
> of moving fast and breaking things more downstream, since it's a proving
> ground for what eventually will be upstreamed), but I think it does
> demonstrate the kind of delegation ability that is sorely lacking in
> many drivers and subsystems in the kernel these days. Maintainers become
> entrenched in their position, long beyond the point where they have the
> time/motivation/ability to drive the code forward, and end up in the way
> of new people who are trying to make a difference. I think Linus knows
> full well the kernel maintainer community is stagnating.
>
> That doesn't mean people should step down entirely. But it does mean
> they need to recognize when this is happening and, at least, proactively
> try to bring new people in, instead of just continuing to play a
> gatekeeping role. The role of maintainers should not be that of a wall
> people have to climb over to get their changes in, it should be to guide
> new contributors and help onboard people who can contribute, as peers
> and eventually as future maintainers.
>
> Kalle, in the other thread you said "this is not fun anymore, this is
> more like a business with requirements and demands coming from
> everywhere.". That's what it feels like to us when our changes get
> rejected because the local vars aren't in reverse Christmas tree order,
> or because our commit messages have "v2:" in them. It feels like some
> manager is trying to justify their position by creating busywork for
> everyone else. Nobody should actually care about any of those things,
> and if they do, they need to step back and really ask themselves how
> they ended up believing that. If the goal is to enforce a reasonable
> shared coding style so things don't spiral into chaos, FFS, let's just
> do what every other project does these days and adopt clang-format. Then
> *all* of us can stop wasting time on these trivialities and go back to
> getting stuff done. And really, nobody cares about commit messages as
> long as the tags are right, the subject line is succinct, and the
> important information is in there. Extra stuff never hurt anyone.

https://docs.kernel.org/process/clang-format.html#clangformat

Enjoy!!

>> I added the ground work for multi-vendor support, but have not decided
>> on the approach to take. Abstract per firmware interface primitive or
>> simply have a cfg80211.c and fwil_types.h per vendor OR implement a
>> vendor-specific cfg80211 callback and override the default callback
>> during the driver attach, ie. in brcmf_fwvid_wcc_attach(). The latter
>> duplicates things, but lean towards that as it may be easier on the
>> long-term. What do your tea leaves tell you ;-)
>
> FWIW, I was hoping you'd stay on at least as a reviewer. Your
> contributions are valuable. You obviously know the driver and hardware
> much better than most people. I encourage you to, at least, post a v2 of
> the MAINTAINERS patch with yourself as an R: line.
>
> As far as the actual driver abstraction architecture, I'm going to leave
> it to Daniel to decide what makes the most sense, since he's the one
> introducing new mechanisms for that already. There's always room for
> refactoring later though, depending on the direction things take with
> the vendor split. BTW, clang-format also makes refactoring a lot less
> painful ;)

Refactoring a single driver is not so painful, but rather a nice
relaxing puzzle ;-)

Attachments:

smime.p7s (4.12 kB)
S/MIME Cryptographic Signature