net80211: refactor out the AAD init code shared between GCMP and CCMP
Needs ReviewPublic
Actions

Authored by adrian on Fri, Mar 14, 11:32 PM.

Details

Reviewers

Group Reviewers

Summary

The 802.11 specification specifically calls out that the GCMP AES
uses the CCMP AES specification, so we can re-use this here.

Changes during refactoring:

* the first block (b0) needs the priority field populated, which was
  being done inline with the CCMP AAD assembly.  So write a new function
  that implements just that.

* Use IEEE80211_IS_QOS_ANY() in the AAD assembly; 802.11-2020
  12.5.3.3.3 (Construct AAD) talks about frames with QoS control field,
  not specifically QoS data frames (ie, not avoiding PS-POLL.)

* mask the HTC/ORDER bit in the FC if the data frame contains a
  QoS control field.  Again, see 802.11-2020 12.5.3.3.3.

* Add extra comments about missing items and functionality for later
  implementation.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 62966
Build 59850: arc lint + arc unit

Event Timeline

adrian created this revision.Fri, Mar 14, 11:32 PM

Herald added subscribers: glebius, melifaro, imp. · View Herald TranscriptFri, Mar 14, 11:32 PM

adrian requested review of this revision.Fri, Mar 14, 11:32 PM

Harbormaster completed remote builds in B62928: Diff 152286.Fri, Mar 14, 11:32 PM

(I haven't yet tested this; I'll do that shortly. I just wanted the diff somewhere!)

ok, now I've tested with:

rtwn, HW encryption, CCMP-128
run, HW encryption, CCMP-128
uath, SW encryption, CCMP-128 and GCMP-128

.. and yes, now is a great time to sit down and clean up those magic numbers. :-)

add comments about missing functionality, link to RFC / IETF specifications.

Harbormaster completed remote builds in B62934: Diff 152323.Sat, Mar 15, 5:43 PM

adrian added inline comments.Sun, Mar 16, 5:22 AM

sys/net80211/ieee80211_crypto.c
955	i /really/ want to verify what the right QoS check is here. in 802.11-2020 12.5.3.3.3 (Construct AAD) it only talks about the presence or not of the QoS field, NOT that it needs to have an 802.11 sequence number (ie a data subtype.) Let's go nail this down before i land this!

bz added a subscriber: bz.Sun, Mar 16, 2:02 PM

bz added inline comments.

sys/net80211/ieee80211_crypto.c
955	9.2.4.1.3 In Data frames, the most significant bit (MSB) of the Subtype subfield, B 7, is defined as the QoS subfield 9.2.4.5.1 The QoS Control field is present in all Data frames in which the QoS subfield of the Subtype subfield is equal to 1 (see 9.2.4.1.3). -> IEEE80211_IS_QOS_ANY I would say but in practice they both (IEEE80211_IS_QOS_ANY and IEEE80211_QOS_HAS_SEQ) check for the same thing (0x80 is 0x80 is MSB set).

adrian added inline comments.Sun, Mar 16, 5:39 PM

sys/net80211/ieee80211_crypto.c
955	9.2.4.1.3 In Data frames, the most significant bit (MSB) of the Subtype subfield, B 7, is defined as the QoS subfield 9.2.4.5.1 The QoS Control field is present in all Data frames in which the QoS subfield of the Subtype subfield is equal to 1 (see 9.2.4.1.3). -> IEEE80211_IS_QOS_ANY I would say but in practice they both (IEEE80211_IS_QOS_ANY and IEEE80211_QOS_HAS_SEQ) check for the same thing (0x80 is 0x80 is MSB set). Yeah that's why it looked slightly wrong to me. I think it should be IEEE80211_IS_QOS_ANY() because technically speaking, it READS like QOS NULL-DATA frames should still be encrypted. IEEE80211_QOS_HAS_SEQ() is more designed for sequence number assignment / checking, esp when forming A-MPDUs. So, I think I'll convert this code to IEEE80211_IS_QOS_ANY(). Is that OK with you?

adrian added inline comments.Mon, Mar 17, 4:49 AM

sys/net80211/ieee80211_crypto_ccmp.c
436	hm, i need to double check this too!
445	and this one!

bz requested changes to this revision.Tue, Mar 18, 10:52 AM

bz added inline comments.

sys/net80211/ieee80211_crypto.c
921	uint8_t ? well technically uint16_t but ...
952	My suggestion given it's so nice that the optional bits are at the end is to use variables and acquire the values upfront: aad_len = 22 a bool for the 4-address; when setting to true: aad_len += 6 a uint8_t for the tid; if IS_QOS_ANY aad_len += 2; the logic to acquire tid only once; else tid = 0 then set aad_len to aad[1] then it should be a simple small if (!4-address) aad[24] = tid else set IEEE80211_ADDR_COPY(&aad[24], A4) and aad[30] = tid Should make the code simpler and more readable, reduce duplication, and follow the documented plot some better. Given we zero everything upfront both else paths inside the if/else below will fall away as well if I am not mistaken (well the tid = 0 above is a one line still there) and the = 0 assignments can go. This seems more feasible now that the b0 handling for the nonce is factored out. Just an idea.
955	I haven't checked if an older spec is more clear about the wording. But the way I currently read it (if not missing another bit), I am fine with that.
982	Following-up on my above suggestion: If you then return the tid you can save yourself the ieee80211_crypto_ccmp_init_nonce_flags() call and assign the return value in ccmp_init_blocks() to b0[1]
sys/net80211/ieee80211_crypto_ccmp.c
445	Sorry but too much code duplication ;-) You do realize that bth code paths here are the same? If my above suggestion works you can save it all ;-)
505	b0[1] = ... if you implement my suggestion
sys/net80211/ieee80211_crypto_gcmp.c
548	this would be void and aad_len = be16toh((const uint16_t )aad); if I am not mistaken (and is the only thing I do not fully like but passing &aad_len iitlaized to GCM_AAD_LEN to ieee80211_crypto_init_aad() instead and returning it updated is also not much better.
567	and aad_len here would be be16toh((const uint16_t )aad) if I am not mistaken.

This revision now requires changes to proceed.Tue, Mar 18, 10:52 AM

updates from bz

Harbormaster completed remote builds in B62966: Diff 152403.Tue, Mar 18, 11:40 PM

adrian edited the summary of this revision. (Show Details)Tue, Mar 18, 11:41 PM

adrian marked 2 inline comments as done.

Thinking about it, i'd rather do the big cleanup in a separate diff versus the refactoring diff I'm doing here.

I do agree with your suggestions in principle, I just think combining both a refactor / re-organise and the code clean-up work in one big hit would make it hard to debug if something subtle IS busted.

What do you think? i'm happy to do it once the rest of the stack lands.