net80211: add driver / crypto methods to set the hardware / software cipher suites

[adrian]

Summary

Drivers currently announce hardware crypto cipher support by
setting up ic_cryptocaps.

This adds two public function calls:

• ieee80211_set_software_cipher_set() - set the software cipher set;
• ieee80211_set_hardware_cipher_set() - set the hardware cipher set.

For now these just call into the newly crypto routines to set the ciphers.

This then adds the two crypto routines, similarly named, to set
the hardware/software cipher suite.

This is a no-op right now - wep/tkip/ccmp are already set by default
so drivers aren't required to call these routines for software
encryption, and drivers already set ic_cryptocaps for hardware
encryption.

[adrian]

add review

[adrian]

fix commit message

[adrian]

• rename software crypto field to better represent what its doing

[bz]

Can we shorten hardware/software to hw/sw?

Why do we need wrappers from general code into crypto? Isn't one of them enough to set (and probably also clear[1]) a field in the (currently) ic?

[1] the current API won't really help you for all use cases as at least ath(4) has one "clear bit" case as well. Possibly using a KPI like ifnet does (ptr, set, clear) would help. I am also wondering if we generally want to have the KPI in a way to |= for set an &~ for clear as multiple callers are doing it bit-by-bit.

I think if we were to adjust the few callers then we could hide the field name entirely and solve that naming problem right away as well with adding the 2nd struct field.

sys/net80211/ieee80211_crypto.c

174

I would ask this to be a |= so current code can continue better but see general comment as to whether you want to pass it differently and also have a clear bit.

184

Same.

[bz]

sys/net80211/ieee80211.c

438

s/setup/device attach/ maybe to make it more clear from where we call this normally? There's also VAP setup and all kinds of other bits. (also below)

[adrian]

In D44827#1023017, @bz wrote:

Can we shorten hardware/software to hw/sw?

Why do we need wrappers from general code into crypto? Isn't one of them enough to set (and probably also clear[1]) a field in the (currently) ic?

Just habit. The routines in ieee80211.c are normally the public facing ones. The crypto layer ones know about the crypto stuff in more detail.

[1] the current API won't really help you for all use cases as at least ath(4) has one "clear bit" case as well. Possibly using a KPI like ifnet does (ptr, set, clear) would help. I am also wondering if we generally want to have the KPI in a way to |= for set an &~ for clear as multiple callers are doing it bit-by-bit.

My intent here is that drivers call it to set them explicitly, not mask them. So eg, ath would simply just call it on the set of bits they care about for both hardware and software crypto.
The ath10k driver would call them based on whether software encryption was enabled or disabled at module load time.
Hardware that say, didn't support TKIP at all (hardware or software) would call the registration without those bits set.

I think if we were to adjust the few callers then we could hide the field name entirely and solve that naming problem right away as well with adding the 2nd struct field.

[adrian]

eg ath(4) would do something like:

        uint32_t hw_cryptocaps;
	if (ath_hal_ciphersupported(ah, HAL_CIPHER_WEP))
		hw_cryptocaps |= IEEE80211_CRYPTO_WEP;
	if (ath_hal_ciphersupported(ah, HAL_CIPHER_AES_OCB))
		hw_cryptocaps |= IEEE80211_CRYPTO_AES_OCB;
	if (ath_hal_ciphersupported(ah, HAL_CIPHER_AES_CCM))
		hw_cryptocaps |= IEEE80211_CRYPTO_AES_CCM;
	if (ath_hal_ciphersupported(ah, HAL_CIPHER_CKIP))
		hw_cryptocaps |= IEEE80211_CRYPTO_CKIP;
	if (ath_hal_ciphersupported(ah, HAL_CIPHER_TKIP)) {
		hw_cryptocaps |= IEEE80211_CRYPTO_TKIP;	
/*
		 * Check if h/w does the MIC and/or whether the
		 * separate key cache entries are required to
		 * handle both tx+rx MIC keys.
		 */
		if (ath_hal_ciphersupported(ah, HAL_CIPHER_MIC))
			hw_cryptocaps |= IEEE80211_CRYPTO_TKIPMIC;
}

ieee80211_set_hardware_cipher_set(hw_cryptocaps);
/* All if_ath(4) HAL providers allow for pass-through unencrypted frames, so
  * software encryption is possible.
 */
