From patchwork Mon Jan 26 17:26:17 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?q?Michael_B=C3=BCsch?= X-Patchwork-Id: 5711521 X-Patchwork-Delegate: kvalo@adurom.com Return-Path: X-Original-To: patchwork-linux-wireless@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id A12F99F333 for ; Mon, 26 Jan 2015 17:27:43 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id D1D4420125 for ; Mon, 26 Jan 2015 17:27:42 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A776520114 for ; Mon, 26 Jan 2015 17:27:41 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755845AbbAZR1k (ORCPT ); Mon, 26 Jan 2015 12:27:40 -0500 Received: from bues.ch ([80.190.117.144]:40405 "EHLO bues.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755183AbbAZR1j (ORCPT ); Mon, 26 Jan 2015 12:27:39 -0500 Received: by bues.ch with esmtpsa (Exim 4.80) (envelope-from ) id 1YFnRm-0006h3-2L; Mon, 26 Jan 2015 18:27:34 +0100 Date: Mon, 26 Jan 2015 18:26:17 +0100 From: Michael =?UTF-8?B?QsO8c2No?= To: Kalle Valo Cc: b43-dev , linux-wireless , =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= , Larry Finger Subject: [PATCH] b43: Fix locking FIXME in beacon update top half Message-ID: <20150126182617.521f58b5@wiggum> X-Mailer: Claws Mail 3.11.1 (GTK+ 2.24.25; x86_64-pc-linux-gnu) MIME-Version: 1.0 Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, T_RP_MATCHES_RCVD, T_TVD_MIME_EPI, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 Tested-by: Larry Finger Index: linux/drivers/net/wireless/b43/b43.h =================================================================== --- linux.orig/drivers/net/wireless/b43/b43.h +++ linux/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]; Index: linux/drivers/net/wireless/b43/main.c =================================================================== --- linux.orig/drivers/net/wireless/b43/main.c +++ linux/drivers/net/wireless/b43/main.c @@ -1601,12 +1601,26 @@ static void b43_write_beacon_template(st 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(st 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_wo 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 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) @@ -5094,7 +5115,6 @@ static int b43_op_beacon_set_tim(struct { struct b43_wl *wl = hw_to_b43_wl(hw); - /* FIXME: add locking */ b43_update_templates(wl); return 0; @@ -5584,6 +5604,7 @@ static struct b43_wl *b43_wireless_init( 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);