[PATCH] nl80211: Update ERP info using NL80211_CMD_UPDATE_CONNECT_PARAMS

[PATCH] nl80211: Update ERP info using NL80211_CMD_UPDATE_CONNECT_PARAMS

[Vidyullatha Kanchanapally]

Use NL80211_CMD_UPDATE_CONNECT_PARAMS to update new ERP information,
Association IEs and the Authentication type to driver / firmware which
will be used in subsequent roamings.

Signed-off-by: Vidyullatha Kanchanapally <vidyullatha@codeaurora.org>
---
 include/net/cfg80211.h |  5 +++++
 net/wireless/nl80211.c | 39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index b903ef7..e34faa5 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2156,9 +2156,14 @@ struct cfg80211_connect_params {
  * have to be updated as part of update_connect_params() call.
  *
  * @UPDATE_ASSOC_IES: Indicates whether association request IEs are updated
+ * @UPDATE_FILS_ERP_INFO: Indicates that FILS connection parameters (realm,
+ *	username, erp sequence number and rrk) are updated
+ * @UPDATE_AUTH_TYPE: Indicates that Authentication type is updated
  */
 enum cfg80211_connect_params_changed {
 	UPDATE_ASSOC_IES		= BIT(0),
+	UPDATE_FILS_ERP_INFO		= BIT(1),
+	UPDATE_AUTH_TYPE		= BIT(2),
 };
 
 /**
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index c5d95c3..c5e11d6 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -9165,6 +9165,45 @@ static int nl80211_update_connect_params(struct sk_buff *skb,
 		changed |= UPDATE_ASSOC_IES;
 	}
 
+	if (wiphy_ext_feature_isset(&rdev->wiphy,
+				    NL80211_EXT_FEATURE_FILS_SK_OFFLOAD) &&
+	    info->attrs[NL80211_ATTR_FILS_ERP_USERNAME] &&
+	    info->attrs[NL80211_ATTR_FILS_ERP_REALM] &&
+	    info->attrs[NL80211_ATTR_FILS_ERP_NEXT_SEQ_NUM] &&
+	    info->attrs[NL80211_ATTR_FILS_ERP_RRK]) {
+		connect.fils_erp_username =
+			nla_data(info->attrs[NL80211_ATTR_FILS_ERP_USERNAME]);
+		connect.fils_erp_username_len =
+			nla_len(info->attrs[NL80211_ATTR_FILS_ERP_USERNAME]);
+		connect.fils_erp_realm =
+			nla_data(info->attrs[NL80211_ATTR_FILS_ERP_REALM]);
+		connect.fils_erp_realm_len =
+			nla_len(info->attrs[NL80211_ATTR_FILS_ERP_REALM]);
+		connect.fils_erp_next_seq_num =
+			nla_get_u16(
+			   info->attrs[NL80211_ATTR_FILS_ERP_NEXT_SEQ_NUM]);
+		connect.fils_erp_rrk =
+			nla_data(info->attrs[NL80211_ATTR_FILS_ERP_RRK]);
+		connect.fils_erp_rrk_len =
+			nla_len(info->attrs[NL80211_ATTR_FILS_ERP_RRK]);
+		changed |= UPDATE_FILS_ERP_INFO;
+	} else if (info->attrs[NL80211_ATTR_FILS_ERP_USERNAME] ||
+		   info->attrs[NL80211_ATTR_FILS_ERP_REALM] ||
+		   info->attrs[NL80211_ATTR_FILS_ERP_NEXT_SEQ_NUM] ||
+		   info->attrs[NL80211_ATTR_FILS_ERP_RRK]) {
+		return -EINVAL;
+	}
+
+	if (info->attrs[NL80211_ATTR_AUTH_TYPE]) {
+		u32 auth_type =
+			nla_get_u32(info->attrs[NL80211_ATTR_AUTH_TYPE]);
+		if (!nl80211_valid_auth_type(rdev, auth_type,
+					     NL80211_CMD_CONNECT))
+			return -EINVAL;
+		connect.auth_type = auth_type;
+		changed |= UPDATE_AUTH_TYPE;
+	}
+
 	wdev_lock(dev->ieee80211_ptr);
 	if (!wdev->current_bss)
 		ret = -ENOLINK;
-- 
1.9.1

Re: [PATCH] nl80211: Update ERP info using NL80211_CMD_UPDATE_CONNECT_PARAMS

[Johannes Berg]

On Wed, 2017-10-25 at 14:50 +0530, Vidyullatha Kanchanapally wrote:

> + * @UPDATE_FILS_ERP_INFO: Indicates that FILS connection parameters (realm,
> + *	username, erp sequence number and rrk) are updated
> + * @UPDATE_AUTH_TYPE: Indicates that Authentication type is updated

These are new here, but you don't know if they were actually supported:

> +	if (wiphy_ext_feature_isset(&rdev->wiphy,
> +				    NL80211_EXT_FEATURE_FILS_SK_OFFLOAD) &&

here.

> +	    info->attrs[NL80211_ATTR_FILS_ERP_USERNAME] &&
> +	    info->attrs[NL80211_ATTR_FILS_ERP_REALM] &&
> +	    info->attrs[NL80211_ATTR_FILS_ERP_NEXT_SEQ_NUM] &&
> +	    info->attrs[NL80211_ATTR_FILS_ERP_RRK]) {
[...]
> +	} else if (info->attrs[NL80211_ATTR_FILS_ERP_USERNAME] ||
> +		   info->attrs[NL80211_ATTR_FILS_ERP_REALM] ||
> +		   info->attrs[NL80211_ATTR_FILS_ERP_NEXT_SEQ_NUM] ||
> +		   info->attrs[NL80211_ATTR_FILS_ERP_RRK]) {
> +		return -EINVAL;
> +	}

This logic is also really odd, why not

if (attrs) {
	if (not flag)
		return -EINVAL;
	/* use attrs etc. */
}

