Received-SPF: pass (google.com: domain of linux-wireless+bounces-1200-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <537c3ebe-ffe6-4579-a791-1450a3c1b107@marcan.st>
Date: Fri, 22 Dec 2023 14:28:08 +0900
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] wifi: brcmfmac: cfg80211: Use WSEC to set SAE password
Content-Language: en-US
To: Arend van Spriel <arend.vanspriel@broadcom.com>,
 Kalle Valo <kvalo@kernel.org>
Cc: linux-wireless@vger.kernel.org, Neal Gompa <neal@gompa.dev>,
 =?UTF-8?Q?Pawe=C5=82_Drewniak?= <czajernia@gmail.com>,
 Kalle Valo <kvalo@codeaurora.org>
References: <20231107-brcmfmac-wpa3-v1-1-4c7db8636680@marcan.st>
 <20231221140416.223639-1-arend.vanspriel@broadcom.com>
From: Hector Martin <marcan@marcan.st>
In-Reply-To: <20231221140416.223639-1-arend.vanspriel@broadcom.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit


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