nl80211: add an option to allow MFP without requiring it
diff mbox

Message ID 20170814134911.20869-1-emmanuel.grumbach@intel.com
State Superseded
Delegated to: Johannes Berg
Headers show

Commit Message

Emmanuel Grumbach Aug. 14, 2017, 1:49 p.m. UTC
User space can now allow the kernel to associate to an AP
that requires MFP or that doesn't have MFP enabled in the
same NL80211_CMD_CONNECT command.
The driver / firmware will decide whether to use it or not.

Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
---
A short tour of the drivers taught me that only Quantenna really look
at cfg80211_connect_params::sme which can now be 2. This is why
the maintainer of this driver is CCed.
---
 include/uapi/linux/nl80211.h | 10 ++++++++--
 net/wireless/nl80211.c       |  1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

Comments

Kalle Valo Aug. 14, 2017, 5:14 p.m. UTC | #1
Emmanuel Grumbach <emmanuel.grumbach@intel.com> writes:

> User space can now allow the kernel to associate to an AP
> that requires MFP or that doesn't have MFP enabled in the
> same NL80211_CMD_CONNECT command.
> The driver / firmware will decide whether to use it or not.
>
> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>

[...]

> @@ -4086,10 +4090,12 @@ enum nl80211_key_type {
>   * enum nl80211_mfp - Management frame protection state
>   * @NL80211_MFP_NO: Management frame protection not used
>   * @NL80211_MFP_REQUIRED: Management frame protection required
> + * @NL80211_MFP_OPTIONAL: Management frame is optional
>   */
>  enum nl80211_mfp {
>  	NL80211_MFP_NO,
>  	NL80211_MFP_REQUIRED,
> +	NL80211_MFP_OPTIONAL,
>  };
>  
>  enum nl80211_wpa_versions {
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 8f035d9868d1..829867132326 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -9115,6 +9115,7 @@ static int nl80211_connect(struct sk_buff *skb, struct genl_info *info)
>  	if (info->attrs[NL80211_ATTR_USE_MFP]) {
>  		connect.mfp = nla_get_u32(info->attrs[NL80211_ATTR_USE_MFP]);
>  		if (connect.mfp != NL80211_MFP_REQUIRED &&
> +		    connect.mfp != NL80211_MFP_OPTIONAL &&
>  		    connect.mfp != NL80211_MFP_NO)
>  			return -EINVAL;
>  	} else {

I guess I'm missing something, but how is backwards compatibility
supposed to work from user space point of view? If user space uses
NL80211_MFP_OPTIONAL with an old kernel, the kernel will reject the
command with -EINVAL and user space will try again without
NL80211_MFP_OPTIONAL?
Emmanuel Grumbach Aug. 14, 2017, 6:13 p.m. UTC | #2
On Mon, 2017-08-14 at 20:14 +0300, Kalle Valo wrote:
> Emmanuel Grumbach <emmanuel.grumbach@intel.com> writes:

> 

> > User space can now allow the kernel to associate to an AP

> > that requires MFP or that doesn't have MFP enabled in the

> > same NL80211_CMD_CONNECT command.

> > The driver / firmware will decide whether to use it or not.

> > 

> > Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>

> 

> [...]

> 

> > @@ -4086,10 +4090,12 @@ enum nl80211_key_type {

> >   * enum nl80211_mfp - Management frame protection state

> >   * @NL80211_MFP_NO: Management frame protection not used

> >   * @NL80211_MFP_REQUIRED: Management frame protection required

> > + * @NL80211_MFP_OPTIONAL: Management frame is optional

> >   */

> >  enum nl80211_mfp {

> >  	NL80211_MFP_NO,

> >  	NL80211_MFP_REQUIRED,

> > +	NL80211_MFP_OPTIONAL,

> >  };

> >  

> >  enum nl80211_wpa_versions {

> > diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c

> > index 8f035d9868d1..829867132326 100644

> > --- a/net/wireless/nl80211.c

> > +++ b/net/wireless/nl80211.c

> > @@ -9115,6 +9115,7 @@ static int nl80211_connect(struct sk_buff

> > *skb, struct genl_info *info)

> >  	if (info->attrs[NL80211_ATTR_USE_MFP]) {

> >  		connect.mfp = nla_get_u32(info-

> > >attrs[NL80211_ATTR_USE_MFP]);

> >  		if (connect.mfp != NL80211_MFP_REQUIRED &&

> > +		    connect.mfp != NL80211_MFP_OPTIONAL &&

> >  		    connect.mfp != NL80211_MFP_NO)

> >  			return -EINVAL;

> >  	} else {

> 

> I guess I'm missing something, but how is backwards compatibility

> supposed to work from user space point of view? If user space uses

> NL80211_MFP_OPTIONAL with an old kernel, the kernel will reject the

> command with -EINVAL and user space will try again without

> NL80211_MFP_OPTIONAL?

> 



No you are not. I simply forgot that point. I guess that this would be
the behavior, yes... This is relevant for ap_scan=2 wpa_s configuration
only which makes it not really common, but still, you are right.
Not sure how easy it will be to write this logic in the supplicant
though... Unless we add an nl80211 feature bit but I feel it'd be a bit
of a waste.
Jouni, what do you think?
I know you haven't seen the supplicant patch yet, but here is the gist
of it. I allow OPTIONAL in connect() only, and not in associate():

@@ -5790,6 +5786,14 @@ static int wpa_driver_nl80211_try_connect(
        if (ret)
                goto fail;
 
+       if (params->mgmt_frame_protection == MGMT_FRAME_PROTECTION_REQUIRED &&
+           nla_put_u32(msg, NL80211_ATTR_USE_MFP, NL80211_MFP_REQUIRED))
+               goto fail;
+
+       if (params->mgmt_frame_protection == MGMT_FRAME_PROTECTION_OPTIONAL &&
+           nla_put_u32(msg, NL80211_ATTR_USE_MFP, NL80211_MFP_OPTIONAL))
+               goto fail;
+
        algs = 0;
        if (params->auth_alg & WPA_AUTH_ALG_OPEN)
                algs++;
Igor Mitsyanko Aug. 14, 2017, 6:44 p.m. UTC | #3
On 08/14/2017 06:49 AM, Emmanuel Grumbach wrote:
> 
> User space can now allow the kernel to associate to an AP
> that requires MFP or that doesn't have MFP enabled in the
> same NL80211_CMD_CONNECT command.
> The driver / firmware will decide whether to use it or not.
> 
> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
> ---

No issues from quantenna driver.

Acked-by Igor Mitsyanko <igor.mitsyanko.os@quantenna.com>

[...]

> @@ -4086,10 +4090,12 @@ enum nl80211_key_type {
>    * enum nl80211_mfp - Management frame protection state
>    * @NL80211_MFP_NO: Management frame protection not used
>    * @NL80211_MFP_REQUIRED: Management frame protection required
> + * @NL80211_MFP_OPTIONAL: Management frame is optional

Probable meant to be "Management frame _protection_ is optional"

>    */
>   enum nl80211_mfp {
>          NL80211_MFP_NO,
>          NL80211_MFP_REQUIRED,
> +       NL80211_MFP_OPTIONAL,
>   };
> 
>   enum nl80211_wpa_versions {
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 8f035d9868d1..829867132326 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -9115,6 +9115,7 @@ static int nl80211_connect(struct sk_buff *skb, struct genl_info *info)
>          if (info->attrs[NL80211_ATTR_USE_MFP]) {
>                  connect.mfp = nla_get_u32(info->attrs[NL80211_ATTR_USE_MFP]);
>                  if (connect.mfp != NL80211_MFP_REQUIRED &&
> +                   connect.mfp != NL80211_MFP_OPTIONAL &&
>                      connect.mfp != NL80211_MFP_NO)
>                          return -EINVAL;
>          } else {
> --
> 2.9.3
>
Igor Mitsyanko Aug. 14, 2017, 6:44 p.m. UTC | #4
On 08/14/2017 06:49 AM, Emmanuel Grumbach wrote:
> 
> User space can now allow the kernel to associate to an AP
> that requires MFP or that doesn't have MFP enabled in the
> same NL80211_CMD_CONNECT command.
> The driver / firmware will decide whether to use it or not.
> 
> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
> ---

No issues from quantenna driver.

Acked-by Igor Mitsyanko <igor.mitsyanko.os@quantenna.com>

[...]

> @@ -4086,10 +4090,12 @@ enum nl80211_key_type {
>    * enum nl80211_mfp - Management frame protection state
>    * @NL80211_MFP_NO: Management frame protection not used
>    * @NL80211_MFP_REQUIRED: Management frame protection required
> + * @NL80211_MFP_OPTIONAL: Management frame is optional

Probably meant to be "Management frame _protection_ is optional"

>    */
>   enum nl80211_mfp {
>          NL80211_MFP_NO,
>          NL80211_MFP_REQUIRED,
> +       NL80211_MFP_OPTIONAL,
>   };
> 
>   enum nl80211_wpa_versions {
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index 8f035d9868d1..829867132326 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -9115,6 +9115,7 @@ static int nl80211_connect(struct sk_buff *skb, struct genl_info *info)
>          if (info->attrs[NL80211_ATTR_USE_MFP]) {
>                  connect.mfp = nla_get_u32(info->attrs[NL80211_ATTR_USE_MFP]);
>                  if (connect.mfp != NL80211_MFP_REQUIRED &&
> +                   connect.mfp != NL80211_MFP_OPTIONAL &&
>                      connect.mfp != NL80211_MFP_NO)
>                          return -EINVAL;
>          } else {
> --
> 2.9.3
>
Arend Van Spriel Aug. 14, 2017, 7:22 p.m. UTC | #5
On 14-08-17 15:49, Emmanuel Grumbach wrote:
> User space can now allow the kernel to associate to an AP
> that requires MFP or that doesn't have MFP enabled in the
> same NL80211_CMD_CONNECT command.
> The driver / firmware will decide whether to use it or not.
> 
> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
> ---
> A short tour of the drivers taught me that only Quantenna really look
> at cfg80211_connect_params::sme which can now be 2. This is why
> the maintainer of this driver is CCed.

Indeed in brcmfmac the IE is processed to determine whether it is 
required or optional. Probably will change our driver to use the 
explicit mfp values from cfg80211_connect_params::sme.

Thanks,
Arend
Igor Mitsyanko Aug. 14, 2017, 8:08 p.m. UTC | #6
On 08/14/2017 12:22 PM, Arend van Spriel wrote:
> On 14-08-17 15:49, Emmanuel Grumbach wrote:
>> User space can now allow the kernel to associate to an AP
>> that requires MFP or that doesn't have MFP enabled in the
>> same NL80211_CMD_CONNECT command.
>> The driver / firmware will decide whether to use it or not.
>>
>> Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
>> ---
>> A short tour of the drivers taught me that only Quantenna really look
>> at cfg80211_connect_params::sme which can now be 2. This is why
>> the maintainer of this driver is CCed.
> 
> Indeed in brcmfmac the IE is processed to determine whether it is
> required or optional. Probably will change our driver to use the
> explicit mfp values from cfg80211_connect_params::sme.

Maybe this is a right way to do that, avoiding that compatibility 
problem (no new NL* flags)? Except for maybe nl80211 layer should parse 
it instead, not driver.

It is kind of not clear right now for drivers where to get information from:
- NL attributes passed from userspace and forwarded to a driver
- IEs passed from userspace, parsed by nl80211 and forwarded to a driver
- IEs that are passed from userspace and parsed by drivers directly

> 
> Thanks,
> Arend
Emmanuel Grumbach Aug. 14, 2017, 8:13 p.m. UTC | #7
On Mon, 2017-08-14 at 13:08 -0700, Igor Mitsyanko wrote:
> On 08/14/2017 12:22 PM, Arend van Spriel wrote:

> > On 14-08-17 15:49, Emmanuel Grumbach wrote:

> > > User space can now allow the kernel to associate to an AP

> > > that requires MFP or that doesn't have MFP enabled in the

> > > same NL80211_CMD_CONNECT command.

> > > The driver / firmware will decide whether to use it or not.

> > > 

> > > Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>

> > > ---

> > > A short tour of the drivers taught me that only Quantenna really

> > > look

> > > at cfg80211_connect_params::sme which can now be 2. This is why

> > > the maintainer of this driver is CCed.

> > 

> > Indeed in brcmfmac the IE is processed to determine whether it is

> > required or optional. Probably will change our driver to use the

> > explicit mfp values from cfg80211_connect_params::sme.

> 

> Maybe this is a right way to do that, avoiding that compatibility 

> problem (no new NL* flags)? Except for maybe nl80211 layer should

> parse 

> it instead, not driver.


Seems odd to me, since there already is an NL attribute for this and
you want the user space to decide on the policy I think.

> 

> It is kind of not clear right now for drivers where to get

> information from:

> - NL attributes passed from userspace and forwarded to a driver


Since those exist, it seems to make more sense to me to use those
rather than any in-driver decided policy. Usually, deciding upon
policies from lower levels is frowned upon I'd say.

> - IEs passed from userspace, parsed by nl80211 and forwarded to a

> driver

> - IEs that are passed from userspace and parsed by drivers directly

> 

> > 

> > Thanks,

> > Arend

> 

>
Igor Mitsyanko Aug. 14, 2017, 8:36 p.m. UTC | #8
On 08/14/2017 01:13 PM, Grumbach, Emmanuel wrote:
>> It is kind of not clear right now for drivers where to get
>> information from:
>> - NL attributes passed from userspace and forwarded to a driver
> 
> Since those exist, it seems to make more sense to me to use those
> rather than any in-driver decided policy. Usually, deciding upon
> policies from lower levels is frowned upon I'd say.

I agree, what I mean is that drivers should know where to take this 
information from, if there are multiple sources.

An example are HT/VHT capabilities for start_ap command: they can be 
parsed directly by drivers by looking at beacon's IEs in 
"cfg80211_ap_settings". At the same time those are also parsed by 
nl80211 which fills cfg80211_ap_settings::ht_cap, 
cfg80211_ap_settings::vht_cap.
Why HT/VHT caps are not passed directly from usespace as one of NL 
attributes? I guess because there is not much point in it since 
userspace passes entire IE set anyway, which can be used by lower levels.


> 
>> - IEs passed from userspace, parsed by nl80211 and forwarded to a
>> driver
>> - IEs that are passed from userspace and parsed by drivers directly
>>
>>>
>>> Thanks,
>>> Arend
>>
>>
Emmanuel Grumbach Aug. 15, 2017, 6:12 a.m. UTC | #9
On Mon, 2017-08-14 at 13:36 -0700, Igor Mitsyanko wrote:
> On 08/14/2017 01:13 PM, Grumbach, Emmanuel wrote:

> > > It is kind of not clear right now for drivers where to get

> > > information from:

> > > - NL attributes passed from userspace and forwarded to a driver

> > 

> > Since those exist, it seems to make more sense to me to use those

> > rather than any in-driver decided policy. Usually, deciding upon

> > policies from lower levels is frowned upon I'd say.

> 

> I agree, what I mean is that drivers should know where to take this 

> information from, if there are multiple sources.

> 

> An example are HT/VHT capabilities for start_ap command: they can be 

> parsed directly by drivers by looking at beacon's IEs in 

> "cfg80211_ap_settings". At the same time those are also parsed by 

> nl80211 which fills cfg80211_ap_settings::ht_cap, 

> cfg80211_ap_settings::vht_cap.

> Why HT/VHT caps are not passed directly from usespace as one of NL 

> attributes? I guess because there is not much point in it since 

> userspace passes entire IE set anyway, which can be used by lower

> levels.


I understand your point about the information being conveyed to the
kernel through multiple pipes. But note that for HT/VHT in AP mode, the
decision maker is the user space. User space can get it from a the user
himself (hostapd.conf) or from internal knowledge (OBSS parameters in
AP mode that are handled internally in hostapd), but it essentially
doesn't matter. The kernel does nothing else but implementing the
policy decided by the user space.

When you start thinking about firmwares with more logic, the picture is
different. When Jouni added MFP support in the kernel in 2009 he wrote:

  Since we are currently using nl80211 for MFP only with drivers that
  use user space SME, only MFP disabled and required values are
  used. However, the NL80211_ATTR_USE_MFP attribute is an enum that can
  be extended with MFP optional in the future, if that is needed with
  some drivers (e.g., if the RSN IE is generated by the driver).

(See commit dc6382ced07d6bad61d0b591fb12ab5da7ca632c).

Here is the thing: until now, MFP was supported only for drivers that
use user space SME, meaning drivers that let the user space decide
about pretty much everything: it sends the Assoc Request which means
that it knows _everything_ about the AP it connects to. When you don't
use user space SME (meaning you use NL80211_CMD_CONNECT and not
NL80211_CMD_{AUTHENTICATE,ASSOCIATE}), the user space may not know much
about the AP including not its RSN caps. Essentially, the user space is
sending a supplicant's network block to the kernel and asks the kernel
to connect to that thing. The user space may not even know that this
network is in the vicinity (which is what ap_scan=2 in the wpa_s means
IIUC).

So what I am trying to do here to shift the power of the decision from
the user space to the kernel in certain cases, or at least to allow the
user space to tell the kernel how to behave in several scenarios since
the user space doesn't have all the information in hand.

NL80211_MFP_NO is clear: you can't associate to an AP that has MFPC=1
MFPR=1
NL80211_MFP_REQUIRED is clear: you can't associate to an AP that has
MFPC=0 MFPR=0

This is possible when the user space knows how the RSN IE of the AP
looks like. If it doesn't, then it'd mean that the user space has no
way to say: I want to use MFP if required by the AP, but don't care all
that much if the AP doesn't support it, and this is exactly what the
NL80211_MFP_OPTIONAL comes to add.

I wrote this to explain that I can't use the IEs for this since I am
adding NL80211_MFP_OPTIONAL precisely because the user space doesn't
have the IEs.

Regarding the backward compatibility, this is a real problem. I am not
even sure that adding a feature flag will help since in case wpa_s
doesn't know the IEs of the AP, it can't really know what MFP
parameters to send (REQ or NO assuming wpa_s detected that the kernel
wouldn't support OPTIONAL). Note that all this is really relevant only
for ap_scan=2 configuration in wpa_s which is not really supported well
in nl80211 driver right now (at least based on the comments in the
wpa_s code). Asking why I am adding this in nl80211 kernel side if the
only user would be wpa_s with nl80211 and ap_scan=2 (which is
reportedly not really working) is a fair question :)
I am basically working in the context of a firmware that does the scan
before connect internally.

> 

> 

> > 

> > > - IEs passed from userspace, parsed by nl80211 and forwarded to a

> > > driver

> > > - IEs that are passed from userspace and parsed by drivers

> > > directly

> > > 

> > > > 

> > > > Thanks,

> > > > Arend

> > > 

> > > 

>
Kalle Valo Aug. 15, 2017, 7:16 a.m. UTC | #10
"Grumbach, Emmanuel" <emmanuel.grumbach@intel.com> writes:

> On Mon, 2017-08-14 at 20:14 +0300, Kalle Valo wrote:
>> Emmanuel Grumbach <emmanuel.grumbach@intel.com> writes:
>> 
>> > User space can now allow the kernel to associate to an AP
>> > that requires MFP or that doesn't have MFP enabled in the
>> > same NL80211_CMD_CONNECT command.
>> > The driver / firmware will decide whether to use it or not.
>> > 
>> > Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>

[...]

>> > --- a/net/wireless/nl80211.c
>> > +++ b/net/wireless/nl80211.c
>> > @@ -9115,6 +9115,7 @@ static int nl80211_connect(struct sk_buff
>> > *skb, struct genl_info *info)
>> >  	if (info->attrs[NL80211_ATTR_USE_MFP]) {
>> >  		connect.mfp = nla_get_u32(info-
>> > >attrs[NL80211_ATTR_USE_MFP]);
>> >  		if (connect.mfp != NL80211_MFP_REQUIRED &&
>> > +		    connect.mfp != NL80211_MFP_OPTIONAL &&
>> >  		    connect.mfp != NL80211_MFP_NO)
>> >  			return -EINVAL;
>> >  	} else {
>> 
>> I guess I'm missing something, but how is backwards compatibility
>> supposed to work from user space point of view? If user space uses
>> NL80211_MFP_OPTIONAL with an old kernel, the kernel will reject the
>> command with -EINVAL and user space will try again without
>> NL80211_MFP_OPTIONAL?
>
> No you are not. I simply forgot that point. I guess that this would be
> the behavior, yes...

I don't think that's very robust. How would user space (wpasupplicant)
know if the the EINVAL is because NL80211_MFP_OPTIONAL is not supported
by the kernel or because of some other error?

> This is relevant for ap_scan=2 wpa_s configuration only which makes it
> not really common, but still, you are right. Not sure how easy it will
> be to write this logic in the supplicant though... Unless we add an
> nl80211 feature bit but I feel it'd be a bit of a waste.

I don't feel that adding a feature bit is waste, I rather use a feature
flag than making ugly hacks to user space. But of course this is up to
Jouni and Johannes.
Emmanuel Grumbach Aug. 15, 2017, 7:49 a.m. UTC | #11
On Tue, 2017-08-15 at 10:16 +0300, Kalle Valo wrote:
> "Grumbach, Emmanuel" <emmanuel.grumbach@intel.com> writes:

> 

> > On Mon, 2017-08-14 at 20:14 +0300, Kalle Valo wrote:

> > > Emmanuel Grumbach <emmanuel.grumbach@intel.com> writes:

> > > 

> > > > User space can now allow the kernel to associate to an AP

> > > > that requires MFP or that doesn't have MFP enabled in the

> > > > same NL80211_CMD_CONNECT command.

> > > > The driver / firmware will decide whether to use it or not.

> > > > 

> > > > Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>

> 

> [...]

> 

> > > > --- a/net/wireless/nl80211.c

> > > > +++ b/net/wireless/nl80211.c

> > > > @@ -9115,6 +9115,7 @@ static int nl80211_connect(struct sk_buff

> > > > *skb, struct genl_info *info)

> > > >  	if (info->attrs[NL80211_ATTR_USE_MFP]) {

> > > >  		connect.mfp = nla_get_u32(info-

> > > > > attrs[NL80211_ATTR_USE_MFP]);

> > > > 

> > > >  		if (connect.mfp != NL80211_MFP_REQUIRED &&

> > > > +		    connect.mfp != NL80211_MFP_OPTIONAL &&

> > > >  		    connect.mfp != NL80211_MFP_NO)

> > > >  			return -EINVAL;

> > > >  	} else {

> > > 

> > > I guess I'm missing something, but how is backwards compatibility

> > > supposed to work from user space point of view? If user space

> > > uses

> > > NL80211_MFP_OPTIONAL with an old kernel, the kernel will reject

> > > the

> > > command with -EINVAL and user space will try again without

> > > NL80211_MFP_OPTIONAL?

> > 

> > No you are not. I simply forgot that point. I guess that this would

> > be

> > the behavior, yes...

> 

> I don't think that's very robust. How would user space

> (wpasupplicant)

> know if the the EINVAL is because NL80211_MFP_OPTIONAL is not

> supported

> by the kernel or because of some other error?


I hear, I hear. This is not an option, you are right.

> 

> > This is relevant for ap_scan=2 wpa_s configuration only which makes

> > it

> > not really common, but still, you are right. Not sure how easy it

> > will

> > be to write this logic in the supplicant though... Unless we add an

> > nl80211 feature bit but I feel it'd be a bit of a waste.

> 

> I don't feel that adding a feature bit is waste, I rather use a

> feature

> flag than making ugly hacks to user space. But of course this is up

> to

> Jouni and Johannes.


My latest point is that it wouldn't help since the useful usage was
with ap_scan=2 in which wpa_supplicant can't really know what to put
there. Or... No. It _can_ work. The user would configure pmf=optional
in the wpa_supplicant configuration, and wpa_supplicant can check if
that the underlying kernel supports it with the help of the feature
flag.
Since the pmf=optional configuration in wpa_supplicant will come from
the user (that configures the network inside wpa_supplicant),
wpa_supplicant can do this check.

I'll add this.
Emmanuel Grumbach Aug. 15, 2017, 8:03 a.m. UTC | #12
On Tue, 2017-08-15 at 10:49 +0300, Emmanuel Grumbach wrote:
> On Tue, 2017-08-15 at 10:16 +0300, Kalle Valo wrote:

> > "Grumbach, Emmanuel" <emmanuel.grumbach@intel.com> writes:

> > 

> > > On Mon, 2017-08-14 at 20:14 +0300, Kalle Valo wrote:

> > > > Emmanuel Grumbach <emmanuel.grumbach@intel.com> writes:

> > > > 

> > > > > User space can now allow the kernel to associate to an AP

> > > > > that requires MFP or that doesn't have MFP enabled in the

> > > > > same NL80211_CMD_CONNECT command.

> > > > > The driver / firmware will decide whether to use it or not.

> > > > > 

> > > > > Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com

> > > > > >

> > 

> > [...]

> > 

> > > > > --- a/net/wireless/nl80211.c

> > > > > +++ b/net/wireless/nl80211.c

> > > > > @@ -9115,6 +9115,7 @@ static int nl80211_connect(struct

> > > > > sk_buff

> > > > > *skb, struct genl_info *info)

> > > > >  	if (info->attrs[NL80211_ATTR_USE_MFP]) {

> > > > >  		connect.mfp = nla_get_u32(info-

> > > > > > attrs[NL80211_ATTR_USE_MFP]);

> > > > > 

> > > > >  		if (connect.mfp != NL80211_MFP_REQUIRED &&

> > > > > +		    connect.mfp != NL80211_MFP_OPTIONAL &&

> > > > >  		    connect.mfp != NL80211_MFP_NO)

> > > > >  			return -EINVAL;

> > > > >  	} else {

> > > > 

> > > > I guess I'm missing something, but how is backwards

> > > > compatibility

> > > > supposed to work from user space point of view? If user space

> > > > uses

> > > > NL80211_MFP_OPTIONAL with an old kernel, the kernel will reject

> > > > the

> > > > command with -EINVAL and user space will try again without

> > > > NL80211_MFP_OPTIONAL?

> > > 

> > > No you are not. I simply forgot that point. I guess that this

> > > would

> > > be

> > > the behavior, yes...

> > 

> > I don't think that's very robust. How would user space

> > (wpasupplicant)

> > know if the the EINVAL is because NL80211_MFP_OPTIONAL is not

> > supported

> > by the kernel or because of some other error?

> 

> I hear, I hear. This is not an option, you are right.

> 

> > 

> > > This is relevant for ap_scan=2 wpa_s configuration only which

> > > makes

> > > it

> > > not really common, but still, you are right. Not sure how easy it

> > > will

> > > be to write this logic in the supplicant though... Unless we add

> > > an

> > > nl80211 feature bit but I feel it'd be a bit of a waste.

> > 

> > I don't feel that adding a feature bit is waste, I rather use a

> > feature

> > flag than making ugly hacks to user space. But of course this is up

> > to

> > Jouni and Johannes.

> 

> My latest point is that it wouldn't help since the useful usage was

> with ap_scan=2 in which wpa_supplicant can't really know what to put

> there. Or... No. It _can_ work. The user would configure pmf=optional

> in the wpa_supplicant configuration, and wpa_supplicant can check if

> that the underlying kernel supports it with the help of the feature

> flag.

> Since the pmf=optional configuration in wpa_supplicant will come from

> the user (that configures the network inside wpa_supplicant),

> wpa_supplicant can do this check.

> 

> I'll add this.

> 


And then, the cfg80211 driver can add this by itself, so that quantenna
folks don't have to fear getting the new value into their firmware,
although they already reported it'd work for them.

Patch
diff mbox

diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 7950c71c0ad4..ea1cfecbf6f4 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1410,8 +1410,12 @@  enum nl80211_commands {
  *
  * @NL80211_ATTR_USE_MFP: Whether management frame protection (IEEE 802.11w) is
  *	used for the association (&enum nl80211_mfp, represented as a u32);
- *	this attribute can be used
- *	with %NL80211_CMD_ASSOCIATE and %NL80211_CMD_CONNECT requests
+ *	this attribute can be used with %NL80211_CMD_ASSOCIATE and
+ *	%NL80211_CMD_CONNECT requests. %NL80211_MFP_OPTIONAL is not allowed for
+ *	%NL80211_CMD_ASSOCIATE since user space SME is expected and hence, it
+ *	must have decided whether to use management frame protection or not.
+ *	Setting %NL80211_MFP_OPTIONAL with a %NL80211_CMD_CONNECT request will
+ *	let the driver (or the firmware) decide whether to use MFP or not.
  *
  * @NL80211_ATTR_STA_FLAGS2: Attribute containing a
  *	&struct nl80211_sta_flag_update.
@@ -4086,10 +4090,12 @@  enum nl80211_key_type {
  * enum nl80211_mfp - Management frame protection state
  * @NL80211_MFP_NO: Management frame protection not used
  * @NL80211_MFP_REQUIRED: Management frame protection required
+ * @NL80211_MFP_OPTIONAL: Management frame is optional
  */
 enum nl80211_mfp {
 	NL80211_MFP_NO,
 	NL80211_MFP_REQUIRED,
+	NL80211_MFP_OPTIONAL,
 };
 
 enum nl80211_wpa_versions {
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 8f035d9868d1..829867132326 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -9115,6 +9115,7 @@  static int nl80211_connect(struct sk_buff *skb, struct genl_info *info)
 	if (info->attrs[NL80211_ATTR_USE_MFP]) {
 		connect.mfp = nla_get_u32(info->attrs[NL80211_ATTR_USE_MFP]);
 		if (connect.mfp != NL80211_MFP_REQUIRED &&
+		    connect.mfp != NL80211_MFP_OPTIONAL &&
 		    connect.mfp != NL80211_MFP_NO)
 			return -EINVAL;
 	} else {