[RFC PATCH] rt2800lib: unconditionally enable MFP

[RFC PATCH] rt2800lib: unconditionally enable MFP

[Rui Salvaterra]

According to Larry [1] (and successfully verified on b43) mac80211
transparently falls back to software for unsupported hardware cyphers. Thus,
there's no reason for not unconditionally enabling MFP. This gives us WPA3
support out of the box, without having to manually disable hardware crypto.

Tested on an RT2790-based Wi-Fi card.

[1] https://lore.kernel.org/linux-wireless/8252e6a1-b83c-64eb-2503-2686374216ae@lwfinger.net/

Signed-off-by: Rui Salvaterra <rsalvaterra@gmail.com>
---
 drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index 6beac1f74e7c..a779fe771a55 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -9971,9 +9971,7 @@ static int rt2800_probe_hw_mode(struct rt2x00_dev *rt2x00dev)
 	if (!rt2x00_is_usb(rt2x00dev))
 		ieee80211_hw_set(rt2x00dev->hw, HOST_BROADCAST_PS_BUFFERING);
 
-	/* Set MFP if HW crypto is disabled. */
-	if (rt2800_hwcrypt_disabled(rt2x00dev))
-		ieee80211_hw_set(rt2x00dev->hw, MFP_CAPABLE);
+	ieee80211_hw_set(rt2x00dev->hw, MFP_CAPABLE);
 
 	SET_IEEE80211_DEV(rt2x00dev->hw, rt2x00dev->dev);
 	SET_IEEE80211_PERM_ADDR(rt2x00dev->hw,
-- 
2.26.2

Re: [RFC PATCH] rt2800lib: unconditionally enable MFP

[Stanislaw Gruszka]

On Sun, May 24, 2020 at 10:47:31AM +0100, Rui Salvaterra wrote:
> According to Larry [1] (and successfully verified on b43) mac80211
> transparently falls back to software for unsupported hardware cyphers. Thus,
> there's no reason for not unconditionally enabling MFP. This gives us WPA3
> support out of the box, without having to manually disable hardware crypto.
> 
> Tested on an RT2790-based Wi-Fi card.
> 
> [1] https://lore.kernel.org/linux-wireless/8252e6a1-b83c-64eb-2503-2686374216ae@lwfinger.net/

AFICT more work need to be done to support MFP by HW encryption properly
on rt2x00. See this message and whole thread:
https://lore.kernel.org/linux-wireless/977a3cf4-3ec5-4aaa-b3d4-eea2e8593652@nbd.name/

Stanislaw

Re: [RFC PATCH] rt2800lib: unconditionally enable MFP

[Julian Calaby]

Hi Stanislaw,

On Sun, May 24, 2020 at 9:27 PM Stanislaw Gruszka <stf_xl@wp.pl> wrote:
>
> On Sun, May 24, 2020 at 10:47:31AM +0100, Rui Salvaterra wrote:
> > According to Larry [1] (and successfully verified on b43) mac80211
> > transparently falls back to software for unsupported hardware cyphers. Thus,
> > there's no reason for not unconditionally enabling MFP. This gives us WPA3
> > support out of the box, without having to manually disable hardware crypto.
> >
> > Tested on an RT2790-based Wi-Fi card.
> >
> > [1] https://lore.kernel.org/linux-wireless/8252e6a1-b83c-64eb-2503-2686374216ae@lwfinger.net/
>
> AFICT more work need to be done to support MFP by HW encryption properly
> on rt2x00. See this message and whole thread:
> https://lore.kernel.org/linux-wireless/977a3cf4-3ec5-4aaa-b3d4-eea2e8593652@nbd.name/

Am I reading this right: rt2x00 offloads some of the processing to the
card which interferes with MFP when using software encryption, so
therefore we need to disable that offload when using software
encryption with MFP.

So if mac80211 knows that this offload is happening and that we can't
use hardware crypto for MFP, could it be smart enough to disable the
offload itself?

And once mac80211 is smart enough to make those decisions, couldn't we
just enable MFP by default?

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [RFC PATCH] rt2800lib: unconditionally enable MFP

[Stanislaw Gruszka]

Hi

On Sun, May 24, 2020 at 09:42:51PM +1000, Julian Calaby wrote:
> Hi Stanislaw,
> 
> On Sun, May 24, 2020 at 9:27 PM Stanislaw Gruszka <stf_xl@wp.pl> wrote:
> >
> > On Sun, May 24, 2020 at 10:47:31AM +0100, Rui Salvaterra wrote:
> > > According to Larry [1] (and successfully verified on b43) mac80211
> > > transparently falls back to software for unsupported hardware cyphers. Thus,
> > > there's no reason for not unconditionally enabling MFP. This gives us WPA3
> > > support out of the box, without having to manually disable hardware crypto.
> > >
> > > Tested on an RT2790-based Wi-Fi card.
> > >
> > > [1] https://lore.kernel.org/linux-wireless/8252e6a1-b83c-64eb-2503-2686374216ae@lwfinger.net/
> >
> > AFICT more work need to be done to support MFP by HW encryption properly
> > on rt2x00. See this message and whole thread:
> > https://lore.kernel.org/linux-wireless/977a3cf4-3ec5-4aaa-b3d4-eea2e8593652@nbd.name/
> 
> Am I reading this right: rt2x00 offloads some of the processing to the
> card which interferes with MFP when using software encryption, so
> therefore we need to disable that offload when using software
> encryption with MFP.

Yes.

We offload encryption to HW based on cipher. Modern ciphers like 
GCMP, BIP_GMAC, etc, are not supported by rt2x00 HW. In such case
rt2x00mac_set_key() will return -EOPNOTSUPP and all encryption will
be done by mac80211 - MFP will work just fine.

But MFP can still be used with CCMP cipher, which we offload to HW,
and that would create problems described by Felix.

> So if mac80211 knows that this offload is happening and that we can't
> use hardware crypto for MFP, could it be smart enough to disable the
> offload itself?
> 
> And once mac80211 is smart enough to make those decisions, couldn't we
> just enable MFP by default?

If we will have indicator from mac80211 that MFP is configured, we can
just return -EOPNOTSUPP from rt2x00mac_set_key() for CCMP and that will
make MFP work without specifying nohwcrypt module parameter - software
encryption will be used anyway.

Optimal solution though would be implement similar code like in mt76, so
we will have HW encryption for MFP+CCMP, but this is not trivial, and
I think handling encryption fully in software is ok.

Stanislaw

Re: [RFC PATCH] rt2800lib: unconditionally enable MFP

[Rui Salvaterra]

Hi, Stanislaw,

On Sun, 24 May 2020 at 12:18, Stanislaw Gruszka <stf_xl@wp.pl> wrote:
>
> AFICT more work need to be done to support MFP by HW encryption properly
> on rt2x00. See this message and whole thread:
> https://lore.kernel.org/linux-wireless/977a3cf4-3ec5-4aaa-b3d4-eea2e8593652@nbd.name/
>
> Stanislaw

This RT2790 has been working just fine with my patch for hours. No
hangs at all. What additional bad behaviour should I expect?

Thanks,
Rui

Re: [RFC PATCH] rt2800lib: unconditionally enable MFP

[Larry Finger]

On 5/24/20 10:07 AM, Rui Salvaterra wrote:
> Hi, Stanislaw,
> 
> On Sun, 24 May 2020 at 12:18, Stanislaw Gruszka <stf_xl@wp.pl> wrote:
>>
>> AFICT more work need to be done to support MFP by HW encryption properly
>> on rt2x00. See this message and whole thread:
>> https://lore.kernel.org/linux-wireless/977a3cf4-3ec5-4aaa-b3d4-eea2e8593652@nbd.name/
>>
>> Stanislaw
> 
> This RT2790 has been working just fine with my patch for hours. No
> hangs at all. What additional bad behaviour should I expect?

I read the above thread. It seems that the best thing to do with b43 is to send 
the MFP_CAPABLE only when nohwcrypt is set as a module option. I wish it could 
have worked the other way, but I think the potential for keys getting out of 
sync should be avoided.I still need to find a place the log something when 
ciphers are present and the option is not set.

Larry

Re: [RFC PATCH] rt2800lib: unconditionally enable MFP

[Stanislaw Gruszka]

Hello

On Sun, May 24, 2020 at 04:07:23PM +0100, Rui Salvaterra wrote:
> Hi, Stanislaw,
> 
> On Sun, 24 May 2020 at 12:18, Stanislaw Gruszka <stf_xl@wp.pl> wrote:
> >
> > AFICT more work need to be done to support MFP by HW encryption properly
> > on rt2x00. See this message and whole thread:
> > https://lore.kernel.org/linux-wireless/977a3cf4-3ec5-4aaa-b3d4-eea2e8593652@nbd.name/
> >
> > Stanislaw
> 
> This RT2790 has been working just fine with my patch for hours. No
> hangs at all. What additional bad behaviour should I expect?

If you use new cipher like WLAN_CIPHER_SUITE_AES_CMAC (what I think is
default for MFP setups) things will work just fine, because all
encryption will be done by software.

For older ciphers that are offloaded to hardware, namely
WLAN_CIPHER_SUITE_CCMP, management frames like Disassociate, Deauthenticate,
Action, will not be sent properly encrypted.

On quoted thread described visible problem was lag and performance drop
due to failed A-MPDU aggregation session setup.

Stanislaw

Re: [RFC PATCH] rt2800lib: unconditionally enable MFP

[Johannes Berg]

On Sun, 2020-05-24 at 14:39 +0200, Stanislaw Gruszka wrote:
> 
> > And once mac80211 is smart enough to make those decisions, couldn't we
> > just enable MFP by default?

For the record, I don't think we'd really want to add such a thing to
mac80211 ... easier done in the driver.

> If we will have indicator from mac80211 that MFP is configured, we can
> just return -EOPNOTSUPP from rt2x00mac_set_key() for CCMP and that will
> make MFP work without specifying nohwcrypt module parameter - software
> encryption will be used anyway.

Not sure mac80211 really knows? Hmm.

johannes

Re: [RFC PATCH] rt2800lib: unconditionally enable MFP

[Johannes Berg]

On Sun, 2020-05-24 at 19:02 -0500, Larry Finger wrote:
> On 5/24/20 10:07 AM, Rui Salvaterra wrote:
> > Hi, Stanislaw,
> > 
> > On Sun, 24 May 2020 at 12:18, Stanislaw Gruszka <stf_xl@wp.pl> wrote:
> > > AFICT more work need to be done to support MFP by HW encryption properly
> > > on rt2x00. See this message and whole thread:
> > > https://lore.kernel.org/linux-wireless/977a3cf4-3ec5-4aaa-b3d4-eea2e8593652@nbd.name/
> > > 
> > > Stanislaw
> > 
> > This RT2790 has been working just fine with my patch for hours. No
> > hangs at all. What additional bad behaviour should I expect?
> 
> I read the above thread. It seems that the best thing to do with b43 is to send 
> the MFP_CAPABLE only when nohwcrypt is set as a module option. I wish it could 
> have worked the other way, but I think the potential for keys getting out of 
> sync should be avoided.I still need to find a place the log something when 
> ciphers are present and the option is not set.

With b43 you have much less to worry about though.

Contrary to what I just said in my other email (oops, sorry) there are
two problems here:

 1) RX of management frames - if hw/fw erroneously attempts to decrypt
 2) PN assignment during TX

