net80211: add a new field specifically for announcing specific ciphers

[adrian]

Summary

This dates way, way back with the original net80211 support w/ atheros chips.

The earliest chip (AR5210) had limitations supporting software encryption.
It only had the four WEP slots, and not any keycache entries. So when
trying to do CCMP/TKIP encryption would be enabled and the key slots
would have nothing useful in them, resulting in garbage encryption/decryption.

I changed this back in 2012 to disable supporting hardware WEP so it's
all done in software and yes, I could do CCMP/TKIP on AR5210 in software.

Fast-forward to newer-ish hardware - the Qualcomm 11ac hardware.
Those also don't support pass-through keycache slots! Well, the hardware
does at that layer, but then there's a whole offload data path encap/decap
layer that's turning the frames from raw wifi into ethernet frames (for
"dumb" AP behaviours) or "wifi direct" frames (ie, "windows".)
This hides a bunch of header frame contents required for doing the software
encryption / decryption path.

But then if you enable the raw transmit/receive frame format it ALSO
bypasses the hardware encryption/decryption engine!

So for those NICs:

• If you want to do encryption, you can only use the firmware supported ciphers w/ wifi direct or ethernet;
• If you want to use software encrypt/decrypt, you MUST disable all encryption and instead use 100% software encryption.

The wpa_supplicant bsd driver code has a specific comment about this and
flips on supporting WEP/TKIP/CCMP, which is understandable but it doesn't
fix the ACTUAL intention of all of this stuff.

So:

• create a new field, ic_sw_cryptocaps
• populate it with the supported set of ciphers for net80211 (right now wep, tkip, ccmp)
• Communicate that upward to wpa_supplicant via the relevant devcap ioctl.

I'll follow this up with a driver_bsd.c change in wpa_supplicant to
trust this again, and then start adding the other cipher support there.

[cc]

sys/net80211/ieee80211_crypto.c

147

I believe you mean this:

Default set of net80211 supported ciphers.

150

Because ic_wpa_cryptocaps is not JUST the hardware supported ciphers from your comment below in ieee80211_ioctl.c, I believe here it means:

ic_wpa_cryptocaps implies some hardware supported ciphers.

And no need to add the slash symbol / in this comment.

155–159

Please re-organize the format of this line and utilize the assumed 80 character's limit. Like this:

	ic->ic_wpa_cryptocaps = IEEE80211_CRYPTO_WEP | IEEE80211_CRYPTO_TKIP |
				IEEE80211_CRYPTO_AES_CCM;

or like this:

	ic->ic_wpa_cryptocaps = IEEE80211_CRYPTO_WEP |
				IEEE80211_CRYPTO_TKIP |
				IEEE80211_CRYPTO_AES_CCM;

[adrian]

Feedback

[bz]

sys/net80211/ieee80211_crypto.c

150

no need for "actually" in that sentence.

153

