net80211: add initial AES-GCMP crypto support

[adrian]

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 .

[adrian]

some code cleanups

[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

393

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.

[adrian]

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]

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

[adrian]

sys/net80211/ieee80211_crypto_gcmp.c

393

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]

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]

sys/net80211/ieee80211_crypto_gcmp.c

244

stupid nit: no space after !

395

stupid nit" use 'for (int i = 0;... )' here

[bz]

sys/net80211/ieee80211_crypto_gcmp.c

393

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]

split out the hostapd code from the gcmp 802.11 code

[adrian]

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]

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

393

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]

feedback from bz/imp

[bz]

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]

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

[imp]

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]

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]

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

8 ↗

(On Diff #152049)

extra blank line? Is that from upstream?

31 ↗

(On Diff #152049)

Do you still need this?

33 ↗

(On Diff #152049)

Doubt this file needs this one.

40 ↗

(On Diff #152049)

Also not sure about that one? If rijndael needs it it should take care of it itself?

sys/net80211/ieee80211_crypto_gcm.h

45 ↗

(On Diff #152049)

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]

address feedback from bz

[adrian]

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

45 ↗

(On Diff #152049)

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

[bz]

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

31 ↗

(On Diff #152049)

#ifdef _KERNEL

possibly
#include <sys/types.h>

53 ↗

(On Diff #152049)

#endif\t/* _KERNEL */

[adrian]

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]

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]

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]

fix buildworld, from bz

[adrian]

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.