1) you can test easily with b43, say send a deauth from the AP to the
client and check the frame goes through properly. If it does, then the
device isn't attempting to decrypt.

2) isn't an issue with b43 since it does it in software (I believe in
mac80211) anyway.

johannes

Re: [RFC PATCH] rt2800lib: unconditionally enable MFP

[Stanislaw Gruszka]

On Mon, May 25, 2020 at 11:15:29AM +0200, Johannes Berg wrote:
> On Sun, 2020-05-24 at 14:39 +0200, Stanislaw Gruszka wrote:
> > 
> > > And once mac80211 is smart enough to make those decisions, couldn't we
> > > just enable MFP by default?
> 
> For the record, I don't think we'd really want to add such a thing to
> mac80211 ... easier done in the driver.
> 
> > If we will have indicator from mac80211 that MFP is configured, we can
> > just return -EOPNOTSUPP from rt2x00mac_set_key() for CCMP and that will
> > make MFP work without specifying nohwcrypt module parameter - software
> > encryption will be used anyway.
> 
> Not sure mac80211 really knows? Hmm.

After looking at this a bit more, seems we have indicator of MFP being
used in ieee80211_sta structure. So maybe adding check like below
will allow to remove nohwcrypt rt2x00 requirement for MFP ?

diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
index 32efbc8e9f92..241e42bb0fd2 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
@@ -468,7 +468,7 @@ int rt2x00mac_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
 	if (!test_bit(DEVICE_STATE_PRESENT, &rt2x00dev->flags))
 		return 0;
 
