linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] module: make module symbols visible after init
@ 2020-06-03 14:12 Cheng Jian
  2020-06-03 17:00 ` Miroslav Benes
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Cheng Jian @ 2020-06-03 14:12 UTC (permalink / raw)
  To: linux-kernel, live-patching
  Cc: cj.chengjian, chenwandun, xiexiuqi, bobo.shaobowang,
	huawei.libin, jeyu, jikos

When lookup the symbols of module by module_kallsyms_lookup_name(),
the symbols address is visible only if the module's status isn't
MODULE_STATE_UNFORMED, This is problematic.

When complete_formation is done, the state of the module is modified
to MODULE_STATE_COMING, and the symbol of module is visible to the
outside.

At this time, the init function of the module has not been called,
so if the address of the function symbol has been found and called,
it may cause some exceptions.

For livepatch module, the relocation information of the livepatch
module is completed in init by klp_write_object_relocations(), and
the symbol name of the old and new functions are the same. Therefore,
when we lookup the symbol, we may get the function address of the
livepatch module. a crash can occurs when we call this function.

	CPU 0				CPU 1
	==================================================
	load_module
	add_unformed_module # MODULE_STATE_UNFORMED;
	post_relocation
	complete_formation  # MODULE_STATE_COMING;
					------------------
					module_kallsymc_lookup_name("A")
					call A()	# CRASH
					------------------
	do_init_module
	klp_write_object_relocations
	mod->state = MODULE_STATE_LIVE;

In commit 0bd476e6c671 ("kallsyms: unexport kallsyms_lookup_name() and
kallsyms_on_each_symbol()") restricts the invocation for kernel unexported
symbols, but it is still incorrect to make the symbols of non-LIVE modules
visible to the outside.

Signed-off-by: Cheng Jian <cj.chengjian@huawei.com>
---
 kernel/module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module.c b/kernel/module.c
index 64a2b4daaaa5..96c9cb64de57 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -4220,7 +4220,7 @@ unsigned long module_kallsyms_lookup_name(const char *name)
 			ret = find_kallsyms_symbol_value(mod, colon+1);
 	} else {
 		list_for_each_entry_rcu(mod, &modules, list) {
-			if (mod->state == MODULE_STATE_UNFORMED)
+			if (mod->state != MODULE_STATE_LIVE)
 				continue;
 			if ((ret = find_kallsyms_symbol_value(mod, name)) != 0)
 				break;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] module: make module symbols visible after init
  2020-06-03 14:12 [PATCH] module: make module symbols visible after init Cheng Jian
@ 2020-06-03 17:00 ` Miroslav Benes
  2020-06-04  9:49   ` Jessica Yu
  2020-06-04  8:57 ` Petr Mladek
  2020-06-04 12:55 ` chengjian (D)
  2 siblings, 1 reply; 5+ messages in thread
From: Miroslav Benes @ 2020-06-03 17:00 UTC (permalink / raw)
  To: Cheng Jian
  Cc: linux-kernel, live-patching, chenwandun, xiexiuqi,
	bobo.shaobowang, huawei.libin, jeyu, jikos

Hi,

I'm confused...

On Wed, 3 Jun 2020, Cheng Jian wrote:

> When lookup the symbols of module by module_kallsyms_lookup_name(),
> the symbols address is visible only if the module's status isn't
> MODULE_STATE_UNFORMED, This is problematic.
> 
> When complete_formation is done, the state of the module is modified
> to MODULE_STATE_COMING, and the symbol of module is visible to the
> outside.
> 
> At this time, the init function of the module has not been called,
> so if the address of the function symbol has been found and called,
> it may cause some exceptions.
> 
> For livepatch module, the relocation information of the livepatch
> module is completed in init by klp_write_object_relocations(), and
> the symbol name of the old and new functions are the same. Therefore,
> when we lookup the symbol, we may get the function address of the
> livepatch module. a crash can occurs when we call this function.
> 
> 	CPU 0				CPU 1
> 	==================================================
> 	load_module
> 	add_unformed_module # MODULE_STATE_UNFORMED;
> 	post_relocation
> 	complete_formation  # MODULE_STATE_COMING;
> 					------------------
> 					module_kallsymc_lookup_name("A")
> 					call A()	# CRASH
> 					------------------
> 	do_init_module
> 	klp_write_object_relocations
> 	mod->state = MODULE_STATE_LIVE;

We don't call module_kallsymc_lookup_name() anywhere in livepatch if I am 
not missing something. So is this your code? Then I could see the problem. 
You get the address of a function from a livepatch module and call it, 
which is not correct.

I see two options...

1. don't use the same name for the new function. Use some kind of prefix. 
It is more bulletproof anyway.

2. module_kallsyms_lookup_name() accepts a module name as a prefix of a 
symbol. So you can use module_kallsyms_lookup_name("module:A") and it 
should return A from that particular module only (if it exists).
 
> In commit 0bd476e6c671 ("kallsyms: unexport kallsyms_lookup_name() and
> kallsyms_on_each_symbol()") restricts the invocation for kernel unexported
> symbols, but it is still incorrect to make the symbols of non-LIVE modules
> visible to the outside.

Why? It could easily break something somewhere. I didn't check properly, 
but module states are not safe to play with, so I'd be conservative here.

> Signed-off-by: Cheng Jian <cj.chengjian@huawei.com>
> ---
>  kernel/module.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 64a2b4daaaa5..96c9cb64de57 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -4220,7 +4220,7 @@ unsigned long module_kallsyms_lookup_name(const char *name)
>  			ret = find_kallsyms_symbol_value(mod, colon+1);
>  	} else {
>  		list_for_each_entry_rcu(mod, &modules, list) {
> -			if (mod->state == MODULE_STATE_UNFORMED)
> +			if (mod->state != MODULE_STATE_LIVE)
>  				continue;
>  			if ((ret = find_kallsyms_symbol_value(mod, name)) != 0)
>  				break;

Thanks,
Miroslav

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] module: make module symbols visible after init
  2020-06-03 14:12 [PATCH] module: make module symbols visible after init Cheng Jian
  2020-06-03 17:00 ` Miroslav Benes
