[PATCH] module: make module symbols visible after init

[PATCH] module: make module symbols visible after init

[Cheng Jian]

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

Re: [PATCH] module: make module symbols visible after init

[Miroslav Benes]

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

Re: [PATCH] module: make module symbols visible after init

[Jessica Yu]

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

Re: [PATCH] module: make module symbols visible after init

[Petr Mladek]

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

Re: [PATCH] module: make module symbols visible after init

[chengjian (D)]

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.