lkml.org 
[lkml]   [2021]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@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

\
 
 \ /
  Last update: 2021-12-15 00:52    [W:0.075 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site