From patchwork Tue Oct 13 13:31:09 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Cristian Marussi X-Patchwork-Id: 11835679 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 43FC76F01 for ; Tue, 13 Oct 2020 18:11:51 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 15718235FF for ; Tue, 13 Oct 2020 13:33:13 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="LCcngbiF" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 15718235FF Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Transfer-Encoding: Content-Type:MIME-Version:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:Message-Id:Date:Subject:To:From:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:In-Reply-To:References:List-Owner; bh=4o3k2kZbNHinPrL0KkylSGJAdxvqE3pDRx8TTRvd1jA=; b=LCcngbiF5Zvx2VMGIi4T43U3ld L47DN/iz1IN8eyJjSYo/7AvFdR7GomUPog6d5kNoscHf9iSA0+lVBiwYITUVrVo6ByLPMT42pKNlx fL4WJ+WuwH27yAvsSb83kr/w7mYaz/TE3v22K3w48qVBVVlhlPdA3Hu8idqK0tmPw1SqmuabrzInJ BEqAkuPF+skywWaRFLg7sb0NOvqijSGMiTH9XwHAvPVKHrnoATRn0lVDUxl9mcNAoGYd5GcpP6ZeK Xq6iGl/w89jZMGiB+S0v+XUVTF3avGeYzFtEQJitCdDwJwXXfRE83QEP2sbF/t9+7ZAT3gxggdOhC QhdLkurg==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kSKOk-00059E-6b; Tue, 13 Oct 2020 13:31:26 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kSKOh-00057w-9C for linux-arm-kernel@lists.infradead.org; Tue, 13 Oct 2020 13:31:24 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 2213C1FB; Tue, 13 Oct 2020 06:31:19 -0700 (PDT) Received: from e120937-lin.home (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 68C123F719; Tue, 13 Oct 2020 06:31:18 -0700 (PDT) From: Cristian Marussi To: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: [PATCH] firmware: arm_scmi: fix notifications locking Date: Tue, 13 Oct 2020 14:31:09 +0100 Message-Id: <20201013133109.49821-1-cristian.marussi@arm.com> X-Mailer: git-send-email 2.17.1 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201013_093123_429345_CCDE5D1E X-CRM114-Status: GOOD ( 16.43 ) X-Spam-Score: -2.3 (--) X-Spam-Report: SpamAssassin version 3.4.4 on merlin.infradead.org summary: Content analysis details: (-2.3 points) pts rule name description ---- ---------------------- -------------------------------------------------- -2.3 RCVD_IN_DNSWL_MED RBL: Sender listed at https://www.dnswl.org/, medium trust [217.140.110.172 listed in list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: cristian.marussi@arm.com, sudeep.holla@arm.com MIME-Version: 1.0 Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org When a protocol registers its events the notification core takes care to re-scan the hashtable of pending event handlers and activate all the possibly existent handlers that refer to any of the events just registered by the new protocol; when a pending handler becomes active the core takes also care to ask the SCMI platform to enable the corresponding events' notifications in the SCMI firmware. If, for whatever reason, the enable fails such invalid event handler must be finally removed and freed but it must be treated as an active handler like it has just become: ensure to use the scmi_put_active_handler() helper which handles properly the needed additional mutexing. Failing to properly acquire all the needed mutexes exposes a race that leads to the following splat being observed: [ 212.840876] ------------[ cut here ]------------ [ 212.845569] refcount_t: underflow; use-after-free. [ 212.850544] WARNING: CPU: 0 PID: 388 at lib/refcount.c:28 refcount_warn_saturate+0xf8/0x148 [ 212.858913] Modules linked in: dummy_scmi_consumer(-) scmi_perf [last unloaded: scmi_cpufreq] [ 212.867478] CPU: 0 PID: 388 Comm: rmmod Tainted: G W 5.9.0-rc1-00020-g9c4395e7867d-dirty #4 [ 212.877153] Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform, BIOS EDK II Jun 30 2020 [ 212.887963] pstate: 40000005 (nZcv daif -PAN -UAO BTYPE=--) [ 212.893554] pc : refcount_warn_saturate+0xf8/0x148 [ 212.898361] lr : refcount_warn_saturate+0xf8/0x148 [ 212.903160] sp : ffff80001298bc00 [ 212.906480] x29: ffff80001298bc00 x28: ffff00097470aac0 [ 212.911807] x27: 0000000000000000 x26: 0000000000000000 [ 212.917135] x25: ffff00097357fc80 x24: 0000000000000002 [ 212.922462] x23: ffff00097357fca8 x22: 0000000000000003 [ 212.927790] x21: ffff000974474688 x20: ffff000971e04f00 [ 212.933117] x19: 0000000000000001 x18: 0000000000000010 [ 212.938444] x17: 0000000000000000 x16: 0000000000000000 [ 212.943771] x15: ffffffffffffffff x14: ffff800012269948 [ 212.949098] x13: ffff80009298b947 x12: ffff80001298b94f [ 212.954426] x11: 0001000000000000 x10: 0000000000000a10 [ 212.959753] x9 : ffff80001039fe88 x8 : ffff00097470b530 [ 212.965080] x7 : 00000000ffffffff x6 : ffff00097ef1b200 [ 212.970407] x5 : ffff00097ef1b200 x4 : 0000000000000000 [ 212.975734] x3 : ffff00097ef2a398 x2 : 0000000000000001 [ 212.981060] x1 : 4b16c3af5d721600 x0 : 0000000000000000 [ 212.986388] Call trace: [ 212.988847] refcount_warn_saturate+0xf8/0x148 [ 212.993308] scmi_put_handler_unlocked.isra.10+0x204/0x208 [ 212.998811] scmi_put_handler+0x50/0xa0 [ 213.002659] scmi_unregister_notifier+0x1bc/0x240 [ 213.007385] scmi_notify_tester_remove+0x4c/0x68 [dummy_scmi_consumer] [ 213.013931] scmi_dev_remove+0x54/0x68 [ 213.017695] device_release_driver_internal+0x114/0x1e8 [ 213.022934] driver_detach+0x58/0xe8 [ 213.026520] bus_remove_driver+0x88/0xe0 [ 213.030454] driver_unregister+0x38/0x68 [ 213.034389] scmi_driver_unregister+0x1c/0x28 [ 213.038763] scmi_drv_exit+0x1c/0xae0 [dummy_scmi_consumer] [ 213.044354] __arm64_sys_delete_module+0x1a4/0x268 [ 213.049160] el0_svc_common.constprop.3+0x94/0x178 [ 213.053963] do_el0_svc+0x2c/0x98 [ 213.057290] el0_sync_handler+0x148/0x1a8 [ 213.061310] el0_sync+0x158/0x180 [ 213.064632] ---[ end trace 5bff2d25d5820911 ]--- Fixes: e7c215f358a35 ("firmware: arm_scmi: Add notification callbacks-registration") Signed-off-by: Cristian Marussi --- Applied and tested on [1] on top of : commit: 9724722fde8f ("firmware: arm_scmi: Add missing Rx size re-initialisation") [1]:https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/log/?h=for-next/scmi --- --- drivers/firmware/arm_scmi/notify.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c index 2754f9d01636..c24e427dce0d 100644 --- a/drivers/firmware/arm_scmi/notify.c +++ b/drivers/firmware/arm_scmi/notify.c @@ -1403,15 +1403,21 @@ static void scmi_protocols_late_init(struct work_struct *work) "finalized PENDING handler - key:%X\n", hndl->key); ret = scmi_event_handler_enable_events(hndl); + if (ret) { + dev_dbg(ni->handle->dev, + "purging INVALID handler - key:%X\n", + hndl->key); + scmi_put_active_handler(ni, hndl); + } } else { ret = scmi_valid_pending_handler(ni, hndl); - } - if (ret) { - dev_dbg(ni->handle->dev, - "purging PENDING handler - key:%X\n", - hndl->key); - /* this hndl can be only a pending one */ - scmi_put_handler_unlocked(ni, hndl); + if (ret) { + dev_dbg(ni->handle->dev, + "purging PENDING handler - key:%X\n", + hndl->key); + /* this hndl can be only a pending one */ + scmi_put_handler_unlocked(ni, hndl); + } } } mutex_unlock(&ni->pending_mtx);