Details

Reviewers

Group Reviewers

Commits

rG2d4583c462a5: net80211: add initial AES-GCMP crypto support

Summary

This adds initial AES-GCMP crypto support. It registers for both
128 and 256 bit support, although the 256 bit support will not work
without extending the net80211/ioctl keylength.

This is not yet enabled by default; drivers will need to opt-in
to supporting it in either hardware or software.

The AES-GCMP code is BSD licenced code from hostapd.git .

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 62750
Build 59634: arc lint + arc unit

Event Timeline

adrian created this revision.Feb 28 2025, 3:23 AM

Herald added subscribers: glebius, melifaro, imp. · View Herald TranscriptFeb 28 2025, 3:23 AM

adrian requested review of this revision.Feb 28 2025, 3:23 AM

Harbormaster completed remote builds in B62668: Diff 151661.Feb 28 2025, 3:23 AM

adrian added a reviewer: wireless.Feb 28 2025, 3:26 AM

adrian added a project: wireless.

adrian added a child revision: D49162: wlanstats: add GCMP crypto counters.

some code cleanups

Harbormaster completed remote builds in B62713: Diff 151719.Mar 1 2025, 5:26 PM

So for one this is missing similar changes as went into CCMP recently.

For the other it has all the MMIC and other bits "copied" from CCMP. Before we duplicate them we should fix them, really.

sys/net80211/ieee80211_crypto_gcmp.c
394	Well I know this is what CCMP is doing though a bit more verbose back then. We do not have the README as part of this codebase and general git repo name without full URl and revision in the commit message does not help. Also this ends at some point again and normal code continues. I would suggest marking this with some kind-of delimiters at least but also in theory the copyright should be at the top of the file. I'll add @imp.

This revision now requires changes to proceed.Mar 1 2025, 10:51 PM

oh yeah I agree that it can do with some refactoring, and I need to update with other changes in CCMP. I just wanted a self-contained stack that's /just/ this to start with :-)

I still have some clean-up to do.

feedback from bz; add missing updates to CCMP since I started this; more counters

Harbormaster completed remote builds in B62726: Diff 151753.Mar 2 2025, 5:26 PM

adrian added inline comments.Mar 2 2025, 5:29 PM

sys/net80211/ieee80211_crypto_gcmp.c
394	Yeah I'm not entirely pleased with the current layout. I'm tempted to create a second source file JUST for the AES-GCM code. That'll at least keep it contained. What do you two think about that?

imp added inline comments.Mar 3 2025, 11:36 PM

sys/net80211/ieee80211_crypto_gcmp.c
6	Nit, please consider adding this line after All Rights Reserved. I'd also change the date to 2025.

imp added inline comments.Mar 3 2025, 11:45 PM

sys/net80211/ieee80211_crypto_gcmp.c
244	stupid nit: no space after !
395	stupid nit" use 'for (int i = 0;... )' here

bz added inline comments.Mar 4 2025, 12:30 AM

sys/net80211/ieee80211_crypto_gcmp.c
394	I like the end marker; am I right that only the last two functions are called from the net80211 code? I was contemplating a /h file with just inline functions but that's likely not great either. If it's just two functions calls we could prefix them with __net80211_ or something so they stay distinct here and will not interfere with TLS or any other crypto and then keep them otherwise private. I wanted to look how it shows on the CCMP file; if it's the same there we could probably do the same there... Sorry I know it's not helpful to get this landed but it'll save us. Alternatively we get it in and factor it out if it's inconvenient in the review.
1005	This will be MIC with D49219

split out the hostapd code from the gcmp 802.11 code

Harbormaster completed remote builds in B62750: Diff 151832.Mar 4 2025, 5:44 AM

I've split the crypto code out into its own source file. This now cleanly separates out what I wrote / copied from ccmp.c versus what I lifted from hostapd.

I've updated the tree; could I please get a re-review with updated locations on all the nits? :-)

thanks!

