[PATCH] ath: Fix hardware decryption of WEP

[PATCH] ath: Fix hardware decryption of WEP

[Bill Jordan]

The first 4 hardware key indexes are reserved for WEP, but
never programmed. The result was that WEP always used
software decryption.

Signed-off-by: Bill Jordan <bjordan@rajant.com>
---
 drivers/net/wireless/ath/key.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath/key.c b/drivers/net/wireless/ath/key.c
index 17b0efd..73177f1 100644
--- a/drivers/net/wireless/ath/key.c
+++ b/drivers/net/wireless/ath/key.c
@@ -501,7 +501,11 @@ int ath_key_config(struct ath_common *common,
 	if (key->keylen)
 		memcpy(hk.kv_val, key->key, key->keylen);
 
-	if (!(key->flags & IEEE80211_KEY_FLAG_PAIRWISE)) {
+	if (hk.kv_type == ATH_CIPHER_WEP) {
+		if (key->keyidx >= IEEE80211_WEP_NKID)
+			return -EOPNOTSUPP;
+		idx = key->keyidx;
+	} else if (!(key->flags & IEEE80211_KEY_FLAG_PAIRWISE)) {
 		switch (vif->type) {
 		case NL80211_IFTYPE_AP:
 			memcpy(gmac, vif->addr, ETH_ALEN);
@@ -577,8 +581,6 @@ EXPORT_SYMBOL(ath_key_config);
 void ath_key_delete(struct ath_common *common, struct ieee80211_key_conf *key)
 {
 	ath_hw_keyreset(common, key->hw_key_idx);
-	if (key->hw_key_idx < IEEE80211_WEP_NKID)
-		return;
 
 	clear_bit(key->hw_key_idx, common->keymap);
 	if (key->cipher != WLAN_CIPHER_SUITE_TKIP)
-- 
1.7.4.4

Re: [PATCH] ath: Fix hardware decryption of WEP

[Felix Fietkau]

On 2011-08-08 1:19 PM, Bill Jordan wrote:
> The first 4 hardware key indexes are reserved for WEP, but
> never programmed. The result was that WEP always used
> software decryption.
>
> Signed-off-by: Bill Jordan<bjordan@rajant.com>
> ---
>   drivers/net/wireless/ath/key.c |    8 +++++---
>   1 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/key.c b/drivers/net/wireless/ath/key.c
> index 17b0efd..73177f1 100644
> --- a/drivers/net/wireless/ath/key.c
> +++ b/drivers/net/wireless/ath/key.c
> @@ -501,7 +501,11 @@ int ath_key_config(struct ath_common *common,
>   	if (key->keylen)
>   		memcpy(hk.kv_val, key->key, key->keylen);
>
> -	if (!(key->flags&  IEEE80211_KEY_FLAG_PAIRWISE)) {
> +	if (hk.kv_type == ATH_CIPHER_WEP) {
> +		if (key->keyidx>= IEEE80211_WEP_NKID)
> +			return -EOPNOTSUPP;
> +		idx = key->keyidx;
> +	} else if (!(key->flags&  IEEE80211_KEY_FLAG_PAIRWISE)) {
I'm not sure this is a good idea, this would break adding pairwise WEP 
keys. Also, maybe WEP keys could use the BSSID in the lookup to avoid 
the first 4 keycache slots, that way it'd work properly in multi-BSSID 
AP setups.

- Felix

Re: [ath9k-devel] [PATCH] ath: Fix hardware decryption of WEP

[Jouni Malinen]

On Mon, Aug 08, 2011 at 08:04:50PM -0600, Felix Fietkau wrote:
> On 2011-08-08 1:19 PM, Bill Jordan wrote:
> > The first 4 hardware key indexes are reserved for WEP, but
> > never programmed. The result was that WEP always used
> > software decryption.

Not really.. They have a special role receiving group addressed frames
and may be used with static WEP configuration, but there is no strict
reservation of them being for WEP or other key indexes not being
available for WEP.

> > diff --git a/drivers/net/wireless/ath/key.c b/drivers/net/wireless/ath/key.c
> > @@ -501,7 +501,11 @@ int ath_key_config(struct ath_common *common,
> >   	if (key->keylen)
> >   		memcpy(hk.kv_val, key->key, key->keylen);
> >
> > -	if (!(key->flags&  IEEE80211_KEY_FLAG_PAIRWISE)) {
> > +	if (hk.kv_type == ATH_CIPHER_WEP) {
> > +		if (key->keyidx>= IEEE80211_WEP_NKID)
> > +			return -EOPNOTSUPP;
> > +		idx = key->keyidx;
> > +	} else if (!(key->flags&  IEEE80211_KEY_FLAG_PAIRWISE)) {
> I'm not sure this is a good idea, this would break adding pairwise WEP 
> keys. Also, maybe WEP keys could use the BSSID in the lookup to avoid 
> the first 4 keycache slots, that way it'd work properly in multi-BSSID 
> AP setups.

Agreed, this should not be applied.

As far as AP mode is concerned, there is no need to actually receive
group addressed frames, so other key slots can be used regardless of the
key index. Multi-BSSID AP setup with multiple BSSes that use static WEP
keys is not going to work well, but then again, no one really should be
using WEP anymore and I have no problems forcing that special use case
to use software encryption/decryption. While dynamic WEP key setup with
IEEE 802.1X is not really that much better, doing changes to improve
some static WEP cases at the cost of reduced dynamic WEP functionality
does not sound like a good direction.

If the goal here was to improve station mode functionality in the case
where a single WEP network is being used, it should be done more
carefully to avoid causing problems to other cases.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [ath9k-devel] [PATCH] ath: Fix hardware decryption of WEP

[Bill Jordan]

On Tue, Aug 9, 2011 at 4:04 AM, Jouni Malinen <jouni@qca.qualcomm.com> wrote:
> On Mon, Aug 08, 2011 at 08:04:50PM -0600, Felix Fietkau wrote:
>> On 2011-08-08 1:19 PM, Bill Jordan wrote:
>> > The first 4 hardware key indexes are reserved for WEP, but
>> > never programmed. The result was that WEP always used
>> > software decryption.
>
> Not really.. They have a special role receiving group addressed frames
> and may be used with static WEP configuration, but there is no strict
> reservation of them being for WEP or other key indexes not being
> available for WEP.
>
>> > diff --git a/drivers/net/wireless/ath/key.c b/drivers/net/wireless/ath/key.c
>> > @@ -501,7 +501,11 @@ int ath_key_config(struct ath_common *common,
>> >     if (key->keylen)
>> >             memcpy(hk.kv_val, key->key, key->keylen);
>> >
>> > -   if (!(key->flags&  IEEE80211_KEY_FLAG_PAIRWISE)) {
>> > +   if (hk.kv_type == ATH_CIPHER_WEP) {
>> > +           if (key->keyidx>= IEEE80211_WEP_NKID)
>> > +                   return -EOPNOTSUPP;
>> > +           idx = key->keyidx;
>> > +   } else if (!(key->flags&  IEEE80211_KEY_FLAG_PAIRWISE)) {
>> I'm not sure this is a good idea, this would break adding pairwise WEP
>> keys. Also, maybe WEP keys could use the BSSID in the lookup to avoid
>> the first 4 keycache slots, that way it'd work properly in multi-BSSID
>> AP setups.
>
> Agreed, this should not be applied.
>
> As far as AP mode is concerned, there is no need to actually receive
> group addressed frames, so other key slots can be used regardless of the
> key index. Multi-BSSID AP setup with multiple BSSes that use static WEP
> keys is not going to work well, but then again, no one really should be
> using WEP anymore and I have no problems forcing that special use case
> to use software encryption/decryption. While dynamic WEP key setup with
> IEEE 802.1X is not really that much better, doing changes to improve
> some static WEP cases at the cost of reduced dynamic WEP functionality
> does not sound like a good direction.
>
> If the goal here was to improve station mode functionality in the case
> where a single WEP network is being used, it should be done more
> carefully to avoid causing problems to other cases.

I was attempting to fix WEP AP mode.  I was surprised to find that received
data packets were decrypted in software.

I thought WEP should be programming the first 4 hardware keys based
on code in ath9k_rx_skb_postprocess:
	} else if (ieee80211_has_protected(fc)
		   && !decrypt_error && skb->len >= hdrlen + 4) {
		keyix = skb->data[hdrlen + 3] >> 6;

		if (test_bit(keyix, common->keymap))
			rxs->flag |= RX_FLAG_DECRYPTED;
	}

This code only works if the first 4 hardware keys are used.

In order to support dynamic WEP keys, all WEP decryption must be done
in software? Am I understanding that correctly?

Bill Jordan

Re: [ath9k-devel] [PATCH] ath: Fix hardware decryption of WEP

[Jouni Malinen]

On Tue, Aug 09, 2011 at 11:58:33AM -0400, Bill Jordan wrote:
> I was attempting to fix WEP AP mode.  I was surprised to find that received
> data packets were decrypted in software.

Ah, I see. This is bit problematic when keeping in mind that there could
be multiple BSSes in use with different set of WEP configuration. If you
put one of them in the key indexes 0..3, you may not be able to use the
other BSSes correctly (in theory, it could be possible with some hacks
when only a single WEP key is configured per BSS, but I don't think it
is worth the effort to do that for WEP). As such, there would need to be
code that is capable of dynamically kicking out the key cache entries
0..3 should another BSS be set up. I don't think mac80211 has support
for this at the moment, so that may require considerable amount of extra
work. And again, it is questionable whether there is really any good
justification for doing this with WEP that is not even allowed to be
used with HT and as such, is limited in throughput anyway.

> I thought WEP should be programming the first 4 hardware keys based
> on code in ath9k_rx_skb_postprocess:
> 	} else if (ieee80211_has_protected(fc)
> 		   && !decrypt_error && skb->len >= hdrlen + 4) {
> 		keyix = skb->data[hdrlen + 3] >> 6;
> 
> 		if (test_bit(keyix, common->keymap))
> 			rxs->flag |= RX_FLAG_DECRYPTED;
> 	}
> 
> This code only works if the first 4 hardware keys are used.

There are some semi-odd hacks needed for figuring out whether the frame
was really decrypted in some cases (i.e., whether the flag in RX
descriptor is valid). This is one of those corner cases..

> In order to support dynamic WEP keys, all WEP decryption must be done
> in software? Am I understanding that correctly?

Not with dynamic WEP keys. With those, the AP does not really receive
any broadcast/multicast frames and only the per-station individual key
is used for RX at the AP. As such, hardware can pick the correct key
based on the transmitter address.

The most problematic case with key cache and support for multiple BSSes
is use of static WEP where the different BSSes have conflicting needs
for finding the key on RX processing if the different BSSes share the
same key index (and especially if they use multiple key indexes). While
it would be theoretically possible to improve this, it is way too easy
to break some other corner cases unless you are very careful with the
key cache use in the driver. As such, I don't really want to spend the
time reviewing this type of changes without a good justification for the
improvement (and anything with WEP in it is unlikely to be considered
good justification in 2011 ;-).

-- 
Jouni Malinen                                            PGP id EFC895FA