[RFC] ath10k: Fix shared WEP

[RFC] ath10k: Fix shared WEP

[Sujith Manoharan]

From: Sujith Manoharan <c_manoha@qca.qualcomm.com>

When static keys are used in shared WEP, when a
station is associated, message 3 is sent with an
encrypted payload. But, for subsequent
authentications that are triggered without a
deauth, the auth frame is decrypted by the HW. To
handle this, check if the WEP keys have already
been set for the peer and if so, mark the
frame as decrypted.

Signed-off-by: Sujith Manoharan <c_manoha@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath10k/mac.c | 25 +++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/mac.h |  2 ++
 drivers/net/wireless/ath/ath10k/wmi.c | 29 +++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 1245ac8..e8fee46 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -179,6 +179,31 @@ static int ath10k_clear_peer_keys(struct ath10k_vif *arvif,
 	return first_errno;
 }
 
+bool ath10k_is_peer_wep_key_set(struct ath10k *ar, const u8 *addr,
+				u8 keyidx)
+{
+	struct ath10k_peer *peer;
+	int i;
+
+	/*
+	 * We don't know which vdev this peer belongs to,
+	 * since WMI doesn't give us that information.
+	 */
+	spin_lock_bh(&ar->data_lock);
+	peer = ath10k_peer_find(ar, 0, addr);
+	spin_unlock_bh(&ar->data_lock);
+
+	if (!peer)
+		return false;
+
+	for (i = 0; i < ARRAY_SIZE(peer->keys); i++) {
+		if (peer->keys[i] && peer->keys[i]->keyidx == keyidx)
+			return true;
+	}
+
+	return false;
+}
+
 static int ath10k_clear_vdev_key(struct ath10k_vif *arvif,
 				 struct ieee80211_key_conf *key)
 {
diff --git a/drivers/net/wireless/ath/ath10k/mac.h b/drivers/net/wireless/ath/ath10k/mac.h
index 4e3c989..f5d0a5b 100644
--- a/drivers/net/wireless/ath/ath10k/mac.h
+++ b/drivers/net/wireless/ath/ath10k/mac.h
@@ -41,6 +41,8 @@ void ath10k_mgmt_over_wmi_tx_work(struct work_struct *work);
 void ath10k_halt(struct ath10k *ar);
 void ath10k_mac_vif_beacon_free(struct ath10k_vif *arvif);
 void ath10k_drain_tx(struct ath10k *ar);
+bool ath10k_is_peer_wep_key_set(struct ath10k *ar, const u8 *addr,
+				u8 keyidx);
 
 static inline struct ath10k_vif *ath10k_vif_to_arvif(struct ieee80211_vif *vif)
 {
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index c2bc828..8f1186e 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -1113,6 +1113,33 @@ static inline u8 get_rate_idx(u32 rate, enum ieee80211_band band)
 	return rate_idx;
 }
 
+static void ath10k_check_wep(struct ath10k *ar, struct sk_buff *skb,
+			     struct ieee80211_rx_status *status)
+{
+	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
+	unsigned int hdrlen;
+	bool peer_key;
+	u8 *addr, keyidx;
+
+	if (ieee80211_is_auth(hdr->frame_control) &&
+	    ieee80211_has_protected(hdr->frame_control)) {
+		hdrlen = ieee80211_hdrlen(hdr->frame_control);
+		if (skb->len < (hdrlen + 4))
+			return;
+
+		keyidx = skb->data[hdrlen + 3] >> 6;
+		addr = ieee80211_get_SA(hdr);
+		peer_key = ath10k_is_peer_wep_key_set(ar, addr, keyidx);
+
+		if (peer_key) {
+			ath10k_dbg(ar, ATH10K_DBG_MAC,
+				   "wep key present for peer: %pM\n", addr);
+			status->flag |= RX_FLAG_DECRYPTED;
+		}
+	}
+
+}
+
 static int ath10k_wmi_event_mgmt_rx(struct ath10k *ar, struct sk_buff *skb)
 {
 	struct wmi_mgmt_rx_event_v1 *ev_v1;
@@ -1200,6 +1227,8 @@ static int ath10k_wmi_event_mgmt_rx(struct ath10k *ar, struct sk_buff *skb)
 	hdr = (struct ieee80211_hdr *)skb->data;
 	fc = le16_to_cpu(hdr->frame_control);
 
+	ath10k_check_wep(ar, skb, status);
+
 	/* FW delivers WEP Shared Auth frame with Protected Bit set and
 	 * encrypted payload. However in case of PMF it delivers decrypted
 	 * frames with Protected Bit set. */
-- 
2.1.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC] ath10k: Fix shared WEP

[Michal Kazior]

On 20 November 2014 15:06, Sujith Manoharan <sujith@msujith.org> wrote:
> From: Sujith Manoharan <c_manoha@qca.qualcomm.com>
>
> When static keys are used in shared WEP, when a
> station is associated, message 3 is sent with an
> encrypted payload. But, for subsequent
> authentications that are triggered without a
> deauth, the auth frame is decrypted by the HW. To
> handle this, check if the WEP keys have already
> been set for the peer and if so, mark the
> frame as decrypted.

IOW This fixes a corner case when station reconnects but ath10k AP for
some reason didn't notice it (e.g. station went out of range and came
back before AP inactivity timer kicked in), right? Or is there another
scenario? It might be a good idea to include this in the commit log.


