Details

Reviewers

bz
cc

Group Reviewers

wireless

Commits

rG1116e8b95c60: net80211: add a new field specifically for announcing specific ciphers

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.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 57176
Build 54064: arc lint + arc unit

Event Timeline

adrian created this revision.Apr 17 2024, 2:12 AM

Herald added subscribers: glebius, melifaro, imp. · View Herald TranscriptApr 17 2024, 2:12 AM

adrian requested review of this revision.Apr 17 2024, 2:12 AM

adrian added a child revision: D44821: wpa: Remove the now not-needed local logic to hard-code cipher support.

Harbormaster completed remote builds in B57140: Diff 137210.Apr 17 2024, 2:13 AM

adrian retitled this revision from [net80211] add a new field specifically for announcing specific ciphers to net80211: add a new field specifically for announcing specific ciphers.Apr 17 2024, 2:51 AM

adrian added a reviewer: wireless.

cc added a subscriber: cc.Apr 17 2024, 2:34 PM

cc added inline comments.

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;

Feedback

Harbormaster completed remote builds in B57145: Diff 137261.Apr 17 2024, 4:46 PM

adrian marked 2 inline comments as done.Apr 17 2024, 5:19 PM

bz requested changes to this revision.Apr 17 2024, 6:47 PM

bz added a subscriber: bz.

bz added inline comments.

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?

This revision now requires changes to proceed.Apr 17 2024, 6:47 PM

adrian added inline comments.Apr 17 2024, 7:09 PM

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 added inline comments.Apr 17 2024, 8:01 PM

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...

address comments from bz@

Harbormaster completed remote builds in B57152: Diff 137273.Apr 18 2024, 1:25 AM

adrian added inline comments.Apr 18 2024, 1:25 AM

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 marked 5 inline comments as done.Apr 18 2024, 1:26 AM

adrian marked an inline comment as done.Apr 18 2024, 1:38 AM

adrian marked an inline comment as done.Apr 18 2024, 3:24 AM

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

Harbormaster completed remote builds in B57176: Diff 137315.Apr 18 2024, 5:18 PM

adrian added a project: wireless.Apr 20 2024, 8:45 PM

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`.

This revision now requires changes to proceed.Apr 24 2024, 2:20 PM

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.

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

address comments from cc@

Harbormaster completed remote builds in B57342: Diff 137643.Apr 25 2024, 4:16 AM

adrian edited the summary of this revision. (Show Details)Apr 25 2024, 4:16 AM

The rest looks good to me.

share/man/man9/ieee80211.9
523–530 ↗	(On Diff #137643)	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.

This revision now requires changes to proceed.Apr 26 2024, 2:18 PM

address feedback

Harbormaster completed remote builds in B57409: Diff 137740.Apr 27 2024, 1:51 AM

adrian marked an inline comment as done.Apr 27 2024, 1:52 AM

push update

Harbormaster completed remote builds in B57440: Diff 137811.Apr 29 2024, 3:02 AM

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 added inline comments.Apr 30 2024, 1:38 PM

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..)

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

Can you please update the commit message? Apart from that it seems fine-ish with what cc@ said.

In D44820#1027551, @bz wrote:

Can you please update the commit message? Apart from that it seems fine-ish with what cc@ said.

Update the commit message with what? I've fixed the new field renaming?

This revision was not accepted when it landed; it landed in state Needs Review.May 9 2024, 12:50 AM

Closed by commit rG1116e8b95c60: net80211: add a new field specifically for announcing specific ciphers (authored by adrian). · Explain Why

This revision was automatically updated to reflect the committed changes.

adrian added a commit: rG1116e8b95c60: net80211: add a new field specifically for announcing specific ciphers.

net80211: add a new field specifically for announcing specific ciphers
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 137315

sys/net80211/ieee80211_crypto.c

sys/net80211/ieee80211_ioctl.c

sys/net80211/ieee80211_var.h

net80211: add a new field specifically for announcing specific ciphersClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 137315

sys/net80211/ieee80211_crypto.c

sys/net80211/ieee80211_ioctl.c

sys/net80211/ieee80211_var.h

net80211: add a new field specifically for announcing specific ciphers
ClosedPublic
Actions

Revision Contents
Changeset List