@ 2020-06-04  8:57 ` Petr Mladek
  2020-06-04 12:55 ` chengjian (D)
  2 siblings, 0 replies; 5+ messages in thread
From: Petr Mladek @ 2020-06-04  8:57 UTC (permalink / raw)
  To: Cheng Jian
  Cc: linux-kernel, live-patching, chenwandun, xiexiuqi,
	bobo.shaobowang, huawei.libin, jeyu, jikos

On Wed 2020-06-03 14:12:00, Cheng Jian wrote:
> When lookup the symbols of module by module_kallsyms_lookup_name(),
> the symbols address is visible only if the module's status isn't
> MODULE_STATE_UNFORMED, This is problematic.
> 
> When complete_formation is done, the state of the module is modified
> to MODULE_STATE_COMING, and the symbol of module is visible to the
> outside.
> 
> At this time, the init function of the module has not been called,
> so if the address of the function symbol has been found and called,
> it may cause some exceptions.

It is really handful that module symbols can be found already when
the module is MODULE_STATE_COMING state. It is used by livepatching,
ftrace, and maybe some other subsystems.

The problem is that nobody is allowed to use (call) module symbols
before mod->init() is called and the module is moved to
MODULE_STATE_LIVE.

By other words. Any code that calls module symbols before the module
is fully initialized is buggy. The caller should get fixed,
not the kallsyms side.

Have you seen such a problem in the real life, please?

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] module: make module symbols visible after init
  2020-06-03 17:00 ` Miroslav Benes
@ 2020-06-04  9:49   ` Jessica Yu
  0 siblings, 0 replies; 5+ messages in thread
From: Jessica Yu @ 2020-06-04  9:49 UTC (permalink / raw)
  To: Miroslav Benes
  Cc: Cheng Jian, linux-kernel, live-patching, chenwandun, xiexiuqi,
	bobo.shaobowang, huawei.libin, jikos

+++ Miroslav Benes [03/06/20 19:00 +0200]:
>> In commit 0bd476e6c671 ("kallsyms: unexport kallsyms_lookup_name() and
>> kallsyms_on_each_symbol()") restricts the invocation for kernel unexported
>> symbols, but it is still incorrect to make the symbols of non-LIVE modules
>> visible to the outside.
>
>Why? It could easily break something somewhere. I didn't check properly,
>but module states are not safe to play with, so I'd be conservative here.

Fully agree here. And it is not incorrect to make the symbols of
non-live modules visible via kallsyms. For one, kallsyms needs to
be able to see a module's symbols already even while in the COMING
state, because we can already oops/panic inside the module during
parse_args(), and symbol resolution via kallsyms is needed here even
though the module is not live. I have not checked carefully yet if
there are other users of kallsyms out there that might need to see
module symbols even when it is still coming, but for the first reason
alone I would not make this change.

Jessica

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] module: make module symbols visible after init
  2020-06-03 14:12 [PATCH] module: make module symbols visible after init Cheng Jian
  2020-06-03 17:00 ` Miroslav Benes
  2020-06-04  8:57 ` Petr Mladek
@ 2020-06-04 12:55 ` chengjian (D)
  2 siblings, 0 replies; 5+ messages in thread
From: chengjian (D) @ 2020-06-04 12:55 UTC (permalink / raw)
  To: linux-kernel, live-patching
  Cc: chenwandun, xiexiuqi, bobo.shaobowang, huawei.libin, jeyu, jikos

Hi, Petr, Jessica and Miroslav

Thank you for your reply

On 2020/6/4 16:57, Petr Mladek wrote:
> On Wed 2020-06-03 14:12:00, Cheng Jian wrote:
>
> It is really handful that module symbols can be found already when
> the module is MODULE_STATE_COMING state. It is used by livepatching,
> ftrace, and maybe some other subsystems.

Yes, you are right, I missed this before.

There are many scenes that lookup the symbols of module when the module is

MODULE_STATE_COMING state.

in livepatch:

     klp_module_coming

-=> klp_write_object_relocations

-=> klp_resolve_symbols

-=> module_kallsyms_on_each_symbol

My patch is incorrect.

> The problem is that nobody is allowed to use (call) module symbols
> before mod->init() is called and the module is moved to
> MODULE_STATE_LIVE.
>
> By other words. Any code that calls module symbols before the module
> is fully initialized is buggy. The caller should get fixed,
> not the kallsyms side.
>
> Have you seen such a problem in the real life, please?
>
> Best Regards,
> Petr
>
> .


     Thank you very much.

         -- Cheng Jian.



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-06-04 12:56 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-03 14:12 [PATCH] module: make module symbols visible after init Cheng Jian
2020-06-03 17:00 ` Miroslav Benes
2020-06-04  9:49   ` Jessica Yu
2020-06-04  8:57 ` Petr Mladek
2020-06-04 12:55 ` chengjian (D)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).