NOT and OUT are not "special language" and no need to yell.
It would probably be better to name "this field" to avoid future changes leaving it unclear (and probably not the best place for that comment either but that's a general problem in net80211 that we document the API in the code rather than the header.)

153

In general I do not like the logic that every driver has to know about the internal field and masking.
Somehow I'd prefer if the driver would tell me what it can do and net80211 does the intersect somewhere and the driver uses the result.
This might require more fields and more logic to cover the possibly different setups.

Otherwise: every time you change the "default set" here by adding something new, you have to review all the drivers and see if you have to mask something (new) off.
It sounds like a quick hack and a long-term support nightmare, especially with all the legacy drivers.

sys/net80211/ieee80211_ioctl.c

714

That part of the comment is describing your change but not what you want to document long-term based the logic you describe?
You are announcing the intersected set of net80211 and driver supported ciphers here or in other words it is a dynamic set of ciphers which can be used.

What I think you are trying to say is "Despite the (historic) field name dc_cryptocaps this does not announce the hardware supported ciphers from similar named ic_cryptocaps but ..."?

719

That half of the comment doesn't belong here as it is no use to user space as to how the bitfield came together?

[adrian]

sys/net80211/ieee80211_crypto.c

153

Yeah, it's kind ugly. The drivers already do have to do this for the hardware caps.

My goal here is that net80211 doesn't add the other crypto types. The existing set is what all of our current drivers support anyway. Any driver that I've stumbled across that can't do this set has either been fixed (eg AR5210) or removed (eg Prism).

This doesn't add any new ciphers and I wouldn't want net80211 to enable them here. Drivers need to announce it.

Let me change this to be "this is the default set, drivers need to add to it if they support software encryption for these ciphers."

Eg if we have a NIC that can't do software encryption, they won't enable further ciphers in this set.

And you're right, I'd prefer there be a function call where the driver announces what hardware and software crypto support they have, rather than just setting ic_cryptocaps directly like they currently do for hardware encryption. Maybe I should add a couple of function calls for drivers to call during init to set the software and hardware supported ciphers? Then they can add to this set as appropriate.

sys/net80211/ieee80211_ioctl.c

714

You know I could just mask the two cryptocaps fields together before returning them. :-) Ha.

[bz]

sys/net80211/ieee80211_crypto.c

153

I am also concerned about what to do with new stuff which doesn't (want to) do WEP or TKIP anymore... (not that we should still be default ...).
But it all requires a lot more thought than just this...

[adrian]

address comments from bz@

[adrian]

sys/net80211/ieee80211_crypto.c

153

We can clean this up later once this lands. My guess is we should add a couple of new calls to setup supported software and hardware ciphers, then audit all the drivers to set it, then we can error out if drivers don't call it before attach finishes.

[adrian]

• rename field / comments to better reflect what's going on
• update commit message to better reflect what's going on

[cc]

I am intending to give approval, but I am afraid of missing the new response to my review comment for the code comment update. So let's be patient. :)

sys/net80211/ieee80211_ioctl.c

716

Both of the comment on top of struct ieee80211_devcaps_req and the comment for field dc_cryptocaps said only "hardware crypto". But now we are adding sw ciphers. So both of these places shall be updated as well.

And luckily, I didn't find anywhere else to update the comment for the struct ieee80211_devcaps_req or the field dc_cryptocaps.

[cc]

Also please update the Summary section in this review, i.e. the mentioned new field ic_wpa_cryptocaps is actually the ic_sw_cryptocaps in the code change.

[cc]

Also do you have any plan to update the man page regarding the new field ic_sw_cryptocaps besides ic_cryptocaps ?

https://man.freebsd.org/cgi/man.cgi?query=ieee80211&sektion=9

[adrian]

address comments from cc@

[cc]

The rest looks good to me.

share/man/man9/ieee80211.9

521–528

There are following separate specifications for each iv_caps, iv_cryptocaps and iv_htcaps. If ic_vhtcaps is added now, I am afraid that a new section of specification for 802.11ac capabilities has to be added, which I think is not necessary in this patch.

Therefore, adding ic_vhtcaps and its individual section of specification shall be delayed later in a different patch.

[adrian]

address feedback

[adrian]

push update

[cc]

Cautious individual accept.

sys/net80211/ieee80211_var.h

168

I accept the current change.

But I still think clarify the name for hw crypto caps will better help read the code, as dc_cryptocaps combines both sw and hw caps. For example, replace ic_cryptocaps with ic_hw_cryptocaps.

However, I think just renaming this field will take effort and scrutiny, which does not fit in this patch. So if possible, please add a plan of a separate patch for the ic_hw_cryptocaps renaming.

[adrian]

sys/net80211/ieee80211_var.h

168

I'd like to rename the field, but later. It'll require touching a few more drivers. :-) (If you notice later in the stack I actually start that work..)

[emaste]

I think this is fine; any further comments @bz?