Page MenuHomeFreeBSD
Differential D49161

net80211: add initial AES-GCMP crypto support
ClosedPublic
Actions

Authored by adrian on Feb 28 2025, 3:23 AM.
Tags
Referenced Files
F167122411: D49161.id151753.diff
Wed, Aug 19, 7:08 AM
Unknown Object (File)
Sun, Aug 16, 11:34 AM
Unknown Object (File)
Sun, Aug 16, 10:31 AM
Unknown Object (File)
Sat, Aug 15, 10:38 PM
Unknown Object (File)
Sat, Aug 15, 5:12 AM
Unknown Object (File)
Sat, Aug 15, 1:33 AM
Unknown Object (File)
Sat, Aug 15, 1:29 AM
Unknown Object (File)
Fri, Aug 14, 11:00 PM
View All Files
Subscribers
bz
glebius
imp
melifaro

Details

Reviewers
bz
Group Reviewers
wireless
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 62811
Build 59695: 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.
adrian updated this revision to Diff 151719.Mar 1 2025, 5:26 PM

some code cleanups

Harbormaster completed remote builds in B62713: Diff 151719.Mar 1 2025, 5:26 PM
bz requested changes to this revision.Mar 1 2025, 10:51 PM
bz added a subscriber: bz.

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
adrian added a comment.Mar 1 2025, 10:54 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.

adrian updated this revision to Diff 151753.Mar 2 2025, 5:26 PM

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

adrian updated this revision to Diff 151832.Mar 4 2025, 5:44 AM

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
adrian added a comment.Mar 4 2025, 5:46 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.

adrian added a comment.Mar 7 2025, 2:27 AM

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.

adrian updated this revision to Diff 151989.Mar 7 2025, 2:37 AM

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
bz added a comment.Mar 7 2025, 12:05 PM
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.

adrian updated this revision to Diff 152049.Mar 8 2025, 5:52 PM

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
imp added a comment.Mar 8 2025, 6:08 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

bz added a comment.Mar 9 2025, 1:07 AM

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
adrian updated this revision to Diff 152072.Mar 10 2025, 4:14 AM

address feedback from bz

Harbormaster completed remote builds in B62845: Diff 152072.Mar 10 2025, 4:14 AM
adrian marked 15 inline comments as done.Mar 10 2025, 4:17 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
bz added a comment.Mar 12 2025, 12:00 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 */

adrian added a comment.Mar 12 2025, 12:07 AM
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

bz added a comment.Mar 12 2025, 12:16 AM
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.

adrian added a comment.Mar 12 2025, 12:23 AM
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.

adrian updated this revision to Diff 152162.Mar 12 2025, 12:28 AM

fix buildworld, from bz

Harbormaster completed remote builds in B62877: Diff 152162.Mar 12 2025, 12:28 AM
adrian added a comment.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.
adrian mentioned this in D45397: net80211: Add crypto module support for AES-GCMP 128 and 256 bit..Tue, Jul 21, 8:01 PM

Revision Contents
Changeset List

PathSize
sys/
conf/
2 lines
modules/
1 line
wlan_gcmp/
10 lines
net80211/
54 lines
368 lines
691 lines
7 lines

Diff 151989

View Options

sys/conf/files

Loading...
View Options

sys/modules/Makefile

Loading...
View Options

sys/modules/wlan_gcmp/Makefile

Loading...
View Options

sys/net80211/ieee80211_crypto_gcm.h

Loading...
View Options

sys/net80211/ieee80211_crypto_gcm.c

Loading...
View Options

sys/net80211/ieee80211_crypto_gcmp.c

Loading...
View Options

sys/net80211/ieee80211_ioctl.h

Loading...