> +
> +	if (info->attrs[NL80211_ATTR_AUTH_TYPE]) {
> +		u32 auth_type =
> +			nla_get_u32(info->attrs[NL80211_ATTR_AUTH_TYPE]);
> +		if (!nl80211_valid_auth_type(rdev, auth_type,
> +					     NL80211_CMD_CONNECT))
> +			return -EINVAL;
> +		connect.auth_type = auth_type;
> +		changed |= UPDATE_AUTH_TYPE;
> +	}

Again, how do you know the driver will actually look at
UPDATE_AUTH_TYPE?

johannes

Re: [PATCH] nl80211: Update ERP info using NL80211_CMD_UPDATE_CONNECT_PARAMS

[Arend van Spriel]

Hi Johannes,

Picking up a somewhat old thread as I did not see a follow-up on this 
patch. I got queried about it over here by our FILS team. So what is 
needed for this patch to pass the bar?

On 12/11/2017 12:12 PM, Johannes Berg wrote:
> On Wed, 2017-10-25 at 14:50 +0530, Vidyullatha Kanchanapally wrote:
>
>> + * @UPDATE_FILS_ERP_INFO: Indicates that FILS connection parameters (realm,
>> + *	username, erp sequence number and rrk) are updated
>> + * @UPDATE_AUTH_TYPE: Indicates that Authentication type is updated
>
> These are new here, but you don't know if they were actually supported:
>
>> +	if (wiphy_ext_feature_isset(&rdev->wiphy,
>> +				    NL80211_EXT_FEATURE_FILS_SK_OFFLOAD) &&
>
> here.

The description of the FILS_SK_OFFLOAD currently says:

  * @NL80211_EXT_FEATURE_FILS_SK_OFFLOAD: Driver SME supports FILS 
shared key
  *      authentication with %NL80211_CMD_CONNECT.

Are you suggesting a new flag to cover the new update attributes?

Drivers reporting FILS_SK_OFFLOAD *and* WIPHY_FLAG_SUPPORTS_FW_ROAM 
really need this info to have any luck roaming.

>> +	    info->attrs[NL80211_ATTR_FILS_ERP_USERNAME] &&
>> +	    info->attrs[NL80211_ATTR_FILS_ERP_REALM] &&
>> +	    info->attrs[NL80211_ATTR_FILS_ERP_NEXT_SEQ_NUM] &&
>> +	    info->attrs[NL80211_ATTR_FILS_ERP_RRK]) {
> [...]
>> +	} else if (info->attrs[NL80211_ATTR_FILS_ERP_USERNAME] ||
>> +		   info->attrs[NL80211_ATTR_FILS_ERP_REALM] ||
>> +		   info->attrs[NL80211_ATTR_FILS_ERP_NEXT_SEQ_NUM] ||
>> +		   info->attrs[NL80211_ATTR_FILS_ERP_RRK]) {
>> +		return -EINVAL;
>> +	}
>
> This logic is also really odd, why not
>
> if (attrs) {
> 	if (not flag)
> 		return -EINVAL;
> 	/* use attrs etc. */
> }
>
>> +
>> +	if (info->attrs[NL80211_ATTR_AUTH_TYPE]) {
>> +		u32 auth_type =
>> +			nla_get_u32(info->attrs[NL80211_ATTR_AUTH_TYPE]);
>> +		if (!nl80211_valid_auth_type(rdev, auth_type,
>> +					     NL80211_CMD_CONNECT))
>> +			return -EINVAL;
>> +		connect.auth_type = auth_type;
>> +		changed |= UPDATE_AUTH_TYPE;
>> +	}
>
> Again, how do you know the driver will actually look at
> UPDATE_AUTH_TYPE?