sys/net80211/ieee80211_crypto_gcmp.c
394	ok, wanna go see if that's any better? I can go prefix them too; they're in their own source file now so it's easy to hide the details.

feedback from bz/imp

Harbormaster completed remote builds in B62811: Diff 151989.Mar 7 2025, 2:37 AM

adrian marked an inline comment as done.Mar 7 2025, 5:27 AM

In D49161#1123631, @adrian wrote:

I've updated the tree; could I please get a re-review with updated locations on all the nits? :-)

I will try tonight when back; sorry fixing the panic was more important yesterday and I would love to see this and CCMP bits in so I'll try my best. Please be patient.

more cleanups from bz/imp, rebase against crypto changes in -head

Harbormaster completed remote builds in B62837: Diff 152049.Mar 8 2025, 5:52 PM

adrian marked 4 inline comments as done.Mar 8 2025, 6:07 PM

Love the fixed copyright and the movement of the crypto functions to their own file.

sys/net80211/ieee80211_crypto_gcmp.c
6	Looks good to me! Thanks for the shuffle.

adrian marked 2 inline comments as done.Mar 8 2025, 10:42 PM

adrian added inline comments.

sys/net80211/ieee80211_crypto_gcmp.c
395	stupid nit" use 'for (int i = 0;... )' here It's no longer relevant for this file; the relevant code moved to ieee80211_aes_gcm.c

I'll need more time for this.

I love the split out parts!

