Skip to contentSkip to conversation

d80211: fix key access race

PATCH NAK Hong Liu 2 participants 2 messages Nov 3, 2006 – Nov 6, 2006

Subsystems (2)
lore
Share & tools
lore

Hong Liu

author
#0 lore raw

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
index 751bc76..1a1f6fb 100644
@@ -99,12 +99,38 @@ struct ieee80211_key *ieee80211_key_allo
 	return 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 ieee802
 	else
 		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_tx
 	else
 		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(si
         init_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
index 89666ec..8e2a4a3 100644
@@ -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
index 9a187af..4455bd2 100644
@@ -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
index 6e73fab..246ae34 100644
@@ -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);
#1 lore raw
Quoted 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_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 :(

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