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

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

[Hector Martin]

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.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
Note: must be applied after:

[PATCH 06/10] brcmfmac: cfg80211: Pass the PMK in binary instead of hex

Since that is reviewed and this isn't yet, I expect that will go in
first anyway.

 .../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 18e6699d4024..e4690d56e7c3 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -1682,52 +1682,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 792adaf880b4..3ba90878c47d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
@@ -574,7 +574,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];
 };
 
 /**
-- 
2.35.1

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

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 3918 bytes --]

+ Double Lo

On 2/14/2023 10:33 AM, Hector Martin 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.

The SAE code in brcmfmac was added by Cypress/Infineon. For my BCA 
devices that did not work, but this change should be verified on Cypress 
hardware.

Regards,
Arend

> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
> Note: must be applied after:
> 
> [PATCH 06/10] brcmfmac: cfg80211: Pass the PMK in binary instead of hex
> 
> Since that is reviewed and this isn't yet, I expect that will go in
> first anyway.
> 
>   .../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 18e6699d4024..e4690d56e7c3 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -1682,52 +1682,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 792adaf880b4..3ba90878c47d 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil_types.h
> @@ -574,7 +574,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];
>   };
>   
>   /**

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

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

[Hector Martin]

On 14/02/2023 19.07, Arend van Spriel wrote:
> + Double Lo
> 
> On 2/14/2023 10:33 AM, Hector Martin 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.
> 
> The SAE code in brcmfmac was added by Cypress/Infineon. For my BCA 
> devices that did not work, but this change should be verified on Cypress 
> hardware.

Do you mean the existing SAE code does not work on BCA, or this version
doesn't?

I assume/hope this version works for WCC in general, since that is what
the Apple-relevant chips are tagged as. If so it sounds like we need a
firmware type conditional on this, if CYW needs the existing behavior.

- Hector

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

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 935 bytes --]



On 2/14/2023 11:30 AM, Hector Martin wrote:
> On 14/02/2023 19.07, Arend van Spriel wrote:
>> + Double Lo
>>
>> On 2/14/2023 10:33 AM, Hector Martin 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.
>>
>> The SAE code in brcmfmac was added by Cypress/Infineon. For my BCA
>> devices that did not work, but this change should be verified on Cypress
>> hardware.
> 
> Do you mean the existing SAE code does not work on BCA, or this version
> doesn't?

I meant the existing SAE code. I will give your patches a spin on the 
devices I have.

> I assume/hope this version works for WCC in general, since that is what
> the Apple-relevant chips are tagged as. If so it sounds like we need a
> firmware type conditional on this, if CYW needs the existing behavior.

Right. Let's hope we get some feedback from them.

Regards,
Arend

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

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

[Kalle Valo]

Hector Martin <marcan@marcan.st> 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.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>

If I understood correctly this patch is not ready yet so I'll drop it
from my queue. Please resend as v2 once it's ready and add "wifi:" to
the title.

Patch set to Changes Requested.

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20230214093319.21077-1-marcan@marcan.st/

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

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

[Hector Martin]

On 14/02/2023 19.38, Arend van Spriel wrote:
> 
> 
> On 2/14/2023 11:30 AM, Hector Martin wrote:
>> On 14/02/2023 19.07, Arend van Spriel wrote:
>>> + Double Lo
>>>
>>> On 2/14/2023 10:33 AM, Hector Martin 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.
>>>
>>> The SAE code in brcmfmac was added by Cypress/Infineon. For my BCA
>>> devices that did not work, but this change should be verified on Cypress
>>> hardware.
>>
>> Do you mean the existing SAE code does not work on BCA, or this version
>> doesn't?
> 
> I meant the existing SAE code. I will give your patches a spin on the 
> devices I have.
> 
>> I assume/hope this version works for WCC in general, since that is what
>> the Apple-relevant chips are tagged as. If so it sounds like we need a
>> firmware type conditional on this, if CYW needs the existing behavior.
> 
> Right. Let's hope we get some feedback from them.

Any news on this? Nothing from the Cypress guys (nor to any of my
previous emails about other stuff, for that matter), so if you can
confirm this works on your chips I'd rather just blindly add the CYW/not
firmware variant check and call it a day.

We can't wait forever for them to show up. If they expect their chips to
continue work with mainline they need to actually interact on the MLs,
otherwise they should expect us to possibly accidentally break things
even if we try not to. As far as I can tell they seem completely
disinterested in talking about anything, and we can't let that block
progress for everyone else.

- Hector