| Message ID | 20200524094730.2684-1-rsalvaterra@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [RFC] rt2800lib: unconditionally enable MFP | expand |
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
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,
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
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
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
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
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
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
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
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
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
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
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
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. ;)
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
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
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.
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,
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(-)