net80211: crypto: fix check for driver having done decryption

[bz]

Summary

At least since fe75b45213a40 (though I am not sure how the original
flag was supposed to work) we can deal with hardware crypto offload
in the RX path (in theory).
Fix the check to see if decryption was done by the driver based on the
per-packet rx_stats information and the IEEE80211_RX_F_DECRYPTED flag.
Now (hw decrpypted) packets no longer dropped with "rx MIC check failed
(CCMP)" errors.

MFC after: 3 days
Tested with: iwlwifi

Test Plan

CCMP with (otherwise fixed first) LinuxKPI and iwlwifi started working
There's likely more work to be done in the area...

[cc]

sys/net80211/ieee80211_crypto_wep.c

256–262

Not sure how the compile could have passed... There should be error about wh not defined. I guess IEEE80211_DEBUG is always defined.

But let's fix this, and put #endif under this IEEE80211_NOTE_MAC() line.

[adrian]

err, lemme double check this first. SWDECRYPT is not related to hardware encryption offload; it's an earlier thing where you could have RX STA keys in the hardware (to accelerate knowing which STA it belongs to) but it still passes it through untouched requiring one to do encrypt/decrypt in software.

For the ath NICs, the keycache served both as a encryption/decryption key cache and also as a way of marking known STAs in hardware for doing things like auto-ACK, block-ACK tracking, hardware antenna diversity in the older NICs, etc. It doesn't /have/ to contain encryption keys - that's why SWDECRYPT is a thing.

Is IEEE80211_KEY_SWDECRYPT being set somewhere in the linux kpi layer?

[adrian]

sys/net80211/ieee80211_crypto_wep.c

256–262

Yeah, it should've been inside the #ifdef, otherwise an "unused variable" happens w/out the ifdef...

IEEE80211_DEBUG being undefined likely makes IEEE80211_NOTE_MAC be a NO-OP.

[bz]

In D43634#1007529, @adrian wrote:

err, lemme double check this first. SWDECRYPT is not related to hardware encryption offload; it's an earlier thing where you could have RX STA keys in the hardware (to accelerate knowing which STA it belongs to) but it still passes it through untouched requiring one to do encrypt/decrypt in software.

For the ath NICs, the keycache served both as a encryption/decryption key cache and also as a way of marking known STAs in hardware for doing things like auto-ACK, block-ACK tracking, hardware antenna diversity in the older NICs, etc. It doesn't /have/ to contain encryption keys - that's why SWDECRYPT is a thing.

Is IEEE80211_KEY_SWDECRYPT being set somewhere in the linux kpi layer?

I don't think so but that means we need an extra bit of logic. The HW decrypted packets come in and hit ccmp_decrypt() which they definitively shouldn't anymore. So pseudo-code:

if (!IEEE80211_RX_F_DECRYPTED && IEEE80211_KEY_SWDECRYPT) error = xxx_decrypt();  Handle error accrodingly.

Something like this?

[adrian]

In D43634#1007535, @bz wrote:

In D43634#1007529, @adrian wrote:

err, lemme double check this first. SWDECRYPT is not related to hardware encryption offload; it's an earlier thing where you could have RX STA keys in the hardware (to accelerate knowing which STA it belongs to) but it still passes it through untouched requiring one to do encrypt/decrypt in software.

For the ath NICs, the keycache served both as a encryption/decryption key cache and also as a way of marking known STAs in hardware for doing things like auto-ACK, block-ACK tracking, hardware antenna diversity in the older NICs, etc. It doesn't /have/ to contain encryption keys - that's why SWDECRYPT is a thing.

Is IEEE80211_KEY_SWDECRYPT being set somewhere in the linux kpi layer?

I don't think so but that means we need an extra bit of logic. The HW decrypted packets come in and hit ccmp_decrypt() which they definitively shouldn't anymore. So pseudo-code:

if (!IEEE80211_RX_F_DECRYPTED && IEEE80211_KEY_SWDECRYPT) error = xxx_decrypt();  Handle error accrodingly.

Something like this?

Wait, the HW crypto offload path should also have a flag set somewhere that says "this was hw decrypted but failed MIC/IV check". I know that's how it works on my ath10k port and I checked WEP, TKIP and CCMP.
That's why I'm confused. :-)

Lemme go get a FreeBSD laptop open and look at this codepath again. Hang tight.

[adrian]

Aha i remember now. Ok, so.

ath10k does HW decrypt AND strips the MIC/IV on successful decrypts. So I wasn't seeing that ccmp_decrypt() path being called.

Checking for IEEE80211_RX_F_DECRYPTED too is likely fine.

Thing is, why's IEEE80211_KEY_SWDECRYPT set if the HW is doing decrypt? You shouldn't have that set if the HW is doing decrypt and you've programmed keys into the HW at this point?

[bz]

In D43634#1008394, @adrian wrote:

Aha i remember now. Ok, so.

ath10k does HW decrypt AND strips the MIC/IV on successful decrypts. So I wasn't seeing that ccmp_decrypt() path being called.

Checking for IEEE80211_RX_F_DECRYPTED too is likely fine.

Thing is, why's IEEE80211_KEY_SWDECRYPT set if the HW is doing decrypt? You shouldn't have that set if the HW is doing decrypt and you've programmed keys into the HW at this point?

Likely because ic_cryptocaps are not set correctly in LinuxKPI and net80211 then sets IEEE80211_KEY_SWCRYPT in ieee80211_crypto_newkey(); see https://reviews.freebsd.org/D44208

[cc]

I am working the "multi-key" support these days on top of D43634(this patch) and D43648, but it turns out this patch is causing more trouble than it is trying to solve.
In fact, it caused this "Abort trap" from the console and then my VM is shutdown silently. The kernel is main with LKPI_80211_HW_CRYPTO on top of this patch.