If they don't they are broken, right? And if they are broken, the 
connection will drop and regular connect will happen anyway, no?

We could add a new flag to signal driver will handle the extra 
parameters in UPDATE_CONNECT_PARAMS, but it is not clear why it would be 
needed. Seems to me user-space has all the info needed with the existing 
flag(s).

Regards,
Arend

Re: [PATCH] nl80211: Update ERP info using NL80211_CMD_UPDATE_CONNECT_PARAMS

[Johannes Berg]

Hi Arend,

> Picking up a somewhat old thread as I did not see a follow-up on this 
> patch. I got queried about it over here by our FILS team. So what is 
> needed for this patch to pass the bar?

That's indeed a bit old :-)

> > > + * @UPDATE_FILS_ERP_INFO: Indicates that FILS connection parameters (realm,
> > > + *	username, erp sequence number and rrk) are updated
> > > + * @UPDATE_AUTH_TYPE: Indicates that Authentication type is updated
> > 
> > These are new here, but you don't know if they were actually supported:
> > 
> > > +	if (wiphy_ext_feature_isset(&rdev->wiphy,
> > > +				    NL80211_EXT_FEATURE_FILS_SK_OFFLOAD) &&
> > 
> > here.
> 
> The description of the FILS_SK_OFFLOAD currently says:
> 
>   * @NL80211_EXT_FEATURE_FILS_SK_OFFLOAD: Driver SME supports FILS 
> shared key
>   *      authentication with %NL80211_CMD_CONNECT.
> 
> Are you suggesting a new flag to cover the new update attributes?

[snip]

> > Again, how do you know the driver will actually look at
> > UPDATE_AUTH_TYPE?
> 
> If they don't they are broken, right? And if they are broken, the 
> connection will drop and regular connect will happen anyway, no?
> 
> We could add a new flag to signal driver will handle the extra 
> parameters in UPDATE_CONNECT_PARAMS, but it is not clear why it would be 
> needed. Seems to me user-space has all the info needed with the existing 
> flag(s).

Agree, and we don't even have any drivers that are setting the
FILS_SK_OFFLOAD flag anyway, so we can still redefine its semantics to
some extent.

So yeah, I'd argue that what the patch needed was somebody taking a
critical look at my review ;-)

