[RFC PATCH 1/3] module: Split module_enable_rodata_ro()

[RFC PATCH 1/3] module: Split module_enable_rodata_ro()

[Christophe Leroy]

module_enable_rodata_ro() is called twice, once before module init
to set rodata sections readonly and once after module init to set
rodata_after_init section readonly.

The second time, only the rodata_after_init section needs to be
set to read-only, no need to re-apply it to already set rodata.

Split module_enable_rodata_ro() in two.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 kernel/module/internal.h   |  3 ++-
 kernel/module/main.c       |  4 ++--
 kernel/module/strict_rwx.c | 13 +++++++++----
 3 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index 2ebece8a789f..994f35a779dc 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -322,7 +322,8 @@ static inline struct module *mod_find(unsigned long addr, struct mod_tree_root *
 }
 #endif /* CONFIG_MODULES_TREE_LOOKUP */
 
-int module_enable_rodata_ro(const struct module *mod, bool after_init);
+int module_enable_rodata_ro(const struct module *mod);
+int module_enable_rodata_ro_after_init(const struct module *mod);
 int module_enable_data_nx(const struct module *mod);
 int module_enable_text_rox(const struct module *mod);
 int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 49b9bca9de12..2de4ad7af335 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2581,7 +2581,7 @@ static noinline int do_init_module(struct module *mod)
 	/* Switch to core kallsyms now init is done: kallsyms may be walking! */
 	rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
 #endif
-	ret = module_enable_rodata_ro(mod, true);
+	ret = module_enable_rodata_ro_after_init(mod);
 	if (ret)
 		goto fail_mutex_unlock;
 	mod_tree_remove_init(mod);
@@ -2751,7 +2751,7 @@ static int complete_formation(struct module *mod, struct load_info *info)
 	module_bug_finalize(info->hdr, info->sechdrs, mod);
 	module_cfi_finalize(info->hdr, info->sechdrs, mod);
 
-	err = module_enable_rodata_ro(mod, false);
+	err = module_enable_rodata_ro(mod);
 	if (err)
 		goto out_strict_rwx;
 	err = module_enable_data_nx(mod);
diff --git a/kernel/module/strict_rwx.c b/kernel/module/strict_rwx.c
index c45caa4690e5..f68c59974ae2 100644
--- a/kernel/module/strict_rwx.c
+++ b/kernel/module/strict_rwx.c
@@ -44,7 +44,7 @@ int module_enable_text_rox(const struct module *mod)
 	return 0;
 }
 
-int module_enable_rodata_ro(const struct module *mod, bool after_init)
+int module_enable_rodata_ro(const struct module *mod)
 {
 	int ret;
 
@@ -58,12 +58,17 @@ int module_enable_rodata_ro(const struct module *mod, bool after_init)
 	if (ret)
 		return ret;
 
-	if (after_init)
-		return module_set_memory(mod, MOD_RO_AFTER_INIT, set_memory_ro);
-
 	return 0;
 }
 
