From patchwork Thu Dec 5 09:21:29 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eliad Peller X-Patchwork-Id: 3286991 Return-Path: X-Original-To: patchwork-linux-wireless@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 90A2DC0D4B for ; Thu, 5 Dec 2013 09:21:53 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 5E9A92051C for ; Thu, 5 Dec 2013 09:21:52 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 29DA820212 for ; Thu, 5 Dec 2013 09:21:51 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754475Ab3LEJVt (ORCPT ); Thu, 5 Dec 2013 04:21:49 -0500 Received: from mail-ea0-f180.google.com ([209.85.215.180]:62784 "EHLO mail-ea0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754329Ab3LEJVn (ORCPT ); Thu, 5 Dec 2013 04:21:43 -0500 Received: by mail-ea0-f180.google.com with SMTP id f15so11281358eak.11 for ; Thu, 05 Dec 2013 01:21:42 -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:in-reply-to :references; bh=lLWialtsBHQDnWAXyLQes8G/BpeVTlKLqHh1KvHOcJ0=; b=KFUmxbLrEw+U+nrkzPjdCJX0rq1kRcB69pwk2gPocRf15KRrwmISISZhq+fy6PXfVF rofdboogg6atAaNQMh8iwRAFQzdg+3JntgWc5NVq92b2L87IgjoceeyreaXmI2uMfZmx Ih3j4SbUVGie0kwaKXcHS30bJaiUEaaz0UOSTcRQlCTXaqZEFEnNX147e8rnB11T0wkc XmAe5y6CbEBaNk2ntUDuK4y8HZ8S9KfarHpUGDu6Wkc6YOVxCWJvrVmBwF/D0TvkQxjc To0dlrYt6aok9cMaX7zSz3tt/9KBWfyyxbdW9TL+qrTraEe329efomn4j8urTccrnLoF 3tfw== X-Gm-Message-State: ALoCoQnLt1d1Lg7C+KsnS1Z6UxjjhJ2y043H/IMw4p2h4ilDUWmFTn28DueBhCDEPQAgYLf/MwhQ X-Received: by 10.15.74.200 with SMTP id j48mr7810097eey.102.1386235302736; Thu, 05 Dec 2013 01:21:42 -0800 (PST) Received: from localhost.localdomain (93-173-177-113.bb.netvision.net.il. [93.173.177.113]) by mx.google.com with ESMTPSA id h3sm78610986eem.15.2013.12.05.01.21.41 for (version=TLSv1.1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 05 Dec 2013 01:21:42 -0800 (PST) From: Eliad Peller To: Johannes Berg Cc: Subject: [PATCH 4/4] cfg80211: prevent race condition on scan request cleanup Date: Thu, 5 Dec 2013 11:21:29 +0200 Message-Id: <1386235289-27278-4-git-send-email-eliad@wizery.com> X-Mailer: git-send-email 1.8.5.rc1 In-Reply-To: <1386235289-27278-1-git-send-email-eliad@wizery.com> References: <1386235289-27278-1-git-send-email-eliad@wizery.com> 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. Solve it by freeing scan_req (and clearing it) only when leak=false. Otherwise, instead of freeing it mark the request as pending_cleanup, and free it later on (by the work). 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 --- i encountered this one while adding some intentional delays in order to reproduce a different issue. this is probably not a real-world scenario. include/net/cfg80211.h | 2 +- net/wireless/scan.c | 13 ++++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 3f4eff0..c12259e 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -1376,7 +1376,7 @@ struct cfg80211_scan_request { /* internal */ struct wiphy *wiphy; unsigned long scan_start; - bool aborted, notified; + bool aborted, notified, pending_cleanup; bool no_cck; u32 min_dwell; diff --git a/net/wireless/scan.c b/net/wireless/scan.c index d960e4a..1ec43a8 100644 --- a/net/wireless/scan.c +++ b/net/wireless/scan.c @@ -176,6 +176,9 @@ void ___cfg80211_scan_done(struct cfg80211_registered_device *rdev, bool leak) if (!request) return; + if (request->pending_cleanup) + goto free_request; + wdev = request->wdev; /* @@ -209,7 +212,6 @@ void ___cfg80211_scan_done(struct cfg80211_registered_device *rdev, bool leak) if (wdev->netdev) dev_put(wdev->netdev); - rdev->scan_req = NULL; /* * OK. If this is invoked with "leak" then we can't @@ -219,8 +221,13 @@ void ___cfg80211_scan_done(struct cfg80211_registered_device *rdev, bool leak) * the scan request or not ... if it accesses the dev * in there (it shouldn't anyway) then it may crash. */ - if (!leak) - kfree(request); + if (leak) { + request->pending_cleanup = true; + return; + } +free_request: + rdev->scan_req = NULL; + kfree(request); } void __cfg80211_scan_done(struct work_struct *wk)