From patchwork Thu Nov 10 18:37:24 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Brian Norris X-Patchwork-Id: 9421975 X-Patchwork-Delegate: kvalo@adurom.com Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 2B28C60512 for ; Thu, 10 Nov 2016 18:37:40 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 42453297D3 for ; Thu, 10 Nov 2016 18:37:40 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 36698297E5; Thu, 10 Nov 2016 18:37:40 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 93C3D297D3 for ; Thu, 10 Nov 2016 18:37:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755842AbcKJShf (ORCPT ); Thu, 10 Nov 2016 13:37:35 -0500 Received: from mail-pf0-f178.google.com ([209.85.192.178]:34779 "EHLO mail-pf0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755770AbcKJSh2 (ORCPT ); Thu, 10 Nov 2016 13:37:28 -0500 Received: by mail-pf0-f178.google.com with SMTP id n85so150487453pfi.1 for ; Thu, 10 Nov 2016 10:37:27 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=c5OP6+AYYa0U3pLNFX2ycumiKhhHHdWFaSpevxgNljk=; b=gD///qICoHX3yWrKHjRNgtP4bPVnvkNcEcAiAD3fW3OU+W8LWzbL1KWf/DlE7Pl37D YrJovP6hGhdKcUuAdIYSJp1N2pSaOq/QiTIlU+7yjGYjlG+etgtw6MKqkXm9lYNt4R8P AcntYaGTHDe7nqJJ2aznMGbAB2rnws5FvmR1w= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=c5OP6+AYYa0U3pLNFX2ycumiKhhHHdWFaSpevxgNljk=; b=m0Bvka1iW4jg/+2DphrnReDRphSewQH8cNjDLyQ9sE+DgTtmeTaQMB9USAU+73ak1n t6aAKoFuevgqW50hAd9Efgq1ElZYi/UCuwoPcch1S+4uIMI1IL/ZDYvAehbVVVc/0Ns1 n7dz88UUJ2HxrUMDp35dDkv1qXqrPskVu69Z6sROW1USVXbFQZf277KgoaFa9ECpbRYV BrNn8OcXlgsm66sQo7GXztEXJL1Y419nYmhhnVE05OJ7icbGGhkkMSubiVlRdnqbN1q6 E7dsZiAPrOGwAKqcu5t0fZ7VDKJZhRcmVbM0kDqEY2sJY4F9+KtZmwpw8j7sFtyuFBUu phDw== X-Gm-Message-State: ABUngvfhtbsUm5NqMQNPzO5/7c9Wg1dHpcxwg+1Y4ZtATcQzenXBotjjNfFc6QfBKRcT7wOC X-Received: by 10.98.163.71 with SMTP id s68mr12558342pfe.60.1478803046520; Thu, 10 Nov 2016 10:37:26 -0800 (PST) Received: from google.com ([2620:0:1000:1301:9049:187b:72ab:1549]) by smtp.gmail.com with ESMTPSA id 65sm9000648pfn.12.2016.11.10.10.37.25 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Thu, 10 Nov 2016 10:37:26 -0800 (PST) Date: Thu, 10 Nov 2016 10:37:24 -0800 From: Brian Norris To: Xinming Hu Cc: Linux Wireless , Kalle Valo , Dmitry Torokhov , Amitkumar Karwar , Cathy Luo Subject: Re: [PATCH RESEND v2 03/11] mwifiex: resolve races between async FW init (failure) and device removal Message-ID: <20161110183723.GA134624@google.com> References: <1478779032-5165-1-git-send-email-huxinming820@marvell.com> <1478779032-5165-3-git-send-email-huxinming820@marvell.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1478779032-5165-3-git-send-email-huxinming820@marvell.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Hi, On Thu, Nov 10, 2016 at 07:57:04PM +0800, Xinming Hu wrote: > From: Brian Norris > > It's possible for the FW init sequence to fail, which will trigger a > device cleanup sequence in mwifiex_fw_dpc(). This sequence can race with > device suspend() or remove() (e.g., reboot or unbind), and can trigger > use-after-free issues. Currently, this driver attempts (poorly) to > synchronize remove() using a semaphore, but it doesn't protect some of > the critical sections properly. Particularly, we grab a pointer to the > adapter struct (card->adapter) without checking if it's being freed or > not. We later do a NULL check on the adapter, but that doesn't work if > the adapter was freed. > > Also note that the PCIe interface driver doesn't ever set card->adapter > to NULL, so even if we get the synchronization right, we still might try > to redo the cleanup in ->remove(), even if the FW init failure sequence > already did it. > > This patch replaces the static semaphore with a per-device completion > struct, and uses that completion to synchronize the remove() thread with > the mwifiex_fw_dpc(). A future patch will utilize this completion to > synchronize the suspend() thread as well. > > Signed-off-by: Brian Norris > --- > v2: Same as v1 > --- > drivers/net/wireless/marvell/mwifiex/main.c | 46 ++++++++++------------------- > drivers/net/wireless/marvell/mwifiex/main.h | 13 +++++--- > drivers/net/wireless/marvell/mwifiex/pcie.c | 18 +++++------ > drivers/net/wireless/marvell/mwifiex/pcie.h | 2 ++ > drivers/net/wireless/marvell/mwifiex/sdio.c | 18 +++++------ > drivers/net/wireless/marvell/mwifiex/sdio.h | 2 ++ > drivers/net/wireless/marvell/mwifiex/usb.c | 23 +++++++-------- > drivers/net/wireless/marvell/mwifiex/usb.h | 2 ++ > 8 files changed, 55 insertions(+), 69 deletions(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c > index c710d5e..09d46d6 100644 > --- a/drivers/net/wireless/marvell/mwifiex/main.c > +++ b/drivers/net/wireless/marvell/mwifiex/main.c > @@ -521,7 +521,6 @@ static void mwifiex_fw_dpc(const struct firmware *firmware, void *context) > struct mwifiex_private *priv; > struct mwifiex_adapter *adapter = context; > struct mwifiex_fw_image fw; > - struct semaphore *sem = adapter->card_sem; > bool init_failed = false; > struct wireless_dev *wdev; > > @@ -670,7 +669,8 @@ static void mwifiex_fw_dpc(const struct firmware *firmware, void *context) > } > if (init_failed) > mwifiex_free_adapter(adapter); > - up(sem); > + /* Tell all current and future waiters we're finished */ > + complete_all(adapter->fw_done); This part introduces a new use-after-free. We need to dereference adapter->fw_done *before* we free the adapter in mwifiex_free_adapter(). So you need a diff that looks something like this: Brian > return; > } > ... --- a/drivers/net/wireless/marvell/mwifiex/main.c +++ b/drivers/net/wireless/marvell/mwifiex/main.c @@ -514,6 +514,7 @@ static void mwifiex_fw_dpc(const struct firmware *firmware, void *context) struct mwifiex_fw_image fw; bool init_failed = false; struct wireless_dev *wdev; + struct completion *fw_done = adapter->fw_done; if (!firmware) { mwifiex_dbg(adapter, ERROR, @@ -654,7 +655,7 @@ done: if (init_failed) mwifiex_free_adapter(adapter); /* Tell all current and future waiters we're finished */ - complete_all(adapter->fw_done); + complete_all(fw_done); return; }