+int module_enable_rodata_ro_after_init(const struct module *mod)
+{
+	if (!IS_ENABLED(CONFIG_STRICT_MODULE_RWX) || !rodata_enabled)
+		return 0;
+
+	return module_set_memory(mod, MOD_RO_AFTER_INIT, set_memory_ro);
+}
+
 int module_enable_data_nx(const struct module *mod)
 {
 	if (!IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
-- 
2.44.0

[RFC PATCH 2/3] module: Don't fail module loading when setting ro_after_init section RO failed

[Christophe Leroy]

Once module init has succeded it is too late to cancel loading.
If setting ro_after_init data section to read-only fails, all we
can do is to inform the user through a warning.

Reported-by: Thomas Gleixner <tglx@linutronix.de>
Closes: https://lore.kernel.org/all/20230915082126.4187913-1-ruanjinjie@huawei.com/
Fixes: d1909c022173 ("module: Don't ignore errors from set_memory_XX()")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 kernel/module/main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 2de4ad7af335..1bf4b0db291b 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2583,7 +2583,9 @@ static noinline int do_init_module(struct module *mod)
 #endif
 	ret = module_enable_rodata_ro_after_init(mod);
 	if (ret)
-		goto fail_mutex_unlock;
+		pr_warn("%s: %s() returned %d, ro_after_init data might still be writable\n",
+			mod->name, __func__, ret);
+
 	mod_tree_remove_init(mod);
 	module_arch_freeing_init(mod);
 	for_class_mod_mem_type(type, init) {
@@ -2622,8 +2624,6 @@ static noinline int do_init_module(struct module *mod)
 
 	return 0;
 
-fail_mutex_unlock:
-	mutex_unlock(&module_mutex);
 fail_free_freeinit:
 	kfree(freeinit);
 fail:
-- 
2.44.0

Re: [RFC PATCH 2/3] module: Don't fail module loading when setting ro_after_init section RO failed

[Daniel Gomez]

On Sat Nov 9, 2024 at 11:35 AM CET, Christophe Leroy wrote:
> Once module init has succeded it is too late to cancel loading.
> If setting ro_after_init data section to read-only fails, all we
> can do is to inform the user through a warning.
>
> Reported-by: Thomas Gleixner <tglx@linutronix.de>
> Closes: https://lore.kernel.org/all/20230915082126.4187913-1-ruanjinjie@huawei.com/
> Fixes: d1909c022173 ("module: Don't ignore errors from set_memory_XX()")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  kernel/module/main.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 2de4ad7af335..1bf4b0db291b 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2583,7 +2583,9 @@ static noinline int do_init_module(struct module *mod)
>  #endif
>  	ret = module_enable_rodata_ro_after_init(mod);
>  	if (ret)
> -		goto fail_mutex_unlock;
> +		pr_warn("%s: %s() returned %d, ro_after_init data might still be writable\n",
> +			mod->name, __func__, ret);
> +
>  	mod_tree_remove_init(mod);
>  	module_arch_freeing_init(mod);
>  	for_class_mod_mem_type(type, init) {
> @@ -2622,8 +2624,6 @@ static noinline int do_init_module(struct module *mod)
>  
>  	return 0;

I think it would make sense to propagate the error. But that would
require changing modprobe.c. What kind of error can we expect when this
happens?

>  
> -fail_mutex_unlock:
> -	mutex_unlock(&module_mutex);
>  fail_free_freeinit:
>  	kfree(freeinit);
>  fail:

Re: [RFC PATCH 2/3] module: Don't fail module loading when setting ro_after_init section RO failed

[Christophe Leroy]

Le 09/11/2024 à 23:17, Daniel Gomez a écrit :
> On Sat Nov 9, 2024 at 11:35 AM CET, Christophe Leroy wrote:
>> Once module init has succeded it is too late to cancel loading.
>> If setting ro_after_init data section to read-only fails, all we
>> can do is to inform the user through a warning.
>>
>> Reported-by: Thomas Gleixner <tglx@linutronix.de>
>> Closes: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20230915082126.4187913-1-ruanjinjie%40huawei.com%2F&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7C26b5ca7363e54210439b08dd010c4865%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638667874457200373%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=ZeJ%2F3%2B2Nx%2FBf%2FWLFEkhxKlDhZk8LNkz0fs%2Fg2xMcOjY%3D&reserved=0
>> Fixes: d1909c022173 ("module: Don't ignore errors from set_memory_XX()")
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   kernel/module/main.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>> index 2de4ad7af335..1bf4b0db291b 100644
>> --- a/kernel/module/main.c
>> +++ b/kernel/module/main.c
>> @@ -2583,7 +2583,9 @@ static noinline int do_init_module(struct module *mod)
>>   #endif
>>   	ret = module_enable_rodata_ro_after_init(mod);
>>   	if (ret)
>> -		goto fail_mutex_unlock;
>> +		pr_warn("%s: %s() returned %d, ro_after_init data might still be writable\n",
>> +			mod->name, __func__, ret);
>> +
>>   	mod_tree_remove_init(mod);
>>   	module_arch_freeing_init(mod);
>>   	for_class_mod_mem_type(type, init) {
>> @@ -2622,8 +2624,6 @@ static noinline int do_init_module(struct module *mod)
>>   
>>   	return 0;
> 
> I think it would make sense to propagate the error. But that would
> require changing modprobe.c. What kind of error can we expect when this
> happens?

AFAIK, on powerpc it fails with EINVAL when
- The area is a vmalloc or module area and is a hugepage area
- The area is not vmalloc or io register and MMU is not powerpc radix MMU

Otherwise it propagates the error from apply_to_existing_page_range(). 
IIUC it will return EINVAL when it hits a leaf PTE in upper directories.

On other architectures it can be different, I know some architecture try 
to split the pages when they hit hugepages and that can fail.


But I believe if it works the first time it should work next time as well.

> 
>>   
>> -fail_mutex_unlock:
>> -	mutex_unlock(&module_mutex);
>>   fail_free_freeinit:
>>   	kfree(freeinit);
>>   fail:
> 

Re: [RFC PATCH 2/3] module: Don't fail module loading when setting ro_after_init section RO failed

[Daniel Gomez]

On Mon Nov 11, 2024 at 7:53 PM CET, Christophe Leroy wrote:
>
>
> Le 09/11/2024 à 23:17, Daniel Gomez a écrit :
>> On Sat Nov 9, 2024 at 11:35 AM CET, Christophe Leroy wrote:
>>> Once module init has succeded it is too late to cancel loading.
>>> If setting ro_after_init data section to read-only fails, all we
>>> can do is to inform the user through a warning.
>>>
>>> Reported-by: Thomas Gleixner <tglx@linutronix.de>
>>> Closes: https://protect2.fireeye.com/v1/url?k=d3deb284-b2a35ac3-d3df39cb-74fe485fff30-288375d7d91e4ad9&q=1&e=701066ca-634d-4525-a77d-1a44451f897a&u=https%3A%2F%2Feur01.safelinks.protection.outlook.com%2F%3Furl%3Dhttps%253A%252F%252Flore.kernel.org%252Fall%252F20230915082126.4187913-1-ruanjinjie%2540huawei.com%252F%26data%3D05%257C02%257Cchristophe.leroy%2540csgroup.eu%257C26b5ca7363e54210439b08dd010c4865%257C8b87af7d86474dc78df45f69a2011bb5%257C0%257C0%257C638667874457200373%257CUnknown%257CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%253D%253D%257C0%257C%257C%257C%26sdata%3DZeJ%252F3%252B2Nx%252FBf%252FWLFEkhxKlDhZk8LNkz0fs%252Fg2xMcOjY%253D%26reserved%3D0
>>> Fixes: d1909c022173 ("module: Don't ignore errors from set_memory_XX()")
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> ---
>>>   kernel/module/main.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>>> index 2de4ad7af335..1bf4b0db291b 100644
>>> --- a/kernel/module/main.c
>>> +++ b/kernel/module/main.c
>>> @@ -2583,7 +2583,9 @@ static noinline int do_init_module(struct module *mod)
>>>   #endif
>>>   	ret = module_enable_rodata_ro_after_init(mod);
>>>   	if (ret)
>>> -		goto fail_mutex_unlock;
>>> +		pr_warn("%s: %s() returned %d, ro_after_init data might still be writable\n",
>>> +			mod->name, __func__, ret);
>>> +
>>>   	mod_tree_remove_init(mod);
>>>   	module_arch_freeing_init(mod);
>>>   	for_class_mod_mem_type(type, init) {
>>> @@ -2622,8 +2624,6 @@ static noinline int do_init_module(struct module *mod)
>>>   
>>>   	return 0;
>> 
>> I think it would make sense to propagate the error. But that would
>> require changing modprobe.c. What kind of error can we expect when this
>> happens?
>
> AFAIK, on powerpc it fails with EINVAL when
> - The area is a vmalloc or module area and is a hugepage area
> - The area is not vmalloc or io register and MMU is not powerpc radix MMU
>
> Otherwise it propagates the error from apply_to_existing_page_range(). 
> IIUC it will return EINVAL when it hits a leaf PTE in upper directories.

Looking at that path I see we can also fail at __apply_to_page_range()
-> apply_to_p4d_range() and return with -ENOMEM.

My proposal was to do something like the change below in modprobe:

diff --git a/tools/modprobe.c b/tools/modprobe.c
index ec66e6f..8876e27 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -572,6 +572,11 @@ static int insmod_insert(struct kmod_module *mod, int flags, const char *extra_o
                err = 0;
        else {
                switch (err) {
+               case -EINVAL:
+                       ERR("module '%s'inserted: ro_after_init data might"
+                           "still be writable (see dmesg)\n",
+                           kmod_module_get_name(mod));
+                       break;
                case -EEXIST:
                        ERR("could not insert '%s': Module already in kernel\n",
                            kmod_module_get_name(mod));

But I think these error codes may be also be reported in other parts
such as simplify_symbols() so may not be a good idea after all.

Maybe we just need to change the default/catch all error message in
modprobe.c and to indicate/include this case:

diff --git a/tools/modprobe.c b/tools/modprobe.c
index ec66e6f..3647d37 100644
--- a/tools/modprobe.c
+++ b/tools/modprobe.c
@@ -582,7 +582,8 @@ static int insmod_insert(struct kmod_module *mod, int flags, const char *extra_o
                            kmod_module_get_name(mod));
                        break;
                default:
-                       ERR("could not insert '%s': %s\n", kmod_module_get_name(mod),
+                       ERR("could not insert '%s' or inserted with error %s, "
+                           "(see dmesg)\n", kmod_module_get_name(mod),
                            strerror(-err));
                        break;
                }


>
> On other architectures it can be different, I know some architecture try 
> to split the pages when they hit hugepages and that can fail.

Is it worth it adding an error code for this case in case we want to
report it back?

>
>
> But I believe if it works the first time it should work next time as well.

Okay. It would be good to know if this is a common behaviour among
different architectures.

>
>> 
>>>   
>>> -fail_mutex_unlock:
>>> -	mutex_unlock(&module_mutex);
>>>   fail_free_freeinit:
>>>   	kfree(freeinit);
>>>   fail:
>> 

Re: [RFC PATCH 2/3] module: Don't fail module loading when setting ro_after_init section RO failed

[Christophe Leroy]

Le 12/11/2024 à 10:43, Daniel Gomez a écrit :
> On Mon Nov 11, 2024 at 7:53 PM CET, Christophe Leroy wrote:
>>
>>
>> Le 09/11/2024 à 23:17, Daniel Gomez a écrit :
>>> On Sat Nov 9, 2024 at 11:35 AM CET, Christophe Leroy wrote:
>>>> Once module init has succeded it is too late to cancel loading.
>>>> If setting ro_after_init data section to read-only fails, all we
>>>> can do is to inform the user through a warning.
>>>>
>>>> Reported-by: Thomas Gleixner <tglx@linutronix.de>
>>>> Closes: https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fprotect2.fireeye.com%2Fv1%2Furl%3Fk%3Dd3deb284-b2a35ac3-d3df39cb-74fe485fff30-288375d7d91e4ad9%26q%3D1%26e%3D701066ca-634d-4525-a77d-1a44451f897a%26u%3Dhttps%253A%252F%252Feur01.safelinks.protection.outlook.com%252F%253Furl%253Dhttps%25253A%25252F%25252Flore.kernel.org%25252Fall%25252F20230915082126.4187913-1-ruanjinjie%252540huawei.com%25252F%2526data%253D05%25257C02%25257Cchristophe.leroy%252540csgroup.eu%25257C26b5ca7363e54210439b08dd010c4865%25257C8b87af7d86474dc78df45f69a2011bb5%25257C0%25257C0%25257C638667874457200373%25257CUnknown%25257CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%25253D%25253D%25257C0%25257C%25257C%25257C%2526sdata%253DZeJ%25252F3%25252B2Nx%25252FBf%25252FWLFEkhxKlDhZk8LNkz0fs%25252Fg2xMcOjY%25253D%2526reserved%253D0&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7Cc86adbd7bad24b1042bd08dd02fe7c8e%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638670014259822622%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=Gpxrx401fRdCGahGcI6GtJp%2BqLTZsnNqxsDoz4TAfU8%3D&reserved=0
>>>> Fixes: d1909c022173 ("module: Don't ignore errors from set_memory_XX()")
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>> ---
>>>>    kernel/module/main.c | 6 +++---
>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>>>> index 2de4ad7af335..1bf4b0db291b 100644
>>>> --- a/kernel/module/main.c
>>>> +++ b/kernel/module/main.c
>>>> @@ -2583,7 +2583,9 @@ static noinline int do_init_module(struct module *mod)
>>>>    #endif
>>>>    	ret = module_enable_rodata_ro_after_init(mod);
>>>>    	if (ret)
>>>> -		goto fail_mutex_unlock;
>>>> +		pr_warn("%s: %s() returned %d, ro_after_init data might still be writable\n",
>>>> +			mod->name, __func__, ret);
>>>> +
>>>>    	mod_tree_remove_init(mod);
>>>>    	module_arch_freeing_init(mod);
>>>>    	for_class_mod_mem_type(type, init) {
>>>> @@ -2622,8 +2624,6 @@ static noinline int do_init_module(struct module *mod)
>>>>    
>>>>    	return 0;
>>>
>>> I think it would make sense to propagate the error. But that would
>>> require changing modprobe.c. What kind of error can we expect when this
>>> happens?
>>
>> AFAIK, on powerpc it fails with EINVAL when
>> - The area is a vmalloc or module area and is a hugepage area
>> - The area is not vmalloc or io register and MMU is not powerpc radix MMU
>>
>> Otherwise it propagates the error from apply_to_existing_page_range().
>> IIUC it will return EINVAL when it hits a leaf PTE in upper directories.
> 
> Looking at that path I see we can also fail at __apply_to_page_range()
> -> apply_to_p4d_range() and return with -ENOMEM.

The -ENOMEM is when 'create' is true, usually when there is not enough 
memory available to create a page table ... in that case I guess you 
have much more problems to happen ...

set_memory_ro() on powerpc calls apply_to_existing_page_range() which 
implies 'create' is false.

Christophe

Re: [RFC PATCH 2/3] module: Don't fail module loading when setting ro_after_init section RO failed

[Petr Pavlu]

On 11/12/24 10:43, Daniel Gomez wrote:
> On Mon Nov 11, 2024 at 7:53 PM CET, Christophe Leroy wrote:
>>
>>
>> Le 09/11/2024 à 23:17, Daniel Gomez a écrit :
>>> On Sat Nov 9, 2024 at 11:35 AM CET, Christophe Leroy wrote:
>>>> Once module init has succeded it is too late to cancel loading.
>>>> If setting ro_after_init data section to read-only fails, all we
>>>> can do is to inform the user through a warning.
>>>>
>>>> Reported-by: Thomas Gleixner <tglx@linutronix.de>
>>>> Closes: https://protect2.fireeye.com/v1/url?k=d3deb284-b2a35ac3-d3df39cb-74fe485fff30-288375d7d91e4ad9&q=1&e=701066ca-634d-4525-a77d-1a44451f897a&u=https%3A%2F%2Feur01.safelinks.protection.outlook.com%2F%3Furl%3Dhttps%253A%252F%252Flore.kernel.org%252Fall%252F20230915082126.4187913-1-ruanjinjie%2540huawei.com%252F%26data%3D05%257C02%257Cchristophe.leroy%2540csgroup.eu%257C26b5ca7363e54210439b08dd010c4865%257C8b87af7d86474dc78df45f69a2011bb5%257C0%257C0%257C638667874457200373%257CUnknown%257CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%253D%253D%257C0%257C%257C%257C%26sdata%3DZeJ%252F3%252B2Nx%252FBf%252FWLFEkhxKlDhZk8LNkz0fs%252Fg2xMcOjY%253D%26reserved%3D0
>>>> Fixes: d1909c022173 ("module: Don't ignore errors from set_memory_XX()")
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>> ---
>>>>   kernel/module/main.c | 6 +++---
>>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>>>> index 2de4ad7af335..1bf4b0db291b 100644
>>>> --- a/kernel/module/main.c
>>>> +++ b/kernel/module/main.c
>>>> @@ -2583,7 +2583,9 @@ static noinline int do_init_module(struct module *mod)
>>>>   #endif
>>>>   	ret = module_enable_rodata_ro_after_init(mod);
>>>>   	if (ret)
>>>> -		goto fail_mutex_unlock;
>>>> +		pr_warn("%s: %s() returned %d, ro_after_init data might still be writable\n",
>>>> +			mod->name, __func__, ret);
>>>> +
>>>>   	mod_tree_remove_init(mod);
>>>>   	module_arch_freeing_init(mod);
>>>>   	for_class_mod_mem_type(type, init) {
>>>> @@ -2622,8 +2624,6 @@ static noinline int do_init_module(struct module *mod)
>>>>   
>>>>   	return 0;
>>>
>>> I think it would make sense to propagate the error. But that would
>>> require changing modprobe.c. What kind of error can we expect when this
>>> happens?
>>
>> AFAIK, on powerpc it fails with EINVAL when
>> - The area is a vmalloc or module area and is a hugepage area
>> - The area is not vmalloc or io register and MMU is not powerpc radix MMU
>>
>> Otherwise it propagates the error from apply_to_existing_page_range(). 
>> IIUC it will return EINVAL when it hits a leaf PTE in upper directories.
> 
> Looking at that path I see we can also fail at __apply_to_page_range()
> -> apply_to_p4d_range() and return with -ENOMEM.
> 
> My proposal was to do something like the change below in modprobe:
> 
> diff --git a/tools/modprobe.c b/tools/modprobe.c
> index ec66e6f..8876e27 100644
> --- a/tools/modprobe.c
> +++ b/tools/modprobe.c
> @@ -572,6 +572,11 @@ static int insmod_insert(struct kmod_module *mod, int flags, const char *extra_o
>                 err = 0;
>         else {
>                 switch (err) {
> +               case -EINVAL:
> +                       ERR("module '%s'inserted: ro_after_init data might"
> +                           "still be writable (see dmesg)\n",
> +                           kmod_module_get_name(mod));
> +                       break;
>                 case -EEXIST:
>                         ERR("could not insert '%s': Module already in kernel\n",
>                             kmod_module_get_name(mod));
> 
> But I think these error codes may be also be reported in other parts
> such as simplify_symbols() so may not be a good idea after all.

It isn't really possible to make a sensible use of the return code from
init_module(), besides some basic check for -EEXIST. The problem is that
any error code from a module's init function is also propagated as
a result from the syscall.

> 
> Maybe we just need to change the default/catch all error message in
> modprobe.c and to indicate/include this case:
> 
> diff --git a/tools/modprobe.c b/tools/modprobe.c
> index ec66e6f..3647d37 100644
> --- a/tools/modprobe.c
> +++ b/tools/modprobe.c
> @@ -582,7 +582,8 @@ static int insmod_insert(struct kmod_module *mod, int flags, const char *extra_o
>                             kmod_module_get_name(mod));
>                         break;
>                 default:
> -                       ERR("could not insert '%s': %s\n", kmod_module_get_name(mod),
> +                       ERR("could not insert '%s' or inserted with error %s, "
> +                           "(see dmesg)\n", kmod_module_get_name(mod),
>                             strerror(-err));
>                         break;
>                 }
> 
> 
>>
>> On other architectures it can be different, I know some architecture try 
>> to split the pages when they hit hugepages and that can fail.
> 
> Is it worth it adding an error code for this case in case we want to
> report it back?

I feel that the proposed kernel warning about this situation is
sufficient and the loader should then return 0 to indicate that the
module got loaded. It would be more confusing to return an error but
with the module actually remaining inserted.

A module loaded without having its RO-after-init section changed
properly to RO is still fully functional. In practice, if this final
set_memory_ro() call fails, the system is already in such a state where
the additional warning is the least of the issues?

-- 
Thanks,
Petr

Re: [RFC PATCH 2/3] module: Don't fail module loading when setting ro_after_init section RO failed

[Daniel Gomez]

On 11/12/2024 3:35 PM, Petr Pavlu wrote:
> On 11/12/24 10:43, Daniel Gomez wrote:
>> On Mon Nov 11, 2024 at 7:53 PM CET, Christophe Leroy wrote:
>>>
>>>
>>> Le 09/11/2024 à 23:17, Daniel Gomez a écrit :
>>>> On Sat Nov 9, 2024 at 11:35 AM CET, Christophe Leroy wrote:
>>>>> Once module init has succeded it is too late to cancel loading.
>>>>> If setting ro_after_init data section to read-only fails, all we
>>>>> can do is to inform the user through a warning.
>>>>>
>>>>> Reported-by: Thomas Gleixner <tglx@linutronix.de>
>>>>> Closes: https://protect2.fireeye.com/v1/url?k=d3deb284-b2a35ac3-d3df39cb-74fe485fff30-288375d7d91e4ad9&q=1&e=701066ca-634d-4525-a77d-1a44451f897a&u=https%3A%2F%2Feur01.safelinks.protection.outlook.com%2F%3Furl%3Dhttps%253A%252F%252Flore.kernel.org%252Fall%252F20230915082126.4187913-1-ruanjinjie%2540huawei.com%252F%26data%3D05%257C02%257Cchristophe.leroy%2540csgroup.eu%257C26b5ca7363e54210439b08dd010c4865%257C8b87af7d86474dc78df45f69a2011bb5%257C0%257C0%257C638667874457200373%257CUnknown%257CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%253D%253D%257C0%257C%257C%257C%26sdata%3DZeJ%252F3%252B2Nx%252FBf%252FWLFEkhxKlDhZk8LNkz0fs%252Fg2xMcOjY%253D%26reserved%3D0
>>>>> Fixes: d1909c022173 ("module: Don't ignore errors from set_memory_XX()")
>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>>> ---
>>>>>    kernel/module/main.c | 6 +++---
>>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>>>>> index 2de4ad7af335..1bf4b0db291b 100644
>>>>> --- a/kernel/module/main.c
>>>>> +++ b/kernel/module/main.c
>>>>> @@ -2583,7 +2583,9 @@ static noinline int do_init_module(struct module *mod)
>>>>>    #endif
>>>>>    	ret = module_enable_rodata_ro_after_init(mod);
>>>>>    	if (ret)
>>>>> -		goto fail_mutex_unlock;
>>>>> +		pr_warn("%s: %s() returned %d, ro_after_init data might still be writable\n",
>>>>> +			mod->name, __func__, ret);
>>>>> +
>>>>>    	mod_tree_remove_init(mod);
>>>>>    	module_arch_freeing_init(mod);
>>>>>    	for_class_mod_mem_type(type, init) {
>>>>> @@ -2622,8 +2624,6 @@ static noinline int do_init_module(struct module *mod)
>>>>>    
>>>>>    	return 0;
>>>>
>>>> I think it would make sense to propagate the error. But that would
>>>> require changing modprobe.c. What kind of error can we expect when this
>>>> happens?
>>>
>>> AFAIK, on powerpc it fails with EINVAL when
>>> - The area is a vmalloc or module area and is a hugepage area
>>> - The area is not vmalloc or io register and MMU is not powerpc radix MMU
>>>
>>> Otherwise it propagates the error from apply_to_existing_page_range().
>>> IIUC it will return EINVAL when it hits a leaf PTE in upper directories.
>>
>> Looking at that path I see we can also fail at __apply_to_page_range()
>> -> apply_to_p4d_range() and return with -ENOMEM.
>>
>> My proposal was to do something like the change below in modprobe:
>>
>> diff --git a/tools/modprobe.c b/tools/modprobe.c
>> index ec66e6f..8876e27 100644
>> --- a/tools/modprobe.c
>> +++ b/tools/modprobe.c
>> @@ -572,6 +572,11 @@ static int insmod_insert(struct kmod_module *mod, int flags, const char *extra_o
>>                  err = 0;
>>          else {
>>                  switch (err) {
>> +               case -EINVAL:
>> +                       ERR("module '%s'inserted: ro_after_init data might"
>> +                           "still be writable (see dmesg)\n",
>> +                           kmod_module_get_name(mod));
>> +                       break;
>>                  case -EEXIST:
>>                          ERR("could not insert '%s': Module already in kernel\n",
>>                              kmod_module_get_name(mod));
>>
>> But I think these error codes may be also be reported in other parts
>> such as simplify_symbols() so may not be a good idea after all.
> 
> It isn't really possible to make a sensible use of the return code from
> init_module(), besides some basic check for -EEXIST. The problem is that
> any error code from a module's init function is also propagated as
> a result from the syscall.
> 
>>
>> Maybe we just need to change the default/catch all error message in
>> modprobe.c and to indicate/include this case:
>>
>> diff --git a/tools/modprobe.c b/tools/modprobe.c
>> index ec66e6f..3647d37 100644
>> --- a/tools/modprobe.c
>> +++ b/tools/modprobe.c
>> @@ -582,7 +582,8 @@ static int insmod_insert(struct kmod_module *mod, int flags, const char *extra_o
>>                              kmod_module_get_name(mod));
>>                          break;
>>                  default:
>> -                       ERR("could not insert '%s': %s\n", kmod_module_get_name(mod),
>> +                       ERR("could not insert '%s' or inserted with error %s, "
>> +                           "(see dmesg)\n", kmod_module_get_name(mod),
>>                              strerror(-err));
>>                          break;
>>                  }
>>
>>
>>>
>>> On other architectures it can be different, I know some architecture try
>>> to split the pages when they hit hugepages and that can fail.
>>
>> Is it worth it adding an error code for this case in case we want to
>> report it back?
> 
> I feel that the proposed kernel warning about this situation is
> sufficient and the loader should then return 0 to indicate that the
> module got loaded. It would be more confusing to return an error but
> with the module actually remaining inserted.
> 
> A module loaded without having its RO-after-init section changed
> properly to RO is still fully functional. In practice, if this final
> set_memory_ro() call fails, the system is already in such a state where
> the additional warning is the least of the issues?
> 

__ro_after_init is used for kernel self protection. We are loading 
"successfully" the module yes, but variables with this attribute are 
marked read-only to reduce the attack surface [1]. Since we have 
considered this stage already too late to unload the module, IMHO we 
should at least indicate that there was an error during the module 
initialization and propagate that to the loader, so it can decide the 
best action for their particular case. Warning once in the kernel log 
system, does not seem sufficient to me.

[1] Documentation/security/self-protection.rst

Re: [RFC PATCH 2/3] module: Don't fail module loading when setting ro_after_init section RO failed

[Petr Pavlu]

On 11/28/24 21:23, Daniel Gomez wrote:
> On 11/12/2024 3:35 PM, Petr Pavlu wrote:
>> On 11/12/24 10:43, Daniel Gomez wrote:
>>> On Mon Nov 11, 2024 at 7:53 PM CET, Christophe Leroy wrote:
>>>>
>>>>
>>>> Le 09/11/2024 à 23:17, Daniel Gomez a écrit :
>>>>> On Sat Nov 9, 2024 at 11:35 AM CET, Christophe Leroy wrote:
>>>>>> Once module init has succeded it is too late to cancel loading.
>>>>>> If setting ro_after_init data section to read-only fails, all we
>>>>>> can do is to inform the user through a warning.
>>>>>>
>>>>>> Reported-by: Thomas Gleixner <tglx@linutronix.de>
>>>>>> Closes: https://protect2.fireeye.com/v1/url?k=d3deb284-b2a35ac3-d3df39cb-74fe485fff30-288375d7d91e4ad9&q=1&e=701066ca-634d-4525-a77d-1a44451f897a&u=https%3A%2F%2Feur01.safelinks.protection.outlook.com%2F%3Furl%3Dhttps%253A%252F%252Flore.kernel.org%252Fall%252F20230915082126.4187913-1-ruanjinjie%2540huawei.com%252F%26data%3D05%257C02%257Cchristophe.leroy%2540csgroup.eu%257C26b5ca7363e54210439b08dd010c4865%257C8b87af7d86474dc78df45f69a2011bb5%257C0%257C0%257C638667874457200373%257CUnknown%257CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%253D%253D%257C0%257C%257C%257C%26sdata%3DZeJ%252F3%252B2Nx%252FBf%252FWLFEkhxKlDhZk8LNkz0fs%252Fg2xMcOjY%253D%26reserved%3D0
>>>>>> Fixes: d1909c022173 ("module: Don't ignore errors from set_memory_XX()")
>>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>>>> ---
>>>>>>    kernel/module/main.c | 6 +++---
>>>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>>>>>> index 2de4ad7af335..1bf4b0db291b 100644
>>>>>> --- a/kernel/module/main.c
>>>>>> +++ b/kernel/module/main.c
>>>>>> @@ -2583,7 +2583,9 @@ static noinline int do_init_module(struct module *mod)
>>>>>>    #endif
>>>>>>    	ret = module_enable_rodata_ro_after_init(mod);
>>>>>>    	if (ret)
>>>>>> -		goto fail_mutex_unlock;
>>>>>> +		pr_warn("%s: %s() returned %d, ro_after_init data might still be writable\n",
>>>>>> +			mod->name, __func__, ret);
>>>>>> +
>>>>>>    	mod_tree_remove_init(mod);
>>>>>>    	module_arch_freeing_init(mod);
>>>>>>    	for_class_mod_mem_type(type, init) {
>>>>>> @@ -2622,8 +2624,6 @@ static noinline int do_init_module(struct module *mod)
>>>>>>    
>>>>>>    	return 0;
>>>>>
>>>>> I think it would make sense to propagate the error. But that would
>>>>> require changing modprobe.c. What kind of error can we expect when this
>>>>> happens?
>>>>
>>>> AFAIK, on powerpc it fails with EINVAL when
>>>> - The area is a vmalloc or module area and is a hugepage area
>>>> - The area is not vmalloc or io register and MMU is not powerpc radix MMU
>>>>
>>>> Otherwise it propagates the error from apply_to_existing_page_range().
>>>> IIUC it will return EINVAL when it hits a leaf PTE in upper directories.
>>>
>>> Looking at that path I see we can also fail at __apply_to_page_range()
>>> -> apply_to_p4d_range() and return with -ENOMEM.
>>>
>>> My proposal was to do something like the change below in modprobe:
>>>
>>> diff --git a/tools/modprobe.c b/tools/modprobe.c
>>> index ec66e6f..8876e27 100644
>>> --- a/tools/modprobe.c
>>> +++ b/tools/modprobe.c
>>> @@ -572,6 +572,11 @@ static int insmod_insert(struct kmod_module *mod, int flags, const char *extra_o
>>>                  err = 0;
>>>          else {
>>>                  switch (err) {
>>> +               case -EINVAL:
>>> +                       ERR("module '%s'inserted: ro_after_init data might"
>>> +                           "still be writable (see dmesg)\n",
>>> +                           kmod_module_get_name(mod));
>>> +                       break;
>>>                  case -EEXIST:
>>>                          ERR("could not insert '%s': Module already in kernel\n",
>>>                              kmod_module_get_name(mod));
>>>
>>> But I think these error codes may be also be reported in other parts
>>> such as simplify_symbols() so may not be a good idea after all.
>>
>> It isn't really possible to make a sensible use of the return code from
>> init_module(), besides some basic check for -EEXIST. The problem is that
>> any error code from a module's init function is also propagated as
>> a result from the syscall.
>>
>>>
>>> Maybe we just need to change the default/catch all error message in
>>> modprobe.c and to indicate/include this case:
>>>
>>> diff --git a/tools/modprobe.c b/tools/modprobe.c
>>> index ec66e6f..3647d37 100644
>>> --- a/tools/modprobe.c
>>> +++ b/tools/modprobe.c
>>> @@ -582,7 +582,8 @@ static int insmod_insert(struct kmod_module *mod, int flags, const char *extra_o
>>>                              kmod_module_get_name(mod));
>>>                          break;
>>>                  default:
>>> -                       ERR("could not insert '%s': %s\n", kmod_module_get_name(mod),
>>> +                       ERR("could not insert '%s' or inserted with error %s, "
>>> +                           "(see dmesg)\n", kmod_module_get_name(mod),
>>>                              strerror(-err));
>>>                          break;
>>>                  }
>>>
>>>
>>>>
>>>> On other architectures it can be different, I know some architecture try
>>>> to split the pages when they hit hugepages and that can fail.
>>>
>>> Is it worth it adding an error code for this case in case we want to
>>> report it back?
>>
>> I feel that the proposed kernel warning about this situation is
>> sufficient and the loader should then return 0 to indicate that the
>> module got loaded. It would be more confusing to return an error but
>> with the module actually remaining inserted.
>>
>> A module loaded without having its RO-after-init section changed
>> properly to RO is still fully functional. In practice, if this final
>> set_memory_ro() call fails, the system is already in such a state where
>> the additional warning is the least of the issues?
>>
> 
> __ro_after_init is used for kernel self protection. We are loading 
> "successfully" the module yes, but variables with this attribute are 
> marked read-only to reduce the attack surface [1]. Since we have 
> considered this stage already too late to unload the module, IMHO we 
> should at least indicate that there was an error during the module 
> initialization and propagate that to the loader, so it can decide the 
> best action for their particular case. Warning once in the kernel log 
> system, does not seem sufficient to me.
> 
> [1] Documentation/security/self-protection.rst

I'd be careful about introducing this new state where (f)init_module()
returns an error, yet the module actually gets loaded.

The init_module() interface has one return value which can originate
from anywhere during the load process, including from the module's own
init function. As mentioned, this means that the userspace cannot
distinguish why the module load failed. It would be needed to have
a specific error code returned only for this case, or to extend the
interface further in some way.

Another concern is consistency of the module loader interface as
a whole. Returning an error from init_module() in this case would mean
that only the specific caller is made aware that the module was loaded
with some issues. A different task that then decides to check the module
state under /sys/module would see it as normally loaded, and similarly a
task trying to insert it again would get -EEXIST. That likely would need
changing too.

What I'd like to understand is how reporting this as an error to the
userspace would help in practice. I think the userspace can decide to
report it as a warning and continue, or treat is as a hard problem and
stop the system? I would expect that most tools/systems would opt for
the former, but then this doesn't seem much different to me than when
the kernel produces the warning itself. The second option, with some
stretch, could be implemented with panic_on_warn=1.

Do you envision that the userspace would handle this problem differently
and it is worth adding the complexity?

As a side node, I've noticed that the module loader could mark also
static_call sections as ro_after_init. I'll post patches for that.
Additionally, both __jump_table and static_call sections could be
possibly turned RO earlier, after prepare_coming_module() ->
blocking_notifier_call_chain_robust() -> ... ->
jump_label_add_module()/static_call_add_module().

-- 
Thanks,
Petr

Re: [RFC PATCH 2/3] module: Don't fail module loading when setting ro_after_init section RO failed

[Daniel Gomez]

On 12/4/2024 4:14 PM, Petr Pavlu wrote:
> On 11/28/24 21:23, Daniel Gomez wrote:
>> On 11/12/2024 3:35 PM, Petr Pavlu wrote:
>>> On 11/12/24 10:43, Daniel Gomez wrote:
>>>> On Mon Nov 11, 2024 at 7:53 PM CET, Christophe Leroy wrote:
>>>>>
>>>>>
>>>>> Le 09/11/2024 à 23:17, Daniel Gomez a écrit :
>>>>>> On Sat Nov 9, 2024 at 11:35 AM CET, Christophe Leroy wrote:
>>>>>>> Once module init has succeded it is too late to cancel loading.
>>>>>>> If setting ro_after_init data section to read-only fails, all we
>>>>>>> can do is to inform the user through a warning.
>>>>>>>
>>>>>>> Reported-by: Thomas Gleixner <tglx@linutronix.de>
>>>>>>> Closes: https://protect2.fireeye.com/v1/url?k=d3deb284-b2a35ac3-d3df39cb-74fe485fff30-288375d7d91e4ad9&q=1&e=701066ca-634d-4525-a77d-1a44451f897a&u=https%3A%2F%2Feur01.safelinks.protection.outlook.com%2F%3Furl%3Dhttps%253A%252F%252Flore.kernel.org%252Fall%252F20230915082126.4187913-1-ruanjinjie%2540huawei.com%252F%26data%3D05%257C02%257Cchristophe.leroy%2540csgroup.eu%257C26b5ca7363e54210439b08dd010c4865%257C8b87af7d86474dc78df45f69a2011bb5%257C0%257C0%257C638667874457200373%257CUnknown%257CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%253D%253D%257C0%257C%257C%257C%26sdata%3DZeJ%252F3%252B2Nx%252FBf%252FWLFEkhxKlDhZk8LNkz0fs%252Fg2xMcOjY%253D%26reserved%3D0
>>>>>>> Fixes: d1909c022173 ("module: Don't ignore errors from set_memory_XX()")
>>>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>>>>> ---
>>>>>>>     kernel/module/main.c | 6 +++---
>>>>>>>     1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>>>>>>> index 2de4ad7af335..1bf4b0db291b 100644
>>>>>>> --- a/kernel/module/main.c
>>>>>>> +++ b/kernel/module/main.c
>>>>>>> @@ -2583,7 +2583,9 @@ static noinline int do_init_module(struct module *mod)
>>>>>>>     #endif
>>>>>>>     	ret = module_enable_rodata_ro_after_init(mod);
>>>>>>>     	if (ret)
>>>>>>> -		goto fail_mutex_unlock;
>>>>>>> +		pr_warn("%s: %s() returned %d, ro_after_init data might still be writable\n",
>>>>>>> +			mod->name, __func__, ret);
>>>>>>> +
>>>>>>>     	mod_tree_remove_init(mod);
>>>>>>>     	module_arch_freeing_init(mod);
>>>>>>>     	for_class_mod_mem_type(type, init) {
>>>>>>> @@ -2622,8 +2624,6 @@ static noinline int do_init_module(struct module *mod)
>>>>>>>     
>>>>>>>     	return 0;
>>>>>>
>>>>>> I think it would make sense to propagate the error. But that would
>>>>>> require changing modprobe.c. What kind of error can we expect when this
>>>>>> happens?
>>>>>
>>>>> AFAIK, on powerpc it fails with EINVAL when
>>>>> - The area is a vmalloc or module area and is a hugepage area
>>>>> - The area is not vmalloc or io register and MMU is not powerpc radix MMU
>>>>>
>>>>> Otherwise it propagates the error from apply_to_existing_page_range().
>>>>> IIUC it will return EINVAL when it hits a leaf PTE in upper directories.
>>>>
>>>> Looking at that path I see we can also fail at __apply_to_page_range()
>>>> -> apply_to_p4d_range() and return with -ENOMEM.
>>>>
>>>> My proposal was to do something like the change below in modprobe:
>>>>
>>>> diff --git a/tools/modprobe.c b/tools/modprobe.c
>>>> index ec66e6f..8876e27 100644
>>>> --- a/tools/modprobe.c
>>>> +++ b/tools/modprobe.c
>>>> @@ -572,6 +572,11 @@ static int insmod_insert(struct kmod_module *mod, int flags, const char *extra_o
>>>>                   err = 0;
>>>>           else {
>>>>                   switch (err) {
>>>> +               case -EINVAL:
>>>> +                       ERR("module '%s'inserted: ro_after_init data might"
>>>> +                           "still be writable (see dmesg)\n",
>>>> +                           kmod_module_get_name(mod));
>>>> +                       break;
>>>>                   case -EEXIST:
>>>>                           ERR("could not insert '%s': Module already in kernel\n",
>>>>                               kmod_module_get_name(mod));
>>>>
>>>> But I think these error codes may be also be reported in other parts
>>>> such as simplify_symbols() so may not be a good idea after all.
>>>
>>> It isn't really possible to make a sensible use of the return code from
>>> init_module(), besides some basic check for -EEXIST. The problem is that
>>> any error code from a module's init function is also propagated as
>>> a result from the syscall.
>>>
>>>>
>>>> Maybe we just need to change the default/catch all error message in
>>>> modprobe.c and to indicate/include this case:
>>>>
>>>> diff --git a/tools/modprobe.c b/tools/modprobe.c
>>>> index ec66e6f..3647d37 100644
>>>> --- a/tools/modprobe.c
>>>> +++ b/tools/modprobe.c
>>>> @@ -582,7 +582,8 @@ static int insmod_insert(struct kmod_module *mod, int flags, const char *extra_o
>>>>                               kmod_module_get_name(mod));
>>>>                           break;
>>>>                   default:
>>>> -                       ERR("could not insert '%s': %s\n", kmod_module_get_name(mod),
>>>> +                       ERR("could not insert '%s' or inserted with error %s, "
>>>> +                           "(see dmesg)\n", kmod_module_get_name(mod),
>>>>                               strerror(-err));
>>>>                           break;
>>>>                   }
>>>>
>>>>
>>>>>
>>>>> On other architectures it can be different, I know some architecture try
>>>>> to split the pages when they hit hugepages and that can fail.
>>>>
>>>> Is it worth it adding an error code for this case in case we want to
>>>> report it back?
>>>
>>> I feel that the proposed kernel warning about this situation is
>>> sufficient and the loader should then return 0 to indicate that the
>>> module got loaded. It would be more confusing to return an error but
>>> with the module actually remaining inserted.
>>>
>>> A module loaded without having its RO-after-init section changed
>>> properly to RO is still fully functional. In practice, if this final
>>> set_memory_ro() call fails, the system is already in such a state where
>>> the additional warning is the least of the issues?
>>>
>>
>> __ro_after_init is used for kernel self protection. We are loading
>> "successfully" the module yes, but variables with this attribute are
>> marked read-only to reduce the attack surface [1]. Since we have
>> considered this stage already too late to unload the module, IMHO we
>> should at least indicate that there was an error during the module
>> initialization and propagate that to the loader, so it can decide the
>> best action for their particular case. Warning once in the kernel log
>> system, does not seem sufficient to me.
>>
>> [1] Documentation/security/self-protection.rst
> 
> I'd be careful about introducing this new state where (f)init_module()
> returns an error, yet the module actually gets loaded.

Perhaps we just need this new stage? module loaded with 
permission/security error?

> 
> The init_module() interface has one return value which can originate
> from anywhere during the load process, including from the module's own
> init function. As mentioned, this means that the userspace cannot
> distinguish why the module load failed. It would be needed to have
> a specific error code returned only for this case, or to extend the
> interface further in some way.
> 
> Another concern is consistency of the module loader interface as
> a whole. Returning an error from init_module() in this case would mean
> that only the specific caller is made aware that the module was loaded
> with some issues. A different task that then decides to check the module
> state under /sys/module would see it as normally loaded, and similarly a

Maybe we need to change this state too to indicate the problem.

> task trying to insert it again would get -EEXIST. That likely would need
> changing too.
> 
> What I'd like to understand is how reporting this as an error to the
> userspace would help in practice. I think the userspace can decide to
> report it as a warning and continue, or treat is as a hard problem and
> stop the system? I would expect that most tools/systems would opt for
> the former, but then this doesn't seem much different to me than when
> the kernel produces the warning itself. The second option, with some
> stretch, could be implemented with panic_on_warn=1.

Ideally, we would reverse the module initialization when encountering 
this error [1]. However, since it's not feasible to undo it correctly at 
this stage, reporting the error back to the caller allows them to assess 
and decide whether to accept the risk.

[1] https://lore.kernel.org/all/Zuv0nmFblHUwuT8v@bombadil.infradead.org/
> 
> Do you envision that the userspace would handle this problem differently
> and it is worth adding the complexity?

What complexity do you mean?

A module driver has ro_after_init for the purpose of protecting the 
kernel from attack [2]. If we ignore it by warning once, the caller will 
not be aware of such risk (unless the caller it's parsing the kernel log).

[2] 
https://lore.kernel.org/all/1455748879-21872-1-git-send-email-keescook@chromium.org/

Another option could be adding a kconfig to decide to report or not 
which I would strongly suggest to report by default.


> 
> As a side node, I've noticed that the module loader could mark also
> static_call sections as ro_after_init. I'll post patches for that.
> Additionally, both __jump_table and static_call sections could be
> possibly turned RO earlier, after prepare_coming_module() ->
> blocking_notifier_call_chain_robust() -> ... ->
> jump_label_add_module()/static_call_add_module().
> 

Re: [RFC PATCH 2/3] module: Don't fail module loading when setting ro_after_init section RO failed

[Daniel Gomez]

Hi Kees,


Could you help clarify the handling of __ro_after_init? What do you 
think is the best approach when a second attempt fails at setting a 
section to RO after a module is already initialized? (please find the 
deatils in this pach series or in [1]. Reporting the failure to the 
caller seems logical to me but adds some complexity. On the other hand, 
logging alone feels insufficient but may be the simplest option. Could 
you advice on handling this corner case and if it's relevant to KSPP?

You can find below the conversation. And the v1 PATCH series here [1].

[1] 
https://lore.kernel.org/all/cover.1733427536.git.christophe.leroy@csgroup.eu/

Thanks,
Daniel


On 12/10/2024 11:49 AM, Daniel Gomez wrote:
> On 12/4/2024 4:14 PM, Petr Pavlu wrote:
>> On 11/28/24 21:23, Daniel Gomez wrote:
>>> On 11/12/2024 3:35 PM, Petr Pavlu wrote:
>>>> On 11/12/24 10:43, Daniel Gomez wrote:
>>>>> On Mon Nov 11, 2024 at 7:53 PM CET, Christophe Leroy wrote:
>>>>>>
>>>>>>
>>>>>> Le 09/11/2024 à 23:17, Daniel Gomez a écrit :
>>>>>>> On Sat Nov 9, 2024 at 11:35 AM CET, Christophe Leroy wrote:
>>>>>>>> Once module init has succeded it is too late to cancel loading.
>>>>>>>> If setting ro_after_init data section to read-only fails, all we
>>>>>>>> can do is to inform the user through a warning.
>>>>>>>>
>>>>>>>> Reported-by: Thomas Gleixner <tglx@linutronix.de>
>>>>>>>> Closes: https://protect2.fireeye.com/v1/url?k=d3deb284-b2a35ac3- 
>>>>>>>> d3df39cb-74fe485fff30-288375d7d91e4ad9&q=1&e=701066ca-634d-4525- 
>>>>>>>> a77d-1a44451f897a&u=https%3A%2F%2Feur01.safelinks.protection.outlook.com%2F%3Furl%3Dhttps%253A%252F%252Flore.kernel.org%252Fall%252F20230915082126.4187913-1-ruanjinjie%2540huawei.com%252F%26data%3D05%257C02%257Cchristophe.leroy%2540csgroup.eu%257C26b5ca7363e54210439b08dd010c4865%257C8b87af7d86474dc78df45f69a2011bb5%257C0%257C0%257C638667874457200373%257CUnknown%257CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%253D%253D%257C0%257C%257C%257C%26sdata%3DZeJ%252F3%252B2Nx%252FBf%252FWLFEkhxKlDhZk8LNkz0fs%252Fg2xMcOjY%253D%26reserved%3D0
>>>>>>>> Fixes: d1909c022173 ("module: Don't ignore errors from 
>>>>>>>> set_memory_XX()")
>>>>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>>>>>> ---
>>>>>>>>     kernel/module/main.c | 6 +++---
>>>>>>>>     1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>>>>>>>> index 2de4ad7af335..1bf4b0db291b 100644
>>>>>>>> --- a/kernel/module/main.c
>>>>>>>> +++ b/kernel/module/main.c
>>>>>>>> @@ -2583,7 +2583,9 @@ static noinline int do_init_module(struct 
>>>>>>>> module *mod)
>>>>>>>>     #endif
>>>>>>>>         ret = module_enable_rodata_ro_after_init(mod);
>>>>>>>>         if (ret)
>>>>>>>> -        goto fail_mutex_unlock;
>>>>>>>> +        pr_warn("%s: %s() returned %d, ro_after_init data might 
>>>>>>>> still be writable\n",
>>>>>>>> +            mod->name, __func__, ret);
>>>>>>>> +
>>>>>>>>         mod_tree_remove_init(mod);
>>>>>>>>         module_arch_freeing_init(mod);
>>>>>>>>         for_class_mod_mem_type(type, init) {
>>>>>>>> @@ -2622,8 +2624,6 @@ static noinline int do_init_module(struct 
>>>>>>>> module *mod)
>>>>>>>>         return 0;
>>>>>>>
>>>>>>> I think it would make sense to propagate the error. But that would
>>>>>>> require changing modprobe.c. What kind of error can we expect 
>>>>>>> when this
>>>>>>> happens?
>>>>>>
>>>>>> AFAIK, on powerpc it fails with EINVAL when
>>>>>> - The area is a vmalloc or module area and is a hugepage area
>>>>>> - The area is not vmalloc or io register and MMU is not powerpc 
>>>>>> radix MMU
>>>>>>
>>>>>> Otherwise it propagates the error from 
>>>>>> apply_to_existing_page_range().
>>>>>> IIUC it will return EINVAL when it hits a leaf PTE in upper 
>>>>>> directories.
>>>>>
>>>>> Looking at that path I see we can also fail at __apply_to_page_range()
>>>>> -> apply_to_p4d_range() and return with -ENOMEM.
>>>>>
>>>>> My proposal was to do something like the change below in modprobe:
>>>>>
>>>>> diff --git a/tools/modprobe.c b/tools/modprobe.c
>>>>> index ec66e6f..8876e27 100644
>>>>> --- a/tools/modprobe.c
>>>>> +++ b/tools/modprobe.c
>>>>> @@ -572,6 +572,11 @@ static int insmod_insert(struct kmod_module 
>>>>> *mod, int flags, const char *extra_o
>>>>>                   err = 0;
>>>>>           else {
>>>>>                   switch (err) {
>>>>> +               case -EINVAL:
>>>>> +                       ERR("module '%s'inserted: ro_after_init 
>>>>> data might"
>>>>> +                           "still be writable (see dmesg)\n",
>>>>> +                           kmod_module_get_name(mod));
>>>>> +                       break;
>>>>>                   case -EEXIST:
>>>>>                           ERR("could not insert '%s': Module 
>>>>> already in kernel\n",
>>>>>                               kmod_module_get_name(mod));
>>>>>
>>>>> But I think these error codes may be also be reported in other parts
>>>>> such as simplify_symbols() so may not be a good idea after all.
>>>>
>>>> It isn't really possible to make a sensible use of the return code from
>>>> init_module(), besides some basic check for -EEXIST. The problem is 
>>>> that
>>>> any error code from a module's init function is also propagated as
>>>> a result from the syscall.
>>>>
>>>>>
>>>>> Maybe we just need to change the default/catch all error message in
>>>>> modprobe.c and to indicate/include this case:
>>>>>
>>>>> diff --git a/tools/modprobe.c b/tools/modprobe.c
>>>>> index ec66e6f..3647d37 100644
>>>>> --- a/tools/modprobe.c
>>>>> +++ b/tools/modprobe.c
>>>>> @@ -582,7 +582,8 @@ static int insmod_insert(struct kmod_module 
>>>>> *mod, int flags, const char *extra_o
>>>>>                               kmod_module_get_name(mod));
>>>>>                           break;
>>>>>                   default:
>>>>> -                       ERR("could not insert '%s': %s\n", 
>>>>> kmod_module_get_name(mod),
>>>>> +                       ERR("could not insert '%s' or inserted with 
>>>>> error %s, "
>>>>> +                           "(see dmesg)\n", 
>>>>> kmod_module_get_name(mod),
>>>>>                               strerror(-err));
>>>>>                           break;
>>>>>                   }
>>>>>
>>>>>
>>>>>>
>>>>>> On other architectures it can be different, I know some 
>>>>>> architecture try
>>>>>> to split the pages when they hit hugepages and that can fail.
>>>>>
>>>>> Is it worth it adding an error code for this case in case we want to
>>>>> report it back?
>>>>
>>>> I feel that the proposed kernel warning about this situation is
>>>> sufficient and the loader should then return 0 to indicate that the
>>>> module got loaded. It would be more confusing to return an error but
>>>> with the module actually remaining inserted.
>>>>
>>>> A module loaded without having its RO-after-init section changed
>>>> properly to RO is still fully functional. In practice, if this final
>>>> set_memory_ro() call fails, the system is already in such a state where
>>>> the additional warning is the least of the issues?
>>>>
>>>
>>> __ro_after_init is used for kernel self protection. We are loading
>>> "successfully" the module yes, but variables with this attribute are
>>> marked read-only to reduce the attack surface [1]. Since we have
>>> considered this stage already too late to unload the module, IMHO we
>>> should at least indicate that there was an error during the module
>>> initialization and propagate that to the loader, so it can decide the
>>> best action for their particular case. Warning once in the kernel log
>>> system, does not seem sufficient to me.
>>>
>>> [1] Documentation/security/self-protection.rst
>>
>> I'd be careful about introducing this new state where (f)init_module()
>> returns an error, yet the module actually gets loaded.
> 
> Perhaps we just need this new stage? module loaded with permission/ 
> security error?
> 
>>
>> The init_module() interface has one return value which can originate
>> from anywhere during the load process, including from the module's own
>> init function. As mentioned, this means that the userspace cannot
>> distinguish why the module load failed. It would be needed to have
>> a specific error code returned only for this case, or to extend the
>> interface further in some way.
>>
>> Another concern is consistency of the module loader interface as
>> a whole. Returning an error from init_module() in this case would mean
>> that only the specific caller is made aware that the module was loaded
>> with some issues. A different task that then decides to check the module
>> state under /sys/module would see it as normally loaded, and similarly a
> 
> Maybe we need to change this state too to indicate the problem.
> 
>> task trying to insert it again would get -EEXIST. That likely would need
>> changing too.
>>
>> What I'd like to understand is how reporting this as an error to the
>> userspace would help in practice. I think the userspace can decide to
>> report it as a warning and continue, or treat is as a hard problem and
>> stop the system? I would expect that most tools/systems would opt for
>> the former, but then this doesn't seem much different to me than when
>> the kernel produces the warning itself. The second option, with some
>> stretch, could be implemented with panic_on_warn=1.
> 
> Ideally, we would reverse the module initialization when encountering 
> this error [1]. However, since it's not feasible to undo it correctly at 
> this stage, reporting the error back to the caller allows them to assess 
> and decide whether to accept the risk.
> 
> [1] https://lore.kernel.org/all/Zuv0nmFblHUwuT8v@bombadil.infradead.org/
>>
>> Do you envision that the userspace would handle this problem differently
>> and it is worth adding the complexity?
> 
> What complexity do you mean?
> 
> A module driver has ro_after_init for the purpose of protecting the 
> kernel from attack [2]. If we ignore it by warning once, the caller will 
> not be aware of such risk (unless the caller it's parsing the kernel log).
> 
> [2] https://lore.kernel.org/all/1455748879-21872-1-git-send-email- 
> keescook@chromium.org/
> 
> Another option could be adding a kconfig to decide to report or not 
> which I would strongly suggest to report by default.
> 
> 
>>
>> As a side node, I've noticed that the module loader could mark also
>> static_call sections as ro_after_init. I'll post patches for that.
>> Additionally, both __jump_table and static_call sections could be
>> possibly turned RO earlier, after prepare_coming_module() ->
>> blocking_notifier_call_chain_robust() -> ... ->
>> jump_label_add_module()/static_call_add_module().
>>
> 

Re: [RFC PATCH 2/3] module: Don't fail module loading when setting ro_after_init section RO failed

[Petr Pavlu]

On 12/10/24 11:49, Daniel Gomez wrote:
> On 12/4/2024 4:14 PM, Petr Pavlu wrote:
>> On 11/28/24 21:23, Daniel Gomez wrote:
>>> On 11/12/2024 3:35 PM, Petr Pavlu wrote:
>>>> On 11/12/24 10:43, Daniel Gomez wrote:
>>>>> On Mon Nov 11, 2024 at 7:53 PM CET, Christophe Leroy wrote:
>>>>>>
>>>>>>
>>>>>> Le 09/11/2024 à 23:17, Daniel Gomez a écrit :
>>>>>>> On Sat Nov 9, 2024 at 11:35 AM CET, Christophe Leroy wrote:
>>>>>>>> Once module init has succeded it is too late to cancel loading.
>>>>>>>> If setting ro_after_init data section to read-only fails, all we
>>>>>>>> can do is to inform the user through a warning.
>>>>>>>>
>>>>>>>> Reported-by: Thomas Gleixner <tglx@linutronix.de>
>>>>>>>> Closes: https://protect2.fireeye.com/v1/url?k=d3deb284-b2a35ac3-d3df39cb-74fe485fff30-288375d7d91e4ad9&q=1&e=701066ca-634d-4525-a77d-1a44451f897a&u=https%3A%2F%2Feur01.safelinks.protection.outlook.com%2F%3Furl%3Dhttps%253A%252F%252Flore.kernel.org%252Fall%252F20230915082126.4187913-1-ruanjinjie%2540huawei.com%252F%26data%3D05%257C02%257Cchristophe.leroy%2540csgroup.eu%257C26b5ca7363e54210439b08dd010c4865%257C8b87af7d86474dc78df45f69a2011bb5%257C0%257C0%257C638667874457200373%257CUnknown%257CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%253D%253D%257C0%257C%257C%257C%26sdata%3DZeJ%252F3%252B2Nx%252FBf%252FWLFEkhxKlDhZk8LNkz0fs%252Fg2xMcOjY%253D%26reserved%3D0
>>>>>>>> Fixes: d1909c022173 ("module: Don't ignore errors from set_memory_XX()")
>>>>>>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>>>>>>> ---
>>>>>>>>     kernel/module/main.c | 6 +++---
>>>>>>>>     1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>>>>>>>> index 2de4ad7af335..1bf4b0db291b 100644
>>>>>>>> --- a/kernel/module/main.c
>>>>>>>> +++ b/kernel/module/main.c
>>>>>>>> @@ -2583,7 +2583,9 @@ static noinline int do_init_module(struct module *mod)
>>>>>>>>     #endif
>>>>>>>>     	ret = module_enable_rodata_ro_after_init(mod);
>>>>>>>>     	if (ret)
>>>>>>>> -		goto fail_mutex_unlock;
>>>>>>>> +		pr_warn("%s: %s() returned %d, ro_after_init data might still be writable\n",
>>>>>>>> +			mod->name, __func__, ret);
>>>>>>>> +
>>>>>>>>     	mod_tree_remove_init(mod);
>>>>>>>>     	module_arch_freeing_init(mod);
>>>>>>>>     	for_class_mod_mem_type(type, init) {
>>>>>>>> @@ -2622,8 +2624,6 @@ static noinline int do_init_module(struct module *mod)
>>>>>>>>     
>>>>>>>>     	return 0;
>>>>>>>
>>>>>>> I think it would make sense to propagate the error. But that would
>>>>>>> require changing modprobe.c. What kind of error can we expect when this
>>>>>>> happens?
>>>>>>
>>>>>> AFAIK, on powerpc it fails with EINVAL when
>>>>>> - The area is a vmalloc or module area and is a hugepage area
>>>>>> - The area is not vmalloc or io register and MMU is not powerpc radix MMU
>>>>>>
>>>>>> Otherwise it propagates the error from apply_to_existing_page_range().
>>>>>> IIUC it will return EINVAL when it hits a leaf PTE in upper directories.
>>>>>
>>>>> Looking at that path I see we can also fail at __apply_to_page_range()
>>>>> -> apply_to_p4d_range() and return with -ENOMEM.
>>>>>
>>>>> My proposal was to do something like the change below in modprobe:
>>>>>
>>>>> diff --git a/tools/modprobe.c b/tools/modprobe.c
>>>>> index ec66e6f..8876e27 100644
>>>>> --- a/tools/modprobe.c
>>>>> +++ b/tools/modprobe.c
>>>>> @@ -572,6 +572,11 @@ static int insmod_insert(struct kmod_module *mod, int flags, const char *extra_o
>>>>>                   err = 0;
>>>>>           else {
>>>>>                   switch (err) {
>>>>> +               case -EINVAL:
>>>>> +                       ERR("module '%s'inserted: ro_after_init data might"
>>>>> +                           "still be writable (see dmesg)\n",
>>>>> +                           kmod_module_get_name(mod));
>>>>> +                       break;
>>>>>                   case -EEXIST:
>>>>>                           ERR("could not insert '%s': Module already in kernel\n",
>>>>>                               kmod_module_get_name(mod));
>>>>>
>>>>> But I think these error codes may be also be reported in other parts
>>>>> such as simplify_symbols() so may not be a good idea after all.
>>>>
>>>> It isn't really possible to make a sensible use of the return code from
>>>> init_module(), besides some basic check for -EEXIST. The problem is that
>>>> any error code from a module's init function is also propagated as
>>>> a result from the syscall.
>>>>
>>>>>
>>>>> Maybe we just need to change the default/catch all error message in
>>>>> modprobe.c and to indicate/include this case:
>>>>>
>>>>> diff --git a/tools/modprobe.c b/tools/modprobe.c
>>>>> index ec66e6f..3647d37 100644
>>>>> --- a/tools/modprobe.c
>>>>> +++ b/tools/modprobe.c
>>>>> @@ -582,7 +582,8 @@ static int insmod_insert(struct kmod_module *mod, int flags, const char *extra_o
>>>>>                               kmod_module_get_name(mod));
>>>>>                           break;
>>>>>                   default:
>>>>> -                       ERR("could not insert '%s': %s\n", kmod_module_get_name(mod),
>>>>> +                       ERR("could not insert '%s' or inserted with error %s, "
>>>>> +                           "(see dmesg)\n", kmod_module_get_name(mod),
>>>>>                               strerror(-err));
>>>>>                           break;
>>>>>                   }
>>>>>
>>>>>
>>>>>>
>>>>>> On other architectures it can be different, I know some architecture try
>>>>>> to split the pages when they hit hugepages and that can fail.
>>>>>
>>>>> Is it worth it adding an error code for this case in case we want to
>>>>> report it back?
>>>>
>>>> I feel that the proposed kernel warning about this situation is
>>>> sufficient and the loader should then return 0 to indicate that the
>>>> module got loaded. It would be more confusing to return an error but
>>>> with the module actually remaining inserted.
>>>>
>>>> A module loaded without having its RO-after-init section changed
>>>> properly to RO is still fully functional. In practice, if this final
>>>> set_memory_ro() call fails, the system is already in such a state where
>>>> the additional warning is the least of the issues?
>>>>
>>>
>>> __ro_after_init is used for kernel self protection. We are loading
>>> "successfully" the module yes, but variables with this attribute are
>>> marked read-only to reduce the attack surface [1]. Since we have
>>> considered this stage already too late to unload the module, IMHO we
>>> should at least indicate that there was an error during the module
>>> initialization and propagate that to the loader, so it can decide the
>>> best action for their particular case. Warning once in the kernel log
>>> system, does not seem sufficient to me.
>>>
>>> [1] Documentation/security/self-protection.rst
>>
>> I'd be careful about introducing this new state where (f)init_module()
>> returns an error, yet the module actually gets loaded.
> 
> Perhaps we just need this new stage? module loaded with 
> permission/security error?
> 
>>
>> The init_module() interface has one return value which can originate
>> from anywhere during the load process, including from the module's own
>> init function. As mentioned, this means that the userspace cannot
>> distinguish why the module load failed. It would be needed to have
>> a specific error code returned only for this case, or to extend the
>> interface further in some way.
>>
>> Another concern is consistency of the module loader interface as
>> a whole. Returning an error from init_module() in this case would mean
>> that only the specific caller is made aware that the module was loaded
>> with some issues. A different task that then decides to check the module
>> state under /sys/module would see it as normally loaded, and similarly a
> 
> Maybe we need to change this state too to indicate the problem.
> 
>> task trying to insert it again would get -EEXIST. That likely would need
>> changing too.
>>
>> What I'd like to understand is how reporting this as an error to the
>> userspace would help in practice. I think the userspace can decide to
>> report it as a warning and continue, or treat is as a hard problem and
>> stop the system? I would expect that most tools/systems would opt for
>> the former, but then this doesn't seem much different to me than when
>> the kernel produces the warning itself. The second option, with some
>> stretch, could be implemented with panic_on_warn=1.
> 
> Ideally, we would reverse the module initialization when encountering 
> this error [1]. However, since it's not feasible to undo it correctly at 
> this stage, reporting the error back to the caller allows them to assess 
> and decide whether to accept the risk.
> 
> [1] https://lore.kernel.org/all/Zuv0nmFblHUwuT8v@bombadil.infradead.org/
>>
>> Do you envision that the userspace would handle this problem differently
>> and it is worth adding the complexity?
> 
> What complexity do you mean?

The complexity that I was referring to here is mainly the earlier
described limitation of the current init_module() interface and the
consistency of the module loader interface as a whole.

Another aspect is that a number of modules is loaded directly by the
kernel via request_module(). I'm not sure how the new error would be
handled in such cases. I suspect request_module() would be also only
able to log it as a kernel warning.

If I had to choose how to handle this corner case better (in long term),
I would rather try to avoid the error in the first place, potentially as
mentioned in my other reply by splitting set_memory_ro().

-- 
Thanks,
Petr

Re: [RFC PATCH 2/3] module: Don't fail module loading when setting ro_after_init section RO failed

[Christophe Leroy]

Le 03/01/2025 à 16:40, Petr Pavlu a écrit :
> On 12/10/24 11:49, Daniel Gomez wrote:>>> Do you envision that the userspace would handle this problem 
differently
>>> and it is worth adding the complexity?
>>
>> What complexity do you mean?
> 
> The complexity that I was referring to here is mainly the earlier
> described limitation of the current init_module() interface and the
> consistency of the module loader interface as a whole.
> 
> Another aspect is that a number of modules is loaded directly by the
> kernel via request_module(). I'm not sure how the new error would be
> handled in such cases. I suspect request_module() would be also only
> able to log it as a kernel warning.

And that's the same approach as for the core part of the kernel. Proper 
protection is verified by fonction rodata_test() which will just print 
an error when verification fails.

> 
> If I had to choose how to handle this corner case better (in long term),
> I would rather try to avoid the error in the first place, potentially as
> mentioned in my other reply by splitting set_memory_ro().
> 

Re: [RFC PATCH 2/3] module: Don't fail module loading when setting ro_after_init section RO failed

[Petr Pavlu]

On 11/9/24 11:35, Christophe Leroy wrote:
> Once module init has succeded it is too late to cancel loading.
> If setting ro_after_init data section to read-only fails, all we
> can do is to inform the user through a warning.

Makes sense to me. If I'm looking correctly, set_memory_ro() could
mostly fail when splitting large pages. If we wanted to fix this
cleanly, I wonder if it would be possible to divide the function into
two. The first one which does the necessary splitting, can fail and is
called prior to a module init, and the second one that eventually
updates page table attributes and is called after the init.

> 
> Reported-by: Thomas Gleixner <tglx@linutronix.de>
> Closes: https://lore.kernel.org/all/20230915082126.4187913-1-ruanjinjie@huawei.com/
> Fixes: d1909c022173 ("module: Don't ignore errors from set_memory_XX()")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  kernel/module/main.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 2de4ad7af335..1bf4b0db291b 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2583,7 +2583,9 @@ static noinline int do_init_module(struct module *mod)
>  #endif
>  	ret = module_enable_rodata_ro_after_init(mod);
>  	if (ret)
> -		goto fail_mutex_unlock;
> +		pr_warn("%s: %s() returned %d, ro_after_init data might still be writable\n",
> +			mod->name, __func__, ret);
> +

The __func__ magic constant here expands to "do_init_module" but the
message should rather say that "module_enable_rodata_ro_after_init"
failed.

>  	mod_tree_remove_init(mod);
>  	module_arch_freeing_init(mod);
>  	for_class_mod_mem_type(type, init) {
> @@ -2622,8 +2624,6 @@ static noinline int do_init_module(struct module *mod)
>  
>  	return 0;
>  
> -fail_mutex_unlock:
> -	mutex_unlock(&module_mutex);
>  fail_free_freeinit:
>  	kfree(freeinit);
>  fail:

-- 
Cheers,
Petr

[RFC PATCH 3/3] module: pre-test setting ro_after_init data read-only

[Christophe Leroy]

To be on the safe side, try to set ro_after_init data section readonly
at the same time as rodata. If it fails it will likely fail again
later so let's cancel module loading while we still can do it.
If it doesn't fail, put it back to read-only, continue module loading
and cross fingers so that it still works after module init. Then it
should in principle never fail so add a WARN_ON_ONCE() to get a big
fat warning in case it happens anyway.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 kernel/module/main.c       | 2 +-
 kernel/module/strict_rwx.c | 5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 1bf4b0db291b..b603c9647e73 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2582,7 +2582,7 @@ static noinline int do_init_module(struct module *mod)
 	rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
 #endif
 	ret = module_enable_rodata_ro_after_init(mod);
-	if (ret)
+	if (WARN_ON_ONCE(ret))
 		pr_warn("%s: %s() returned %d, ro_after_init data might still be writable\n",
 			mod->name, __func__, ret);
 
diff --git a/kernel/module/strict_rwx.c b/kernel/module/strict_rwx.c
index f68c59974ae2..329afd43f06b 100644
--- a/kernel/module/strict_rwx.c
+++ b/kernel/module/strict_rwx.c
@@ -58,7 +58,10 @@ int module_enable_rodata_ro(const struct module *mod)
 	if (ret)
 		return ret;
 
-	return 0;
+	ret = module_set_memory(mod, MOD_RO_AFTER_INIT, set_memory_ro);
+	if (ret)
+		return ret;
+	return module_set_memory(mod, MOD_RO_AFTER_INIT, set_memory_rw);
 }
 
 int module_enable_rodata_ro_after_init(const struct module *mod)
-- 
2.44.0

Re: [RFC PATCH 3/3] module: pre-test setting ro_after_init data read-only

[Daniel Gomez]

On Sat Nov 9, 2024 at 11:35 AM CET, Christophe Leroy wrote:
> To be on the safe side, try to set ro_after_init data section readonly
> at the same time as rodata. If it fails it will likely fail again
> later so let's cancel module loading while we still can do it.
> If it doesn't fail, put it back to read-only, continue module loading

I think you mean put it back to rw?

> and cross fingers so that it still works after module init. Then it
> should in principle never fail so add a WARN_ON_ONCE() to get a big
> fat warning in case it happens anyway.

I agree this is the best we can do. But I don't think there's any
guarantee that we won't fail on the second try?

>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  kernel/module/main.c       | 2 +-
>  kernel/module/strict_rwx.c | 5 ++++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 1bf4b0db291b..b603c9647e73 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2582,7 +2582,7 @@ static noinline int do_init_module(struct module *mod)
>  	rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
>  #endif
>  	ret = module_enable_rodata_ro_after_init(mod);
> -	if (ret)
> +	if (WARN_ON_ONCE(ret))
>  		pr_warn("%s: %s() returned %d, ro_after_init data might still be writable\n",
>  			mod->name, __func__, ret);
>  
> diff --git a/kernel/module/strict_rwx.c b/kernel/module/strict_rwx.c
> index f68c59974ae2..329afd43f06b 100644
> --- a/kernel/module/strict_rwx.c
> +++ b/kernel/module/strict_rwx.c
> @@ -58,7 +58,10 @@ int module_enable_rodata_ro(const struct module *mod)
>  	if (ret)
>  		return ret;
>  
> -	return 0;
> +	ret = module_set_memory(mod, MOD_RO_AFTER_INIT, set_memory_ro);
> +	if (ret)
> +		return ret;
> +	return module_set_memory(mod, MOD_RO_AFTER_INIT, set_memory_rw);
>  }
>  
>  int module_enable_rodata_ro_after_init(const struct module *mod)

Re: [RFC PATCH 3/3] module: pre-test setting ro_after_init data read-only

[Christophe Leroy]

Le 09/11/2024 à 22:03, Daniel Gomez a écrit :
> On Sat Nov 9, 2024 at 11:35 AM CET, Christophe Leroy wrote:
>> To be on the safe side, try to set ro_after_init data section readonly
>> at the same time as rodata. If it fails it will likely fail again
>> later so let's cancel module loading while we still can do it.
>> If it doesn't fail, put it back to read-only, continue module loading
> 
> I think you mean put it back to rw?

Indeed

> 
>> and cross fingers so that it still works after module init. Then it
>> should in principle never fail so add a WARN_ON_ONCE() to get a big
>> fat warning in case it happens anyway.
> 
> I agree this is the best we can do. But I don't think there's any
> guarantee that we won't fail on the second try?

I think if it works once it will always work, see my response to patch 
2/3. But I added that WARN_ON_ONCE() so that if it doesn't at least we 
know it.

Re: [RFC PATCH 3/3] module: pre-test setting ro_after_init data read-only

[Luis Chamberlain]

On Sat, Nov 09, 2024 at 11:35:37AM +0100, Christophe Leroy wrote:
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 1bf4b0db291b..b603c9647e73 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2582,7 +2582,7 @@ static noinline int do_init_module(struct module *mod)
>  	rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
>  #endif
>  	ret = module_enable_rodata_ro_after_init(mod);
> -	if (ret)
> +	if (WARN_ON_ONCE(ret))

Do we want panic on warn systems to crash with this?

  Luis

Re: [RFC PATCH 3/3] module: pre-test setting ro_after_init data read-only

[Christophe Leroy]

Le 12/11/2024 à 21:28, Luis Chamberlain a écrit :
> On Sat, Nov 09, 2024 at 11:35:37AM +0100, Christophe Leroy wrote:
>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>> index 1bf4b0db291b..b603c9647e73 100644
>> --- a/kernel/module/main.c
>> +++ b/kernel/module/main.c
>> @@ -2582,7 +2582,7 @@ static noinline int do_init_module(struct module *mod)
>>   	rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
>>   #endif
>>   	ret = module_enable_rodata_ro_after_init(mod);
>> -	if (ret)
>> +	if (WARN_ON_ONCE(ret))
> 
> Do we want panic on warn systems to crash with this?
> 

I would say yes, for two reasons:
1/ It should never happen
2/ Such systems care about security and don't want vulnerable systems

Christophe

Re: [RFC PATCH 3/3] module: pre-test setting ro_after_init data read-only

[Luis Chamberlain]

On Wed, Nov 13, 2024 at 07:49:24AM +0100, Christophe Leroy wrote:
> 
> 
> Le 12/11/2024 à 21:28, Luis Chamberlain a écrit :
> > On Sat, Nov 09, 2024 at 11:35:37AM +0100, Christophe Leroy wrote:
> > > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > > index 1bf4b0db291b..b603c9647e73 100644
> > > --- a/kernel/module/main.c
> > > +++ b/kernel/module/main.c
> > > @@ -2582,7 +2582,7 @@ static noinline int do_init_module(struct module *mod)
> > >   	rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
> > >   #endif
> > >   	ret = module_enable_rodata_ro_after_init(mod);
> > > -	if (ret)
> > > +	if (WARN_ON_ONCE(ret))
> > 
> > Do we want panic on warn systems to crash with this?
> > 
> 
> I would say yes, for two reasons:
> 1/ It should never happen
> 2/ Such systems care about security and don't want vulnerable systems

OK thanks for thinking about this.

I think making this clear in the commit log would help as people these
days scream at random WARN_ON_ONCE() sprinkles.

  Luis

Re: [RFC PATCH 1/3] module: Split module_enable_rodata_ro()

[Daniel Gomez]

On Sat Nov 9, 2024 at 11:35 AM CET, Christophe Leroy wrote:
> module_enable_rodata_ro() is called twice, once before module init
> to set rodata sections readonly and once after module init to set
> rodata_after_init section readonly.
>
> The second time, only the rodata_after_init section needs to be
> set to read-only, no need to re-apply it to already set rodata.
>
> Split module_enable_rodata_ro() in two.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

Tested-by: Daniel Gomez <da.gomez@samsung.com>

> ---
>  kernel/module/internal.h   |  3 ++-
>  kernel/module/main.c       |  4 ++--
>  kernel/module/strict_rwx.c | 13 +++++++++----
>  3 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index 2ebece8a789f..994f35a779dc 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -322,7 +322,8 @@ static inline struct module *mod_find(unsigned long addr, struct mod_tree_root *
>  }
>  #endif /* CONFIG_MODULES_TREE_LOOKUP */
>  
> -int module_enable_rodata_ro(const struct module *mod, bool after_init);
> +int module_enable_rodata_ro(const struct module *mod);
> +int module_enable_rodata_ro_after_init(const struct module *mod);
>  int module_enable_data_nx(const struct module *mod);
>  int module_enable_text_rox(const struct module *mod);
>  int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 49b9bca9de12..2de4ad7af335 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2581,7 +2581,7 @@ static noinline int do_init_module(struct module *mod)
>  	/* Switch to core kallsyms now init is done: kallsyms may be walking! */
>  	rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms);
>  #endif
> -	ret = module_enable_rodata_ro(mod, true);
> +	ret = module_enable_rodata_ro_after_init(mod);
>  	if (ret)
>  		goto fail_mutex_unlock;
>  	mod_tree_remove_init(mod);
> @@ -2751,7 +2751,7 @@ static int complete_formation(struct module *mod, struct load_info *info)
>  	module_bug_finalize(info->hdr, info->sechdrs, mod);
>  	module_cfi_finalize(info->hdr, info->sechdrs, mod);
>  
> -	err = module_enable_rodata_ro(mod, false);
> +	err = module_enable_rodata_ro(mod);
>  	if (err)
>  		goto out_strict_rwx;
>  	err = module_enable_data_nx(mod);
> diff --git a/kernel/module/strict_rwx.c b/kernel/module/strict_rwx.c
> index c45caa4690e5..f68c59974ae2 100644
> --- a/kernel/module/strict_rwx.c
> +++ b/kernel/module/strict_rwx.c
> @@ -44,7 +44,7 @@ int module_enable_text_rox(const struct module *mod)
>  	return 0;
>  }
>  
> -int module_enable_rodata_ro(const struct module *mod, bool after_init)
> +int module_enable_rodata_ro(const struct module *mod)
>  {
>  	int ret;
>  
> @@ -58,12 +58,17 @@ int module_enable_rodata_ro(const struct module *mod, bool after_init)
>  	if (ret)
>  		return ret;
>  
> -	if (after_init)
> -		return module_set_memory(mod, MOD_RO_AFTER_INIT, set_memory_ro);
> -
>  	return 0;
>  }
>  
> +int module_enable_rodata_ro_after_init(const struct module *mod)
> +{
> +	if (!IS_ENABLED(CONFIG_STRICT_MODULE_RWX) || !rodata_enabled)
> +		return 0;
> +
> +	return module_set_memory(mod, MOD_RO_AFTER_INIT, set_memory_ro);
> +}
> +
>  int module_enable_data_nx(const struct module *mod)
>  {
>  	if (!IS_ENABLED(CONFIG_STRICT_MODULE_RWX))

Re: [RFC PATCH 1/3] module: Split module_enable_rodata_ro()

[Luis Chamberlain]

On Sat, Nov 09, 2024 at 11:35:35AM +0100, Christophe Leroy wrote:
> module_enable_rodata_ro() is called twice, once before module init
> to set rodata sections readonly and once after module init to set
> rodata_after_init section readonly.
> 
> The second time, only the rodata_after_init section needs to be
> set to read-only, no need to re-apply it to already set rodata.
> 
> Split module_enable_rodata_ro() in two.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

Didn't see a respin so this will have to be a post v6.13-rc1 fix.

  Luis

Re: [RFC PATCH 1/3] module: Split module_enable_rodata_ro()

[Christophe Leroy]

Le 26/11/2024 à 20:58, Luis Chamberlain a écrit :
> On Sat, Nov 09, 2024 at 11:35:35AM +0100, Christophe Leroy wrote:
>> module_enable_rodata_ro() is called twice, once before module init
>> to set rodata sections readonly and once after module init to set
>> rodata_after_init section readonly.
>>
>> The second time, only the rodata_after_init section needs to be
>> set to read-only, no need to re-apply it to already set rodata.
>>
>> Split module_enable_rodata_ro() in two.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> 
> Didn't see a respin so this will have to be a post v6.13-rc1 fix.

Indeed I was waiting for v6.13-rc1 before sending the non RFC rebased 
version, but I can send it now if you prefer.

I expect it to spend a few days in linux-next before being applied to 
mainline.

Christophe

Re: [RFC PATCH 1/3] module: Split module_enable_rodata_ro()

[Luis Chamberlain]

On Wed, Nov 27, 2024 at 02:37:40PM +0100, Christophe Leroy wrote:
> 
> 
> Le 26/11/2024 à 20:58, Luis Chamberlain a écrit :
> > On Sat, Nov 09, 2024 at 11:35:35AM +0100, Christophe Leroy wrote:
> > > module_enable_rodata_ro() is called twice, once before module init
> > > to set rodata sections readonly and once after module init to set
> > > rodata_after_init section readonly.
> > > 
> > > The second time, only the rodata_after_init section needs to be
> > > set to read-only, no need to re-apply it to already set rodata.
> > > 
> > > Split module_enable_rodata_ro() in two.
> > > 
> > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> > 
> > Didn't see a respin so this will have to be a post v6.13-rc1 fix.
> 
> Indeed I was waiting for v6.13-rc1 before sending the non RFC rebased
> version, but I can send it now if you prefer.
> 
> I expect it to spend a few days in linux-next before being applied to
> mainline.

No rush on my end, let's wait as you noted until v6.13-rc1 is out.

  Luis