Received-SPF: pass (google.com: domain of linux-wireless+bounces-3411-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <ae2a91ed-841f-47a3-b0b8-1e46244721f9@quicinc.com>
Date: Fri, 9 Feb 2024 17:01:23 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/1] wifi: nl80211: Add support for plumbing SAE groups to
 driver
Content-Language: en-US
To: Vinayak Yadawad <vinayak.yadawad@broadcom.com>,
        <johannes@sipsolutions.net>
CC: <linux-wireless@vger.kernel.org>, <jithu.jance@broadcom.com>
References: <309965e8ef4d220053ca7e6bd34393f892ea1bb8.1707486287.git.vinayak.yadawad@broadcom.com>
From: Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <309965e8ef4d220053ca7e6bd34393f892ea1bb8.1707486287.git.vinayak.yadawad@broadcom.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

On 2/9/2024 5:50 AM, Vinayak Yadawad wrote:
> In case of SAE authentication offload, the driver would need
> the info of SAE DH groups for both STA connection and soft AP.
> In the current change we update the SAE DH group support info

who is we?

<https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes>
Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to
do frotz", as if you are giving orders to the codebase to change its
behaviour.


> to driver in the order/priority as provided by the supplicant/
> upper layer.
> 
> Signed-off-by: Vinayak Yadawad <vinayak.yadawad@broadcom.com>
> ---
>  include/net/cfg80211.h       |  6 ++++++
>  include/uapi/linux/nl80211.h |  7 +++++++
>  net/wireless/nl80211.c       | 22 ++++++++++++++++++++++
>  3 files changed, 35 insertions(+)
> 
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index 5b42bfc..0b2db0d 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -1194,6 +1194,7 @@ struct survey_info {
>  };
>  
>  #define CFG80211_MAX_NUM_AKM_SUITES	10
> +#define CFG80211_MAX_NUM_SAE_DH_GROUPS 10
>  
>  /**
>   * struct cfg80211_crypto_settings - Crypto settings
> @@ -1235,6 +1236,9 @@ struct survey_info {
>   *
>   *	NL80211_SAE_PWE_BOTH
>   *	  Allow either hunting-and-pecking loop or hash-to-element
> + *
> + * @sae_dh_groups: SAE DH groups in preference order.
> + * @n_sae_dhd_groups: No of SAE DH groups assigned.
>   */
>  struct cfg80211_crypto_settings {
>  	u32 wpa_versions;
> @@ -1252,6 +1256,8 @@ struct cfg80211_crypto_settings {
>  	const u8 *sae_pwd;
>  	u8 sae_pwd_len;
>  	enum nl80211_sae_pwe_mechanism sae_pwe;
> +	u32 sae_dh_groups[CFG80211_MAX_NUM_SAE_DH_GROUPS];
> +	u8 n_sae_dh_groups;
>  };
>  
>  /**
> diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
> index 853ac53..7c1a7b4 100644
> --- a/include/uapi/linux/nl80211.h
> +++ b/include/uapi/linux/nl80211.h
> @@ -2855,6 +2855,11 @@ enum nl80211_commands {
>   *	%NL80211_CMD_ASSOCIATE indicating the SPP A-MSDUs
>   *	are used on this connection
>   *
> + * @NL80211_ATTR_SAE_DH_GROUPS: Attribute to indicate the supported SAE DH

suggest you be more specific and say this is a nest attribute containing
zero or more u32 attributes containing a DH Group number

> + *	groups to driver. For STA role, the dh groups should be tried in the

s/dh/DH/

> + *	indicated order. For AP role, the order does not have any specific
> + *	significance.
> + *
>   * @NUM_NL80211_ATTR: total number of nl80211_attrs available
>   * @NL80211_ATTR_MAX: highest attribute number currently defined
>   * @__NL80211_ATTR_AFTER_LAST: internal use
> @@ -3400,6 +3405,8 @@ enum nl80211_attrs {
>  
>  	NL80211_ATTR_ASSOC_SPP_AMSDU,
>  
> +	NL80211_ATTR_SAE_DH_GROUPS,
> +
>  	/* add attributes here, update the policy in nl80211.c */
>  
>  	__NL80211_ATTR_AFTER_LAST,
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 68c2040..555eb0f 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -826,6 +826,7 @@ static int validate_he_capa(const struct nlattr *attr,
>  	[NL80211_ATTR_MLO_TTLM_DLINK] = NLA_POLICY_EXACT_LEN(sizeof(u16) * 8),
>  	[NL80211_ATTR_MLO_TTLM_ULINK] = NLA_POLICY_EXACT_LEN(sizeof(u16) * 8),
>  	[NL80211_ATTR_ASSOC_SPP_AMSDU] = { .type = NLA_FLAG },
> +	[NL80211_ATTR_SAE_DH_GROUPS] = { .type = NLA_NESTED },

can/should this have a policy? not sure it can have a policy, so see
below...

>  };
>  
>  /* policy for the key attributes */
> @@ -10883,6 +10884,27 @@ static int nl80211_crypto_settings(struct cfg80211_registered_device *rdev,
>  	else
>  		settings->sae_pwe = NL80211_SAE_PWE_UNSPECIFIED;
>  
> +	if (info->attrs[NL80211_ATTR_SAE_DH_GROUPS]) {
> +		struct nlattr *dh_group;
> +		int tmp, i = 0;
> +
> +		if (!wiphy_ext_feature_isset(&rdev->wiphy,
> +					     NL80211_EXT_FEATURE_SAE_OFFLOAD) &&
> +		    !wiphy_ext_feature_isset(&rdev->wiphy,
> +					     NL80211_EXT_FEATURE_SAE_OFFLOAD_AP))
> +			return -EINVAL;
> +
> +		nla_for_each_nested(dh_group, info->attrs[NL80211_ATTR_SAE_DH_GROUPS],
> +		    tmp) {
> +			settings->sae_dh_groups[i] = nla_get_u32(dh_group);

...note that since you don't have a policy to ensure that each attribute
actually has 4 bytes of data, this can result in a buffer overread if
userspace provides less than 4 bytes of payload.

perhaps add logic like the following (borrowed from validate_scan_freqs():

		if (nla_len(dh_group) != sizeof(u32))
			return -EINVAL;

also if it passes the u32 size test, don't you also want to check each
value to make sure it is a valid DH Group #?

of course, this begs the question of whether cfg80211 or the individual
drivers should validate the DH Group values. if we don't validate in
cfg80211 then perhaps add to the documentation of @sae_dh_groups that
the values have not been validated

> +			i++;
> +
> +			if (i == CFG80211_MAX_NUM_SAE_DH_GROUPS)
> +				break;
> +		}
> +		settings->n_sae_dh_groups = i;
> +	}
> +
>  	return 0;
>  }
>