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

[Petr Mladek <pmladek@suse.com>]

On Tue 2021-12-14 15:51:28, Josh Poimboeuf wrote:
> 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 */

I see in the subject and the commit message:

   "Fix leak"
   "may be leaked"
   "to be leaked"
   "would cause leak"

But the discussion suggests that nobody sees any real leak. I would
like to make this clear in the commit message.

Well, I still believe that this is just a cargo cult. And I would prefer
to finish the discussion about it, first, see
https://lore.kernel.org/all/YbmlL0ZyfSuek9OB@alley/


> 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);

Note that klp_init_*_early() functions iterate through the arrays
using klp_for_each_*_static. While klp_free_*() functions iterate
via the lists using klp_for_each_*_safe().

> 		}
> 	}
> 
> 
> 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;
> 	}

We should not need the pre-early-init check when the lists include only
structures with initialized kobjects.

Otherwise, I like the idea to do module_get() before
klp_init_patch_early(). I was never happy with the "hidden"
side effect.

I am also fine with calling klp_free() when the early init fails
if we agreed that it is a good practice. I just do want to pretend
that it fixes a leak what nobody sees any leak.

Please, wait few days until the discussion finishes before sending v3.

Best Regards,
Petr