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]

Date:   Tue, 14 Dec 2021 10:17:08 +0100 (CET)
From:   Miroslav Benes <mbenes@...e.cz>
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, joe.lawrence@...hat.com,
        corbet@....net, yhs@...com, songliubraving@...com,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure
 path

On Tue, 14 Dec 2021, 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?

It would help to share warning outputs (or whatever) from DEBUG_KOBJECTS.
 
> 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.
>  */
> 
> I believe that this comment is misleading. IMHO, kobject_init() allows
> to call kobject_put(). 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.

I think that this might be, once again, a false positive. We use kobjects 
differently than what the kobject implementation and its documentation 
assume. So not doing anything after kobject_init() and kobject_add() in 
_init_early stages could be perfectly fine. DEBUG_KOBJECTS output would be 
really welcome.

And if it is not a false positive, we should implement some rollback for 
processed klp_funcs and klp_objects if an error happens. It is not only 
klp_patch kobject affected.

Regards
Miroslav