-	if (!rt2x00_has_cap_hw_crypto(rt2x00dev))
+	if (!rt2x00_has_cap_hw_crypto(rt2x00dev) || sta->mfp)
 		return -EOPNOTSUPP;
 
 	/*

Stanislaw

Re: [RFC PATCH] rt2800lib: unconditionally enable MFP

[Johannes Berg]

On Mon, 2020-05-25 at 11:31 +0200, Stanislaw Gruszka wrote:
> On Mon, May 25, 2020 at 11:15:29AM +0200, Johannes Berg wrote:
> > On Sun, 2020-05-24 at 14:39 +0200, Stanislaw Gruszka wrote:
> > > > And once mac80211 is smart enough to make those decisions, couldn't we
> > > > just enable MFP by default?
> > 
> > For the record, I don't think we'd really want to add such a thing to
> > mac80211 ... easier done in the driver.
> > 
> > > If we will have indicator from mac80211 that MFP is configured, we can
> > > just return -EOPNOTSUPP from rt2x00mac_set_key() for CCMP and that will
> > > make MFP work without specifying nohwcrypt module parameter - software
> > > encryption will be used anyway.
> > 
> > Not sure mac80211 really knows? Hmm.
> 
> After looking at this a bit more, seems we have indicator of MFP being
> used in ieee80211_sta structure.

Yeah, where's my head ... sorry.

> So maybe adding check like below
> will allow to remove nohwcrypt rt2x00 requirement for MFP ?

Seems reasonable, but will still do _everything_ in software for such
connections. Still better than not connecting, I guess.

johannes

Re: [RFC PATCH] rt2800lib: unconditionally enable MFP

[Stanislaw Gruszka]

On Mon, May 25, 2020 at 11:49:56AM +0200, Johannes Berg wrote:
> On Mon, 2020-05-25 at 11:31 +0200, Stanislaw Gruszka wrote:
> > On Mon, May 25, 2020 at 11:15:29AM +0200, Johannes Berg wrote:
> > > On Sun, 2020-05-24 at 14:39 +0200, Stanislaw Gruszka wrote:
> > > > > And once mac80211 is smart enough to make those decisions, couldn't we
> > > > > just enable MFP by default?
> > > 
> > > For the record, I don't think we'd really want to add such a thing to
> > > mac80211 ... easier done in the driver.
> > > 
> > > > If we will have indicator from mac80211 that MFP is configured, we can
> > > > just return -EOPNOTSUPP from rt2x00mac_set_key() for CCMP and that will
> > > > make MFP work without specifying nohwcrypt module parameter - software
> > > > encryption will be used anyway.
> > > 
> > > Not sure mac80211 really knows? Hmm.
> > 
> > After looking at this a bit more, seems we have indicator of MFP being
> > used in ieee80211_sta structure.
> 
> Yeah, where's my head ... sorry.
> 
> > So maybe adding check like below
> > will allow to remove nohwcrypt rt2x00 requirement for MFP ?
> 
> Seems reasonable, but will still do _everything_ in software for such
> connections. Still better than not connecting, I guess.

Yeah, and at least without nohwcrypt=y we can still use HW encryption
for non-MFP stations.

Rui, feel free to repost your patch with additional sta->mfp check.

If someone is willing to implement mt76 approach to have HW encryption offload
for MFP+CCMP, I'll be happy to review patch. From other hand, most people will
use MFP with modern ciphers anyway, so I'm not sure how much need is for such
patch.

Stanislaw

Re: [RFC PATCH] rt2800lib: unconditionally enable MFP

[Rui Salvaterra]

Hi, Stanislaw,

On Mon, 25 May 2020 at 11:58, Stanislaw Gruszka <stf_xl@wp.pl> wrote:
>
> Yeah, and at least without nohwcrypt=y we can still use HW encryption
> for non-MFP stations.
>
> Rui, feel free to repost your patch with additional sta->mfp check.

Sure, will do. :)

> If someone is willing to implement mt76 approach to have HW encryption offload
> for MFP+CCMP, I'll be happy to review patch. From other hand, most people will
> use MFP with modern ciphers anyway, so I'm not sure how much need is for such
> patch.

I've been having a look at how mt76 solves the problem, but I haven't
fully understood it yet. I feel it's out of my league, since I only
started looking at wireless drivers very recently (I don't grasp all
the concepts and the architecture). But one thing that also bugs me
about software fallbacks is that for most of the older CPUs we don't
have SIMD implementations of the required crypto. So, not only we fall
back to software, but we're also stuck with scalar C algorithms on
CPUs which are already slow.

Rui

Re: [RFC PATCH] rt2800lib: unconditionally enable MFP

[Stanislaw Gruszka]

Hello

On Mon, May 25, 2020 at 12:14:54PM +0100, Rui Salvaterra wrote:
> > If someone is willing to implement mt76 approach to have HW encryption offload
> > for MFP+CCMP, I'll be happy to review patch. From other hand, most people will
> > use MFP with modern ciphers anyway, so I'm not sure how much need is for such
> > patch.
> 
> I've been having a look at how mt76 solves the problem, but I haven't
> fully understood it yet. I feel it's out of my league, since I only
> started looking at wireless drivers very recently (I don't grasp all
> the concepts and the architecture).

Yeah, it's somewhat complicated.

> But one thing that also bugs me
> about software fallbacks is that for most of the older CPUs we don't
> have SIMD implementations of the required crypto. So, not only we fall
> back to software, but we're also stuck with scalar C algorithms on
> CPUs which are already slow.

If we talk about x86, we have AES-NI instructions since 2008:
https://en.wikipedia.org/wiki/AES_instruction_set
Which make CPU crypto quite fast. Though it can be a bottleneck,
I think, if wifi encryption is performed together with other encryption
applications like SSL .

Stanislaw

Re: [RFC PATCH] rt2800lib: unconditionally enable MFP

[Rui Salvaterra]

On Mon, 25 May 2020 at 13:25, Stanislaw Gruszka <stf_xl@wp.pl> wrote:
>
> If we talk about x86, we have AES-NI instructions since 2008:
> https://en.wikipedia.org/wiki/AES_instruction_set
> Which make CPU crypto quite fast. Though it can be a bottleneck,
> I think, if wifi encryption is performed together with other encryption
> applications like SSL .

Hah, AES-NI would be great, but the best this CPU can do is SSSE3. ;)

Re: [RFC PATCH] rt2800lib: unconditionally enable MFP

[Rui Salvaterra]

On Mon, 25 May 2020 at 12:14, Rui Salvaterra <rsalvaterra@gmail.com> wrote:
>
> Hi, Stanislaw,
>
> On Mon, 25 May 2020 at 11:58, Stanislaw Gruszka <stf_xl@wp.pl> wrote:
> >
> > Yeah, and at least without nohwcrypt=y we can still use HW encryption
> > for non-MFP stations.
> >
> > Rui, feel free to repost your patch with additional sta->mfp check.
>
> Sure, will do. :)

Wait, scratch that. The additional sta->mfp check causes a NPE, sta is
probably null at that point.

Rui

Re: [RFC PATCH] rt2800lib: unconditionally enable MFP

[Johannes Berg]

On Mon, 2020-05-25 at 14:13 +0100, Rui Salvaterra wrote:
> On Mon, 25 May 2020 at 12:14, Rui Salvaterra <rsalvaterra@gmail.com> wrote:
> > Hi, Stanislaw,
> > 
> > On Mon, 25 May 2020 at 11:58, Stanislaw Gruszka <stf_xl@wp.pl> wrote:
> > > Yeah, and at least without nohwcrypt=y we can still use HW encryption
> > > for non-MFP stations.
> > > 
> > > Rui, feel free to repost your patch with additional sta->mfp check.
> > 
> > Sure, will do. :)
> 
> Wait, scratch that. The additional sta->mfp check causes a NPE, sta is
> probably null at that point.

Not at this point - but the GTK comes with null STA, so you want
	(sta && sta->mfp)

instead.

johannes

Re: [RFC PATCH] rt2800lib: unconditionally enable MFP

[Rui Salvaterra]

On Mon, 25 May 2020 at 14:14, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> Not at this point - but the GTK comes with null STA, so you want
>         (sta && sta->mfp)
>
> instead.

Indeed, that did the trick, thanks. Will send the patch soon.