* 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