From patchwork Thu Dec 5 16:30:17 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eliad Peller X-Patchwork-Id: 3289191 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.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id D4A379F373 for ; Thu, 5 Dec 2013 16:30:32 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 9014D204AF for ; Thu, 5 Dec 2013 16:30:31 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 66800201DD for ; Thu, 5 Dec 2013 16:30:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756892Ab3LEQaY (ORCPT ); Thu, 5 Dec 2013 11:30:24 -0500 Received: from mail-ee0-f52.google.com ([74.125.83.52]:45850 "EHLO mail-ee0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756771Ab3LEQaX (ORCPT ); Thu, 5 Dec 2013 11:30:23 -0500 Received: by mail-ee0-f52.google.com with SMTP id d17so3245544eek.39 for ; Thu, 05 Dec 2013 08:30:22 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=AmclmDWbWcm4p+ZFYJCp4O8B2SGZAcSjKqgGUb3wn+o=; b=a0Uh0koTYIzA1G4VX2+QqQmxtWsN+9t7lYUwsudNaglQOWKiGVEf3M6TJicxBqgI0f HDvhAOyxpO2if1t1rRuIpiMHXwxyke1cACzV2kU04NqRMpgH01lm1W40C3vHkbpEf0qM +Fur0EVWzm9xOTOc3QNpVonlBYIibTYWnGUWPicce5JBTxv3vMkXUGHfDUIn/3gwlmha fkijJrM0o/8K61vpbiusxhfCDhWQOjOGGlpi4FI0jLKequ3vHX/hrnaYSC68kJHl/UkV qz2P8coX/6UNaSWh7O7+SsXfxoz+f3I/LNWOsmTWwd6J1ps77wnO8ogyO3N4ci4rYP4M UgHg== X-Gm-Message-State: ALoCoQkk7xxdPhzQmncuRdmIG4LzCcvJrZ2OuguXozx+1k69FCoJstpZl+CeuAgqY8F9rurvDTTa X-Received: by 10.14.29.66 with SMTP id h42mr33298551eea.4.1386261022332; Thu, 05 Dec 2013 08:30:22 -0800 (PST) Received: from muse.amr.corp.intel.com (93-173-177-113.bb.netvision.net.il. [93.173.177.113]) by mx.google.com with ESMTPSA id h3sm82702433eem.15.2013.12.05.08.30.20 for (version=TLSv1.1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 05 Dec 2013 08:30:21 -0800 (PST) From: Eliad Peller To: Johannes Berg Cc: Subject: [PATCH v2 4/4] cfg80211: don't "leak" uncompleted scans Date: Thu, 5 Dec 2013 18:30:17 +0200 Message-Id: <1386261017-5513-1-git-send-email-eliad@wizery.com> X-Mailer: git-send-email 1.8.5.rc1 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, RP_MATCHES_RCVD, 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 ___cfg80211_scan_done() can be called in some cases (e.g. on NETDEV_DOWN) before the low level driver notified scan completion (which is indicated by passing leak=true). Clearing rdev->scan_req in this case is buggy, as scan_done_wk might have already being queued/running (and can't be flushed as it takes rtnl()). If a new scan will be requested at this stage, the scan_done_wk will try freeing it (instead of the previous scan), and this will later result in a use after free. Simply remove the "leak" option, and replace it with a standard WARN_ON. An example backtrace after such crash: Unable to handle kernel paging request at virtual address fffffee5 pgd = c0004000 [fffffee5] *pgd=9fdf6821, *pte=00000000, *ppte=00000000 Internal error: Oops: 17 [#1] SMP ARM PC is at cfg80211_scan_done+0x28/0xc4 [cfg80211] LR is at __ieee80211_scan_completed+0xe4/0x2dc [mac80211] [] (cfg80211_scan_done+0x28/0xc4 [cfg80211]) [] (__ieee80211_scan_completed+0xe4/0x2dc [mac80211]) [] (ieee80211_scan_work+0x94/0x4f0 [mac80211]) [] (process_one_work+0x1b0/0x4a8) [] (worker_thread+0x138/0x37c) [] (kthread+0xa4/0xb0) Signed-off-by: Eliad Peller --- net/wireless/core.c | 20 ++++---------------- net/wireless/core.h | 2 +- net/wireless/scan.c | 16 +++------------- 3 files changed, 8 insertions(+), 30 deletions(-) diff --git a/net/wireless/core.c b/net/wireless/core.c index a8e48c7..1f6ad1d 100644 --- a/net/wireless/core.c +++ b/net/wireless/core.c @@ -203,17 +203,8 @@ void cfg80211_stop_p2p_device(struct cfg80211_registered_device *rdev, rdev->opencount--; - if (rdev->scan_req && rdev->scan_req->wdev == wdev) { - /* - * If the scan request wasn't notified as done, set it - * to aborted and leak it after a warning. The driver - * should have notified us that it ended at the latest - * during rdev_stop_p2p_device(). - */ - if (WARN_ON(!rdev->scan_req->notified)) - rdev->scan_req->aborted = true; - ___cfg80211_scan_done(rdev, !rdev->scan_req->notified); - } + WARN_ON(rdev->scan_req && rdev->scan_req->wdev == wdev && + !rdev->scan_req->notified); } static int cfg80211_rfkill_set_block(void *data, bool blocked) @@ -870,11 +861,8 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb, break; case NETDEV_DOWN: cfg80211_update_iface_num(rdev, wdev->iftype, -1); - if (rdev->scan_req && rdev->scan_req->wdev == wdev) { - if (WARN_ON(!rdev->scan_req->notified)) - rdev->scan_req->aborted = true; - ___cfg80211_scan_done(rdev, true); - } + WARN_ON(rdev->scan_req && rdev->scan_req->wdev == wdev && + !rdev->scan_req->notified); if (WARN_ON(rdev->sched_scan_req && rdev->sched_scan_req->dev == wdev->netdev)) { diff --git a/net/wireless/core.h b/net/wireless/core.h index eb0f7a3..04a2022 100644 --- a/net/wireless/core.h +++ b/net/wireless/core.h @@ -364,7 +364,7 @@ int cfg80211_validate_key_settings(struct cfg80211_registered_device *rdev, struct key_params *params, int key_idx, bool pairwise, const u8 *mac_addr); void __cfg80211_scan_done(struct work_struct *wk); -void ___cfg80211_scan_done(struct cfg80211_registered_device *rdev, bool leak); +void ___cfg80211_scan_done(struct cfg80211_registered_device *rdev); void __cfg80211_sched_scan_results(struct work_struct *wk); int __cfg80211_stop_sched_scan(struct cfg80211_registered_device *rdev, bool driver_initiated); diff --git a/net/wireless/scan.c b/net/wireless/scan.c index d960e4a..ae21156 100644 --- a/net/wireless/scan.c +++ b/net/wireless/scan.c @@ -161,7 +161,7 @@ static void __cfg80211_bss_expire(struct cfg80211_registered_device *dev, dev->bss_generation++; } -void ___cfg80211_scan_done(struct cfg80211_registered_device *rdev, bool leak) +void ___cfg80211_scan_done(struct cfg80211_registered_device *rdev) { struct cfg80211_scan_request *request; struct wireless_dev *wdev; @@ -210,17 +210,7 @@ void ___cfg80211_scan_done(struct cfg80211_registered_device *rdev, bool leak) dev_put(wdev->netdev); rdev->scan_req = NULL; - - /* - * OK. If this is invoked with "leak" then we can't - * free this ... but we've cleaned it up anyway. The - * driver failed to call the scan_done callback, so - * all bets are off, it might still be trying to use - * the scan request or not ... if it accesses the dev - * in there (it shouldn't anyway) then it may crash. - */ - if (!leak) - kfree(request); + kfree(request); } void __cfg80211_scan_done(struct work_struct *wk) @@ -231,7 +221,7 @@ void __cfg80211_scan_done(struct work_struct *wk) scan_done_wk); rtnl_lock(); - ___cfg80211_scan_done(rdev, false); + ___cfg80211_scan_done(rdev); rtnl_unlock(); }