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

[adrian]

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.

[adrian]

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

[adrian]

ok, now I've tested with:

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

[adrian]

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

[adrian]

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

[adrian]

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]

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]

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]

sys/net80211/ieee80211_crypto_ccmp.c

438

hm, i need to double check this too!

447

and this one!

[bz]

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

447

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

486

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.

[adrian]

updates from bz