* key races in mac80211
@ 2007-08-28 10:42 Johannes Berg
2007-08-28 11:22 ` Johannes Berg
0 siblings, 1 reply; 3+ messages in thread
From: Johannes Berg @ 2007-08-28 10:42 UTC (permalink / raw)
To: linux-wireless; +Cc: Michael Wu, Jiri Benc, John Linville
[-- Attachment #1: Type: text/plain, Size: 1593 bytes --]
Working on the key code made me see a lot clearer and notice various
problems.
For one, during receive processing, we can select the key before using
it and because there's no locking it is possible that we kfree() the key
after having selected it but before using it for crypto operations.
Secondly, during transmit processing, there are two possible races: We
have a similar race between select_key() and using it for encryption,
but we also have a race here between select_key() and hardware
encryption when a key is removed.
The best solution I can come up with so far is to use RCU: when a key is
to be removed, we first remove the pointer from the appropriate places
(sdata->keys, sdata->default_key, sta->key) using rcu_assign_pointer()
and then synchronize_rcu(). Then, we can safely kfree() the key and
remove it from the hardware. There's a window here where the hardware
may still be using it for decryption, but we can't work around that
without having two hardware callbacks, one to disable the key for RX and
one to disable it for TX, and that's not worth the extra complexity.
When we add a key, we first need to upload it to the hardware and then,
using rcu_assign_pointer() again, link it into our structures.
In the code using keys (TX/RX paths) we use rcu_dereference() to get the
key and enclose the whole tx/rx section in a rcu_read_lock() ...
rcu_read_unlock() block. Because we've uploaded the key to hardware
before linking it into internal structures, we can guarantee that it is
valid once get to into tx().
Thoughts?
johannes
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 190 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: key races in mac80211
2007-08-28 10:42 key races in mac80211 Johannes Berg
@ 2007-08-28 11:22 ` Johannes Berg
2007-08-30 10:54 ` Johannes Berg
0 siblings, 1 reply; 3+ messages in thread
From: Johannes Berg @ 2007-08-28 11:22 UTC (permalink / raw)
To: linux-wireless; +Cc: Michael Wu, Jiri Benc, John Linville
On Tue, 2007-08-28 at 12:42 +0200, Johannes Berg wrote:
> The best solution I can come up with so far is to use RCU
This untested patch might solve it.
However, there remains one problem: when we have hardware acceleration
for crypto and the driver shuts down the queues, we end up queueing the
frame. If now somebody removes the key, the key will be removed from
hwaccel and then then driver will be asked to encrypt the frame with a
key index that has been removed. Hence, drivers will need to be aware
that the hw_key_index they are passed might not be valid. Most drivers
will, however, simply ignore that, but this may result in a frame being
encrypted with a wrong key. Not sure what we can do about it, even
keeping a "valid key index" bitmap in the driver isn't going to help
because of reuse.
This will, however, most likely be solved once we add multiqueue support
so I didn't worry about it too much.
johannes
---
net/mac80211/ieee80211_ioctl.c | 5 ----
net/mac80211/key.c | 47 ++++++++++++++++++++++++-----------------
net/mac80211/rx.c | 19 +++++++++++++---
net/mac80211/tx.c | 18 ++++++++++++---
4 files changed, 59 insertions(+), 30 deletions(-)
--- wireless-dev.orig/net/mac80211/key.c 2007-08-28 12:48:21.674634041 +0200
+++ wireless-dev/net/mac80211/key.c 2007-08-28 13:18:56.084634041 +0200
@@ -12,6 +12,7 @@
#include <linux/if_ether.h>
#include <linux/etherdevice.h>
#include <linux/list.h>
+#include <linux/rcupdate.h>
#include <net/mac80211.h>
#include "ieee80211_i.h"
#include "debugfs_key.h"
@@ -130,6 +131,7 @@ struct ieee80211_key *ieee80211_key_allo
{
struct ieee80211_key *key;
+ BUG_ON(idx < 0 || idx >= NUM_DEFAULT_KEYS);
BUG_ON(alg == ALG_NONE);
key = kzalloc(sizeof(struct ieee80211_key) + key_len, GFP_KERNEL);
@@ -167,9 +169,15 @@ struct ieee80211_key *ieee80211_key_allo
ieee80211_debugfs_key_add(key->local, key);
+ /* remove key first */
+ if (sta)
+ ieee80211_key_free(sta->key);
+ else
+ ieee80211_key_free(sdata->keys[idx]);
+
if (sta) {
ieee80211_debugfs_key_sta_link(key, sta);
- sta->key = key;
+
/*
* some hardware cannot handle TKIP with QoS, so
* we indicate whether QoS could be in use.
@@ -189,23 +197,21 @@ struct ieee80211_key *ieee80211_key_allo
sta_info_put(ap);
}
}
-
- if (idx >= 0 && idx < NUM_DEFAULT_KEYS) {
- if (!sdata->keys[idx])
- sdata->keys[idx] = key;
- else
- WARN_ON(1);
- } else
- WARN_ON(1);
}
+ /* enable hwaccel if appropriate */
+ if (netif_running(key->sdata->dev))
+ ieee80211_key_enable_hw_accel(key);
+
+ if (sta)
+ rcu_assign_pointer(sta->key, key);
+ else
+ rcu_assign_pointer(sdata->keys[idx], key);
+
mutex_lock(&sdata->key_mtx);
list_add(&key->list, &sdata->key_list);
mutex_unlock(&sdata->key_mtx);
- if (netif_running(key->sdata->dev))
- ieee80211_key_enable_hw_accel(key);
-
return key;
}
@@ -214,27 +220,30 @@ static void __ieee80211_key_free(struct
if (!key)
return;
- ieee80211_key_disable_hw_accel(key);
-
if (key->sta) {
- key->sta->key = NULL;
+ rcu_assign_pointer(key->sta->key, NULL);
} else {
if (key->sdata->default_key == key)
ieee80211_set_default_key(key->sdata, -1);
if (key->conf.keyidx >= 0 &&
key->conf.keyidx < NUM_DEFAULT_KEYS)
- key->sdata->keys[key->conf.keyidx] = NULL;
+ rcu_assign_pointer(key->sdata->keys[key->conf.keyidx],
+ NULL);
else
WARN_ON(1);
}
+ /* wait for all key users to complete */
+ synchronize_rcu();
+
+ /* remove from hwaccel if appropriate */
+ ieee80211_key_disable_hw_accel(key);
+
if (key->conf.alg == ALG_CCMP)
ieee80211_aes_key_free(key->u.ccmp.tfm);
ieee80211_debugfs_key_remove(key);
- mutex_lock(&key->sdata->key_mtx);
list_del(&key->list);
- mutex_unlock(&key->sdata->key_mtx);
kfree(key);
}
@@ -263,7 +272,7 @@ void ieee80211_set_default_key(struct ie
if (sdata->default_key != key) {
ieee80211_debugfs_key_remove_default(sdata);
- sdata->default_key = key;
+ rcu_assign_pointer(sdata->default_key, key);
if (sdata->default_key)
ieee80211_debugfs_key_add_default(sdata);
--- wireless-dev.orig/net/mac80211/ieee80211_ioctl.c 2007-08-28 12:57:41.534634041 +0200
+++ wireless-dev/net/mac80211/ieee80211_ioctl.c 2007-08-28 12:58:08.744634041 +0200
@@ -457,11 +457,8 @@ static int ieee80211_set_encryption(stru
key = NULL;
} else {
/*
- * Need to free it before allocating a new one with
- * with the same index or the ordering to the driver's
- * set_key() callback becomes confused.
+ * Automatically frees any old key if present.
*/
- ieee80211_key_free(key);
key = ieee80211_key_alloc(sdata, sta, alg, idx, 0,
key_len, _key);
if (!key) {
--- wireless-dev.orig/net/mac80211/rx.c 2007-08-28 12:58:38.574634041 +0200
+++ wireless-dev/net/mac80211/rx.c 2007-08-28 13:15:46.344634041 +0200
@@ -13,6 +13,7 @@
#include <linux/skbuff.h>
#include <linux/netdevice.h>
#include <linux/etherdevice.h>
+#include <linux/rcupdate.h>
#include <net/mac80211.h>
#include <net/ieee80211_radiotap.h>
@@ -339,6 +340,7 @@ ieee80211_rx_h_load_key(struct ieee80211
int keyidx;
int hdrlen;
u8 *sta_mac = NULL;
+ struct ieee80211_key *stakey = NULL;
/*
* Key selection 101
@@ -376,8 +378,11 @@ ieee80211_rx_h_load_key(struct ieee80211
if (!(rx->flags & IEEE80211_TXRXD_RXRA_MATCH))
return TXRX_CONTINUE;
- if (!is_multicast_ether_addr(hdr->addr1) && rx->sta && rx->sta->key) {
- rx->key = rx->sta->key;
+ if (rx->sta)
+ stakey = rcu_dereference(rx->sta->key);
+
+ if (!is_multicast_ether_addr(hdr->addr1) && stakey) {
+ rx->key = stakey;
} else {
/*
* The device doesn't give us the IV so we won't be
@@ -403,7 +408,7 @@ ieee80211_rx_h_load_key(struct ieee80211
*/
keyidx = rx->skb->data[hdrlen + 3] >> 6;
- rx->key = rx->sdata->keys[keyidx];
+ rx->key = rcu_dereference(rx->sdata->keys[keyidx]);
/*
* RSNA-protected unicast frames should always be sent with
@@ -1545,6 +1550,12 @@ void __ieee80211_rx(struct ieee80211_hw
skb_pull(skb, radiotap_len);
}
+ /*
+ * key references are protected using RCU and this requires that
+ * we are in a read-site RCU section during receive processing
+ */
+ rcu_read_lock();
+
hdr = (struct ieee80211_hdr *) skb->data;
memset(&rx, 0, sizeof(rx));
rx.skb = skb;
@@ -1651,6 +1662,8 @@ void __ieee80211_rx(struct ieee80211_hw
dev_kfree_skb(skb);
read_unlock(&local->sub_if_lock);
+ rcu_read_unlock();
+
end:
if (sta)
sta_info_put(sta);
--- wireless-dev.orig/net/mac80211/tx.c 2007-08-28 13:01:05.214634041 +0200
+++ wireless-dev/net/mac80211/tx.c 2007-08-28 13:15:37.534634041 +0200
@@ -17,6 +17,7 @@
#include <linux/skbuff.h>
#include <linux/etherdevice.h>
#include <linux/bitmap.h>
+#include <linux/rcupdate.h>
#include <net/ieee80211_radiotap.h>
#include <net/cfg80211.h>
#include <net/mac80211.h>
@@ -427,14 +428,15 @@ static ieee80211_txrx_result
ieee80211_tx_h_select_key(struct ieee80211_txrx_data *tx)
{
u8 *sta_mac = NULL;
+ struct ieee80211_key *key;
if (unlikely(tx->u.tx.control->flags & IEEE80211_TXCTL_DO_NOT_ENCRYPT))
tx->key = NULL;
- else if (tx->sta && tx->sta->key) {
+ else if (tx->sta && (key = rcu_dereference(tx->sta->key))) {
sta_mac = tx->sta->addr;
- tx->key = tx->sta->key;
- } else if (tx->sdata->default_key)
- tx->key = tx->sdata->default_key;
+ tx->key = key;
+ } else if ((key = rcu_dereference(tx->sdata->default_key)))
+ tx->key = key;
else if (tx->sdata->drop_unencrypted &&
!(tx->sdata->eapol && ieee80211_is_eapol(tx->skb))) {
I802_DEBUG_INC(tx->local->tx_handlers_drop_unencrypted);
@@ -1138,6 +1140,12 @@ static int ieee80211_tx(struct net_devic
return 0;
}
+ /*
+ * key references are protected using RCU and this requires that
+ * we are in a read-site RCU section during receive processing
+ */
+ rcu_read_lock();
+
sta = tx.sta;
tx.u.tx.mgmt_interface = mgmt;
tx.u.tx.mode = local->hw.conf.mode;
@@ -1222,6 +1230,7 @@ retry:
store->last_frag_rate_ctrl_probe =
!!(tx.flags & IEEE80211_TXRXD_TXPROBE_LAST_FRAG);
}
+ rcu_read_unlock();
return 0;
drop:
@@ -1231,6 +1240,7 @@ retry:
if (tx.u.tx.extra_frag[i])
dev_kfree_skb(tx.u.tx.extra_frag[i]);
kfree(tx.u.tx.extra_frag);
+ rcu_read_unlock();
return 0;
}
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: key races in mac80211
2007-08-28 11:22 ` Johannes Berg
@ 2007-08-30 10:54 ` Johannes Berg
0 siblings, 0 replies; 3+ messages in thread
From: Johannes Berg @ 2007-08-30 10:54 UTC (permalink / raw)
To: linux-wireless; +Cc: Michael Wu, Jiri Benc, John Linville
On Tue, 2007-08-28 at 13:22 +0200, Johannes Berg wrote:
> This untested patch might solve it.
It showed that it was untested, it had a few missing rcu_read_unlock()
statements. Updated and working patch below.
johannes
---
net/mac80211/ieee80211_ioctl.c | 5 ----
net/mac80211/key.c | 43 +++++++++++++++++++++++++----------------
net/mac80211/rx.c | 20 ++++++++++++++++---
net/mac80211/tx.c | 20 +++++++++++++++----
4 files changed, 61 insertions(+), 27 deletions(-)
--- wireless-dev.orig/net/mac80211/key.c 2007-08-29 15:48:54.731382599 +0200
+++ wireless-dev/net/mac80211/key.c 2007-08-29 15:48:55.071382599 +0200
@@ -12,6 +12,7 @@
#include <linux/if_ether.h>
#include <linux/etherdevice.h>
#include <linux/list.h>
+#include <linux/rcupdate.h>
#include <net/mac80211.h>
#include "ieee80211_i.h"
#include "debugfs_key.h"
@@ -120,6 +121,7 @@ struct ieee80211_key *ieee80211_key_allo
{
struct ieee80211_key *key;
+ BUG_ON(idx < 0 || idx >= NUM_DEFAULT_KEYS);
BUG_ON(alg == ALG_NONE);
key = kzalloc(sizeof(struct ieee80211_key) + key_len, GFP_KERNEL);
@@ -157,9 +159,15 @@ struct ieee80211_key *ieee80211_key_allo
ieee80211_debugfs_key_add(key->local, key);
+ /* remove key first */
+ if (sta)
+ ieee80211_key_free(sta->key);
+ else
+ ieee80211_key_free(sdata->keys[idx]);
+
if (sta) {
ieee80211_debugfs_key_sta_link(key, sta);
- sta->key = key;
+
/*
* some hardware cannot handle TKIP with QoS, so
* we indicate whether QoS could be in use.
@@ -179,21 +187,19 @@ struct ieee80211_key *ieee80211_key_allo
sta_info_put(ap);
}
}
-
- if (idx >= 0 && idx < NUM_DEFAULT_KEYS) {
- if (!sdata->keys[idx])
- sdata->keys[idx] = key;
- else
- WARN_ON(1);
- } else
- WARN_ON(1);
}
- list_add(&key->list, &sdata->key_list);
-
+ /* enable hwaccel if appropriate */
if (netif_running(key->sdata->dev))
ieee80211_key_enable_hw_accel(key);
+ if (sta)
+ rcu_assign_pointer(sta->key, key);
+ else
+ rcu_assign_pointer(sdata->keys[idx], key);
+
+ list_add(&key->list, &sdata->key_list);
+
return key;
}
@@ -202,20 +208,25 @@ void ieee80211_key_free(struct ieee80211
if (!key)
return;
- ieee80211_key_disable_hw_accel(key);
-
if (key->sta) {
- key->sta->key = NULL;
+ rcu_assign_pointer(key->sta->key, NULL);
} else {
if (key->sdata->default_key == key)
ieee80211_set_default_key(key->sdata, -1);
if (key->conf.keyidx >= 0 &&
key->conf.keyidx < NUM_DEFAULT_KEYS)
- key->sdata->keys[key->conf.keyidx] = NULL;
+ rcu_assign_pointer(key->sdata->keys[key->conf.keyidx],
+ NULL);
else
WARN_ON(1);
}
+ /* wait for all key users to complete */
+ synchronize_rcu();
+
+ /* remove from hwaccel if appropriate */
+ ieee80211_key_disable_hw_accel(key);
+
if (key->conf.alg == ALG_CCMP)
ieee80211_aes_key_free(key->u.ccmp.tfm);
ieee80211_debugfs_key_remove(key);
@@ -235,7 +246,7 @@ void ieee80211_set_default_key(struct ie
if (sdata->default_key != key) {
ieee80211_debugfs_key_remove_default(sdata);
- sdata->default_key = key;
+ rcu_assign_pointer(sdata->default_key, key);
if (sdata->default_key)
ieee80211_debugfs_key_add_default(sdata);
--- wireless-dev.orig/net/mac80211/ieee80211_ioctl.c 2007-08-29 15:48:54.641382599 +0200
+++ wireless-dev/net/mac80211/ieee80211_ioctl.c 2007-08-29 15:48:55.081382599 +0200
@@ -420,11 +420,8 @@ static int ieee80211_set_encryption(stru
key = NULL;
} else {
/*
- * Need to free it before allocating a new one with
- * with the same index or the ordering to the driver's
- * set_key() callback becomes confused.
+ * Automatically frees any old key if present.
*/
- ieee80211_key_free(key);
key = ieee80211_key_alloc(sdata, sta, alg, idx, key_len, _key);
if (!key) {
ret = -ENOMEM;
--- wireless-dev.orig/net/mac80211/rx.c 2007-08-29 15:48:54.701382599 +0200
+++ wireless-dev/net/mac80211/rx.c 2007-08-29 15:49:48.981382599 +0200
@@ -13,6 +13,7 @@
#include <linux/skbuff.h>
#include <linux/netdevice.h>
#include <linux/etherdevice.h>
+#include <linux/rcupdate.h>
#include <net/mac80211.h>
#include <net/ieee80211_radiotap.h>
@@ -321,6 +322,7 @@ ieee80211_rx_h_load_key(struct ieee80211
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) rx->skb->data;
int keyidx;
int hdrlen;
+ struct ieee80211_key *stakey = NULL;
/*
* Key selection 101
@@ -358,8 +360,11 @@ ieee80211_rx_h_load_key(struct ieee80211
if (!(rx->flags & IEEE80211_TXRXD_RXRA_MATCH))
return TXRX_CONTINUE;
- if (!is_multicast_ether_addr(hdr->addr1) && rx->sta && rx->sta->key) {
- rx->key = rx->sta->key;
+ if (rx->sta)
+ stakey = rcu_dereference(rx->sta->key);
+
+ if (!is_multicast_ether_addr(hdr->addr1) && stakey) {
+ rx->key = stakey;
} else {
/*
* The device doesn't give us the IV so we won't be
@@ -384,7 +389,7 @@ ieee80211_rx_h_load_key(struct ieee80211
*/
keyidx = rx->skb->data[hdrlen + 3] >> 6;
- rx->key = rx->sdata->keys[keyidx];
+ rx->key = rcu_dereference(rx->sdata->keys[keyidx]);
/*
* RSNA-protected unicast frames should always be sent with
@@ -1512,6 +1517,12 @@ void __ieee80211_rx(struct ieee80211_hw
skb_pull(skb, radiotap_len);
}
+ /*
+ * key references are protected using RCU and this requires that
+ * we are in a read-site RCU section during receive processing
+ */
+ rcu_read_lock();
+
hdr = (struct ieee80211_hdr *) skb->data;
memset(&rx, 0, sizeof(rx));
rx.skb = skb;
@@ -1552,6 +1563,7 @@ void __ieee80211_rx(struct ieee80211_hw
ieee80211_invoke_rx_handlers(local, local->rx_handlers, &rx,
rx.sta);
sta_info_put(sta);
+ rcu_read_unlock();
return;
}
@@ -1613,6 +1625,8 @@ void __ieee80211_rx(struct ieee80211_hw
read_unlock(&local->sub_if_lock);
end:
+ rcu_read_unlock();
+
if (sta)
sta_info_put(sta);
}
--- wireless-dev.orig/net/mac80211/tx.c 2007-08-29 15:48:54.831382599 +0200
+++ wireless-dev/net/mac80211/tx.c 2007-08-29 15:50:20.991382599 +0200
@@ -17,6 +17,7 @@
#include <linux/skbuff.h>
#include <linux/etherdevice.h>
#include <linux/bitmap.h>
+#include <linux/rcupdate.h>
#include <net/ieee80211_radiotap.h>
#include <net/cfg80211.h>
#include <net/mac80211.h>
@@ -426,14 +427,16 @@ ieee80211_tx_h_ps_buf(struct ieee80211_t
static ieee80211_txrx_result
ieee80211_tx_h_select_key(struct ieee80211_txrx_data *tx)
{
+ struct ieee80211_key *key;
+
tx->u.tx.control->key_idx = HW_KEY_IDX_INVALID;
if (unlikely(tx->u.tx.control->flags & IEEE80211_TXCTL_DO_NOT_ENCRYPT))
tx->key = NULL;
- else if (tx->sta && tx->sta->key)
- tx->key = tx->sta->key;
- else if (tx->sdata->default_key)
- tx->key = tx->sdata->default_key;
+ else if (tx->sta && (key = rcu_dereference(tx->sta->key)))
+ tx->key = key;
+ else if ((key = rcu_dereference(tx->sdata->default_key)))
+ tx->key = key;
else if (tx->sdata->drop_unencrypted &&
!(tx->sdata->eapol && ieee80211_is_eapol(tx->skb))) {
I802_DEBUG_INC(tx->local->tx_handlers_drop_unencrypted);
@@ -1111,6 +1114,12 @@ static int ieee80211_tx(struct net_devic
return 0;
}
+ /*
+ * key references are protected using RCU and this requires that
+ * we are in a read-site RCU section during receive processing
+ */
+ rcu_read_lock();
+
sta = tx.sta;
tx.u.tx.mgmt_interface = mgmt;
tx.u.tx.mode = local->hw.conf.mode;
@@ -1138,6 +1147,7 @@ static int ieee80211_tx(struct net_devic
if (unlikely(res == TXRX_QUEUED)) {
I802_DEBUG_INC(local->tx_handlers_queued);
+ rcu_read_unlock();
return 0;
}
@@ -1195,6 +1205,7 @@ retry:
store->last_frag_rate_ctrl_probe =
!!(tx.flags & IEEE80211_TXRXD_TXPROBE_LAST_FRAG);
}
+ rcu_read_unlock();
return 0;
drop:
@@ -1204,6 +1215,7 @@ retry:
if (tx.u.tx.extra_frag[i])
dev_kfree_skb(tx.u.tx.extra_frag[i]);
kfree(tx.u.tx.extra_frag);
+ rcu_read_unlock();
return 0;
}
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2007-08-30 11:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-08-28 10:42 key races in mac80211 Johannes Berg
2007-08-28 11:22 ` Johannes Berg
2007-08-30 10:54 ` Johannes Berg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox