Re: [PATCH] livepatch: Fix leak on klp_init_patch_early failure path

[Petr Mladek <pmladek@suse.com>]

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.
 */

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.

Adding Greg into Cc.

Best Regards,
Petr

> For example, if we added the following object entry to the sample livepatch
> code, it would cause us to leak the vmlinux `klp_object`, and its `struct
> klp_func` which updates `cmdline_proc_show`:
> 
> ```
> static struct klp_object objs[] = {
>         {
>                 .name = "kvm",
>         }, { }
> };
> ```
> 
> Without this change, if we enable `CONFIG_DEBUG_KOBJECT` and try to `kpatch
> load livepatch-sample.ko`, we don't observe the kobjects being released
> (though of course we do observe `insmod` failing to insert the module).
> With the change, we do observe that the `kobject` for the patch and its
> `vmlinux` object are released.
> 
> Signed-off-by: David Vernet <void@manifault.com>
> ---
>  kernel/livepatch/core.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 335d988bd811..16e96836a825 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -1052,10 +1052,7 @@ int klp_enable_patch(struct klp_patch *patch)
>  	}
>  
>  	ret = klp_init_patch_early(patch);
> -	if (ret) {
> -		mutex_unlock(&klp_mutex);
> -		return ret;
> -	}
> +		goto err;
>  
>  	ret = klp_init_patch(patch);
>  	if (ret)
> -- 
> 2.30.2