linux-kernel - Re: [PATCH] bus: ti-sysc: Fix potential double free in sysc_add_named_clock_from

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: <CAH-r-ZG_m++B8u6qhV-pbG5s+YUVWUrKj4far+rOF6xDuHOcgw@mail.gmail.com>
Date: Mon, 18 Aug 2025 11:23:53 +0800
From: 林妙倩 <linmq006@...il.com>
To: Andreas Kemnade <andreas@...nade.info>
Cc: Aaro Koskinen <aaro.koskinen@....fi>, Kevin Hilman <khilman@...libre.com>, 
	Roger Quadros <rogerq@...nel.org>, Tony Lindgren <tony@...mide.com>, linux-omap@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org
Subject: Re: [PATCH] bus: ti-sysc: Fix potential double free in sysc_add_named_clock_from_child()

Hi, Andreas

Thanks for your reply.

Andreas Kemnade <andreas@...nade.info> 于2025年8月11日周一 22:56写道：
>
> Hi,
>
> Am Mon,  4 Aug 2025 20:04:03 +0800
> schrieb Miaoqian Lin <linmq006@...il.com>:
>
> > The devm_get_clk_from_child() function uses device-managed resources
> > that are automatically cleaned up. The clk_put() call after
> > devm_get_clk_from_child() is redundant and
> > may lead to double-free issues.
> >
> > Fixes: a54275f4ab20 ("bus: ti-sysc: Add quirk handling for external optional functional clock")
> > Signed-off-by: Miaoqian Lin <linmq006@...il.com>
> > ---
> >  drivers/bus/ti-sysc.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c
> > index 9f624e5da991..5441b0739faa 100644
> > --- a/drivers/bus/ti-sysc.c
> > +++ b/drivers/bus/ti-sysc.c
> > @@ -362,7 +362,6 @@ static int sysc_add_named_clock_from_child(struct sysc *ddata,
> >       cl->clk = clock;
> >       clkdev_add(cl);
> >
> > -     clk_put(clock);
> >
> >       return 0;
> >  }
> I understand the double-free issue, but I have some questions to make
> sure I understand it correctly what we are doing here. So lets ask the
> possibly stupid questions and check assumptions:
>
> - clk_hw hardware still lives after clk_put(), so we do not have
>   problems normally here after that put when we do not remove the
>   device?
>

Yes, the main problem caused here is reference counting imbalance.
After the manual clk_put(clock), the devm framework calls clk_put()
again during device removal, causing issues.

> - With your patch the put is delayed, so things live longer. So why
> we do not use devm_clk_put() or avoid using devres at all here?
>

Since cl->clk = clock; passes the clock pointer to the lookup table,
I'm not sure if calling devm_clk_put() early here is appropriate.
So I just remove the clk_put(clock) and let the devm framework handle this.

If you think there's a better way, I can submit an updated patch.

> Regards,
> Andreas