[...]
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index 1245ac8..e8fee46 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -179,6 +179,31 @@ static int ath10k_clear_peer_keys(struct ath10k_vif *arvif,
>         return first_errno;
>  }
>
> +bool ath10k_is_peer_wep_key_set(struct ath10k *ar, const u8 *addr,
> +                               u8 keyidx)

Functions in mac.c should have ath10k_mac_ prefix. I'm aware not all
function follow this rule (albeit they should).


> +{
> +       struct ath10k_peer *peer;
> +       int i;
> +
> +       /*
> +        * We don't know which vdev this peer belongs to,
> +        * since WMI doesn't give us that information.
> +        */
> +       spin_lock_bh(&ar->data_lock);
> +       peer = ath10k_peer_find(ar, 0, addr);
> +       spin_unlock_bh(&ar->data_lock);

I don't think the above will work for mBSS, e.g. consider vdev_id=0
being an open network and vdev_id=1 being wep-shared. Maybe it's about
time to introduce ath10k_peer_find_by_addr(struct ath10k *ar, const u8
*)?


> +
> +       if (!peer)
> +               return false;
> +
> +       for (i = 0; i < ARRAY_SIZE(peer->keys); i++) {
> +               if (peer->keys[i] && peer->keys[i]->keyidx == keyidx)
> +                       return true;
> +       }

Read access to peer->keys should be protected by either data_lock or
conf_mutex. Obviously the latter can't be used because this function
will be called from an atomic context leaving the data_lock. The
entire is_peer_wep_key_set() should require data_lock to be held while
it is called to avoid races.

Oh, and apparently this is buggy anyway because
ath10k_install_peer_wep_keys() is oblivious to this. Oops..


[...]
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> index c2bc828..8f1186e 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -1113,6 +1113,33 @@ static inline u8 get_rate_idx(u32 rate, enum ieee80211_band band)
>         return rate_idx;
>  }
>
> +static void ath10k_check_wep(struct ath10k *ar, struct sk_buff *skb,
> +                            struct ieee80211_rx_status *status)

Function in wmi.c should have ath10k_wmi_ prefix. I also don't like
the name as it doesn't convey what it does in the slightest. Perhaps
ath10k_wmi_mgmt_rx_h_wep_reauth() or ath10k_wmi_handle_wep_reauth()
would be a bit better?


> +{
> +       struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
> +       unsigned int hdrlen;
> +       bool peer_key;
> +       u8 *addr, keyidx;
> +
> +       if (ieee80211_is_auth(hdr->frame_control) &&
> +           ieee80211_has_protected(hdr->frame_control)) {

My preference is to use guards in functions like these, i.e.
  if (!auth || !protected) return;
and then continue with the code without extra indentation.


> +               hdrlen = ieee80211_hdrlen(hdr->frame_control);
> +               if (skb->len < (hdrlen + 4))

It's probably a good idea to use IEEE80211_WEP_IV_LEN instead of literal 4.


> +                       return;
> +
> +               keyidx = skb->data[hdrlen + 3] >> 6;
> +               addr = ieee80211_get_SA(hdr);
> +               peer_key = ath10k_is_peer_wep_key_set(ar, addr, keyidx);
> +
> +               if (peer_key) {

Again, I'd do a guard: `if (!peer_key) return`. But I'm just picky :-P
I leave the final decision up to you or Kalle.


[...]
> @@ -1200,6 +1227,8 @@ static int ath10k_wmi_event_mgmt_rx(struct ath10k *ar, struct sk_buff *skb)
>         hdr = (struct ieee80211_hdr *)skb->data;
>         fc = le16_to_cpu(hdr->frame_control);
>
> +       ath10k_check_wep(ar, skb, status);
> +
>         /* FW delivers WEP Shared Auth frame with Protected Bit set and
>          * encrypted payload. However in case of PMF it delivers decrypted
>          * frames with Protected Bit set. */

This is getting a little messy. Did you consider to somehow unify the
previous wep shared & pmf code with your new fix?


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC] ath10k: Fix shared WEP

[Sujith Manoharan]

Michal Kazior wrote:
> IOW This fixes a corner case when station reconnects but ath10k AP for
> some reason didn't notice it (e.g. station went out of range and came
> back before AP inactivity timer kicked in), right? Or is there another
> scenario? It might be a good idea to include this in the commit log.

The corner case is when a station changes its default key and tries
to reassociate, but does not send a deauth first.

> I don't think the above will work for mBSS, e.g. consider vdev_id=0
> being an open network and vdev_id=1 being wep-shared. Maybe it's about
> time to introduce ath10k_peer_find_by_addr(struct ath10k *ar, const u8
> *)?

Yep, I was aware of this. I think we can do this for a single BSS
now and have a FIXME to address multiple BSS later. This issue
is rarely seen in practice.

> Read access to peer->keys should be protected by either data_lock or
> conf_mutex. Obviously the latter can't be used because this function
> will be called from an atomic context leaving the data_lock. The
> entire is_peer_wep_key_set() should require data_lock to be held while
> it is called to avoid races.
> 
> Oh, and apparently this is buggy anyway because
> ath10k_install_peer_wep_keys() is oblivious to this. Oops..

I don't think it can happen, though ? ath10k_install_peer_wep_keys()
would be called only when a station transitions to ASSOC and message 3
can't be received when that happens ?

(But, I did see a lockdep splat when using WEP).

> This is getting a little messy. Did you consider to somehow unify the
> previous wep shared & pmf code with your new fix?

For shared wep, the FW delivers the first auth frame as encrypted and
subsequent auth frames as decrypted, so we need to differentiate them somehow...

I'll address the other stylistic comments and send a v2.

Sujith

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC] ath10k: Fix shared WEP

