[PATCH] klp: make object/func-walking helpers more robust

[PATCH] klp: make object/func-walking helpers more robust

[Miroslav Benes]

Current object-walking helper checks the presence of obj->funcs to
determine the end of objs array in klp_object structure. This is
somewhat fragile because one can easily forget about funcs definition
during livepatch creation. In such a case the livepatch module is
successfully loaded and all objects after the incorrect one are omitted.
This is very confusing. Let's make the helper more robust and check also
for the other external member, name. Thus the helper correctly stops on
an empty item of the array. We need to have a check for obj->funcs in
klp_init_object() to make it work.

The same applies to a func-walking helper.

As a benefit we'll check for new_func member definition during the
livepatch initialization. There is no such check anywhere in the code
now.

Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
 include/linux/livepatch.h | 6 ++++--
 kernel/livepatch/core.c   | 3 +++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 0933ca47791c..a93a0b23dc8d 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -104,10 +104,12 @@ struct klp_patch {
 };
 
 #define klp_for_each_object(patch, obj) \
-	for (obj = patch->objs; obj->funcs; obj++)
+	for (obj = patch->objs; obj->funcs || obj->name; obj++)
 
 #define klp_for_each_func(obj, func) \
-	for (func = obj->funcs; func->old_name; func++)
+	for (func = obj->funcs; \
+	     func->old_name || func->new_func || func->old_sympos; \
+	     func++)
 
 int klp_register_patch(struct klp_patch *);
 int klp_unregister_patch(struct klp_patch *);
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index a19f1954f4ac..5c2bc1052691 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -747,6 +747,9 @@ static void klp_free_patch(struct klp_patch *patch)
 
 static int klp_init_func(struct klp_object *obj, struct klp_func *func)
 {
+	if (!func->old_name || !func->new_func)
+		return -EINVAL;
+
 	INIT_LIST_HEAD(&func->stack_node);
 	func->state = KLP_DISABLED;
 
-- 
2.8.1

Re: klp: make object/func-walking helpers more robust

[Jessica Yu]

+++ Miroslav Benes [28/04/16 16:34 +0200]:
>Current object-walking helper checks the presence of obj->funcs to
>determine the end of objs array in klp_object structure. This is
>somewhat fragile because one can easily forget about funcs definition
>during livepatch creation. In such a case the livepatch module is
>successfully loaded and all objects after the incorrect one are omitted.
>This is very confusing. Let's make the helper more robust and check also
>for the other external member, name. Thus the helper correctly stops on
>an empty item of the array. We need to have a check for obj->funcs in
>klp_init_object() to make it work.
>
>The same applies to a func-walking helper.
>
>As a benefit we'll check for new_func member definition during the
>livepatch initialization. There is no such check anywhere in the code
>now.
>
>Signed-off-by: Miroslav Benes <mbenes@suse.cz>
>---
> include/linux/livepatch.h | 6 ++++--
> kernel/livepatch/core.c   | 3 +++
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
>diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>index 0933ca47791c..a93a0b23dc8d 100644
>--- a/include/linux/livepatch.h
>+++ b/include/linux/livepatch.h
>@@ -104,10 +104,12 @@ struct klp_patch {
> };
>
> #define klp_for_each_object(patch, obj) \
>-	for (obj = patch->objs; obj->funcs; obj++)
>+	for (obj = patch->objs; obj->funcs || obj->name; obj++)

Remember that for patches to vmlinux, obj->name and obj->mod will also
both be NULL. So if someone happens to forget to fill in obj->funcs
for a vmlinux patch, we won't catch that case here. Perhaps we need a
better way of determining whether we've reached the end of the array,
or determining that the struct is truly empty..

Jessica

Re: klp: make object/func-walking helpers more robust

[Josh Poimboeuf]

On Thu, Apr 28, 2016 at 02:21:31PM -0400, Jessica Yu wrote:
> +++ Miroslav Benes [28/04/16 16:34 +0200]:
> > Current object-walking helper checks the presence of obj->funcs to
> > determine the end of objs array in klp_object structure. This is
> > somewhat fragile because one can easily forget about funcs definition
> > during livepatch creation. In such a case the livepatch module is
> > successfully loaded and all objects after the incorrect one are omitted.
> > This is very confusing. Let's make the helper more robust and check also
> > for the other external member, name. Thus the helper correctly stops on
> > an empty item of the array. We need to have a check for obj->funcs in
> > klp_init_object() to make it work.
> > 
> > The same applies to a func-walking helper.
> > 
> > As a benefit we'll check for new_func member definition during the
> > livepatch initialization. There is no such check anywhere in the code
> > now.
> > 
> > Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> > ---
> > include/linux/livepatch.h | 6 ++++--
> > kernel/livepatch/core.c   | 3 +++
> > 2 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > index 0933ca47791c..a93a0b23dc8d 100644
> > --- a/include/linux/livepatch.h
> > +++ b/include/linux/livepatch.h
> > @@ -104,10 +104,12 @@ struct klp_patch {
> > };
> > 
> > #define klp_for_each_object(patch, obj) \
> > -	for (obj = patch->objs; obj->funcs; obj++)
> > +	for (obj = patch->objs; obj->funcs || obj->name; obj++)
> 
> Remember that for patches to vmlinux, obj->name and obj->mod will also
> both be NULL. So if someone happens to forget to fill in obj->funcs
> for a vmlinux patch, we won't catch that case here. Perhaps we need a
> better way of determining whether we've reached the end of the array,
> or determining that the struct is truly empty..

That would be nice, but I'm not sure how we could do that.  I suppose we
could add a patch->nr_objs field.  But that might arguably be even
easier for the user to mess up.

-- 
Josh

Re: klp: make object/func-walking helpers more robust

[Miroslav Benes]

On Thu, 28 Apr 2016, Josh Poimboeuf wrote:

> On Thu, Apr 28, 2016 at 02:21:31PM -0400, Jessica Yu wrote:
> > +++ Miroslav Benes [28/04/16 16:34 +0200]:
> > > Current object-walking helper checks the presence of obj->funcs to
> > > determine the end of objs array in klp_object structure. This is
> > > somewhat fragile because one can easily forget about funcs definition
> > > during livepatch creation. In such a case the livepatch module is
> > > successfully loaded and all objects after the incorrect one are omitted.
> > > This is very confusing. Let's make the helper more robust and check also
> > > for the other external member, name. Thus the helper correctly stops on
> > > an empty item of the array. We need to have a check for obj->funcs in
> > > klp_init_object() to make it work.
> > > 
> > > The same applies to a func-walking helper.
> > > 
> > > As a benefit we'll check for new_func member definition during the
> > > livepatch initialization. There is no such check anywhere in the code
> > > now.
> > > 
> > > Signed-off-by: Miroslav Benes <mbenes@suse.cz>
> > > ---
> > > include/linux/livepatch.h | 6 ++++--
> > > kernel/livepatch/core.c   | 3 +++
> > > 2 files changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> > > index 0933ca47791c..a93a0b23dc8d 100644
> > > --- a/include/linux/livepatch.h
> > > +++ b/include/linux/livepatch.h
> > > @@ -104,10 +104,12 @@ struct klp_patch {
> > > };
> > > 
> > > #define klp_for_each_object(patch, obj) \
> > > -	for (obj = patch->objs; obj->funcs; obj++)
> > > +	for (obj = patch->objs; obj->funcs || obj->name; obj++)
> > 
> > Remember that for patches to vmlinux, obj->name and obj->mod will also
> > both be NULL. So if someone happens to forget to fill in obj->funcs
> > for a vmlinux patch, we won't catch that case here.

Yes, that is true. My reasoning is that if someone even accidently writes 
{ } somewhere in the middle of the array, there is nothing we can do to 
help :). I consider it improbable whereas an omission of one field is 
possible.

