Date:   Tue, 14 Dec 2021 15:51:28 -0800
From:   Josh Poimboeuf <jpoimboe@redhat.com>
To:     David Vernet <void@manifault.com>
Cc:     pmladek@suse.com, live-patching@vger.kernel.org,
        linux-kernel@vger.kernel.org, jikos@kernel.org, mbenes@suse.cz,
        joe.lawrence@redhat.com, corbet@lwn.net, songliubraving@fb.com,
        gregkh@linuxfoundation.org
Subject: Re: [PATCH v2] livepatch: Fix leak on klp_init_patch_early failure
 path
Message-ID: <20211214235128.ckaozqsvcr6iqcnu@treble>
References: <20211214220124.2911264-1-void@manifault.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20211214220124.2911264-1-void@manifault.com>
Precedence: bulk

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@manifault.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