linux-kernel - Re: [PATCH] livepatch: Fix leak on klp_init_patch

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: <Ybi9yeEnKqq7HtS5@kroah.com>
Date:   Tue, 14 Dec 2021 16:52:41 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Petr Mladek <pmladek@...e.com>
Cc:     David Vernet <void@...ifault.com>, linux-doc@...r.kernel.org,
        live-patching@...r.kernel.org, linux-kernel@...r.kernel.org,
        jpoimboe@...hat.com, jikos@...nel.org, mbenes@...e.cz,
        joe.lawrence@...hat.com, corbet@....net, yhs@...com,
        songliubraving@...com
Subject: Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

Ah, found the thread...

On Tue, Dec 14, 2021 at 09:45:53AM +0100, Petr Mladek wrote:
> On Mon 2021-12-13 11:17:35, David Vernet wrote:
> > When enabling a KLP patch with `klp_enable_patch`, we invoke
> > `klp_init_patch_early` to initialize the kobjects for the patch itself, as
> > well as the `struct klp_object*`'s and `struct klp_func*`'s that comprise
> > it. However, there are some paths where we may fail to do an
> > early-initialization of an object or its functions if certain conditions
> > are not met, such as an object having a `NULL` funcs pointer. In these
> > paths, we may currently leak the `struct klp_patch*`'s kobject, as well as
> > any of its objects or functions, as we don't free the patch in
> > `klp_enable_patch` if `klp_init_patch_early` returns an error code.
> 
> Could you please explain what exactly are we leaking?
> 
> I do not see anything allocated in klp_init_*_early() functions.
> Also I do not see anything allocated in kobject_init().
> 
> Documentation/core-api/kobject.rst says that kobject_put() must be
> used after calling kobject_add():
> 
>    "Once you registered your kobject via kobject_add(), you must never use
>     kfree() to free it directly. The only safe way is to use kobject_put(). It
>     is good practice to always use kobject_put() after kobject_init() to avoid
>     errors creeping in."
> 
> 
> Hmm, the comment in lib/kobject.c says something else:
> 
> /**
>  * kobject_init() - Initialize a kobject structure.
>  * @kobj: pointer to the kobject to initialize
>  * @ktype: pointer to the ktype for this kobject.
>  *
>  * This function will properly initialize a kobject such that it can then
>  * be passed to the kobject_add() call.
>  *
>  * After this function is called, the kobject MUST be cleaned up by a call
>  * to kobject_put(), not by a call to kfree directly to ensure that all of
>  * the memory is cleaned up properly.
>  */

These say the same thing as "good practice" == "MUST" here.  You can NOT
call kfree after calling kobject_init().  Bad things will happen if you
try to do so.

> I believe that this comment is misleading. IMHO, kobject_init() allows
> to call kobject_put().

You are FORCED TO call kobject_put() after kobject_init() is called.
Anything else is a bug.

> And it might be used to free memory that has
> already been allocated when initializing the structure where this
> kobject is bundled. But simple free() is perfectly fine when nothing
> else was allocated.

Nope, sorry, you have to call kobject_put().

thanks,

greg k-h