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