Re: [patch] d80211: fix key access race

2006-11-06 Thread Jiri Benc
On Fri, 03 Nov 2006 11:48:22 +0800, Hong Liu wrote:
 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

-- 
Jiri Benc
SUSE Labs
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[patch] d80211: fix key access race

2006-11-02 Thread 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 :(

Thanks,
Hong

Signed-off-by: Hong Liu [EMAIL PROTECTED]

diff --git a/net/d80211/ieee80211.c b/net/d80211/ieee80211.c
index 751bc76..1a1f6fb 100644
--- a/net/d80211/ieee80211.c
+++ b/net/d80211/ieee80211.c
@@ -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;
diff --git a/net/d80211/ieee80211_i.h b/net/d80211/ieee80211_i.h
index 89666ec..8e2a4a3 100644
--- a/net/d80211/ieee80211_i.h
+++ b/net/d80211/ieee80211_i.h
@@ -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