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

971

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

971

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

971

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

436

hm, i need to double check this too!

445

and this one!

[bz]

sys/net80211/ieee80211_crypto.c

937

uint8_t ? well technically uint16_t but ...

968

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.

971

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.

998

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

533

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

[adrian]

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.

[adrian]

more feedback from bz

[adrian]

sys/net80211/ieee80211_crypto.c

968

Yeah, i do like the logic cleanup idea, I'll do it after the refactor.

Honestly, even getting the tid from the header should just be using ieee80211_gettid(), with a check for tid==16 as "there's no QoS / TID here".

[adrian]

update from rebase

[bz]

I have not re-checked the logic bits.
If it wasn't for the QOS_ANY I would have ignored the .) -> ). in the comments but given you need to touch it anyway again please do change them too.

If you get to this before Monday don't wait for me and just commit it Approved once the 5 places are fixed (given we'll do the logic adjustments later)

sys/net80211/ieee80211_crypto.c

931

The final dot goes outside the ).

sys/net80211/ieee80211_crypto_ccmp.c

414

the final . goes outside the )

429

Here the . too.

436

Did you? As it's not IEEE80211_IS_QOS_ANY()

445

and this one too.

[adrian]

review from bz, also go for a bit of a deep dive into the nonce definitions

[adrian]

Note: approved by bz after the 5 last comments dealt with; so I'll land this shortly.