linux-kernel - Re: [PATCH] irqchip/gic-v3: fix resource leak in partition_domain

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] [day] [month] [year] [list]

Message-ID: <86pldt9600.wl-maz@kernel.org>
Date: Mon, 21 Jul 2025 11:45:51 +0100
From: Marc Zyngier <maz@...nel.org>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: jackysliu <1972843537@...com>,
	<tglx@...utronix.de>,
	<herve.codina@...tlin.com>,
	<antonio.borneo@...s.st.com>,
	<anup@...infault.org>,
	<jirislaby@...nel.org>,
	<linux-arm-kernel@...ts.infradead.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] irqchip/gic-v3: fix resource leak in partition_domain_translate()

On Mon, 21 Jul 2025 11:27:18 +0100,
Jonathan Cameron <Jonathan.Cameron@...wei.com> wrote:
> 
> On Mon, 21 Jul 2025 09:25:53 +0100
> Marc Zyngier <maz@...nel.org> wrote:
> 
> > On Mon, 21 Jul 2025 08:28:04 +0100,
> > jackysliu <1972843537@...com> wrote:
> > > 
> > > There is a device node reference leak in partition_domain_translate().
> > > After the function obtains the device node np via of_find_node_by_phandle,
> > > it does not call of_node_put(np) to release the node reference
> > > in both the error path and the normal return path.
> > > This causes the node reference count to increase each time
> > > the function is called, causing a resource leak.
> > >
> > > This issue was detected by rule based static tools
> > > developed by Tencent.
> > > 
> > > Fixes: 87228532e7e9 ("irqchip: Switch to of_fwnode_handle()")
> > > 
> > > Signed-off-by: jackysliu <1972843537@...com>  
> > 
> > Drop the spurious blank line.
> > 
> > > ---
> > >  drivers/irqchip/irq-gic-v3.c | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > > index efc791c43d44..61c1d404b726 100644
> > > --- a/drivers/irqchip/irq-gic-v3.c
> > > +++ b/drivers/irqchip/irq-gic-v3.c
> > > @@ -1821,12 +1821,16 @@ static int partition_domain_translate(struct irq_domain *d,
> > >  		return -EINVAL;
> > >  
> > >  	ret = gic_irq_domain_translate(d, fwspec, &ppi_intid, type);
> > > -	if (WARN_ON_ONCE(ret))
> > > +	if (WARN_ON_ONCE(ret)) {
> > > +		of_node_put(np);
> > >  		return 0;
> > > +	}
> > >  
> > >  	ppi_idx = __gic_get_ppi_index(ppi_intid);
> > >  	ret = partition_translate_id(gic_data.ppi_descs[ppi_idx],
> > >  				     of_fwnode_handle(np));
> > > +	of_node_put(np);
> > > +
> > >  	if (ret < 0)
> > >  		return ret;
> > >    
> > 
> > Frankly, this looks awful, and we have much better ways to solve this
> > whole class of problems. Why can't the (untested) patch below do the
> > right thing, without the ugliness?
> > 
> > 	M.
> > 
> > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> > index efc791c43d441..c4839032ce8d0 100644
> > --- a/drivers/irqchip/irq-gic-v3.c
> > +++ b/drivers/irqchip/irq-gic-v3.c
> > @@ -1808,8 +1808,8 @@ static int partition_domain_translate(struct irq_domain *d,
> >  				      unsigned long *hwirq,
> >  				      unsigned int *type)
> >  {
> > +	struct device_node *np __free(device_node) = NULL;
> 
> Linus has expressed fairly strongly that he really doesn't like separation
> of the constructor and destructor.  See guidance in cleanup.h which was
> based on that and bunch of other early discussion around this stuff.
> 
> Here it's easy to solve though. Just move that declaration down to
> give something like:
> 
> 
> >  	unsigned long ppi_intid;
> > -	struct device_node *np;
> >  	unsigned int ppi_idx;
> >  	int ret;
> >  
> 	if (!gic_data.ppi_descs)
> 		return -ENOMEM;
> 
> 	struct device_node *np __free(device_node) =
> 		of_find_node_by_phandle(fwspec->param[3]);
> 
> 
> 	if (WARN_ON(!np))
> 		return -EINVAL;
> 

Yeah, something like that -- it doesn't change a thing in this
context, but I can see how that could degenerate if we added other
cleanup statements.

In any case, this is the sort of thing I want to see going forward.
Nothing like the original patch.

	M.

-- 
Without deviation from the norm, progress is not possible.