| Message ID | 20231221140416.223639-1-arend.vanspriel@broadcom.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: brcmfmac: cfg80211: Use WSEC to set SAE password | expand |
On 12/21/2023 3:04 PM, Arend van Spriel wrote: > From: Hector Martin <marcan@marcan.st> > > 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 <marcan@marcan.st> > Reviewed-by: Neal Gompa <neal@gompa.dev> > Signed-off-by: Paweł Drewniak <czajernia@gmail.com> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> 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. Regards, Arend > [arend.vanspriel@broadcom.com: use multi-vendor framework] > Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
On 2023/12/21 23:04, Arend van Spriel wrote: > From: Hector Martin <marcan@marcan.st> > > 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 <marcan@marcan.st> > Reviewed-by: Neal Gompa <neal@gompa.dev> > Signed-off-by: Paweł Drewniak <czajernia@gmail.com> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> > [arend.vanspriel@broadcom.com: use multi-vendor framework] > Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> > --- > 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
On 2023/12/21 23:11, Arend van Spriel wrote: > On 12/21/2023 3:04 PM, Arend van Spriel wrote: >> From: Hector Martin <marcan@marcan.st> >> >> 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 <marcan@marcan.st> >> Reviewed-by: Neal Gompa <neal@gompa.dev> >> Signed-off-by: Paweł Drewniak <czajernia@gmail.com> >> Signed-off-by: Kalle Valo <kvalo@codeaurora.org> > > 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
On December 22, 2023 6:28:12 AM Hector Martin <marcan@marcan.st> wrote: > On 2023/12/21 23:04, Arend van Spriel wrote: >> From: Hector Martin <marcan@marcan.st> >> >> 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 <marcan@marcan.st> >> Reviewed-by: Neal Gompa <neal@gompa.dev> >> Signed-off-by: Paweł Drewniak <czajernia@gmail.com> >> Signed-off-by: Kalle Valo <kvalo@codeaurora.org> >> [arend.vanspriel@broadcom.com: use multi-vendor framework] >> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com> >> --- >> 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? Agree. Just whipped up a first draft and this came out. Maybe I will make it a series, because there's more groundwork to be done like exporting all fwil functions and probably inlining a few to limit the exports. Regards, Arend
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c index 133c5ea6429c..989d0f3d4b3b 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c @@ -32,6 +32,7 @@ #include "vendor.h" #include "bus.h" #include "common.h" +#include "fwvid.h" #define BRCMF_SCAN_IE_LEN_MAX 2048 @@ -1693,6 +1694,12 @@ static int brcmf_set_pmk(struct brcmf_if *ifp, const u8 *pmk_data, u16 pmk_len) struct brcmf_wsec_pmk_le pmk; int err; + if (pmk_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 */ @@ -1710,7 +1717,7 @@ static int brcmf_set_pmk(struct brcmf_if *ifp, const u8 *pmk_data, u16 pmk_len) return err; } -static int brcmf_set_sae_password(struct brcmf_if *ifp, const u8 *pwd_data, +int brcmf_set_sae_password(struct brcmf_if *ifp, const u8 *pwd_data, u16 pwd_len) { struct brcmf_pub *drvr = ifp->drvr; @@ -1734,6 +1741,7 @@ static int brcmf_set_sae_password(struct brcmf_if *ifp, const u8 *pwd_data, return err; } +BRCMF_EXPORT_SYMBOL_GPL(brcmf_set_sae_password); static void brcmf_link_down(struct brcmf_cfg80211_vif *vif, u16 reason, bool locally_generated) @@ -2503,8 +2511,7 @@ brcmf_cfg80211_connect(struct wiphy *wiphy, struct net_device *ndev, bphy_err(drvr, "failed to clean up user-space RSNE\n"); goto done; } - err = brcmf_set_sae_password(ifp, sme->crypto.sae_pwd, - sme->crypto.sae_pwd_len); + err = brcmf_fwvid_set_sae_password(ifp, &sme->crypto); if (!err && sme->crypto.psk) err = brcmf_set_pmk(ifp, sme->crypto.psk, BRCMF_WSEC_MAX_PSK_LEN); diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h index 0e1fa3f0dea2..7c08e545e73f 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.h @@ -468,4 +468,7 @@ void brcmf_set_mpc(struct brcmf_if *ndev, int mpc); void brcmf_abort_scanning(struct brcmf_cfg80211_info *cfg); void brcmf_cfg80211_free_netdev(struct net_device *ndev); +int brcmf_set_sae_password(struct brcmf_if *ifp, const u8 *pwd_data, + u16 pwd_len); + #endif /* BRCMFMAC_CFG80211_H */ diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.c index 72fe8bce6eaf..65604477ad2f 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.c @@ -142,6 +142,7 @@ brcmf_fil_cmd_data_set(struct brcmf_if *ifp, u32 cmd, void *data, u32 len) return err; } +BRCMF_EXPORT_SYMBOL_GPL(brcmf_fil_cmd_data_set); s32 brcmf_fil_cmd_data_get(struct brcmf_if *ifp, u32 cmd, void *data, u32 len) diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h index 9d248ba1c0b2..e74a23e11830 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]; }; /** diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwvid.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwvid.h index 43df58bb70ad..6fb3b8fc398a 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwvid.h +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwvid.h @@ -6,12 +6,15 @@ #define FWVID_H_ #include "firmware.h" +#include "cfg80211.h" struct brcmf_pub; +struct brcmf_if; struct brcmf_fwvid_ops { int (*attach)(struct brcmf_pub *drvr); void (*detach)(struct brcmf_pub *drvr); + int (*set_sae_password)(struct brcmf_if *ifp, struct cfg80211_crypto_settings *crypto); }; /* exported functions */ @@ -44,4 +47,15 @@ static inline void brcmf_fwvid_detach(struct brcmf_pub *drvr) brcmf_fwvid_detach_ops(drvr); } +static inline int brcmf_fwvid_set_sae_password(struct brcmf_if *ifp, + struct cfg80211_crypto_settings *crypto) +{ + const struct brcmf_fwvid_ops *vops = ifp->drvr->vops; + + if (!vops || !vops->set_sae_password) + return brcmf_set_sae_password(ifp, crypto->sae_pwd, crypto->sae_pwd_len); + + return vops->set_sae_password(ifp, crypto); +} + #endif /* FWVID_H_ */ 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, };