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

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

[David Vernet]

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.

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.

 kernel/livepatch/core.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 335d988bd811..98c2b0d02770 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -867,9 +867,6 @@ static int klp_init_patch_early(struct klp_patch *patch)
 	struct klp_object *obj;
 	struct klp_func *func;
 
-	if (!patch->objs)
-		return -EINVAL;
-
 	INIT_LIST_HEAD(&patch->list);
 	INIT_LIST_HEAD(&patch->obj_list);
 	kobject_init(&patch->kobj, &klp_ktype_patch);
@@ -889,9 +886,6 @@ static int klp_init_patch_early(struct klp_patch *patch)
 		}
 	}
 
-	if (!try_module_get(patch->mod))
-		return -ENODEV;
-
 	return 0;
 }
 
@@ -1025,7 +1019,7 @@ int klp_enable_patch(struct klp_patch *patch)
 {
 	int ret;
 
-	if (!patch || !patch->mod)
+	if (!patch || !patch->mod || !patch->objs)
 		return -EINVAL;
 
 	if (!is_livepatch_module(patch->mod)) {
@@ -1051,11 +1045,12 @@ int klp_enable_patch(struct klp_patch *patch)
 		return -EINVAL;
 	}
 
+	if (!try_module_get(patch->mod))
+		return -ENODEV;
+
 	ret = klp_init_patch_early(patch);
-	if (ret) {
-		mutex_unlock(&klp_mutex);
-		return ret;
-	}
+	if (ret)
+		goto err;
 
 	ret = klp_init_patch(patch);
 	if (ret)
-- 
2.30.2

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

[Josh Poimboeuf]

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

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

[Petr Mladek]

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

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

[David Vernet]

Petr Mladek <pmladek@suse.com> wrote on Wed [2021-Dec-15 11:06:15 +0100]:
> 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/

No problem, I won't send out v3 until we've finished the discussion and
have consensus. I'll assume that the discussion on whether or not there is
a leak will continue on the thread you linked to above, so I won't comment
on it here.

> 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().

Correct, as I've understood it, klp_for_each_*_safe() should only iterate
over the objects that have been added to the patch and klp_object's lists,
and thus for which kobject_init() has been invoked. So if we fail a check
on 'struct klp_object' N, then we'll only iterate over the first N - 1
objects in klp_for_each_*_safe().

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

Not sure I quite follow. We have to do NULL checks for obj->funcs at some
point, and per Josh's suggestion it seems cleaner to do it outside the
critical section, and before we actually invoke kobject_init(). Apologies
if I've misunderstood your point.

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

Ack!

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

Ack, no problem, I'll wait until we're all in alignment. Thanks, Petr and
Josh for taking a look at the patch.

Regards,
David

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

[Petr Mladek]

On Wed 2021-12-15 07:20:04, David Vernet wrote:
> Petr Mladek <pmladek@suse.com> wrote on Wed [2021-Dec-15 11:06:15 +0100]:
> > 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/
> 
> No problem, I won't send out v3 until we've finished the discussion and
> have consensus. I'll assume that the discussion on whether or not there is
> a leak will continue on the thread you linked to above, so I won't comment
> on it here.
> 
> > 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().
> 
> Correct, as I've understood it, klp_for_each_*_safe() should only iterate
> over the objects that have been added to the patch and klp_object's lists,
> and thus for which kobject_init() has been invoked. So if we fail a check
> on 'struct klp_object' N, then we'll only iterate over the first N - 1
> objects in klp_for_each_*_safe().
> 
> > We should not need the pre-early-init check when the lists include only
> > structures with initialized kobjects.
> 
> Not sure I quite follow. We have to do NULL checks for obj->funcs at some
> point, and per Josh's suggestion it seems cleaner to do it outside the
> critical section, and before we actually invoke kobject_init(). Apologies
> if I've misunderstood your point.

The original purpose of klp_init_*_early() was to allow calling
klp_free_patch_*() when klp_init_*() fails. The idea was to
initialize all fields properly so that free functions would
do the right thing.

Josh's proposal adds pre-early-init() to allow calling
klp_free_patch_*() already when klp_init_*_early() fails.
The purpose is to make sure that klp_init_*_early()
will actually never fail.

This might make things somehow complicated. Any future change
in klp_init_*_early() might require change in pre-early-init()
to catch eventual problems earlier.

Also I am not sure what to do with the existing checks
in klp_init_patch_early(). I am uneasy with removing them
and hoping that pre-early-init() did the right job.
But if we keep the checks then klp_init_patch_early() then it
might fail and the code will not be ready for this.


My proposal is to use simple trick. klp_free_patch_*() iterate
using the dynamic list (list_for_each_entry) while klp_init_*_early()
iterate using the arrays.

Now, we just need to make sure that klp_init_*_early() will only add
fully initialized structures into the dynamic list. As a result,
klp_free_patch() will see only the fully initialized structures
and could release them. It will not see that not-yet-initialized
structures but it is fine. They are not initialized and they do not
need to be freed.

In fact, I think that klp_init_*_early() functions already do
the right thing. IMHO, if you move the module_get() then you
could safely do:

int klp_enable_patch(struct klp_patch *patch)
{
[...]
	if (!try_module_get(patch->mod)) {
		mutex_unlock(&klp_mutex);
		return -ENODEV;
	}

	ret = klp_init_patch_early(patch);
	if (ret)
		goto err;


Note that it would need to get tested.

Anyway, does it make sense now?

Best Regards,
Petr

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

[Josh Poimboeuf]

On Fri, Dec 17, 2021 at 02:51:42PM +0100, Petr Mladek wrote:
> On Wed 2021-12-15 07:20:04, David Vernet wrote:
> > Petr Mladek <pmladek@suse.com> wrote on Wed [2021-Dec-15 11:06:15 +0100]:
> > > 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/
> > 
> > No problem, I won't send out v3 until we've finished the discussion and
> > have consensus. I'll assume that the discussion on whether or not there is
> > a leak will continue on the thread you linked to above, so I won't comment
> > on it here.
> > 
> > > 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().
> > 
> > Correct, as I've understood it, klp_for_each_*_safe() should only iterate
> > over the objects that have been added to the patch and klp_object's lists,
> > and thus for which kobject_init() has been invoked. So if we fail a check
> > on 'struct klp_object' N, then we'll only iterate over the first N - 1
> > objects in klp_for_each_*_safe().
> > 
> > > We should not need the pre-early-init check when the lists include only
> > > structures with initialized kobjects.
> > 
> > Not sure I quite follow. We have to do NULL checks for obj->funcs at some
> > point, and per Josh's suggestion it seems cleaner to do it outside the
> > critical section, and before we actually invoke kobject_init(). Apologies
> > if I've misunderstood your point.
> 
> The original purpose of klp_init_*_early() was to allow calling
> klp_free_patch_*() when klp_init_*() fails. The idea was to
> initialize all fields properly so that free functions would
> do the right thing.
> 
> Josh's proposal adds pre-early-init() to allow calling
> klp_free_patch_*() already when klp_init_*_early() fails.
> The purpose is to make sure that klp_init_*_early()
> will actually never fail.
> 
> This might make things somehow complicated. Any future change
> in klp_init_*_early() might require change in pre-early-init()
> to catch eventual problems earlier.

I'm not sure why that would be a problem.  If we can separate out the
things which fail from the things which don't, it simplifies things.

And if klp_init_object_early() returns void then it would make sense for
klp_init_patch_early() to also return void.

> Also I am not sure what to do with the existing checks
> in klp_init_patch_early(). I am uneasy with removing them
> and hoping that pre-early-init() did the right job.

klp_init_patch_early() already depends on some of the other checks in
klp_enable_patch() anyway.  I don't see that as much of a problem since
it only has one caller.

The benefit of moving the rest of the checks out is that it simplifies
the error handling, with no possibility of half-baked structures.

> But if we keep the checks then klp_init_patch_early() then it
> might fail and the code will not be ready for this.
> 
> 
> My proposal is to use simple trick. klp_free_patch_*() iterate
> using the dynamic list (list_for_each_entry) while klp_init_*_early()
> iterate using the arrays.
> 
> Now, we just need to make sure that klp_init_*_early() will only add
> fully initialized structures into the dynamic list. As a result,
> klp_free_patch() will see only the fully initialized structures
> and could release them. It will not see that not-yet-initialized
> structures but it is fine. They are not initialized and they do not
> need to be freed.
> 
> In fact, I think that klp_init_*_early() functions already do
> the right thing. IMHO, if you move the module_get() then you
> could safely do:
> 
> int klp_enable_patch(struct klp_patch *patch)
> {
> [...]
> 	if (!try_module_get(patch->mod)) {
> 		mutex_unlock(&klp_mutex);
> 		return -ENODEV;
> 	}
> 
> 	ret = klp_init_patch_early(patch);
> 	if (ret)
> 		goto err;
> 
> 
> Note that it would need to get tested.
> 
> Anyway, does it make sense now?

It would work, but it's too clever/non-obvious.  If we rely on tricks
then we may eventually forget about them and accidentally break
assumptions later.

-- 
Josh

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

[Petr Mladek]

On Fri 2021-12-17 13:50:42, Josh Poimboeuf wrote:
> On Fri, Dec 17, 2021 at 02:51:42PM +0100, Petr Mladek wrote:
> > On Wed 2021-12-15 07:20:04, David Vernet wrote:
> > Josh's proposal adds pre-early-init() to allow calling
> > klp_free_patch_*() already when klp_init_*_early() fails.
> > The purpose is to make sure that klp_init_*_early()
> > will actually never fail.
> > 
> > This might make things somehow complicated. Any future change
> > in klp_init_*_early() might require change in pre-early-init()
> > to catch eventual problems earlier.
> 
> I'm not sure why that would be a problem.  If we can separate out the
> things which fail from the things which don't, it simplifies things.

I think that there is no right answer. It depends on personal
preferences.


> > My proposal is to use simple trick. klp_free_patch_*() iterate
> > using the dynamic list (list_for_each_entry) while klp_init_*_early()
> > iterate using the arrays.
> > 
> > Now, we just need to make sure that klp_init_*_early() will only add
> > fully initialized structures into the dynamic list. As a result,
> > klp_free_patch() will see only the fully initialized structures
> > and could release them. It will not see that not-yet-initialized
> > structures but it is fine. They are not initialized and they do not
> > need to be freed.
> > 
> > In fact, I think that klp_init_*_early() functions already do
> > the right thing. IMHO, if you move the module_get() then you
> > could safely do:
> > 
> > int klp_enable_patch(struct klp_patch *patch)
> > {
> > [...]
> > 	if (!try_module_get(patch->mod)) {
> > 		mutex_unlock(&klp_mutex);
> > 		return -ENODEV;
> > 	}
> > 
> > 	ret = klp_init_patch_early(patch);
> > 	if (ret)
> > 		goto err;
> > 
> > 
> > Note that it would need to get tested.
> > 
> > Anyway, does it make sense now?
> 
> It would work, but it's too clever/non-obvious.  If we rely on tricks
> then we may eventually forget about them and accidentally break
> assumptions later.

It is not that super magic from my POV. But again, it is a personal
taste.

I do not want to bikeshed about it. Feel free to use the pre-early
thing.

Best Regards,
Petr