| Message ID | ff0778a86574b552769027496f12596e2e627931.1699530774.git.vinayak.yadawad@broadcom.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [v3,1/1] wifi: nl80211: Extend del pmksa support for SAE and OWE security | expand |
Hi Johannes, Could you please let us know whether this patch is fine. If fine, we shall go ahead and submit the patch for wpa_supplicant as well. This patch is useful for allowing the user space to flush PMKs generated at firmware for the SAE/OWE offloads when a user changes credential/removes the connection profile. Thanks, Jithu Jance Jithu Jance On Thu, Nov 9, 2023 at 6:00 PM Vinayak Yadawad <vinayak.yadawad@broadcom.com> wrote: > > Current handling of del pmksa with SSID is limited to FILS > security. In the current change the del pmksa support is extended > to SAE/OWE security offloads as well. For OWE/SAE offloads, the > PMK is generated and cached at driver/FW, so user app needs the > capability to request cache deletion based on SSID for drivers > supporting SAE/OWE offload. > > Signed-off-by: Vinayak Yadawad <vinayak.yadawad@broadcom.com> > --- > v1->v2: Addressed review comments for indentation > v2->v3: Addressed review comments for version update in header > --- > net/wireless/nl80211.c | 27 ++++++++++++++++++++------- > 1 file changed, 20 insertions(+), 7 deletions(-) > > diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c > index 569234bc2be6..8dc1c800f171 100644 > --- a/net/wireless/nl80211.c > +++ b/net/wireless/nl80211.c > @@ -12183,24 +12183,37 @@ static int nl80211_setdel_pmksa(struct sk_buff *skb, struct genl_info *info) > > memset(&pmksa, 0, sizeof(struct cfg80211_pmksa)); > > - if (!info->attrs[NL80211_ATTR_PMKID]) > + if ((info->genlhdr->cmd == NL80211_CMD_SET_PMKSA) && > + (!info->attrs[NL80211_ATTR_PMKID])) > return -EINVAL; > > - pmksa.pmkid = nla_data(info->attrs[NL80211_ATTR_PMKID]); > + if (info->attrs[NL80211_ATTR_PMKID]) > + pmksa.pmkid = nla_data(info->attrs[NL80211_ATTR_PMKID]); > > if (info->attrs[NL80211_ATTR_MAC]) { > pmksa.bssid = nla_data(info->attrs[NL80211_ATTR_MAC]); > - } else if (info->attrs[NL80211_ATTR_SSID] && > - info->attrs[NL80211_ATTR_FILS_CACHE_ID] && > - (info->genlhdr->cmd == NL80211_CMD_DEL_PMKSA || > + } else if (info->attrs[NL80211_ATTR_SSID]) { > + /* SSID based pmksa flush suppported only for FILS, > + * OWE/SAE OFFLOAD cases > + */ > + if (info->attrs[NL80211_ATTR_FILS_CACHE_ID] && > + (info->genlhdr->cmd == NL80211_CMD_DEL_PMKSA || > info->attrs[NL80211_ATTR_PMK])) { > + pmksa.cache_id = > + nla_data(info->attrs[NL80211_ATTR_FILS_CACHE_ID]); > + } else if ((info->genlhdr->cmd == NL80211_CMD_DEL_PMKSA) && > + (!wiphy_ext_feature_isset( > + &rdev->wiphy, NL80211_EXT_FEATURE_SAE_OFFLOAD) && > + (!wiphy_ext_feature_isset( > + &rdev->wiphy,NL80211_EXT_FEATURE_OWE_OFFLOAD)))){ > + return -EINVAL; > + } > pmksa.ssid = nla_data(info->attrs[NL80211_ATTR_SSID]); > pmksa.ssid_len = nla_len(info->attrs[NL80211_ATTR_SSID]); > - pmksa.cache_id = > - nla_data(info->attrs[NL80211_ATTR_FILS_CACHE_ID]); > } else { > return -EINVAL; > } > + > if (info->attrs[NL80211_ATTR_PMK]) { > pmksa.pmk = nla_data(info->attrs[NL80211_ATTR_PMK]); > pmksa.pmk_len = nla_len(info->attrs[NL80211_ATTR_PMK]); > -- > 2.32.0 >
On Thu, 2023-11-09 at 18:00 +0530, Vinayak Yadawad wrote: > > +++ b/net/wireless/nl80211.c > @@ -12183,24 +12183,37 @@ static int nl80211_setdel_pmksa(struct sk_buff *skb, struct genl_info *info) > > memset(&pmksa, 0, sizeof(struct cfg80211_pmksa)); > > - if (!info->attrs[NL80211_ATTR_PMKID]) > + if ((info->genlhdr->cmd == NL80211_CMD_SET_PMKSA) && > + (!info->attrs[NL80211_ATTR_PMKID])) > return -EINVAL; Maybe it'd be better to split set/del now? The code kind of looks awkward now, don't you think? Or split this part of the parsing depending on set or del? > - pmksa.pmkid = nla_data(info->attrs[NL80211_ATTR_PMKID]); > + if (info->attrs[NL80211_ATTR_PMKID]) > + pmksa.pmkid = nla_data(info->attrs[NL80211_ATTR_PMKID]); > > if (info->attrs[NL80211_ATTR_MAC]) { > pmksa.bssid = nla_data(info->attrs[NL80211_ATTR_MAC]); > - } else if (info->attrs[NL80211_ATTR_SSID] && > - info->attrs[NL80211_ATTR_FILS_CACHE_ID] && > - (info->genlhdr->cmd == NL80211_CMD_DEL_PMKSA || > + } else if (info->attrs[NL80211_ATTR_SSID]) { > + /* SSID based pmksa flush suppported only for FILS, > + * OWE/SAE OFFLOAD cases > + */ > + if (info->attrs[NL80211_ATTR_FILS_CACHE_ID] && > + (info->genlhdr->cmd == NL80211_CMD_DEL_PMKSA || > info->attrs[NL80211_ATTR_PMK])) { > + pmksa.cache_id = > + nla_data(info->attrs[NL80211_ATTR_FILS_CACHE_ID]); > + } else if ((info->genlhdr->cmd == NL80211_CMD_DEL_PMKSA) && > + (!wiphy_ext_feature_isset( > + &rdev->wiphy, NL80211_EXT_FEATURE_SAE_OFFLOAD) && > + (!wiphy_ext_feature_isset( > + &rdev->wiphy,NL80211_EXT_FEATURE_OWE_OFFLOAD)))){ The indentation here is also really awful ... I'd rather go over 80 columns than break like that. But you could just have local variables for all the feature checks too. And if you don't split set/del, I'd recommend a variable for that too, set at the beginning, perhaps moving the "rdev_ops" thing up? But I'm thinking it's probably nicer to split it. See how that looks like? > + return -EINVAL; > + } > pmksa.ssid = nla_data(info->attrs[NL80211_ATTR_SSID]); > pmksa.ssid_len = nla_len(info->attrs[NL80211_ATTR_SSID]); > - pmksa.cache_id = > - nla_data(info->attrs[NL80211_ATTR_FILS_CACHE_ID]); > } else { > return -EINVAL; > } > + > if (info->attrs[NL80211_ATTR_PMK]) { Then again, that isn't even relevant for DEL, is it? Should we even parse it? Does anyone want to "delete only if it's exactly this PMK"? Also seems like this should come with some nl80211.h updates though? johannes
Hi Johannes, Thanks for the review comments. >Maybe it'd be better to split set/del now? The code kind of looks >awkward now, don't you think? >Or split this part of the parsing depending on set or del? As suggested, set and del pmksa handling is split. >The indentation here is also really awful ... I'd rather go over 80 >columns than break like that. But you could just have local variables >for all the feature checks too. >And if you don't split set/del, I'd recommend a variable for that too, >set at the beginning, perhaps moving the "rdev_ops" thing up? But I'm >thinking it's probably nicer to split it. See how that looks like? Addressed comments by splitting set and del pmksa and using local variables for feature checks. >Then again, that isn't even relevant for DEL, is it? Should we even >parse it? Does anyone want to "delete only if it's exactly this PMK"? Removed this handling for DEL pmksa. >Also seems like this should come with some nl80211.h updates though? Added additional description for the DEL pmksa documentation. Regards, Vinayak On Sat, Nov 25, 2023 at 12:28 AM Johannes Berg <johannes@sipsolutions.net> wrote: > > On Thu, 2023-11-09 at 18:00 +0530, Vinayak Yadawad wrote: > > > > +++ b/net/wireless/nl80211.c > > @@ -12183,24 +12183,37 @@ static int nl80211_setdel_pmksa(struct sk_buff *skb, struct genl_info *info) > > > > memset(&pmksa, 0, sizeof(struct cfg80211_pmksa)); > > > > - if (!info->attrs[NL80211_ATTR_PMKID]) > > + if ((info->genlhdr->cmd == NL80211_CMD_SET_PMKSA) && > > + (!info->attrs[NL80211_ATTR_PMKID])) > > return -EINVAL; > > Maybe it'd be better to split set/del now? The code kind of looks > awkward now, don't you think? > > Or split this part of the parsing depending on set or del? > > > - pmksa.pmkid = nla_data(info->attrs[NL80211_ATTR_PMKID]); > > + if (info->attrs[NL80211_ATTR_PMKID]) > > + pmksa.pmkid = nla_data(info->attrs[NL80211_ATTR_PMKID]); > > > > if (info->attrs[NL80211_ATTR_MAC]) { > > pmksa.bssid = nla_data(info->attrs[NL80211_ATTR_MAC]); > > - } else if (info->attrs[NL80211_ATTR_SSID] && > > - info->attrs[NL80211_ATTR_FILS_CACHE_ID] && > > - (info->genlhdr->cmd == NL80211_CMD_DEL_PMKSA || > > + } else if (info->attrs[NL80211_ATTR_SSID]) { > > + /* SSID based pmksa flush suppported only for FILS, > > + * OWE/SAE OFFLOAD cases > > + */ > > + if (info->attrs[NL80211_ATTR_FILS_CACHE_ID] && > > + (info->genlhdr->cmd == NL80211_CMD_DEL_PMKSA || > > info->attrs[NL80211_ATTR_PMK])) { > > + pmksa.cache_id = > > + nla_data(info->attrs[NL80211_ATTR_FILS_CACHE_ID]); > > + } else if ((info->genlhdr->cmd == NL80211_CMD_DEL_PMKSA) && > > + (!wiphy_ext_feature_isset( > > + &rdev->wiphy, NL80211_EXT_FEATURE_SAE_OFFLOAD) && > > + (!wiphy_ext_feature_isset( > > + &rdev->wiphy,NL80211_EXT_FEATURE_OWE_OFFLOAD)))){ > > > The indentation here is also really awful ... I'd rather go over 80 > columns than break like that. But you could just have local variables > for all the feature checks too. > > And if you don't split set/del, I'd recommend a variable for that too, > set at the beginning, perhaps moving the "rdev_ops" thing up? But I'm > thinking it's probably nicer to split it. See how that looks like? > > > + return -EINVAL; > > + } > > pmksa.ssid = nla_data(info->attrs[NL80211_ATTR_SSID]); > > pmksa.ssid_len = nla_len(info->attrs[NL80211_ATTR_SSID]); > > - pmksa.cache_id = > > - nla_data(info->attrs[NL80211_ATTR_FILS_CACHE_ID]); > > } else { > > return -EINVAL; > > } > > + > > if (info->attrs[NL80211_ATTR_PMK]) { > > Then again, that isn't even relevant for DEL, is it? Should we even > parse it? Does anyone want to "delete only if it's exactly this PMK"? > > > Also seems like this should come with some nl80211.h updates though? > > johannes
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 569234bc2be6..8dc1c800f171 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -12183,24 +12183,37 @@ static int nl80211_setdel_pmksa(struct sk_buff *skb, struct genl_info *info) memset(&pmksa, 0, sizeof(struct cfg80211_pmksa)); - if (!info->attrs[NL80211_ATTR_PMKID]) + if ((info->genlhdr->cmd == NL80211_CMD_SET_PMKSA) && + (!info->attrs[NL80211_ATTR_PMKID])) return -EINVAL; - pmksa.pmkid = nla_data(info->attrs[NL80211_ATTR_PMKID]); + if (info->attrs[NL80211_ATTR_PMKID]) + pmksa.pmkid = nla_data(info->attrs[NL80211_ATTR_PMKID]); if (info->attrs[NL80211_ATTR_MAC]) { pmksa.bssid = nla_data(info->attrs[NL80211_ATTR_MAC]); - } else if (info->attrs[NL80211_ATTR_SSID] && - info->attrs[NL80211_ATTR_FILS_CACHE_ID] && - (info->genlhdr->cmd == NL80211_CMD_DEL_PMKSA || + } else if (info->attrs[NL80211_ATTR_SSID]) { + /* SSID based pmksa flush suppported only for FILS, + * OWE/SAE OFFLOAD cases + */ + if (info->attrs[NL80211_ATTR_FILS_CACHE_ID] && + (info->genlhdr->cmd == NL80211_CMD_DEL_PMKSA || info->attrs[NL80211_ATTR_PMK])) { + pmksa.cache_id = + nla_data(info->attrs[NL80211_ATTR_FILS_CACHE_ID]); + } else if ((info->genlhdr->cmd == NL80211_CMD_DEL_PMKSA) && + (!wiphy_ext_feature_isset( + &rdev->wiphy, NL80211_EXT_FEATURE_SAE_OFFLOAD) && + (!wiphy_ext_feature_isset( + &rdev->wiphy,NL80211_EXT_FEATURE_OWE_OFFLOAD)))){ + return -EINVAL; + } pmksa.ssid = nla_data(info->attrs[NL80211_ATTR_SSID]); pmksa.ssid_len = nla_len(info->attrs[NL80211_ATTR_SSID]); - pmksa.cache_id = - nla_data(info->attrs[NL80211_ATTR_FILS_CACHE_ID]); } else { return -EINVAL; } + if (info->attrs[NL80211_ATTR_PMK]) { pmksa.pmk = nla_data(info->attrs[NL80211_ATTR_PMK]); pmksa.pmk_len = nla_len(info->attrs[NL80211_ATTR_PMK]);
Current handling of del pmksa with SSID is limited to FILS security. In the current change the del pmksa support is extended to SAE/OWE security offloads as well. For OWE/SAE offloads, the PMK is generated and cached at driver/FW, so user app needs the capability to request cache deletion based on SSID for drivers supporting SAE/OWE offload. Signed-off-by: Vinayak Yadawad <vinayak.yadawad@broadcom.com> --- v1->v2: Addressed review comments for indentation v2->v3: Addressed review comments for version update in header --- net/wireless/nl80211.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-)