From patchwork Sat Jul 2 17:35:03 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Danny van Heumen X-Patchwork-Id: 587087 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2FF6AC433EF for ; Sat, 2 Jul 2022 17:35:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232257AbiGBRfQ (ORCPT ); Sat, 2 Jul 2022 13:35:16 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:41644 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231520AbiGBRfQ (ORCPT ); Sat, 2 Jul 2022 13:35:16 -0400 Received: from mail-4022.proton.ch (mail-4022.proton.ch [185.70.40.22]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 955FADFB8 for ; Sat, 2 Jul 2022 10:35:12 -0700 (PDT) Date: Sat, 02 Jul 2022 17:35:03 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=dannyvanheumen.nl; s=protonmail2; t=1656783309; x=1657042509; bh=iILpwwk1OP0q6tgIJOYEDphJskRvDbtg4pkBqD471MQ=; h=Date:To:From:Cc:Reply-To:Subject:Message-ID:Feedback-ID:From:To: Cc:Date:Subject:Reply-To:Feedback-ID:Message-ID; b=RC86Mz3D1BeW22m6igcOeaz9Nt8ooH5ipU+EUgC/IUD637MJVtNx+CqvvcXIaoQp8 SC19quCJMBOw5ynd5R4zVk6/l6bTkQSVKpWNrbHEsook73arf/bROzVivuRVGb14dt dnOt0+QKqdqPBBsRONM4ZJcUOQR/66yS4IHSrtkBOTEEXyfrn5u0ILF5E32sD3b0R6 Cg2XkteEBky7bGFjoNTNmODDxhLihn7HNwRhbtg0ZmfdVBEhls3510cCfGVsRc5AQb X2Mg8xE83dN8TQmpRwIW9BbgO+w47/G5cWcZtboNzu29dRz6K3DWyeuEDOkEkPBTmp yzfFXlSyJDdQA== To: "linux-wireless@vger.kernel.org" From: Danny van Heumen Cc: Arend van Spriel , Franky Lin , Hante Meuleman , "ulf.hansson@linaro.org" Reply-To: Danny van Heumen Subject: [PATCH v4] brcmfmac: prevent double-free on hardware-reset Message-ID: Feedback-ID: 15073070:user:proton MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org In case of buggy firmware, brcmfmac may perform a hardware reset. If during reset and subsequent probing an early failure occurs, a memory region is accidentally double-freed. With hardened memory allocation enabled, this error will be detected. - return early where appropriate to skip unnecessary clean-up. - set '.freezer' pointer to NULL to prevent double-freeing under possible other circumstances and to re-align result under various different behaviors of memory allocation freeing. - correctly claim host on func1 for disabling func2. - after reset, do not initiate probing immediately, but rely on events. Given a firmware crash, function 'brcmf_sdio_bus_reset' is called. It calls 'brcmf_sdiod_remove', then follows up with 'brcmf_sdiod_probe' to reinitialize the hardware. If 'brcmf_sdiod_probe' fails to "set F1 blocksize", it exits early, which includes calling 'brcmf_sdiod_remove'. In both cases 'brcmf_sdiod_freezer_detach' is called to free allocated '.freezer', which has not yet been re-allocated the second time. Stacktrace of (failing) hardware reset after firmware-crash: Code: b9402b82 8b0202c0 eb1a02df 54000041 (d4210000) ret_from_fork+0x10/0x20 kthread+0x154/0x160 worker_thread+0x188/0x504 process_one_work+0x1f4/0x490 brcmf_core_bus_reset+0x34/0x44 [brcmfmac] brcmf_sdio_bus_reset+0x68/0xc0 [brcmfmac] brcmf_sdiod_probe+0x170/0x21c [brcmfmac] brcmf_sdiod_remove+0x48/0xc0 [brcmfmac] kfree+0x210/0x220 __slab_free+0x58/0x40c Call trace: x2 : 0000000000000040 x1 : fffffc00002d2b80 x0 : ffff00000b4aee40 x5 : ffff8000013fa728 x4 : 0000000000000001 x3 : ffff00000b4aee00 x8 : ffff800009967ce0 x7 : ffff8000099bfce0 x6 : 00000006f8005d01 x11: ffff8000099bfce0 x10: 00000000fffff000 x9 : ffff8000083401d0 x14: 0000000000000000 x13: 657a69736b636f6c x12: 6220314620746573 x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000030 x20: fffffc00002d2ba0 x19: fffffc00002d2b80 x18: 0000000000000000 x23: ffff00000b4aee00 x22: ffff00000b4aee00 x21: 0000000000000001 x26: ffff00000b4aee00 x25: ffff0000f7753705 x24: 000000000001288a x29: ffff80000a22bbf0 x28: ffff000000401200 x27: 000000008020001a sp : ffff80000a22bbf0 lr : kfree+0x210/0x220 pc : __slab_free+0x58/0x40c pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) Workqueue: events brcmf_core_bus_reset [brcmfmac] Hardware name: Pine64 Pinebook Pro (DT) CPU: 2 PID: 639 Comm: kworker/2:2 Tainted: G C 5.16.0-0.bpo.4-arm64 #1 Debian 5.16.12-1~bpo11+1 nvmem_rockchip_efuse industrialio_triggered_buffer videodev snd_soc_core snd_pcm_dmaengine kfifo_buf snd_pcm io_domain mc industrialio mt> Modules linked in: snd_seq_dummy snd_hrtimer snd_seq snd_seq_device nft_fib_inet nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reje> Internal error: Oops - BUG: 0 [#1] SMP kernel BUG at mm/slub.c:379! Signed-off-by: Danny van Heumen --- .../broadcom/brcm80211/brcmfmac/bcmsdh.c | 31 ++++++++++++------- .../broadcom/brcm80211/brcmfmac/sdio.c | 10 +----- 2 files changed, 21 insertions(+), 20 deletions(-) -- 2.34.1 diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c index ac02244a6fdf..dd634edaa0b3 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c @@ -802,6 +802,7 @@ static void brcmf_sdiod_freezer_detach(struct brcmf_sdio_dev *sdiodev) if (sdiodev->freezer) { WARN_ON(atomic_read(&sdiodev->freezer->freezing)); kfree(sdiodev->freezer); + sdiodev->freezer = NULL; } } @@ -875,13 +876,9 @@ int brcmf_sdiod_remove(struct brcmf_sdio_dev *sdiodev) brcmf_sdiod_freezer_detach(sdiodev); - /* Disable Function 2 */ - sdio_claim_host(sdiodev->func2); - sdio_disable_func(sdiodev->func2); - sdio_release_host(sdiodev->func2); - - /* Disable Function 1 */ + /* Disable functions 2 then 1. */ sdio_claim_host(sdiodev->func1); + sdio_disable_func(sdiodev->func2); sdio_disable_func(sdiodev->func1); sdio_release_host(sdiodev->func1); @@ -911,7 +908,7 @@ int brcmf_sdiod_probe(struct brcmf_sdio_dev *sdiodev) if (ret) { brcmf_err("Failed to set F1 blocksize\n"); sdio_release_host(sdiodev->func1); - goto out; + return ret; } switch (sdiodev->func2->device) { case SDIO_DEVICE_ID_BROADCOM_CYPRESS_4373: @@ -933,7 +930,7 @@ int brcmf_sdiod_probe(struct brcmf_sdio_dev *sdiodev) if (ret) { brcmf_err("Failed to set F2 blocksize\n"); sdio_release_host(sdiodev->func1); - goto out; + return ret; } else { brcmf_dbg(SDIO, "set F2 blocksize to %d\n", f2_blksz); } @@ -1096,12 +1093,24 @@ static void brcmf_ops_sdio_remove(struct sdio_func *func) if (bus_if) { sdiodev = bus_if->bus_priv.sdio; + if (func->num != 1) { + /* Satisfy kernel expectation that the irq is released once the + * '.remove' callback has executed, while respecting the design + * that removal is executed for 'sdiodev', instead of individual + * function. + */ + brcmf_dbg(SDIO, "Only release irq for function %d", func->num); + sdio_claim_host(sdiodev->func1); + sdio_release_irq(func); + sdio_release_host(sdiodev->func1); + return; + } + + /* func 1: so do full clean-up and removal */ + /* start by unregistering irqs */ brcmf_sdiod_intr_unregister(sdiodev); - if (func->num != 1) - return; - /* only proceed with rest of cleanup if func 1 */ brcmf_sdiod_remove(sdiodev); diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c index 212fbbe1cd7e..2ed70f809097 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c @@ -4152,7 +4152,6 @@ int brcmf_sdio_get_fwname(struct device *dev, const char *ext, u8 *fw_name) static int brcmf_sdio_bus_reset(struct device *dev) { - int ret = 0; struct brcmf_bus *bus_if = dev_get_drvdata(dev); struct brcmf_sdio_dev *sdiodev = bus_if->bus_priv.sdio; @@ -4169,14 +4168,7 @@ static int brcmf_sdio_bus_reset(struct device *dev) sdio_release_host(sdiodev->func1); brcmf_bus_change_state(sdiodev->bus_if, BRCMF_BUS_DOWN); - - ret = brcmf_sdiod_probe(sdiodev); - if (ret) { - brcmf_err("Failed to probe after sdio device reset: ret %d\n", - ret); - } - - return ret; + return 0; } static const struct brcmf_bus_ops brcmf_sdio_bus_ops = {