And perhaps fixing the weird flags thing I pointed out.

johannes

Re: [PATCH] nl80211: Update ERP info using NL80211_CMD_UPDATE_CONNECT_PARAMS

[Arend van Spriel]

+ Jithu, Eylon

On 3/29/2018 1:16 PM, Johannes Berg wrote:
> Hi Arend,
>
>> Picking up a somewhat old thread as I did not see a follow-up on this
>> patch. I got queried about it over here by our FILS team. So what is
>> needed for this patch to pass the bar?
>
> That's indeed a bit old :-)
>
>>>> + * @UPDATE_FILS_ERP_INFO: Indicates that FILS connection parameters (realm,
>>>> + *	username, erp sequence number and rrk) are updated
>>>> + * @UPDATE_AUTH_TYPE: Indicates that Authentication type is updated
>>>
>>> These are new here, but you don't know if they were actually supported:
>>>
>>>> +	if (wiphy_ext_feature_isset(&rdev->wiphy,
>>>> +				    NL80211_EXT_FEATURE_FILS_SK_OFFLOAD) &&
>>>
>>> here.
>>
>> The description of the FILS_SK_OFFLOAD currently says:
>>
>>    * @NL80211_EXT_FEATURE_FILS_SK_OFFLOAD: Driver SME supports FILS
>> shared key
>>    *      authentication with %NL80211_CMD_CONNECT.
>>
>> Are you suggesting a new flag to cover the new update attributes?
>
> [snip]
>
>>> Again, how do you know the driver will actually look at
>>> UPDATE_AUTH_TYPE?
>>
>> If they don't they are broken, right? And if they are broken, the
>> connection will drop and regular connect will happen anyway, no?
>>
>> We could add a new flag to signal driver will handle the extra
>> parameters in UPDATE_CONNECT_PARAMS, but it is not clear why it would be
>> needed. Seems to me user-space has all the info needed with the existing
>> flag(s).
>
> Agree, and we don't even have any drivers that are setting the
> FILS_SK_OFFLOAD flag anyway, so we can still redefine its semantics to
> some extent.

There is some implied behavior about the UPDATE_AUTH_TYPE. The 
FILS_SK_OFFLOAD only seems to cover NL80211_AUTHTYPE_FILS_SK. So to me 
it seems that changing the auth type really means the driver should give 
up on roaming and let user-space handle it.

> So yeah, I'd argue that what the patch needed was somebody taking a
> critical look at my review ;-)
>
> And perhaps fixing the weird flags thing I pointed out.

Yup. That made sense.

Also there is a DOC section about FILS shared key authentication 
offload" so I suppose that should be extended as well.

Regards,
Arend

Re: [PATCH] nl80211: Update ERP info using NL80211_CMD_UPDATE_CONNECT_PARAMS

[Arend van Spriel]

On 3/29/2018 1:31 PM, Arend van Spriel wrote:
>> So yeah, I'd argue that what the patch needed was somebody taking a
>> critical look at my review ;-)
>>
>> And perhaps fixing the weird flags thing I pointed out.
>
> Yup. That made sense.

Hi Johannes,

Started working on this and actually the "weird flags thing" is done for 
a reason. Maybe the reason was because it is done like that in the 
CMD_CONNECT case, but the better reason is that we need to return 
-EINVAL for "no-fils-offload-support, any-fils-param" *and* 
"fils-offload-support, not-all-fils-param".

> Also there is a DOC section about FILS shared key authentication
> offload" so I suppose that should be extended as well.

So looking at the DOC section I am reading the following:

  * When FILS shared key authentication is completed, driver needs to 
