b43: Fix locking FIXME in beacon update top half

b43 has a FIXME about locking in the mac80211 set-beacon-int callback for a long time.
As it turns out there actually is a tiny race window that could result in
a use-after-free bug of the 'current_beacon' memory.
Nobody ever reported this, so it probably never happened.

Fix this by adding a spin lock that protects the current_beacon access.
We must not be in atomic context while accessing hardware (due to SDIO),
so the beacon update bottom half has to clone the skb and release the lock
before writing it to hardware.

Let's all hope that this stops the troll who is trying to submit incorrect
fixes for this issue repeatedly.
And let's hope that I'm not a troll, too, who just hides even more evil code
in an even more complex attempt to fix the issue.

Signed-off-by: Michael Buesch <m@bues.ch>
Tested-by: Larry Finger <Larry.Finger@lwfinger.net>
Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

diff --git a/drivers/net/wireless/b43/b43.h b/drivers/net/wireless/b43/b43.h
index bb12586cd7cdfb05b0ac1e909a37a15d5bdc985e..65b2dd80ecd55a41ec9125ccbe674ee424b6781c 100644 (file)
--- a/drivers/net/wireless/b43/b43.h
+++ b/drivers/net/wireless/b43/b43.h
@@ -941,6 +941,7 @@ struct b43_wl {
        bool beacon1_uploaded;
        bool beacon_templates_virgin; /* Never wrote the templates? */
        struct work_struct beacon_update_trigger;
+       spinlock_t beacon_lock;
 
        /* The current QOS parameters for the 4 queues. */
        struct b43_qos_params qos_params[B43_QOS_QUEUE_NUM];

diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
index 58a2e88631fb75132cec50f99f80a5cef872e208..d1c397162a6ac9dfb3e4e0c66b0f50912f89319e 100644 (file)
--- a/drivers/net/wireless/b43/main.c
+++ b/drivers/net/wireless/b43/main.c
@@ -1601,12 +1601,26 @@ static void b43_write_beacon_template(struct b43_wldev *dev,
        unsigned int rate;
        u16 ctl;
        int antenna;
-       struct ieee80211_tx_info *info = IEEE80211_SKB_CB(dev->wl->current_beacon);
+       struct ieee80211_tx_info *info;
+       unsigned long flags;
+       struct sk_buff *beacon_skb;
 
-       bcn = (const struct ieee80211_mgmt *)(dev->wl->current_beacon->data);
-       len = min_t(size_t, dev->wl->current_beacon->len,
-                 0x200 - sizeof(struct b43_plcp_hdr6));
+       spin_lock_irqsave(&dev->wl->beacon_lock, flags);
+       info = IEEE80211_SKB_CB(dev->wl->current_beacon);
        rate = ieee80211_get_tx_rate(dev->wl->hw, info)->hw_value;
+       /* Clone the beacon, so it cannot go away, while we write it to hw. */
+       beacon_skb = skb_clone(dev->wl->current_beacon, GFP_ATOMIC);
+       spin_unlock_irqrestore(&dev->wl->beacon_lock, flags);
+
+       if (!beacon_skb) {
+               b43dbg(dev->wl, "Could not upload beacon. "
+                      "Failed to clone beacon skb.");
+               return;
+       }
+
+       bcn = (const struct ieee80211_mgmt *)(beacon_skb->data);
+       len = min_t(size_t, beacon_skb->len,
+                   0x200 - sizeof(struct b43_plcp_hdr6));
 
        b43_write_template_common(dev, (const u8 *)bcn,
                                  len, ram_offset, shm_size_offset, rate);
@@ -1674,6 +1688,8 @@ static void b43_write_beacon_template(struct b43_wldev *dev,
                                B43_SHM_SH_DTIMPER, 0);
        }
        b43dbg(dev->wl, "Updated beacon template at 0x%x\n", ram_offset);
+
+       dev_kfree_skb_any(beacon_skb);
 }
 
 static void b43_upload_beacon0(struct b43_wldev *dev)
@@ -1790,13 +1806,13 @@ static void b43_beacon_update_trigger_work(struct work_struct *work)
        mutex_unlock(&wl->mutex);
 }
 
-/* Asynchronously update the packet templates in template RAM.
- * Locking: Requires wl->mutex to be locked. */
+/* Asynchronously update the packet templates in template RAM. */
 static void b43_update_templates(struct b43_wl *wl)
 {
-       struct sk_buff *beacon;
+       struct sk_buff *beacon, *old_beacon;
+       unsigned long flags;
 
-       /* This is the top half of the ansynchronous beacon update.
+       /* This is the top half of the asynchronous beacon update.
         * The bottom half is the beacon IRQ.
         * Beacon update must be asynchronous to avoid sending an
         * invalid beacon. This can happen for example, if the firmware
@@ -1810,12 +1826,17 @@ static void b43_update_templates(struct b43_wl *wl)
        if (unlikely(!beacon))
                return;
 
-       if (wl->current_beacon)
-               dev_kfree_skb_any(wl->current_beacon);
+       spin_lock_irqsave(&wl->beacon_lock, flags);
+       old_beacon = wl->current_beacon;
        wl->current_beacon = beacon;
        wl->beacon0_uploaded = false;
        wl->beacon1_uploaded = false;
+       spin_unlock_irqrestore(&wl->beacon_lock, flags);
+
        ieee80211_queue_work(wl->hw, &wl->beacon_update_trigger);
+
+       if (old_beacon)
+               dev_kfree_skb_any(old_beacon);
 }
 
 static void b43_set_beacon_int(struct b43_wldev *dev, u16 beacon_int)
@@ -5095,7 +5116,6 @@ static int b43_op_beacon_set_tim(struct ieee80211_hw *hw,
 {
        struct b43_wl *wl = hw_to_b43_wl(hw);
 
-       /* FIXME: add locking */
        b43_update_templates(wl);
 
        return 0;
@@ -5585,6 +5605,7 @@ static struct b43_wl *b43_wireless_init(struct b43_bus_dev *dev)
        wl->hw = hw;
        mutex_init(&wl->mutex);
        spin_lock_init(&wl->hardirq_lock);
+       spin_lock_init(&wl->beacon_lock);
        INIT_WORK(&wl->beacon_update_trigger, b43_beacon_update_trigger_work);
        INIT_WORK(&wl->txpower_adjust_work, b43_phy_txpower_adjust_work);
        INIT_WORK(&wl->tx_work, b43_tx_work);