> > Perhaps we need a
> > better way of determining whether we've reached the end of the array,
> > or determining that the struct is truly empty..
> 
> That would be nice, but I'm not sure how we could do that.  I suppose we
> could add a patch->nr_objs field.  But that might arguably be even
> easier for the user to mess up.

Yeah, that is perhaps the only way (ARRAY_SIZE won't work here) besides 
introducing some special mark. I think this is not worth it. I agree it is 
even more error-prone.

The idea behind this patch is that there is at least something we can do 
to help without imposing much on the user.

Miroslav

Re: klp: make object/func-walking helpers more robust

[Jessica Yu]

+++ Miroslav Benes [29/04/16 09:48 +0200]:
>On Thu, 28 Apr 2016, Josh Poimboeuf wrote:
>
>> On Thu, Apr 28, 2016 at 02:21:31PM -0400, Jessica Yu wrote:
>> > +++ Miroslav Benes [28/04/16 16:34 +0200]:
>> > > Current object-walking helper checks the presence of obj->funcs to
>> > > determine the end of objs array in klp_object structure. This is
>> > > somewhat fragile because one can easily forget about funcs definition
>> > > during livepatch creation. In such a case the livepatch module is
>> > > successfully loaded and all objects after the incorrect one are omitted.
>> > > This is very confusing. Let's make the helper more robust and check also
>> > > for the other external member, name. Thus the helper correctly stops on
>> > > an empty item of the array. We need to have a check for obj->funcs in
>> > > klp_init_object() to make it work.
>> > >
>> > > The same applies to a func-walking helper.
>> > >
>> > > As a benefit we'll check for new_func member definition during the
>> > > livepatch initialization. There is no such check anywhere in the code
>> > > now.
>> > >
>> > > Signed-off-by: Miroslav Benes <mbenes@suse.cz>
>> > > ---
>> > > include/linux/livepatch.h | 6 ++++--
>> > > kernel/livepatch/core.c   | 3 +++
>> > > 2 files changed, 7 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
>> > > index 0933ca47791c..a93a0b23dc8d 100644
>> > > --- a/include/linux/livepatch.h
>> > > +++ b/include/linux/livepatch.h
>> > > @@ -104,10 +104,12 @@ struct klp_patch {
>> > > };
>> > >
>> > > #define klp_for_each_object(patch, obj) \
>> > > -	for (obj = patch->objs; obj->funcs; obj++)
>> > > +	for (obj = patch->objs; obj->funcs || obj->name; obj++)
>> >
>> > Remember that for patches to vmlinux, obj->name and obj->mod will also
>> > both be NULL. So if someone happens to forget to fill in obj->funcs
>> > for a vmlinux patch, we won't catch that case here.
>
>Yes, that is true. My reasoning is that if someone even accidently writes
>{ } somewhere in the middle of the array, there is nothing we can do to
>help :). I consider it improbable whereas an omission of one field is
>possible.
>
>> > Perhaps we need a
>> > better way of determining whether we've reached the end of the array,
>> > or determining that the struct is truly empty..
>>
>> That would be nice, but I'm not sure how we could do that.  I suppose we
>> could add a patch->nr_objs field.  But that might arguably be even
>> easier for the user to mess up.
>
>Yeah, that is perhaps the only way (ARRAY_SIZE won't work here) besides
>introducing some special mark. I think this is not worth it. I agree it is
>even more error-prone.
>
>The idea behind this patch is that there is at least something we can do
>to help without imposing much on the user.

Yeah, agreed. Then no more objections from me :-)

