linux-kernel - Re: [PATCH] firmware: psci: Fix refcount leak in psci_dt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z9qgmTbeLh0cCULw@bogus>
Date: Wed, 19 Mar 2025 10:46:49 +0000
From: Sudeep Holla <sudeep.holla@....com>
To: Miaoqian Lin <linmq006@...il.com>, Gavin Shan <gshan@...hat.com>
Cc: Mark Rutland <mark.rutland@....com>,
	Sudeep Holla <sudeep.holla@....com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Will Deacon <will@...nel.org>, Hanjun Guo <guohanjun@...wei.com>,
	Catalin Marinas <catalin.marinas@....com>,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] firmware: psci: Fix refcount leak in psci_dt_init

On Wed, Mar 19, 2025 at 08:28:38PM +1000, Gavin Shan wrote:
> Hi Miaoqian,
> 
> On 3/19/25 1:17 AM, Miaoqian Lin wrote:
> > Fix a reference counter leak in psci_dt_init() where of_node_put(np) was
> > missing after of_find_matching_node_and_match() when np is unavailable.
> > 
> > Fixes: bff60792f994 ("arm64: psci: factor invocation code to drivers")
> > Signed-off-by: Miaoqian Lin <linmq006@...il.com>
> > ---
> >   drivers/firmware/psci/psci.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> 
> I'm wandering if the fix tag is correct enough because !of_device_is_available(np)
> wasn't added by bff60792f994.
> 
> > diff --git a/drivers/firmware/psci/psci.c b/drivers/firmware/psci/psci.c
> > index a1ebbe9b73b1..38ca190d4a22 100644
> > --- a/drivers/firmware/psci/psci.c
> > +++ b/drivers/firmware/psci/psci.c
> > @@ -804,8 +804,10 @@ int __init psci_dt_init(void)
> >   	np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
> > -	if (!np || !of_device_is_available(np))
> > +	if (!np || !of_device_is_available(np)) {
> > +		of_node_put(np);
> >   		return -ENODEV;
> > +	}
> 
> The fix looks good to me. The duplicated of_node_put() can be avoided with
> a 'out' tag added, something like below.
> 
> 	if (!np || !of_device_is_available(np)) {
> 		ret = -ENODEV;
> 		goto out;
> 	}
> 
> 	:
> 
> out:
> 	of_node_put(np);
> 	return ret;
> 
> >   	init_fn = (psci_initcall_t)matched_np->data;
> >   	ret = init_fn(np);
> 

Any reason why we can't move to the new scoped usage like below?

-- 
Regards,
Sudeep

-->8

diff --git i/drivers/firmware/psci/psci.c w/drivers/firmware/psci/psci.c
index a1ebbe9b73b1..a4269078b2a2 100644
--- i/drivers/firmware/psci/psci.c
+++ w/drivers/firmware/psci/psci.c
@@ -797,12 +797,11 @@ static const struct of_device_id psci_of_match[] __initconst = {

 int __init psci_dt_init(void)
 {
-       struct device_node *np;
        const struct of_device_id *matched_np;
        psci_initcall_t init_fn;
        int ret;
-
-       np = of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);
+       struct device_node *np __free(device_node) =
+               of_find_matching_node_and_match(NULL, psci_of_match, &matched_np);

        if (!np || !of_device_is_available(np))
                return -ENODEV;
@@ -810,7 +809,6 @@ int __init psci_dt_init(void)
        init_fn = (psci_initcall_t)matched_np->data;
        ret = init_fn(np);

-       of_node_put(np);
        return ret;
 }