[RFC] Add target module check before livepatch module loading

[RFC] Add target module check before livepatch module loading

[zhang warden]

I had faced a scenario like this:

There is a fuse.ko which is built as module of kernel source.
However, our team maintain the fuse as oot module.

There is a bug of (name it as B1) the original fuse.ko. 
And our team fix B1 of fuse.ko as release it as oot module fuse_o1.ko.
Our system loaded fuse_o1.ko. Now, another team made a livepatch module base on fuse.ko to fix B2 bug.
They load this livepatch_fuse.ko to the system, it fixed B2 bug, however, the livepatch_fuse.ko revert the fix of fuse_o1.ko.

It expose the B1 bug which is already fix in fuse_o1.ko
The exposed B1 bug make fault to our cluster, which is a bad thing :(

This  scenario shows the vulnerable of live-patching when handling 
out-of-tree module.

I have a original solution to handle this:
    • In kpatch-build, we would record the patched object, take the object of ko as a list of parameters.
    • Pass this ko list as parameter to create-klp-moudle.c
    • For each patched ko object, we should read its srcversion from the original module. If we use --oot-module, we would read the srcversion from the oot moudle version.
    • Store the target srcversion to a section named '.klp.target_srcversions'
    • When the kpatch module loading, we shoud check if section '.klp.target_srcversion' existed. If existed, we should check srcversion of the patch target in the system match our recorded srcversion or not. If thet are not match, refuse to load it. This can make sure the livepatch module would not load the wrong target.
This function can avoid livepatch from patching the wrong version of the function.

The original discussion can be seem on [1]. (Discussion with Joe Lawrence)

After the discussion, we think that the actual enforcement of this seems like a job for kernel/livepatch/core.c.
Or it should be the process of sys call `init_module` when loading a module.

I am here waiting for Request For Comment. Before I do codes.

Thanks~~ ;)
Wardenjohn.

[1]: https://github.com/dynup/kpatch/issues/1435

Re: [RFC] Add target module check before livepatch module loading

[Petr Mladek]

