Message ID | 20230228174019.4004581-1-jjhiblot@traphandler.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:5915:0:0:0:0:0 with SMTP id v21csp3160260wrd; Tue, 28 Feb 2023 09:50:57 -0800 (PST) X-Google-Smtp-Source: AK7set+q6VHpbYE4nJYYIRubuIH79e/pMLBNldv1hmc2npmhk4kcmbBCkQCph/bJvSGbn6wwapI6 X-Received: by 2002:a17:906:9b89:b0:88e:e498:109b with SMTP id dd9-20020a1709069b8900b0088ee498109bmr4123941ejc.5.1677606657274; Tue, 28 Feb 2023 09:50:57 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1677606657; cv=none; d=google.com; s=arc-20160816; b=xON6vU/wX/GWl6P/N/0x3wdaR+Zf+QwfaWH8Xs7Lj42HzX9JHiwd9sFJ1v00viVfur dZXWCMT4tNh/ZlXdCGPRqucPI3SIMIl2yZlN8bu/blYtjybz6kq4cAZ9DRTDXeTNEO27 RdMYqaEiXnCbyOBJxnDUZaUkqlGG1W44UTNf09Xbx0aN+o8sHtal4Ml1dls6RFu/eUt9 Xq1yLhhzSEzDmmuU3jXt2v4a1bdpV0nGKEqHC+z/nFxsZx8r1hrAtBurEfwT/WHRFmH+ H5jZmAlnTUfR+A3GkRZb+Y1HSdqZo9GIHrTCoQfSuX4X6yuM+voYeDHennnAgIRVwKdO DFkQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from; bh=70u3OzbAoGJL5vTG6xLXtYWE8C28b6RdAzXVBclZ8FY=; b=Ume8kMTc9KMCpGKg0d4sTtG6MHIhyqXk6D40E7H2YByckHGCe8qCn4YBqF2OJr2ltM GYqjtlvxhjalqsSh48FT1cdUdjcsZPk1jjMpplEjddD4kSJJzf8EfI4OUyububAY4aje a73OHSXVhk3Y/GzxP0fhswwEIDfUGd5M1Rd/U1L9GnxpmVu9ALpBQJEq+YAe5M/1hIQl WW7bNpECEiPeKSwoohXSKymjGlPQkQ7gd4f9OxjqgCEOzA7vEj/2WQ4lU7e5uE+crabv CT2HZ3dqe6CD+J5rncTAH6uxo5PL8uYrFwzB5ALp3JlxYACjrGn6sLUwgHLFslMBYu8/ Kyjw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id b16-20020aa7c910000000b004a228df7d48si1328751edt.560.2023.02.28.09.50.31; Tue, 28 Feb 2023 09:50:57 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229652AbjB1Rkd (ORCPT <rfc822;brysonjbanks@gmail.com> + 99 others); Tue, 28 Feb 2023 12:40:33 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58778 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229486AbjB1Rkb (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 28 Feb 2023 12:40:31 -0500 Received: from smtpout1.mo528.mail-out.ovh.net (smtpout1.mo528.mail-out.ovh.net [46.105.34.251]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9CDD7EB48; Tue, 28 Feb 2023 09:40:30 -0800 (PST) Received: from pro2.mail.ovh.net (unknown [10.109.146.223]) by mo528.mail-out.ovh.net (Postfix) with ESMTPS id D1E9B20D60; Tue, 28 Feb 2023 17:40:28 +0000 (UTC) Received: from localhost.localdomain (88.161.25.233) by DAG1EX1.emp2.local (172.16.2.1) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.21; Tue, 28 Feb 2023 18:40:28 +0100 From: Jean-Jacques Hiblot <jjhiblot@traphandler.com> To: <robh+dt@kernel.org>, <frowand.list@gmail.com>, <saravanak@google.com> CC: <gregkh@linuxfoundation.org>, <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>, Jean Jacques Hiblot <jjhiblot@traphandler.com> Subject: [PATCH] of: property: Add missing of_node_get() in parse_interrupt() Date: Tue, 28 Feb 2023 18:40:19 +0100 Message-ID: <20230228174019.4004581-1-jjhiblot@traphandler.com> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 Content-Transfer-Encoding: 7BIT Content-Type: text/plain; charset=US-ASCII X-Originating-IP: [88.161.25.233] X-ClientProxiedBy: DAG4EX2.emp2.local (172.16.2.32) To DAG1EX1.emp2.local (172.16.2.1) X-Ovh-Tracer-Id: 4362862141621811675 X-VR-SPAMSTATE: OK X-VR-SPAMSCORE: -100 X-VR-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrgedvhedrudelfedggedtucetufdoteggodetrfdotffvucfrrhhofhhilhgvmecuqfggjfdpvefjgfevmfevgfenuceurghilhhouhhtmecuhedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmnecujfgurhephffvvefufffkofgggfgtihesthekredtredttdenucfhrhhomheplfgvrghnqdflrggtqhhuvghsucfjihgslhhothcuoehjjhhhihgslhhothesthhrrghphhgrnhgulhgvrhdrtghomheqnecuggftrfgrthhtvghrnhepjeeuhfeklefghfelhfethfegkedtvedvgfekledtheegueejuedtheekuefhffdtnecukfhppeduvdejrddtrddtrddupdekkedrudeiuddrvdehrddvfeefnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehinhgvthepuddvjedrtddrtddruddpmhgrihhlfhhrohhmpeeojhhjhhhisghlohhtsehtrhgrphhhrghnughlvghrrdgtohhmqedpnhgspghrtghpthhtohepuddprhgtphhtthhopehrohgshhdoughtsehkvghrnhgvlhdrohhrghdpfhhrohifrghnugdrlhhishhtsehgmhgrihhlrdgtohhmpdhsrghrrghvrghnrghksehgohhoghhlvgdrtghomhdpghhrvghgkhhhsehlihhnuhigfhhouhhnuggrthhiohhnrdhorhhgpdguvghvihgtvghtrhgvvgesvhhgvghrrdhkvghrnhgvlhdrohhrghdplhhinhhugidqkhgvrhhnvghlsehvghgvrhdrkhgvrhhnvghlrdhorhhgpdfovfetjfhoshhtpehmoh ehvdekpdhmohguvgepshhmthhpohhuth X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1759098078465773229?= X-GMAIL-MSGID: =?utf-8?q?1759098078465773229?= |
Series |
of: property: Add missing of_node_get() in parse_interrupt()
|
|
Commit Message
Jean-Jacques Hiblot
Feb. 28, 2023, 5:40 p.m. UTC
From: Jean Jacques Hiblot <jjhiblot@traphandler.com> As all the other parsers do, parse_interrupt() must increase the refcount of the device_node. Otherwise the refcount is decremented every time parse_interrupt() is called on this node, leading to a potential use-after-free. This is a regression introduced by commit f265f06af194 ("of: property: Fix fw_devlink handling of interrupts/interrupts-extended"). The reason is that of_irq_parse_one() does not increase the refcount while the previously used of_irq_find_parent() does. Fixes: f265f06af194 ("of: property: Fix fw_devlink handling of interrupts/interrupts-extended") Signed-off-by: Jean Jacques Hiblot <jjhiblot@traphandler.com> --- drivers/of/property.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Comments
On Tue, Feb 28, 2023 at 9:40 AM Jean-Jacques Hiblot <jjhiblot@traphandler.com> wrote: > > From: Jean Jacques Hiblot <jjhiblot@traphandler.com> > > As all the other parsers do, parse_interrupt() must increase the refcount > of the device_node. Otherwise the refcount is decremented every time > parse_interrupt() is called on this node, leading to a potential > use-after-free. > > This is a regression introduced by commit f265f06af194 ("of: property: > Fix fw_devlink handling of interrupts/interrupts-extended"). The reason is > that of_irq_parse_one() does not increase the refcount while the previously > used of_irq_find_parent() does. Thanks for catching the issue Jean! This feels like a bug in of_irq_parse_one() to me. It's returning a reference to a node without doing a of_node_get() on it. Rob, Marc, Do you agree? Jean, If they agree, can you please fix of_irq_parse_one() and add a of_node_put() to existing callers (if they aren't already doing a put()). Thanks, Saravana > > Fixes: f265f06af194 ("of: property: Fix fw_devlink handling of interrupts/interrupts-extended") > Signed-off-by: Jean Jacques Hiblot <jjhiblot@traphandler.com> > --- > drivers/of/property.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 134cfc980b70b..1f23bcb765c4e 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -1380,7 +1380,10 @@ static struct device_node *parse_interrupts(struct device_node *np, > strcmp(prop_name, "interrupts-extended")) > return NULL; > > - return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np; > + if (of_irq_parse_one(np, index, &sup_args)) > + return NULL; > + > + return of_node_get(sup_args.np); > } > > static const struct supplier_bindings of_supplier_bindings[] = { > -- > 2.25.1 >
On Tue, Feb 28, 2023 at 1:07 PM Saravana Kannan <saravanak@google.com> wrote: > > On Tue, Feb 28, 2023 at 9:40 AM Jean-Jacques Hiblot > <jjhiblot@traphandler.com> wrote: > > > > From: Jean Jacques Hiblot <jjhiblot@traphandler.com> > > > > As all the other parsers do, parse_interrupt() must increase the refcount > > of the device_node. Otherwise the refcount is decremented every time > > parse_interrupt() is called on this node, leading to a potential > > use-after-free. > > > > This is a regression introduced by commit f265f06af194 ("of: property: > > Fix fw_devlink handling of interrupts/interrupts-extended"). The reason is > > that of_irq_parse_one() does not increase the refcount while the previously > > used of_irq_find_parent() does. > > Thanks for catching the issue Jean! > > This feels like a bug in of_irq_parse_one() to me. It's returning a > reference to a node without doing a of_node_get() on it. > > Rob, Marc, Do you agree? I think you are right. If we look at the 'interrupts-extended' path, it just calls of_parse_phandle_with_args() which does a get. > Jean, > > If they agree, can you please fix of_irq_parse_one() and add a > of_node_put() to existing callers (if they aren't already doing a > put()). I think it is not that simple. The correct thing for callers may also be to hold the ref. We wouldn't want to just blindly do a put that is clearly wrong just to keep current behavior. But not having the put means we're leaking refcounts as calling the APIs originally had no side effect. For example, IIRC, of_irq_get() is called again on each deferred probe. There is no of_irq_put() because Linux IRQ numbers aren't (or weren't?) refcounted. Really, I'd like to get rid of exposing of_irq_parse_one() in the first place. Rob
On Tue, Feb 28, 2023 at 1:01 PM Rob Herring <robh+dt@kernel.org> wrote: > > On Tue, Feb 28, 2023 at 1:07 PM Saravana Kannan <saravanak@google.com> wrote: > > > > On Tue, Feb 28, 2023 at 9:40 AM Jean-Jacques Hiblot > > <jjhiblot@traphandler.com> wrote: > > > > > > From: Jean Jacques Hiblot <jjhiblot@traphandler.com> > > > > > > As all the other parsers do, parse_interrupt() must increase the refcount > > > of the device_node. Otherwise the refcount is decremented every time > > > parse_interrupt() is called on this node, leading to a potential > > > use-after-free. > > > > > > This is a regression introduced by commit f265f06af194 ("of: property: > > > Fix fw_devlink handling of interrupts/interrupts-extended"). The reason is > > > that of_irq_parse_one() does not increase the refcount while the previously > > > used of_irq_find_parent() does. > > > > Thanks for catching the issue Jean! > > > > This feels like a bug in of_irq_parse_one() to me. It's returning a > > reference to a node without doing a of_node_get() on it. > > > > Rob, Marc, Do you agree? > > I think you are right. If we look at the 'interrupts-extended' path, > it just calls of_parse_phandle_with_args() which does a get. > > > Jean, > > > > If they agree, can you please fix of_irq_parse_one() and add a > > of_node_put() to existing callers (if they aren't already doing a > > put()). > > I think it is not that simple. The correct thing for callers may also > be to hold the ref. We wouldn't want to just blindly do a put that is > clearly wrong just to keep current behavior. Right, I was just giving the approximate idea. If the caller keeps using the node pointer, they shouldn't do a put(). > But not having the put > means we're leaking refcounts as calling the APIs originally had no > side effect. For example, IIRC, of_irq_get() is called again on each > deferred probe. There is no of_irq_put() because Linux IRQ numbers > aren't (or weren't?) refcounted. Hopefully fw_devlink will avoid a lot of these deferred probes. But if it comes to wasting memory (leaking) vs use after free, we should for the short term switch to leaking. IRQ themselves can't be freed once they are registered with the IRQ framework, but I'd think the consumers can still do a get/put on an IRQ. So, at the least, we should be able to do some put() from the consumer context. > Really, I'd like to get rid of exposing of_irq_parse_one() in the first place. I don't have enough context to comment here. -Saravana
On Tue, Feb 28, 2023 at 3:58 PM Saravana Kannan <saravanak@google.com> wrote: > > On Tue, Feb 28, 2023 at 1:01 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > On Tue, Feb 28, 2023 at 1:07 PM Saravana Kannan <saravanak@google.com> wrote: > > > > > > On Tue, Feb 28, 2023 at 9:40 AM Jean-Jacques Hiblot > > > <jjhiblot@traphandler.com> wrote: > > > > > > > > From: Jean Jacques Hiblot <jjhiblot@traphandler.com> > > > > > > > > As all the other parsers do, parse_interrupt() must increase the refcount > > > > of the device_node. Otherwise the refcount is decremented every time > > > > parse_interrupt() is called on this node, leading to a potential > > > > use-after-free. > > > > > > > > This is a regression introduced by commit f265f06af194 ("of: property: > > > > Fix fw_devlink handling of interrupts/interrupts-extended"). The reason is > > > > that of_irq_parse_one() does not increase the refcount while the previously > > > > used of_irq_find_parent() does. > > > > > > Thanks for catching the issue Jean! > > > > > > This feels like a bug in of_irq_parse_one() to me. It's returning a > > > reference to a node without doing a of_node_get() on it. > > > > > > Rob, Marc, Do you agree? > > > > I think you are right. If we look at the 'interrupts-extended' path, > > it just calls of_parse_phandle_with_args() which does a get. > > > > > Jean, > > > > > > If they agree, can you please fix of_irq_parse_one() and add a > > > of_node_put() to existing callers (if they aren't already doing a > > > put()). > > > > I think it is not that simple. The correct thing for callers may also > > be to hold the ref. We wouldn't want to just blindly do a put that is > > clearly wrong just to keep current behavior. > > Right, I was just giving the approximate idea. If the caller keeps > using the node pointer, they shouldn't do a put(). > > > But not having the put > > means we're leaking refcounts as calling the APIs originally had no > > side effect. For example, IIRC, of_irq_get() is called again on each > > deferred probe. There is no of_irq_put() because Linux IRQ numbers > > aren't (or weren't?) refcounted. > > Hopefully fw_devlink will avoid a lot of these deferred probes. But if > it comes to wasting memory (leaking) vs use after free, we should for > the short term switch to leaking. A refcount overflow can cause a use after free too, but I guess the underlying kref protects against that now. The issue is we have what was a non-refcounted API and we've halfway bolted on refcounting for what's 99% static anyways. I really wish we were only worrying about refcounts for the 1% of the cases that matter. > IRQ themselves can't be freed once they are registered with the IRQ > framework, but I'd think the consumers can still do a get/put on an > IRQ. So, at the least, we should be able to do some put() from the > consumer context. > > > Really, I'd like to get rid of exposing of_irq_parse_one() in the first place. > > I don't have enough context to comment here. Looking at the ~10 users, they are mostly cases wanting to get their hwirq number. That then puts knowledge of the parent interrupt cell format into the client which isn't great. There's also one case (regulator-quirk-rcar-gen2.c) looking for shared interrupts. Probably better ways to do both of those... Rob
On 28/02/2023 20:07, Saravana Kannan wrote: > On Tue, Feb 28, 2023 at 9:40 AM Jean-Jacques Hiblot > <jjhiblot@traphandler.com> wrote: >> From: Jean Jacques Hiblot <jjhiblot@traphandler.com> >> >> As all the other parsers do, parse_interrupt() must increase the refcount >> of the device_node. Otherwise the refcount is decremented every time >> parse_interrupt() is called on this node, leading to a potential >> use-after-free. >> >> This is a regression introduced by commit f265f06af194 ("of: property: >> Fix fw_devlink handling of interrupts/interrupts-extended"). The reason is >> that of_irq_parse_one() does not increase the refcount while the previously >> used of_irq_find_parent() does. > Thanks for catching the issue Jean! > > This feels like a bug in of_irq_parse_one() to me. It's returning a > reference to a node without doing a of_node_get() on it. > > Rob, Marc, Do you agree? Sarvana, it looks like you're right. The bug seems to be in of_irq_parse_one(). It doesn't behave in the same way for "interrupts-extended" where it does a get() and 'interrupts" where it doesn't. So please ignore this patch. Thanks > > Jean, > > If they agree, can you please fix of_irq_parse_one() and add a > of_node_put() to existing callers (if they aren't already doing a > put()). > > Thanks, > Saravana > >> Fixes: f265f06af194 ("of: property: Fix fw_devlink handling of interrupts/interrupts-extended") >> Signed-off-by: Jean Jacques Hiblot <jjhiblot@traphandler.com> >> --- >> drivers/of/property.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/of/property.c b/drivers/of/property.c >> index 134cfc980b70b..1f23bcb765c4e 100644 >> --- a/drivers/of/property.c >> +++ b/drivers/of/property.c >> @@ -1380,7 +1380,10 @@ static struct device_node *parse_interrupts(struct device_node *np, >> strcmp(prop_name, "interrupts-extended")) >> return NULL; >> >> - return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np; >> + if (of_irq_parse_one(np, index, &sup_args)) >> + return NULL; >> + >> + return of_node_get(sup_args.np); >> } >> >> static const struct supplier_bindings of_supplier_bindings[] = { >> -- >> 2.25.1 >>
diff --git a/drivers/of/property.c b/drivers/of/property.c index 134cfc980b70b..1f23bcb765c4e 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -1380,7 +1380,10 @@ static struct device_node *parse_interrupts(struct device_node *np, strcmp(prop_name, "interrupts-extended")) return NULL; - return of_irq_parse_one(np, index, &sup_args) ? NULL : sup_args.np; + if (of_irq_parse_one(np, index, &sup_args)) + return NULL; + + return of_node_get(sup_args.np); } static const struct supplier_bindings of_supplier_bindings[] = {