net80211: bump crypto keysize to 384 bits

[adrian]

Summary

This isn't to land; it's purely for testing 256 bit CCMP/GCMP support.

[adrian]

I've just put this here so my git stack doesn't get out of sync with the review stack.

I'm not intending to land this, it's just here for completeness!

[bz]

sys/net80211/ieee80211_crypto.h

34

We really need to bump this to much further. Just because. I mean 512 bits is kind-of the lowest limit to go for.

[adrian]

sys/net80211/ieee80211_crypto.h

34

I agree.

I think it's only 384 bits in 802.11-2020 but honestly if we're gonna bump it, it should be bigger and/or handle varying key sizes already.

[adrian]

Hm, something IS odd with this diff, I tried associating w/ my test laptop to my TKIP AP here, and it's failing:

wlan0: tkip_setkey: Invalid key length 32, expecting 16

The whole way that TKIP keys and length is calculated is odd; so it's worth digging into it.

[adrian]

ok, i bumped into this when doing this last time, but I didn't add the extra code here to handle it.

• wpa_supplicant sets the keylen to 32 bytes for a TKIP key - 16 byte key, 2 * 8 byte MMIC
• tkip_setkey will error if it's passed in a key w/ keylen 32 bytes
• ieee80211_ioctl_setkey() has some logic to clamp the key length in wk->wk_keylen to IEEE80211_KEYBUF_SIZE, which without this diff would nicely align up as a 16 byte key, then 8 byte tx MMIC and 8 byte rx MMIC
• but it STILL COPIES the whole key buffer via ik.ik_keylen to wk->wk_key, which will include both the key and tx/rx MMIC!
• The TKIP MMIC, which needs to be at the /end/ of the buffer, due to the wk_txmic and wk_rxmic macros.

So, to /correctly/ support the silly TKIP key assembly in the current ioctl/code format, a couple things have to happen.

• First, the tkip key passed in by the ioctl API will be 16 byte key, 2 * 8 byte MIC, no padding
• for TKIP keys it needs to relocate the 16 bytes of MMIC from immediately after the key to the end of the buffer, so wk_txmic / wk_rxmic still function.

Ideally the whole txmic/rxmic setup would be separate fields in the ioctl and key struct, rather than just doing magic stuff to buffers, but that's a much larger change that I don't want to have to do just to support larger keys.

[adrian]

This definitely needs changes, including I think cleaning up how we define the ioctl struct and key struct to have separate key and mic sections, before this is toyed with a bit more.

[bz]

Sorry I don't know; this window came up in a restored browser session; I thought I had submitted this ages ago.

sys/net80211/ieee80211_crypto.h

99

I think your solution is too complicated but I need to go and see if wpa_supplicant does the same silly logic or not. You just need to fix the macros here ideally.

wpa_supplicant still should only set keylen to 16 for TKIP immediately followed by the 2*8 for the MIC.
Unless this bit in ieee80211_ioctl.c silently fixes this?

if (wk->wk_keylen > IEEE80211_KEYBUF_SIZE)
        wk->wk_keylen = IEEE80211_KEYBUF_SIZE;

I guess it does by the comment before and the copyout code for the other direction, but that sounds ill-coded and this will definitively start failing; that should really be an error.

The 2*8 so to say should fly blind.
I think we could even transparently fix this in driver_bsd and the net80211 ioctl handler without breaking any combination.

Was a lot of confusion to figure this out as in LinuxKPI the two offsets had been misplaced (who knows when and why) into a TDLS enum so so were off. Finding that was fun. But in the end the format was the same as on FreeBSD (also TX before RX if I remember correctly from a few days ago) but I still coded it in a way that things could change.

I guess what it really means: auditing all IEEE80211_KEYBUF_SIZE uses.
Doesn't seem too hard for the intr-bits.
rum(4) needs to used the macros above and then it's the ioctl. That seems all. Sounds fair. Why do we not add to the ioctl chaos btw and simply duplicate the ioctl with minimal changes?

Also we should probably fix wpi, rum, rsu, ral, amlo, iwi and ipw (and ifconfig) to stop using IEEE80211_KEYBUF_SIZE as storage for whatever.
Gosh I hate how often we "imported" rewritten Linux (driver) code from OtherBSD. All the same yet all slightly different.

wpi will also fail. At least for wpi_cmd_data
rum will totally fail (KEY_SIZE)
rsu goes into the same problem catgegory as wpi as well; embedded into structs to FW
ral has it also embeedded into rx/tx descriptors
malo could possibly be fine not using struct malo_cmd_wepkey?
iwi has it in the TX descr, and iwi_setwepkeys at the end and may be fine as wk_keylen is used and the array is at the end of the struct
ipw has it embedded in the data_hdr and we use sizeof on that; that will fail

I have one or two of these still but I think if wpi, iwi, and ipw and malo got broken so be it; ral, rum and rsu some more people may still be cranky.

Should I go ahead and do anything about any of that?

[adrian]

In D49419#1147670, @bz wrote:

Sorry I don't know; this window came up in a restored browser session; I thought I had submitted this ages ago.

I think eventually, yes, we do need to go and clean up these uses of IEEE80211_KEY_SIZE.

Lemme get some more refactoring / clean-up done in this stack first, and then start peeling off some tasks to remove the driver dependencies on IEEE80211_KEY_SIZE like you've covered? It's going to take some time but I think it's well worth doing, and we may be able to rope others into helping!