Re: klp: make object/func-walking helpers more robust

[Jiri Kosina]

On Thu, 28 Apr 2016, Jessica Yu wrote:

> > #define klp_for_each_object(patch, obj) \
> > -	for (obj = patch->objs; obj->funcs; obj++)
> > +	for (obj = patch->objs; obj->funcs || obj->name; obj++)
> 
> Remember that for patches to vmlinux, obj->name and obj->mod will also
> both be NULL. So if someone happens to forget to fill in obj->funcs
> for a vmlinux patch, we won't catch that case here. Perhaps we need a
> better way of determining whether we've reached the end of the array,
> or determining that the struct is truly empty..

I'd rather not over-compilcate it.

Admittedly, the change in the termination condition catches most of the 
errors made by the patch author, but not all.
But there are many other places in the kernel where inserting an empty 
item into the middle of statically initialized array will make the whole 
thing explode, so let's not try to be more clever than necessary.

I plan to queue Miroslav's patch unless there are serious objections 
raised.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] klp: make object/func-walking helpers more robust

[Josh Poimboeuf]

On Thu, Apr 28, 2016 at 04:34:08PM +0200, Miroslav Benes wrote:
> Current object-walking helper checks the presence of obj->funcs to
> determine the end of objs array in klp_object structure. This is
> somewhat fragile because one can easily forget about funcs definition
> during livepatch creation. In such a case the livepatch module is
> successfully loaded and all objects after the incorrect one are omitted.
> This is very confusing. Let's make the helper more robust and check also
> for the other external member, name. Thus the helper correctly stops on
> an empty item of the array. We need to have a check for obj->funcs in
> klp_init_object() to make it work.
> 
> The same applies to a func-walking helper.
> 
> As a benefit we'll check for new_func member definition during the
> livepatch initialization. There is no such check anywhere in the code
> now.
> 
> Signed-off-by: Miroslav Benes <mbenes@suse.cz>

Acked-by: Josh Poimboeuf <jpoimboe@redhat.com>

-- 
Josh

Re: klp: make object/func-walking helpers more robust

[Jessica Yu]

+++ Miroslav Benes [28/04/16 16:34 +0200]:
>Current object-walking helper checks the presence of obj->funcs to
>determine the end of objs array in klp_object structure. This is
>somewhat fragile because one can easily forget about funcs definition
>during livepatch creation. In such a case the livepatch module is
>successfully loaded and all objects after the incorrect one are omitted.
>This is very confusing. Let's make the helper more robust and check also
>for the other external member, name. Thus the helper correctly stops on
>an empty item of the array. We need to have a check for obj->funcs in
>klp_init_object() to make it work.
>
>The same applies to a func-walking helper.
>
>As a benefit we'll check for new_func member definition during the
>livepatch initialization. There is no such check anywhere in the code
>now.
>
>Signed-off-by: Miroslav Benes <mbenes@suse.cz>

Acked-by: Jessica Yu <jeyu@redhat.com>

Re: [PATCH] klp: make object/func-walking helpers more robust

[Jiri Kosina]

On Thu, 28 Apr 2016, Miroslav Benes wrote:

> Current object-walking helper checks the presence of obj->funcs to
> determine the end of objs array in klp_object structure. This is
> somewhat fragile because one can easily forget about funcs definition
> during livepatch creation. In such a case the livepatch module is
> successfully loaded and all objects after the incorrect one are omitted.
> This is very confusing. Let's make the helper more robust and check also
> for the other external member, name. Thus the helper correctly stops on
> an empty item of the array. We need to have a check for obj->funcs in
> klp_init_object() to make it work.
> 
> The same applies to a func-walking helper.
> 
> As a benefit we'll check for new_func member definition during the
> livepatch initialization. There is no such check anywhere in the code
> now.

Applied to livepatching.git#for-4.7/core. Thanks,

-- 
Jiri Kosina
SUSE Labs