Search Linux Wireless

Re: [PATCH v3 1/1] wifi: nl80211: Extend del pmksa support for SAE and OWE security

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxx> 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

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature


[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Wireless Personal Area Network]     [Linux Bluetooth]     [Wireless Regulations]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Hiking]     [MIPS Linux]     [ARM Linux]     [Linux RAID]

  Powered by Linux