d80211: fix key access race
Subsystems (2)
PATCH 1/1 d80211: fix key access race
It seems we don't have any protection when accessing the key. The RX/TX path may acquire a key which can be freed by the ioctl cmd.
I put a key_lock spinlock to protect all the accesses to the key (whether the sta_info->key or ieee80211_sub_if_data->keys[]). Don't find a good way to handle it :(
Thanks,
Hong
net/d80211/ieee80211.c +45 -1
@@ -99,12 +99,38 @@ struct ieee80211_key *ieee80211_key_alloreturn key; } -void ieee80211_key_free (struct ieee80211_key *key) +void ieee80211_key_put (struct ieee80211_key *key) {if (key)kobject_put (&key->kobj); } +void ieee80211_key_get (struct ieee80211_key *key) +{ +if (key) +kobject_get (&key->kobj); +} + +void ieee80211_key_free (struct ieee80211_local *local, +struct ieee80211_key **key, +struct ieee80211_sub_if_data *sdata) +{ +int remove_def =0 ; + +if (*key && sdata && sdata->default_key == *key) { +ieee80211_key_sysfs_remove_default (sdata); + remove_def =1 ; + } +ieee80211_key_sysfs_remove (*key); + +spin_lock_bh (&local->key_lock); +if (remove_def) + sdata->default_key =NULL ; + *key =NULL ; +ieee80211_key_put (*key); +spin_unlock_bh (&local->key_lock); +} +void ieee80211_key_release (struct kobject *kobj) {struct ieee80211_key *key;
@@ -402,6 +428,7 @@ ieee80211_tx_h_select_key(struct ieee802else tx->u.tx.control->key_idx =HW_KEY_IDX_INVALID ; +spin_lock_bh (&tx->local->key_lock);if (unlikely (tx->u.tx.control->flags &IEEE80211_TXCTL_DO_NOT_ENCRYPT )) tx->key =NULL ;else if (tx->sta && tx->sta->key)
@@ -410,11 +437,16 @@ ieee80211_tx_h_select_key(struct ieee802 tx->key = tx->sdata->default_key; else if (tx->sdata->drop_unencrypted && !(tx->sdata->eapol && ieee80211_is_eapol(tx->skb))) { +spin_unlock_bh (&tx->local->key_lock);I802_DEBUG_INC (tx->local->tx_handlers_drop_unencrypted);return TXRX_DROP ; } else tx->key = NULL; +if (tx->key) +ieee80211_key_get (tx->key); +spin_unlock_bh (&tx->local->key_lock); +if (tx->key) { tx->key->tx_rx_count++; if (unlikely(tx->local->key_tx_rx_threshold &&
@@ -1216,6 +1248,9 @@ static int ieee80211_tx(struct net_devic skb = tx.skb;/* handlers are allowed to change skb */ +if (tx.key) +ieee80211_key_put (tx.key); +if (sta)sta_info_put (sta);
@@ -3092,6 +3127,7 @@ ieee80211_rx_h_check(struct ieee80211_txelse always_sta_key =1 ; +spin_lock (&rx->local->key_lock);if (rx->sta && rx->sta->key && always_sta_key) { rx->key = rx->sta->key; }else {
@@ -3109,6 +3145,7 @@ ieee80211_rx_h_check(struct ieee80211_tx rx->key = rx->sdata->keys[keyidx];if (!rx->key) { +spin_unlock (&rx->local->key_lock);if (!rx->u.rx.ra_match)return TXRX_DROP ; printk(KERN_DEBUG "%s: RX WEP frame with "
@@ -3126,6 +3163,10 @@ ieee80211_rx_h_check(struct ieee80211_tx } } +if (rx->key) +ieee80211_key_get (rx->key); +spin_unlock (&rx->local->key_lock); +if (rx->fc &IEEE80211_FCTL_PROTECTED && rx->key && rx->u.rx.ra_match) { rx->key->tx_rx_count++; if (unlikely(rx->local->key_tx_rx_threshold &&
@@ -3705,6 +3746,8 @@ void __ieee80211_rx(struct net_device *d } end: +if (rx.key) +ieee80211_key_put (rx.key);if (sta)sta_info_put (sta); }
@@ -4411,6 +4454,7 @@ struct net_device *ieee80211_alloc_hw(siinit_timer (&local->scan.timer);/* clear it out */ spin_lock_init (&local->generic_lock); +spin_lock_init (&local->key_lock);init_timer (&local->stat_timer); local->stat_timer.function = ieee80211_stat_refresh; local->stat_timer.data = (unsigned long ) local;
net/d80211/ieee80211_i.h +6 -1
@@ -361,6 +361,7 @@ #define IEEE80211_IRQSAFE_QUEUE_LIMIT 12 } ieee80211_msg_enum;spinlock_t generic_lock; +spinlock_t key_lock;/* Station data structures */ struct kset sta_kset;spinlock_t sta_lock;/* mutex for STA data structures */
@@ -568,7 +569,11 @@ ieee80211_key_data2conf(struct ieee80211 struct ieee80211_key *data);struct ieee80211_key *ieee80211_key_alloc (struct ieee80211_sub_if_data *sdata,int idx,size_t key_len,gfp_t flags); -void ieee80211_key_free (struct ieee80211_key *key); +void ieee80211_key_get (struct ieee80211_key *key); +void ieee80211_key_put (struct ieee80211_key *key); +void ieee80211_key_free (struct ieee80211_local *local, +struct ieee80211_key **key, +struct ieee80211_sub_if_data *sdata);void ieee80211_key_release (struct kobject *kobj);void ieee80211_rx_mgmt (struct net_device *dev,struct sk_buff *skb,struct ieee80211_rx_status *status,u32 msg_type);
net/d80211/ieee80211_iface.c +1 -1
@@ -239,7 +239,7 @@ #if 0 local->hw->set_key (dev,DISABLE_KEY , addr, local->keys[i],0 );#endif -ieee80211_key_free (sdata->keys[i]); +ieee80211_key_free (local, &sdata->keys[i],NULL ); }/* Shouldn't be necessary but won't hurt */
net/d80211/ieee80211_ioctl.c +33 -56
169 lines not on this page · raw message
net/d80211/sta_info.c +1 -5
@@ -197,11 +197,7 @@ #ifdef CONFIG_D80211_VERBOSE_DEBUG local->mdev->name, MAC_ARG(sta->addr));#endif /* CONFIG_D80211_VERBOSE_DEBUG */ -if (sta->key) { -ieee80211_key_sysfs_remove (sta->key); -ieee80211_key_free (sta->key); - sta->key =NULL ; - } +ieee80211_key_free (local, &sta->key,NULL );rate_control_remove_sta_attrs (sta, &sta->kobj);ieee80211_sta_sysfs_remove (sta);
Jiri Benc
NAKQuoted 7 lines · Hong Liu
It seems we don't have any protection when accessing the key. The RX/TX path may acquire a key which can be freed by the ioctl cmd.
I put a
key_lockspinlock to protect all the accesses to the key (whether thesta_info->keyor ieee80211_sub_if_data->keys[]). Don't find a good way to handle it :(
NAK, this is too expensive.
I'm aware of the problem and figured how to fix it correctly while working on fixing of sta_list locking. Will send a patch later this week, stay tuned :-)
Thanks,
Jiri
Signature · 2 lines
Jiri Benc SUSE Labs