provide the
  * below additional parameters to userspace.
  *	%NL80211_ATTR_FILS_KEK - used for key renewal
  *	%NL80211_ATTR_FILS_ERP_NEXT_SEQ_NUM - used in further EAP-RP exchanges
  *	%NL80211_ATTR_PMKID - used to identify the PMKSA used/generated
  *	%Nl80211_ATTR_PMK - used to update PMKSA cache in userspace
  * The PMKSA can be maintained in userspace persistently so that it can 
be used
  * later after reboots or wifi turn off/on also.

So to me it seems we need these for the ROAM event as well. Agree?

Regards,
Arend

Re: [PATCH] nl80211: Update ERP info using NL80211_CMD_UPDATE_CONNECT_PARAMS

[Johannes Berg]

Hi,

> Started working on this and actually the "weird flags thing" is done for 
> a reason. Maybe the reason was because it is done like that in the 
> CMD_CONNECT case, but the better reason is that we need to return 
> -EINVAL for "no-fils-offload-support, any-fils-param" *and* 
> "fils-offload-support, not-all-fils-param".

Ok, fair enough.

> > Also there is a DOC section about FILS shared key authentication
> > offload" so I suppose that should be extended as well.
> 
> So looking at the DOC section I am reading the following:
> 
>   * When FILS shared key authentication is completed, driver needs to 
> provide the
>   * below additional parameters to userspace.
>   *	%NL80211_ATTR_FILS_KEK - used for key renewal
>   *	%NL80211_ATTR_FILS_ERP_NEXT_SEQ_NUM - used in further EAP-RP exchanges
>   *	%NL80211_ATTR_PMKID - used to identify the PMKSA used/generated
>   *	%Nl80211_ATTR_PMK - used to update PMKSA cache in userspace
>   * The PMKSA can be maintained in userspace persistently so that it can 
> be used
>   * later after reboots or wifi turn off/on also.
> 
> So to me it seems we need these for the ROAM event as well. Agree?

Maybe not all of them, you could be using the same PMKSA, but yes, I
tend to agree.

johannes

Re: [PATCH] nl80211: Update ERP info using NL80211_CMD_UPDATE_CONNECT_PARAMS

[Arend van Spriel]

On 4/4/2018 12:36 PM, Johannes Berg wrote:
> Hi,
>
>> Started working on this and actually the "weird flags thing" is done for
>> a reason. Maybe the reason was because it is done like that in the
>> CMD_CONNECT case, but the better reason is that we need to return
>> -EINVAL for "no-fils-offload-support, any-fils-param" *and*
>> "fils-offload-support, not-all-fils-param".
>
> Ok, fair enough.

I added a comment for this in the patch.

>>> Also there is a DOC section about FILS shared key authentication
>>> offload" so I suppose that should be extended as well.
>>
>> So looking at the DOC section I am reading the following:
>>
>>    * When FILS shared key authentication is completed, driver needs to
>> provide the
>>    * below additional parameters to userspace.
>>    *	%NL80211_ATTR_FILS_KEK - used for key renewal
>>    *	%NL80211_ATTR_FILS_ERP_NEXT_SEQ_NUM - used in further EAP-RP exchanges
>>    *	%NL80211_ATTR_PMKID - used to identify the PMKSA used/generated
>>    *	%Nl80211_ATTR_PMK - used to update PMKSA cache in userspace
>>    * The PMKSA can be maintained in userspace persistently so that it can
>> be used
>>    * later after reboots or wifi turn off/on also.
>>
>> So to me it seems we need these for the ROAM event as well. Agree?
>
> Maybe not all of them, you could be using the same PMKSA, but yes, I
> tend to agree.

I would argue that for the scenario where you do CMD_CONNECT(auth=open) 
and CMD_UPDATE_CONNECT_PARAMS(auth=fils-sk) the ROAM event should 
provide all the above. From what I understand from my colleagues this is 
a supported scenario.

Regards,
Arend