[Michal Kazior]

On 21 November 2014 10:54, Sujith Manoharan <sujith@msujith.org> wrote:
> Michal Kazior wrote:
[...]
>> Read access to peer->keys should be protected by either data_lock or
>> conf_mutex. Obviously the latter can't be used because this function
>> will be called from an atomic context leaving the data_lock. The
>> entire is_peer_wep_key_set() should require data_lock to be held while
>> it is called to avoid races.
>>
>> Oh, and apparently this is buggy anyway because
>> ath10k_install_peer_wep_keys() is oblivious to this. Oops..
>
> I don't think it can happen, though ? ath10k_install_peer_wep_keys()
> would be called only when a station transitions to ASSOC and message 3
> can't be received when that happens ?

Good point, but what I'm worried it'll be easier to miss this subtlety
and introduce races in the future.


> (But, I did see a lockdep splat when using WEP).

Do you have it saved somewhere? If so, can you post it, please?


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC] ath10k: Fix shared WEP

[Sujith Manoharan]

Michal Kazior wrote:
> Good point, but what I'm worried it'll be easier to miss this subtlety
> and introduce races in the future.

I am not very familiar with ath10k locking, but peer_keys is
also used in ath10k_clear_peer_keys(), where it relies on conf_mutex.
Should that be addressed too ?

> Do you have it saved somewhere? If so, can you post it, please?

http://pastebin.com/yywiPZXL

Sujith

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC] ath10k: Fix shared WEP

[Michal Kazior]

On 21 November 2014 11:36, Sujith Manoharan <sujith@msujith.org> wrote:
> Michal Kazior wrote:
>> Good point, but what I'm worried it'll be easier to miss this subtlety
>> and introduce races in the future.
>
> I am not very familiar with ath10k locking, but peer_keys is
> also used in ath10k_clear_peer_keys(), where it relies on conf_mutex.
> Should that be addressed too ?

Yes. Basically peer->keys[] should be protected by both conf_mutex and
data_lock if you want to modify it. If you want to read it you need
either one.


>> Do you have it saved somewhere? If so, can you post it, please?
>
> http://pastebin.com/yywiPZXL

Thanks! Looks like the `cancel_work_sync(&arvif->wep_key_work);`
should go _before_ `mutex_lock(&ar->conf_mutex);`.

I'm busy with some stuff so feel free to send a patch :-)


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC] ath10k: Fix shared WEP

[Sujith Manoharan]

Michal Kazior wrote:
> Yes. Basically peer->keys[] should be protected by both conf_mutex and
> data_lock if you want to modify it. If you want to read it you need
> either one.

Ok.

> Thanks! Looks like the `cancel_work_sync(&arvif->wep_key_work);`
> should go _before_ `mutex_lock(&ar->conf_mutex);`.
> 
> I'm busy with some stuff so feel free to send a patch :-)

I'll send a fix after this one. Thanks for the review !

Sujith

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [RFC] ath10k: Fix shared WEP

[Kalle Valo]

Sujith Manoharan <sujith@msujith.org> writes:

> From: Sujith Manoharan <c_manoha@qca.qualcomm.com>
>
> When static keys are used in shared WEP, when a
> station is associated, message 3 is sent with an
> encrypted payload. But, for subsequent
> authentications that are triggered without a
> deauth, the auth frame is decrypted by the HW. To
> handle this, check if the WEP keys have already
> been set for the peer and if so, mark the
> frame as decrypted.
>
> Signed-off-by: Sujith Manoharan <c_manoha@qca.qualcomm.com>

[...]

> +		if (peer_key) {
> +			ath10k_dbg(ar, ATH10K_DBG_MAC,
> +				   "wep key present for peer: %pM\n", addr);
> +			status->flag |= RX_FLAG_DECRYPTED;
> +		}
> +	}
> +
> +}

A checkpatch warning:

drivers/net/wireless/ath/ath10k/wmi.c:1141: CHECK: Blank lines aren't necessary before a close brace '}'

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k