On Fri 2025-03-07 11:12:18, zhang warden wrote:
> I had faced a scenario like this:
> 
> There is a fuse.ko which is built as module of kernel source.
> However, our team maintain the fuse as oot module.
> 
> There is a bug of (name it as B1) the original fuse.ko. 
> And our team fix B1 of fuse.ko as release it as oot module fuse_o1.ko.
> Our system loaded fuse_o1.ko. Now, another team made a livepatch module base on fuse.ko to fix B2 bug.
> They load this livepatch_fuse.ko to the system, it fixed B2 bug, however, the livepatch_fuse.ko revert the fix of fuse_o1.ko.
> 
> It expose the B1 bug which is already fix in fuse_o1.ko
> The exposed B1 bug make fault to our cluster, which is a bad thing :(
> 
> This  scenario shows the vulnerable of live-patching when handling 
> out-of-tree module.
> 
> I have a original solution to handle this:
>     • In kpatch-build, we would record the patched object, take the object of ko as a list of parameters.
>     • Pass this ko list as parameter to create-klp-moudle.c
>     • For each patched ko object, we should read its srcversion from the original module. If we use --oot-module, we would read the srcversion from the oot moudle version.
>     • Store the target srcversion to a section named '.klp.target_srcversions'
>     • When the kpatch module loading, we shoud check if section '.klp.target_srcversion' existed. If existed, we should check srcversion of the patch target in the system match our recorded srcversion or not. If thet are not match, refuse to load it. This can make sure the livepatch module would not load the wrong target.

The work with the elf sections is tricky. I would prefer to add
.srcversion into struct klp_object, something like:

 struct klp_object {
 	/* external */
 	const char *name;
+	const char *srcversion;
 	struct klp_func *funcs;
 	struct klp_callbacks callbacks;
 	[...]
 }

It would be optional. I made just a quick look. It might be enough
to check it in klp_find_object_module() and do not set obj->mod
when obj->srcversion != NULL and does not match mod->srcversion.

Wait! We need to check it also in klp_write_section_relocs().
Otherwise, klp_apply_section_relocs() might apply the relocations
for non-compatible module.

Another question is whether the same livepatch could support more
srcversions of the same module. It might be safe when the livepatched
functions are compatible. But it would be error prone.

If we wanted to allow support for incompatible modules with the same
name, we would need to encode the srcversion also into the name
of the special .klp_rela sections so that we could have separate
relocations for each variant.


Alternative: I think about using "mod->build_id" instead of "srcversion".
	It would be even more strict because it would make dependency
	on a particular build.

	An advantage is that it is defined also for vmlinux,
	see vmlinux_build_id. So that we could easily use
	the same approach also for vmlinux.

	I do not have strong opinion on this though.


> This function can avoid livepatch from patching the wrong version of the function.
> 
> The original discussion can be seem on [1]. (Discussion with Joe Lawrence)
> 
> After the discussion, we think that the actual enforcement of this seems like a job for kernel/livepatch/core.c.
> Or it should be the process of sys call `init_module` when loading a module.
>
> I am here waiting for Request For Comment. Before I do codes.

I am open to accept such a feature. It might improve reliability of
the livepatching.

Let's see how the code looks like and how complicated would be to
create such livepatches from sources or using kPatch.

Best Regards,
Petr

Re: [RFC] Add target module check before livepatch module loading

[zhang warden]

Hi, Petr!

> The work with the elf sections is tricky. I would prefer to add
> .srcversion into struct klp_object, something like:
> 
> struct klp_object {
> /* external */
> const char *name;
> + const char *srcversion;
> struct klp_func *funcs;
> struct klp_callbacks callbacks;
> [...]
> }
> 

In fact, I have a though in mind that we can easily use the 
srcversion in `struct module` like:

struct module {
    enum module_state state;

    /* Member of list of modules */
    struct list_head list;

    /* Unique handle for this module */
    char name[MODULE_NAME_LEN];

#ifdef CONFIG_STACKTRACE_BUILD_ID
    /* Module build ID */
    unsigned char build_id[BUILD_ID_SIZE_MAX];
#endif

    /* Sysfs stuff. */
    struct module_kobject mkobj;
    struct module_attribute *modinfo_attrs;
    const char *version;
    const char *srcversion;
    struct kobject *holders_dir;

    /* Exported symbols */
    const struct kernel_symbol *syms;
    const u32 *crcs;
    unsigned int num_syms;

becase when we are loading a livepatch module, the syscall `init_module`
will be called and we can check the srcversion here.

What I want to do in the elf section is that I want to add a new section
to store the target module src version inside. 

> It would be optional. I made just a quick look. It might be enough
> to check it in klp_find_object_module() and do not set obj->mod
> when obj->srcversion != NULL and does not match mod->srcversion.
> 
> Wait! We need to check it also in klp_write_section_relocs().
> Otherwise, klp_apply_section_relocs() might apply the relocations
> for non-compatible module.
> 

As previously mentioned, if we can check the srcversion when calling
the syscall `init_module`, refuse to load the module if the livepatch
module have srcversion and the srcversion is not equal to the target
in the system. Can it avoid such relocations problem?

> Another question is whether the same livepatch could support more
> srcversions of the same module. It might be safe when the livepatched
> functions are compatible. But it would be error prone.
> 
> If we wanted to allow support for incompatible modules with the same
> name, we would need to encode the srcversion also into the name
> of the special .klp_rela sections so that we could have separate
> relocations for each variant.
> 

I am not sure if supporting more srcversions is necessary. Because 
I can't find a senario that more than one version of a module are running
in the system at the same time.

> 
> Alternative: I think about using "mod->build_id" instead of "srcversion".
> It would be even more strict because it would make dependency
> on a particular build.
> 
> An advantage is that it is defined also for vmlinux,
> see vmlinux_build_id. So that we could easily use
> the same approach also for vmlinux.
> 
> I do not have strong opinion on this though.
> 

Petr, using "mod->build_id" instead of "srcversion" may not be good.
Because livepatch can not only handle the function in vmlinux but also
the function in modules.

From my point of view, each build will have a different srcversion generated.
Is it necessary to introduce a "mod->build_id"?

> 
>> This function can avoid livepatch from patching the wrong version of the function.
>> 
>> The original discussion can be seem on [1]. (Discussion with Joe Lawrence)
>> 
>> After the discussion, we think that the actual enforcement of this seems like a job for kernel/livepatch/core.c.
>> Or it should be the process of sys call `init_module` when loading a module.
>> 
>> I am here waiting for Request For Comment. Before I do codes.
> 
> I am open to accept such a feature. It might improve reliability of
> the livepatching.
> 
> Let's see how the code looks like and how complicated would be to
> create such livepatches from sources or using kPatch.
> 
> Best Regards,
> Petr

Thanks, Petr. And I will start to code the draft soon. If you have any suggestions, please be
open to tell me :)

Best Regards 
Wardenjohn

Re: [RFC] Add target module check before livepatch module loading

[Petr Mladek]

On Mon 2025-03-10 10:22:19, zhang warden wrote:
> 
> Hi, Petr!
> 
> > The work with the elf sections is tricky. I would prefer to add
> > .srcversion into struct klp_object, something like:
> > 
> > struct klp_object {
> > /* external */
> > const char *name;
> > + const char *srcversion;
> > struct klp_func *funcs;
> > struct klp_callbacks callbacks;
> > [...]
> > }
> > 
> 
> In fact, I have a though in mind that we can easily use the 
> srcversion in `struct module` like:
> 
> struct module {
>     enum module_state state;
> 
>     /* Member of list of modules */
>     struct list_head list;
> 
>     /* Unique handle for this module */
>     char name[MODULE_NAME_LEN];
> 
> #ifdef CONFIG_STACKTRACE_BUILD_ID
>     /* Module build ID */
>     unsigned char build_id[BUILD_ID_SIZE_MAX];
> #endif
> 
>     /* Sysfs stuff. */
>     struct module_kobject mkobj;
>     struct module_attribute *modinfo_attrs;
>     const char *version;
>     const char *srcversion;
>     struct kobject *holders_dir;
> 
>     /* Exported symbols */
>     const struct kernel_symbol *syms;
>     const u32 *crcs;
>     unsigned int num_syms;
> 
> becase when we are loading a livepatch module, the syscall `init_module`
> will be called and we can check the srcversion here.
> 
> What I want to do in the elf section is that I want to add a new section
> to store the target module src version inside. 

Why do you want to store the target module src version in an elf
section, please?

IMHO, it would be much easier to store it in struct klp_object
as I proposed above. Then the check might be as simple as:

diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
index 51a258c24ff5..dfd7132eec4e 100644
--- a/include/linux/livepatch.h
+++ b/include/linux/livepatch.h
@@ -104,6 +104,7 @@ struct klp_callbacks {
 /**
  * struct klp_object - kernel object structure for live patching
  * @name:	module name (or NULL for vmlinux)
+ * @srcversion  compactible srcversion (optional)
  * @funcs:	function entries for functions to be patched in the object
  * @callbacks:	functions to be executed pre/post (un)patching
  * @kobj:	kobject for sysfs resources
@@ -117,6 +118,7 @@ struct klp_callbacks {
 struct klp_object {
 	/* external */
 	const char *name;
+	const char *srcversion;
 	struct klp_func *funcs;
 	struct klp_callbacks callbacks;
 
diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 0cd39954d5a1..61004502e72d 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -66,6 +66,13 @@ static void klp_find_object_module(struct klp_object *obj)
 	 * klp_module_going() instead.
 	 */
 	mod = find_module(obj->name);
+
+	/* Do not livepatch an incompatible object. */
+	if (mod && obj->srcversion && mod->srcversion) {
+		if (strcmp(obj->srcversion, mod->srcversion) != 0)
+			goto out;
+	}
+
 	/*
 	 * Do not mess work of klp_module_coming() and klp_module_going().
 	 * Note that the patch might still be needed before klp_module_going()
@@ -75,7 +82,7 @@ static void klp_find_object_module(struct klp_object *obj)
 	 */
 	if (mod && mod->klp_alive)
 		obj->mod = mod;
-
+out:
 	rcu_read_unlock_sched();
 }

The above code causes that the livepatch would ignore an incompatible
object.

Maybe, you want to return an error instead and block the incompatible
module load.

> > It would be optional. I made just a quick look. It might be enough
> > to check it in klp_find_object_module() and do not set obj->mod
> > when obj->srcversion != NULL and does not match mod->srcversion.
> > 
> > Wait! We need to check it also in klp_write_section_relocs().
> > Otherwise, klp_apply_section_relocs() might apply the relocations
> > for non-compatible module.
> > 
> 
> As previously mentioned, if we can check the srcversion when calling
> the syscall `init_module`, refuse to load the module if the livepatch
> module have srcversion and the srcversion is not equal to the target
> in the system. Can it avoid such relocations problem?

Honestly, I am not sure what you mean by target module src version.

Anyway, you could prevent the module load also when
klp_find_object_module() returns an error.


> > Alternative: I think about using "mod->build_id" instead of "srcversion".
> > It would be even more strict because it would make dependency
> > on a particular build.
> > 
> > An advantage is that it is defined also for vmlinux,
> > see vmlinux_build_id. So that we could easily use
> > the same approach also for vmlinux.
> > 
> > I do not have strong opinion on this though.
> > 
> 
> Petr, using "mod->build_id" instead of "srcversion" may not be good.
> Because livepatch can not only handle the function in vmlinux but also
> the function in modules.

I do not understand this urgument.

vmlinux can be identified by build_id stored in "vmlinux_build_id".

And modules can be identified by both build_id and srcversion. Both
information are stored in struct module.

A single livepatch could modify more objects: vmlinux and several
modules. The metadata for each modified object are in "struct
klp_object". The related obect is currently identified only by obj->name.
And we could add more precision identification by setting
also "obj->srcversion" and/or "obj->build_id".


> From my point of view, each build will have a different srcversion generated.
> Is it necessary to introduce a "mod->build_id"?

My understanding is that "srcversion" a kind of checksum of the
sources. I guess that it will be always the same for the sources.
I guess that it is not affected by the compiler or compiler options.

But build_id should be unique with each rebuild. It should be
afftected by the compiler or compiler options.

Note that the compiler options might affect how the functions are
opimized (inlining). And it might affect compactibility of the livepatch.

Best Regards,
Petr

Re: [RFC] Add target module check before livepatch module loading

[zhang warden]

> Why do you want to store the target module src version in an elf
> section, please?
> 

Well, let me explain it again. Take fuse.ko as an example.

Now, the running system have fuse.ko(v1) inside.
I found fuse.ko(v1) have a bug, and try to fix it.
fuse.ko(v1) have srcversion(src_v1) to tag its build.

Now, I build an livepatch.ko for fuse.ko(v1). But how to
make sure that this livepatch.ko will only patch the fuse.ko(v1)?

( Becase in other system, there may be fuse.ko(v2) or fuse.ko(v3) in
the system. If this livepatch.ko patch to fuse.ko(v2) or fuse.ko(v3)
may cause bugs.)

To make sure this livepatch.ko will only patch the fuse.ko(v1), I 
can store the target srcversion of fuse.ko(v1) into the livepatch.ko.

livepatch.ko now can carry its owner's information. When the system
loading livepatch module, the system can know that this livepatch module
can just only patch the target with srcversion(src_v1). And avoid the wrong
patching.


> IMHO, it would be much easier to store it in struct klp_object
> as I proposed above. Then the check might be as simple as:
> 
> diff --git a/include/linux/livepatch.h b/include/linux/livepatch.h
> index 51a258c24ff5..dfd7132eec4e 100644
> --- a/include/linux/livepatch.h
> +++ b/include/linux/livepatch.h
> @@ -104,6 +104,7 @@ struct klp_callbacks {
> /**
>  * struct klp_object - kernel object structure for live patching
>  * @name: module name (or NULL for vmlinux)
> + * @srcversion  compactible srcversion (optional)
>  * @funcs: function entries for functions to be patched in the object
>  * @callbacks: functions to be executed pre/post (un)patching
>  * @kobj: kobject for sysfs resources
> @@ -117,6 +118,7 @@ struct klp_callbacks {
> struct klp_object {
> /* external */
> const char *name;
> + const char *srcversion;
> struct klp_func *funcs;
> struct klp_callbacks callbacks;
> 
> diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
> index 0cd39954d5a1..61004502e72d 100644
> --- a/kernel/livepatch/core.c
> +++ b/kernel/livepatch/core.c
> @@ -66,6 +66,13 @@ static void klp_find_object_module(struct klp_object *obj)
>  * klp_module_going() instead.
>  */
> mod = find_module(obj->name);
> +
> + /* Do not livepatch an incompatible object. */
> + if (mod && obj->srcversion && mod->srcversion) {
> + if (strcmp(obj->srcversion, mod->srcversion) != 0)
> + goto out;
> + }
> +
> /*
>  * Do not mess work of klp_module_coming() and klp_module_going().
>  * Note that the patch might still be needed before klp_module_going()
> @@ -75,7 +82,7 @@ static void klp_find_object_module(struct klp_object *obj)
>  */
> if (mod && mod->klp_alive)
> obj->mod = mod;
> -
> +out:
> rcu_read_unlock_sched();
> }
> 
> The above code causes that the livepatch would ignore an incompatible
> object.
> 
> Maybe, you want to return an error instead and block the incompatible
> module load.
> 

Oh, I got your point. Because I usually use livepatch to fix problems
point to point, it is rare that all fixes are put in one patch....lol...

So, I admit my design have shortcomings, which is ignored the situation
that one livepatch module can patch many functions in one module...


>>> It would be optional. I made just a quick look. It might be enough
>>> to check it in klp_find_object_module() and do not set obj->mod
>>> when obj->srcversion != NULL and does not match mod->srcversion.
>>> 
>>> Wait! We need to check it also in klp_write_section_relocs().
>>> Otherwise, klp_apply_section_relocs() might apply the relocations
>>> for non-compatible module.
>>> 
>> 
>> As previously mentioned, if we can check the srcversion when calling
>> the syscall `init_module`, refuse to load the module if the livepatch
>> module have srcversion and the srcversion is not equal to the target
>> in the system. Can it avoid such relocations problem?
> 
> Honestly, I am not sure what you mean by target module src version.
> 
> Anyway, you could prevent the module load also when
> klp_find_object_module() returns an error.
> 

Well, my target module srcversion is the fuse.ko(v1) as previous mentioned.
Is it clear now?

> 
> I do not understand this urgument.
> 
> vmlinux can be identified by build_id stored in "vmlinux_build_id".
> 
> And modules can be identified by both build_id and srcversion. Both
> information are stored in struct module.
> 
> A single livepatch could modify more objects: vmlinux and several
> modules. The metadata for each modified object are in "struct
> klp_object". The related obect is currently identified only by obj->name.
> And we could add more precision identification by setting
> also "obj->srcversion" and/or "obj->build_id".
> 

Yep, but how can we get the obj->srcversion? If we tring to store it 
in klp_object, the information should be carried when livepatch is being build.
Otherwise, we don't know which srcversion is good to patch, isn't it?

> 
> My understanding is that "srcversion" a kind of checksum of the
> sources. I guess that it will be always the same for the sources.
> I guess that it is not affected by the compiler or compiler options.
> 

You are right, Petr. I made some tests on a module, and seems that srcversion 
depends on the source code. 

> But build_id should be unique with each rebuild. It should be
> afftected by the compiler or compiler options.
> 
> Note that the compiler options might affect how the functions are
> opimized (inlining). And it might affect compactibility of the livepatch.

IMHO, we just need to make sure the target version (using srcversion) is correct 
will be  enough?

Re: [RFC] Add target module check before livepatch module loading

[Petr Mladek]

On Tue 2025-03-11 11:53:59, zhang warden wrote:
> > A single livepatch could modify more objects: vmlinux and several
> > modules. The metadata for each modified object are in "struct
> > klp_object". The related obect is currently identified only by obj->name.
> > And we could add more precision identification by setting
> > also "obj->srcversion" and/or "obj->build_id".
> > 
> 
> Yep, but how can we get the obj->srcversion? If we tring to store it 
> in klp_object, the information should be carried when livepatch is being build.
> Otherwise, we don't know which srcversion is good to patch, isn't it?

I am not sure if I get the question correctly.

Anyway, struct klp_object must be defined in any livepatch, for example, see
/prace/kernel/linux/samples/livepatch/livepatch-sample.c

I guess that you are using kPatch. I am not sure how it initializes
these klp_patch, klp_object, and klp_func structures.
But it has to create struct klp_object for the livepatched module
and initialize at least .name, .func items.

The srcversion of the livepatched module can be read by modinfo,
for example:

# modinfo test_printf
filename:       /lib/modules/6.13.0-default+/kernel/lib/test_printf.ko
license:        GPL
description:    Test cases for printf facility
author:         Rasmus Villemoes <linux@rasmusvillemoes.dk>
test:           Y
srcversion:     AF319FC942A3220645E7E99
depends:        
intree:         Y
name:           test_printf
retpoline:      Y
vermagic:       6.13.0-default+ SMP preempt mod_unload modversions 

You need to teach kPatch to read the srcversion of the livepatched
module and set it in the related struct klp_object.

Best Regards,
Petr

Re: [RFC] Add target module check before livepatch module loading

[zhang warden]

> On Mar 11, 2025, at 20:25, Petr Mladek <pmladek@suse.com> wrote:
> 
> On Tue 2025-03-11 11:53:59, zhang warden wrote:
>>> A single livepatch could modify more objects: vmlinux and several
>>> modules. The metadata for each modified object are in "struct
>>> klp_object". The related obect is currently identified only by obj->name.
>>> And we could add more precision identification by setting
>>> also "obj->srcversion" and/or "obj->build_id".
>>> 
>> 
>> Yep, but how can we get the obj->srcversion? If we tring to store it 
>> in klp_object, the information should be carried when livepatch is being build.
>> Otherwise, we don't know which srcversion is good to patch, isn't it?
> 
> I am not sure if I get the question correctly.
> 
> Anyway, struct klp_object must be defined in any livepatch, for example, see
> /prace/kernel/linux/samples/livepatch/livepatch-sample.c
> 
> I guess that you are using kPatch. I am not sure how it initializes
> these klp_patch, klp_object, and klp_func structures.
> But it has to create struct klp_object for the livepatched module
> and initialize at least .name, .func items.
> 
> The srcversion of the livepatched module can be read by modinfo,
> for example:
> 
> # modinfo test_printf
> filename:       /lib/modules/6.13.0-default+/kernel/lib/test_printf.ko
> license:        GPL
> description:    Test cases for printf facility
> author:         Rasmus Villemoes <linux@rasmusvillemoes.dk>
> test:           Y
> srcversion:     AF319FC942A3220645E7E99
> depends:        
> intree:         Y
> name:           test_printf
> retpoline:      Y
> vermagic:       6.13.0-default+ SMP preempt mod_unload modversions 
> 
> You need to teach kPatch to read the srcversion of the livepatched
> module and set it in the related struct klp_object.
> 
> Best Regards,
> Petr

Oh,yeah.

You got the point. I am using kpatch[1] as userspace tool to build livepatch
module. Therefore, I am focusing on combining kPatch and kernel.

So, I need to teach kpatch module to learn the srvcversion, that's why 
I am trying to put srcversion into elf file. This elf file I mentioned is the 
livepatch module built by kPatch.

So, maybe now you can understand why my livepatch module miss the information
of the target srcversion. Because this livepatch module is built by kPatch, not building
it manually.

But now I think I have a mistake here. I should focus on the kernel first. And then,
 make adjustment to kpatch[1] after this feature is ready in kernel.

Regards
Wardenjohn

[1] kpatch: https://github.com/dynup/kpatch