sys/modules/wlan_gcmp/Makefile
2	Empty line?
sys/net80211/ieee80211_crypto_gcm.c
9	extra blank line? Is that from upstream?
32	Do you still need this?
34	Doubt this file needs this one.
41	Also not sure about that one? If rijndael needs it it should take care of it itself?
sys/net80211/ieee80211_crypto_gcm.h
46	Indentations look weird here and for the next one. Could be Phabricator, can you check?
sys/net80211/ieee80211_crypto_gcmp.c
30	Do you need this?
34	I assume this is gone into its own file here?
58	How does this compile? It's a re-define from crypto_gcm.h
87	Indent looks weird.
90	I need to sort my head around what ic_trailer and ic_miclen (or enmimc()) means in net80211 and implies.
162	In theory this and the above coul go into std. ieee80211_crypto.h and be called ieee80211_crypto_get_XXX_len()
182	I still think there should be one internal implementation which takes two arguments and these two can be one-line-wrappers.
207	I wonder why we don't name them PN5,4 and PN3/2/1/0 like gcmp_init_iv does. I wonder if ccmp got this the other way before as well?
210	Indent looks weird.
242	And no () either. Should be a bool anyway...
273	extra empty line
303	if ((rxs != NULL) && (rxs->c_pktflags & IEEE80211_RX_F_IV_STRIP) != 0)
351	if ((rxs == NULL) \|\| (rxs->c_pktflags & IEEE80211_RX_F_IV_STRIP) == 0) {
357	if ((rxs == NULL) \|\| (rxs->c_pktflags & IEEE80211_RX_F_MIC_STRIP) == 0)
363	if ((rxs == NULL) \|\| (rxs->c_pktflags & IEEE80211_RX_F_IV_STRIP) == 0) {

adrian marked an inline comment as done.Mar 9 2025, 4:13 AM

address feedback from bz

Harbormaster completed remote builds in B62845: Diff 152072.Mar 10 2025, 4:14 AM

ok, i've addressed most of the most recent comments. I'll check the indenting ones tomorrow.

I want to just go and re-verify this again tomorrow against my test ath9k AP that I have setup to do GCMP in 11n mode so I know it's still A-OK.

sys/net80211/ieee80211_crypto_gcm.h
46	They're tab and tab + 4 spaces. What should they be?
sys/net80211/ieee80211_crypto_gcmp.c
162	It's a good candidate for a bit of a refactor after this lands. same with the aad assembly code too, i can refactor that out after this lands. The 802.11 spec actually states the AAD assembly for GCMP is the same as CCMP.
182	heh, fine. :-)

adrian marked an inline comment as done.Mar 10 2025, 4:17 AM

You need guards around the header file; otherwise the test run during make buildworld will fail as they cannot find rij*.h but this head file also isn't one to be installed for user space so guards will just be fine for now. Unless @imp has ideas how to exclude these kinds of header files from the checks?

sys/net80211/ieee80211_crypto_gcm.h
32	#ifdef _KERNEL possibly #include <sys/types.h>
54	#endif\t/* _KERNEL */

In D49161#1124769, @bz wrote:

You need guards around the header file; otherwise the test run during make buildworld will fail as they cannot find rij*.h but this head file also isn't one to be installed for user space so guards will just be fine for now. Unless @imp has ideas how to exclude these kinds of header files from the checks?

hi! how do I do a test run? I'm sure I've done a full buildworld and it hasn't crapped out?

-adrian

In D49161#1124773, @adrian wrote:

In D49161#1124769, @bz wrote:

You need guards around the header file; otherwise the test run during make buildworld will fail as they cannot find rij*.h but this head file also isn't one to be installed for user space so guards will just be fine for now. Unless @imp has ideas how to exclude these kinds of header files from the checks?

hi! how do I do a test run? I'm sure I've done a full buildworld and it hasn't crapped out?

I do not know what is running tools/build/test-includes/ but that barfed for me.

In D49161#1124780, @bz wrote:

In D49161#1124773, @adrian wrote:

In D49161#1124769, @bz wrote:

You need guards around the header file; otherwise the test run during make buildworld will fail as they cannot find rij*.h but this head file also isn't one to be installed for user space so guards will just be fine for now. Unless @imp has ideas how to exclude these kinds of header files from the checks?

hi! how do I do a test run? I'm sure I've done a full buildworld and it hasn't crapped out?

I do not know what is running tools/build/test-includes/ but that barfed for me.

OH this is something new after I refactored out that code. one sec, lemme figure it out.

fix buildworld, from bz

Harbormaster completed remote builds in B62877: Diff 152162.Mar 12 2025, 12:28 AM

In D49161#1124782, @adrian wrote:

In D49161#1124780, @bz wrote:

In D49161#1124773, @adrian wrote:

In D49161#1124769, @bz wrote:

You need guards around the header file; otherwise the test run during make buildworld will fail as they cannot find rij*.h but this head file also isn't one to be installed for user space so guards will just be fine for now. Unless @imp has ideas how to exclude these kinds of header files from the checks?

hi! how do I do a test run? I'm sure I've done a full buildworld and it hasn't crapped out?

I do not know what is running tools/build/test-includes/ but that barfed for me.

OH this is something new after I refactored out that code. one sec, lemme figure it out.

Ok, you're right! I've added a guard.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 13 2025, 12:14 AM

Closed by commit rG2d4583c462a5: net80211: add initial AES-GCMP crypto support (authored by adrian). · Explain Why

This revision was automatically updated to reflect the committed changes.

adrian added a commit: rG2d4583c462a5: net80211: add initial AES-GCMP crypto support.

net80211: add initial AES-GCMP crypto support
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 151832

sys/conf/files

sys/modules/Makefile

sys/modules/wlan_gcmp/Makefile

sys/net80211/ieee80211_crypto_gcm.h

sys/net80211/ieee80211_crypto_gcm.c

sys/net80211/ieee80211_crypto_gcmp.c

sys/net80211/ieee80211_ioctl.h

net80211: add initial AES-GCMP crypto supportClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 151832

sys/conf/files

sys/modules/Makefile

sys/modules/wlan_gcmp/Makefile

sys/net80211/ieee80211_crypto_gcm.h

sys/net80211/ieee80211_crypto_gcm.c

sys/net80211/ieee80211_crypto_gcmp.c

sys/net80211/ieee80211_ioctl.h

net80211: add initial AES-GCMP crypto support
ClosedPublic
Actions

Revision Contents
Changeset List