From mboxrd@z Thu Jan 1 00:00:00 1970 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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 46D28C433FE for ; Thu, 13 Oct 2022 11:49:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=l6LwC5Yixvpe/1FgSD3aT9cV4mUvWDNji/TcL7AHIR0=; b=wLgrqytkY3dR9b OTFgOQ+9JqLsbTrADE7XGa+97D80xuSEprFf84LblRBbdT9tAvjNOUWCw45UYsuLqzTR0yG5pYPnB RX2Rzr0JdF+BRkewpRNO+KXwHkhTzYWOFDVRmrvoXl18ZRlcMTIODnMaM141DPfUOESogJcbBJ/5h iQtH9cL5G9yjZUe1sA/7soWN/3clonz+U/3pqTyENdZLvLnQ8Frhc+pgUA4vfEDOadyFb49CBUxZw QH+QxWeMmT/lLAPLry4D9CKV3G9aKUH3v1RGtEyY4qu5dUpZk4qXLr/QRXjLDfYsbrZFyGMGQBhrm 31MEG6SbJ5FzcnPM+7pg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oiwhM-00Baw0-KI; Thu, 13 Oct 2022 11:48:24 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oiwhI-00Baul-Ul for linux-arm-kernel@lists.infradead.org; Thu, 13 Oct 2022 11:48:22 +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 A6D1115A1; Thu, 13 Oct 2022 04:48:20 -0700 (PDT) Received: from e120937-lin (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id BE96A3F792; Thu, 13 Oct 2022 04:48:13 -0700 (PDT) Date: Thu, 13 Oct 2022 12:48:03 +0100 From: Cristian Marussi To: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= Cc: Sudeep Holla , linux-arm-kernel@lists.infradead.org, kernel@pengutronix.de Subject: Re: resource leak in firmware/arm_scmi driver Message-ID: References: <20221010084545.tute3hyg6kyrdtle@pengutronix.de> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20221010084545.tute3hyg6kyrdtle@pengutronix.de> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221013_044821_120534_DF231A6E X-CRM114-Status: GOOD ( 27.98 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Mon, Oct 10, 2022 at 10:45:45AM +0200, Uwe Kleine-K=F6nig wrote: > Hello, > = Hi Uwe, thanks for reporting this. > during some janitorial cleanup I stumbled over a resource leak in > drivers/firmware/arm_scmi/driver.c. > = > The problem is as follows: > = > scmi_remove() might return early if info->users is non-zero. The driver > core however ignores the return value of scmi_remove() and removes the > device and frees the devm-allocated resources (e.g. *info). > = > So letting aside that some resources are never freed after a failed call > of scmi_remove(), the user of the scmi node will probably stumble over > accessing freed memory soon. I wouldn't be surprised if that was > exploitable. > = At first, I thought that this was only some kind of mess that needed to be cleaned up in the remove/exit path but without any real way of being exposed: this *bad* assumption of mine came from the fact that the SCMI drivers (like scmi-cpufreq scmi-hwmon) register to use the core SCMI stack services using an exported symbol (scmi_register_driver) and then, addition= ally, each SCMI driver willing to use some the core SCMI protocols issues a try_module_get (since the protocols could have been modularized) So that in a module scenario (with scmi-module being the core platform driver: root@deb-buster-arm64:~# lsmod = Module Size Used by scmi_hwmon 16384 0 scmi_module 131072 2 scmi_hwmon root@deb-buster-arm64:~# rmmod ./scmi-module.ko = rmmod: ERROR: Module scmi_module is in use by: scmi_hwmon BUT, then I realized that all of this refcounting is not going to save me from an explicitly triggered unbind: root@deb-buster-arm64:~# echo 'firmware:scmi' > /sys/bus/platform/drivers/a= rm-scmi/unbind = [11601.798671] arm-scmi firmware:scmi: remove callback returned a non-zero = value. This will be ignored. This will indeed expose the issue you reported. > I quickly tried to fix this issue, but didn't understand the driver good > enough. I think a fix would involve adding a get_device() call to > scmi_handle_get() to prevent scmi_remove() being called while a handle > exists. > = I tried to address the issue with the get_device() calls as you suggested, but it has really no effect and in fact looking at the code on the remove path there is nothing that seems to be taken into consideration to stop a removal (and this indeed is consistent with the fact that any error returned on the removal/exit path is effectively ignore= d). What instead DOES seem to work, it is to add a proper devlink with device_link_add() declaring any SCMI driver device (like from cpufreq) as a consumer of the core stack platform device: this will NOT stop the removal, BUT it does trigger an implicit unbind at first of all the SCMI drivers currently loaded when an unbind is requested for the core SCMI platform device: as a consequence the removal can now be triggered safely by an unbind even if some SCMI driver users are present (they will be unbound too) Now, for this cleanup to fully work, some (long due) rework of the SCMI core devices handling and removal is needed (and it has partially already started in other unrelated series), so my plan would be to, at first, immediately add a '.suppress_bind_attrs =3D true' to the core driver to mitigate this (which is also easily portable to stable) and then rework properly the core SCMI removeal routines to safely re-enable a proper unbind. (or not re-enabling it if deemed not worth BUT anyway reworking the core SCMI remove/exit path) I'll check my plan with Sudeep, any thoughts from you about this ? Am I missing something more ? Thanks a lot. Regards, Cristian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel