linux-kernel - Re: [PATCH] clk: boston: fix memory leak of 'onecell' on error return paths

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]

Date:   Wed, 9 May 2018 17:44:40 +0100
From:   Colin Ian King <colin.king@...onical.com>
To:     Paul Burton <paul.burton@...s.com>,
        Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>, linux-mips@...ux-mips.org,
        linux-clk@...r.kernel.org, kernel-janitors@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] clk: boston: fix memory leak of 'onecell' on error return
 paths

On 09/05/18 17:33, Paul Burton wrote:
> Hi Colin & Dan,
> 
> On Wed, May 09, 2018 at 05:01:35PM +0300, Dan Carpenter wrote:
>> On Wed, May 09, 2018 at 02:40:31PM +0100, Colin King wrote:
>>> From: Colin Ian King <colin.king@...onical.com>
>>>
>>> There are several error return paths that don't free up onecell
>>> and hence we have some memory leaks. Add an error exit path that
>>> kfree's onecell to fix the leaks.
>>>
>>> Signed-off-by: Colin Ian King <colin.king@...onical.com>
>>> ---
>>>  drivers/clk/imgtec/clk-boston.c | 15 +++++++++++----
>>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/clk/imgtec/clk-boston.c b/drivers/clk/imgtec/clk-boston.c
>>> index 15af423cc0c9..d6bc468ff551 100644
>>> --- a/drivers/clk/imgtec/clk-boston.c
>>> +++ b/drivers/clk/imgtec/clk-boston.c
>>> @@ -73,27 +73,34 @@ static void __init clk_boston_setup(struct device_node *np)
>>>  	hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
>>>  	if (IS_ERR(hw)) {
>>>  		pr_err("failed to register input clock: %ld\n", PTR_ERR(hw));
>>> -		return;
>>> +		goto error;
>>
>> I hate vague label names like "error" and "out"...
>>
>> There are a bunch of other resources that we should free if we decide
>> it's worth freeing things.
> 
> Agreed - for example unregistering the clocks that we'd be discarding
> references to by freeing onecell.
> 
>> Can this even boot without the clk?
> 
> Nope. If this clock setup fails then whether you free this memory or not
> you're going to be unable to do anything useful with it.
> 
> I imagine this patch is the result of some static analysis rather than a
> problem being observed at runtime?

Indeed. I propose ignoring this then as it's fairly terminal if one
can't get memory anyhow in the early boot.

Colin
> 
> Thanks,
>     Paul
> 
>> When
>> the label names says what is freed, then you mentally only have to keep
>> track of the most recently allocated resource.  So if
>>
>> 	hw = clk_hw_register_fixed_rate(NULL, "input", NULL, 0, in_freq);
>>
>> succeeds then the next goto is going to "goto free_clk_input;".
>>
>> regards,
>> dan carpenter