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 <[email protected]>
---
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
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
+ 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
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
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
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
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
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