Page MenuHomeFreeBSD

net80211: fail setting a key if the cipher isn't HW/SW supported
Needs RevisionPublic

Authored by adrian on Mon, Mar 17, 3:25 AM.
Referenced Files
F112543735: D49393.diff
Wed, Mar 19, 3:25 PM
F112500986: D49393.diff
Tue, Mar 18, 11:09 PM
Unknown Object (File)
Mon, Mar 17, 11:47 PM
Unknown Object (File)
Mon, Mar 17, 10:49 PM
Unknown Object (File)
Mon, Mar 17, 9:55 PM

Details

Reviewers
bz
Group Reviewers
wireless
Summary

The key alloc path was checking if the key was supported in hardware
but treated /all/ keys as supported in software. As I discovered
during my ath10k port, not all NICs that support ciphers in hardware
support enough of an 802.11 frame transmit/receive path to actually
handle software encryption.

So, do a second check after the hardware encryption check to see
if it's in the software list and hard fail it if it isn't in there.

Otherwise a fun failure mode occurs - the frames are marked as
protected, but since there's no GCMP support setup/enabled, they
just get marked as "protected" but they don't go through the
encryption path, and the receiver dutifully tosses them as invalid.

I've verified this by trying to use GCMP in wpa_supplicant with
a NIC that doesn't announce GCMP HW/SW encryption, and now it actually
fails.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 62938
Build 59822: arc lint + arc unit

Event Timeline

Your description for the commit message is very misleading (*); I read it thrice in the hope to understand what you mean. Trying to paraphrase:

Reject a key for a cipher suite [forcefully (given it is not announced as supported)] set by user space (i.e., wpa_supplicant) although it is not set as supported in software crypto or device driver hardware crypto at all.

Which makes me wonder: I do not believe that we can do any hardware crypto without any software support so the code for setting ic_cryptocaps should enforce that the crypto suite is also set in ic_sw_cryptocaps (that implies an order of setting -- or given we have a default rely on that). Also the code for setting ic_sw_cryptocaps should check that we actually have an implementation ...

That's why I do not like "dead code" without any clear usage.

(*) I do not fully understand your comment about athp; I would assume GCMP will work fine if you try plain B/A mode with only sw encryption (no atempt of hw accel); only the moment you do HT/VHT that will fail? the other way round if you try to do HW accel for GCMP 128 it will fail if you do not have the net80211 sw support for it.

sys/net80211/ieee80211_ioctl.h
262

+ wlanstats

bz requested changes to this revision.Tue, Mar 18, 11:28 AM
This revision now requires changes to proceed.Tue, Mar 18, 11:28 AM
In D49393#1126451, @bz wrote:

Your description for the commit message is very misleading (*); I read it thrice in the hope to understand what you mean. Trying to paraphrase:

Reject a key for a cipher suite [forcefully (given it is not announced as supported)] set by user space (i.e., wpa_supplicant) although it is not set as supported in software crypto or device driver hardware crypto at all.

Which makes me wonder: I do not believe that we can do any hardware crypto without any software support so the code for setting ic_cryptocaps should enforce that the crypto suite is also set in ic_sw_cryptocaps (that implies an order of setting -- or given we have a default rely on that). Also the code for setting ic_sw_cryptocaps should check that we actually have an implementation ...

Ah! Ok, the software support is for software encryption/decryption, not /support/ per se. The challenge we have with some NICs (ath10k is a great example) is they can NOT do software encryption. You can't actually do CCMP in software - the NIC's firmware and configured data path (native (microsoft) wifi packet format, or 802.3 ethernet frames) are not sufficiently "raw" enough to actually /do/ the encryption/decryption in software.

There /is/ a way to do it - using raw 802.11 frame mode - but then it disables the tx/rx A-MPDU / A-MSDU and fragment handling in firmware. That mode was never actually designed to be used in production, only for diagnostics. And it shows (and I argued about it during design :-) )

So technically speaking for ath10k NIC support, we'd only set ic_cryptocaps (for hardware support), but we would NOT set ic_sw_cryptocaps (for software encryption / decryption support) because of the above.

That's why I do not like "dead code" without any clear usage.

(*) I do not fully understand your comment about athp; I would assume GCMP will work fine if you try plain B/A mode with only sw encryption (no atempt of hw accel); only the moment you do HT/VHT that will fail? the other way round if you try to do HW accel for GCMP 128 it will fail if you do not have the net80211 sw support for it.

.. and whilst i'm at it, I can /also/ talk about the original atheros AR5210 NIC, which yes is old, but I /can/ super duper give it as an example.

It supports WEP and WEP only. It doesn't have an "empty" key slot like the AR5211 and later NICs have.

So, it CAN do WEP in hardware, but if the WEP hardware crypto engine is enabled, it can't support TKIP/CCMP/GCMP/etc at all. The hardware WEP engine will see the protection bit set, and will treat it as /only/ global WEP keys.

So you have two choices - you either enable WEP in hardware and disable CCMP/TKIP/etc from even being negotiated, or you disable the WEP encryption engine, and you do everything in software.

That's why years ago I disabled the crypto engine in the AR5210, because net80211 didn't have a way to say "I support this crypto scheme in hardware, but not for software encrypt/decrypt." These days I could provide it as an attach time option for AR5210 (but I won't! CPUs are fast enough to do WEP in software, heh!) since all the pieces are now in net80211 to support it.

In D49393#1126451, @bz wrote:

Your description for the commit message is very misleading (*); I read it thrice in the hope to understand what you mean. Trying to paraphrase:

Reject a key for a cipher suite [forcefully (given it is not announced as supported)] set by user space (i.e., wpa_supplicant) although it is not set as supported in software crypto or device driver hardware crypto at all.

Which makes me wonder: I do not believe that we can do any hardware crypto without any software support so the code for setting ic_cryptocaps should enforce that the crypto suite is also set in ic_sw_cryptocaps (that implies an order of setting -- or given we have a default rely on that). Also the code for setting ic_sw_cryptocaps should check that we actually have an implementation ...

Ah! Ok, the software support is for software encryption/decryption, not /support/ per se.

Where would we know if there is support per-se? We only know because .. hard coded .. at this point?

The challenge we have with some NICs (ath10k is a great example) is they can NOT do software encryption. You can't actually do CCMP in software - the NIC's firmware and configured data path (native (microsoft) wifi packet format, or 802.3 ethernet frames) are not sufficiently "raw" enough to actually /do/ the encryption/decryption in software.

There /is/ a way to do it - using raw 802.11 frame mode - but then it disables the tx/rx A-MPDU / A-MSDU and fragment handling in firmware. That mode was never actually designed to be used in production, only for diagnostics. And it shows (and I argued about it during design :-) )

This is basically true for all modern firmware based wifi NICS (iwlwifi, rtw89,...) as without being able to do the crypto in hardware A-MPDU / A-MSDU / etc. offloading doesn't work. Hence me saying if you do plain 11a/b/g software crypto will just work fine. That's how iwlwifi worked for more than two years for people. This has nothing to do with ATH10k specifically. I would be surprised if software CCMP would not work with a 11a basic rate -- at least with the Linux version -- cannot speak for athp -- but of course with HT/VHT it will not.

I am nor arguing the code check, I am just arguing the descriptions. That said I am still not sure what ic_sw_cryptocaps will really solve for anything that has worked for a decade or two. For anything more modern it will certainly be irrelevant by the way you explain you envision it works. I'll put it in the cleanup drawer. Can you make the descriptions more generally precise somehow?