ieee80211_set_software_cipher_set(
IEEE80211_CRYPTO_WEP |
 IEEE80211_CRYPTO_AES_OCB |
 IEEE80211_CRYPTO_AES_CCM |
 IEEE80211_CRYPTO_CKIP |
 IEEE80211_CRYPTO_TKIP);

Yes it means that if we add new cipher suites then we will have to add them to each of the drivers, but then by doing this we actually ensure we don't break things by enabling support for drivers when new ciphers (and later new key exchange algorithms) are added without verifying and manually enabling it on each driver.

[bz]

In D44827#1023208, @adrian wrote:

eg ath(4) would do something like:

How do you want to implement ath_settkipmic() "nicely" without exposing the field?

[adrian]

In D44827#1023209, @bz wrote:

In D44827#1023208, @adrian wrote:

eg ath(4) would do something like:

How do you want to implement ath_settkipmic() "nicely" without exposing the field?

oops my bad. it'll be a flag for hardware encryption support! I fixed the code snippet.

[adrian]

comments from bz

[emaste]

I find it a little confusing with the verb & noun use of set in set_software_cipher_set - do you think set_software_ciphers (or set_sw_ciphers) is reasonable?

[adrian]

In D44827#1023673, @emaste wrote:

I find it a little confusing with the verb & noun use of set in set_software_cipher_set - do you think set_software_ciphers (or set_sw_ciphers) is reasonable?

I think renaming it to set_software_ciphers() / set_hardware_ciphers() is fine!

[adrian]

comment from emaste

[cc]

In D44827#1023217, @adrian wrote:

In D44827#1023209, @bz wrote:

In D44827#1023208, @adrian wrote:

eg ath(4) would do something like:

How do you want to implement ath_settkipmic() "nicely" without exposing the field?

oops my bad. it'll be a flag for hardware encryption support! I fixed the code snippet.

I don't get it. Without exposing the field ic_cryptocaps? And where is the "fixed the code snippet"?

[bz]

Are you going to change the few drivers which need change to get rid of the public exposure as well? Otherwise this is a dead code before added.

[adrian]

In D44827#1024561, @bz wrote:

Are you going to change the few drivers which need change to get rid of the public exposure as well? Otherwise this is a dead code before added.

Yup a diff later in the stack (D44936) uses it.

[adrian]

In D44827#1024357, @cc wrote:

In D44827#1023217, @adrian wrote:

In D44827#1023209, @bz wrote:

In D44827#1023208, @adrian wrote:

eg ath(4) would do something like:

How do you want to implement ath_settkipmic() "nicely" without exposing the field?

oops my bad. it'll be a flag for hardware encryption support! I fixed the code snippet.

I don't get it. Without exposing the field ic_cryptocaps? And where is the "fixed the code snippet"?

oh THAT. yeah, that's dirty and I hate it.

I'll think about it a bit. I may end up just removing doing HW TKIP support for NICs that can't do it with WME because honestly it's 2024 and those NICs, if still being used, would likely be used on something with enough CPU for it not to matter. (And I'd honestly be surprised if someone really complains about the performance change.)

[adrian]

sys/net80211/ieee80211_crypto.c

174

It needs to be a set, rather than an or, for platforms that do NOT support ciphers for software encryption.

[cc]

Just some minor issues.

sys/net80211/ieee80211.c

441–447

The following alignment looks better and it does not break the 80 characters limit:

void
ieee80211_set_software_ciphers(struct ieee80211com *ic, uint32_t cipher_suite)
{
	ieee80211_crypto_set_supported_software_ciphers(ic, cipher_suite);
}

454–459

Same alignment issue as above.

sys/net80211/ieee80211_crypto.c

178

typo: supporter => supported

[adrian]

address comments by cc@

[bz]

This still has changes not marked "done".
Is there a change in the stack which does change the drivers to use this?

[adrian]

In D44827#1027553, @bz wrote:

This still has changes not marked "done".
Is there a change in the stack which does change the drivers to use this?

There's no native drivers in our tree right now that require the software cipher call, but in the linux side ath10k will likely require it.
D44936 replaces most of the ic_cryptocaps references to use the hardware cipher call.