linux-kernel - Re: [PATCH v2] 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 14 Dec 2021 15:51:28 -0800
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     David Vernet <void@...ifault.com>
Cc:     pmladek@...e.com, live-patching@...r.kernel.org,
        linux-kernel@...r.kernel.org, jikos@...nel.org, mbenes@...e.cz,
        joe.lawrence@...hat.com, corbet@....net, songliubraving@...com,
        gregkh@...uxfoundation.org
Subject: Re: [PATCH v2] livepatch: Fix leak on klp_init_patch_early failure
 path

On Tue, Dec 14, 2021 at 02:01:26PM -0800, David Vernet wrote:
> When enabling a klp patch with klp_enable_patch(), klp_init_patch_early()
> is invoked to initialize the kobjects for the patch itself, as well as the
> 'struct klp_object' and 'struct klp_func' objects that comprise it.
> However, there are some error paths in klp_enable_patch() where some
> kobjects may have been initialized with kobject_init(), but an error code
> is still returned due to e.g. a 'struct klp_object' having a NULL funcs
> pointer.
> 
> In these paths, the kobject of the 'struct klp_patch' may be leaked, along
> with one or more of its objects and their functions, as kobject_put() is
> not invoked on the cleanup path if klp_init_patch_early() returns an error
> code.
> 
> For example, if an object entry such as the following were added to the
> sample livepatch module's klp patch, it would cause the vmlinux klp_object,
> and its klp_func which updates 'cmdline_proc_show', to be leaked:
> 
> static struct klp_object objs[] = {
> 	{
> 		/* name being NULL means vmlinux */
> 		.funcs = funcs,
> 	},
> 	{
> 		.name = "kvm",
> 		/* NULL funcs -- would cause leak */
> 	}, { }
> };
> 
> Without this change, if CONFIG_DEBUG_KOBJECT is enabled, and the sample klp
> patch is loaded, the kobjects (the patch, the vmlinux 'struct klp_obj', and
> its func) are not observed to be released in the dmesg log output.  With
> the change, the kobjects are observed to be released.

This looks much better, thanks.

> Signed-off-by: David Vernet <void@...ifault.com>
> ---
> v2:
>   - Move try_module_get() and the patch->objs NULL check out of
>     klp_init_patch_early() to ensure that it's safe to jump to the 'err' label
>     on the error path in klp_enable_patch().
>   - Fix the patch description to not use markdown, and to use imperative
>     language.

Looking good overall.

Though, klp_init_patch_early() still has a failure mode which looks a
little sketchy:

	klp_for_each_object_static(patch, obj) {
		if (!obj->funcs)
			return -EINVAL;

		klp_init_object_early(patch, obj);

		klp_for_each_func_static(obj, func) {
			klp_init_func_early(obj, func);
		}
	}


While I don't see any actual leaks associated with it, it'd be cleaner
and more robust to move the per-object !obj->funcs check to the top of
klp_enable_patch(), with the other EINVAL checks.  Like:


int klp_enable_patch(struct klp_patch *patch)
{
	struct klp_object *obj;
	int ret;

	if (!patch || !patch->mod || !patch->objs)
		return -EINVAL;

	klp_for_each_object_static(patch, obj) {
		if (!obj->funcs)
			return -EINVAL;
	}


Then klp_init_patch_early() can be changed to return void, and we can
more easily convince ourselves there aren't any remaining leaks.

-- 
Josh