linux-kernel - Re: [PATCH] clk/meson: fixes memleak issue in init err branch

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, 29 Apr 2020 19:43:55 +0200
From:   Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To:     Jerome Brunet <jbrunet@...libre.com>
Cc:     Bernard Zhao <bernard@...o.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Kevin Hilman <khilman@...libre.com>,
        linux-amlogic@...ts.infradead.org, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org, opensource.kernel@...o.com
Subject: Re: [PATCH] clk/meson: fixes memleak issue in init err branch

Hi Jerome,

On Wed, Apr 29, 2020 at 2:37 PM Jerome Brunet <jbrunet@...libre.com> wrote:
>
>
> On Wed 29 Apr 2020 at 05:14, Bernard Zhao <bernard@...o.com> wrote:
>
> > In common init function, when run into err branch, we didn`t
> > use kfree to release kzmalloc area, this may bring in memleak
>
> Thx for reporting this Bernard.
> I'm not a fan of adding kfree everywhere. I'd much prefer a label and
> clear error exit path.
>
> That being said, the allocation is probably not the only thing that
> needs to be undone in case of error. I guess this is due to conversion
> to CLK_OF_DECLARE_DRIVER() which forced to drop all the devm_
> This was done because the clock controller was required early in the
> boot sequence.
>
> There is 2 paths to properly solve this:
> 1) Old school: manually undo everything with every error exit condition
>    Doable but probably a bit messy
> 2) Convert back the driver to a real platform driver and use devm_.
>    We would still need the controller to register early but I wonder if
>    we could use the same method as drivers/clk/mediatek/clk-mt2701.c and
>    use arch_initcall() ?
>
> Martin, you did the initial conversion, what do you think of option 2 ?
I tried it with the attached patch
unfortunately my "m8b_clkc_test_probe" is still run too late

> Would it still answer the problem you were trying to solve back then ?
I'm afraid it does not:
- the resets are needed early for SMP initialization
- the clocks are needed even earlier for timer registration (we have
both, the ARM TWD timer and some Amlogic custom timer. both have clock
inputs)

> One added benefit of option 2 is we could drop CLK_OF_DECLARE_DRIVER().
> We could even do the same in for the other SoCs, which I suppose would
> avoid a fair amount of probe deferral.
it would be great, indeed
but this will only work once timer initialization and SMP boot can
happen at a later stage

If the clock controller registration fails the board won't boot. Yes,
cleaning up memory is good, but in this specific case it will add a
couple of extra CPU cycles before the kernel is dead
So, if we want to ignore that fact then I agree with your first option
(undoing things the "old school" way).


Martin

View attachment "test-dmesg.txt" of type "text/plain" (5282 bytes)

View attachment "test.patch" of type "text/x-patch" (1059 bytes)