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@xxxxxxxxxxxxx>
> ---
> 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
