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

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

[Christophe Leroy]

This series reworks module loading to avoid leaving the module in a
stale state when protecting ro_after_init section fails.

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. This is what patch 2 does.

Then patch 3 tries to go a bit further by testing the ability to write
protect ro-after-init section prior to initialising the module.

Changes between RFC and v1:
- Patch 2: Fixed comment from Petr about __func__
- Patch 3: Expanded the commit message based on feedback from RFC series

Christophe Leroy (3):
  module: Split module_enable_rodata_ro()
  module: Don't fail module loading when setting ro_after_init section
    RO failed
  module: pre-test setting ro_after_init data read-only

 kernel/module/internal.h   |  3 ++-
 kernel/module/main.c       | 13 +++++++------
 kernel/module/strict_rwx.c | 16 ++++++++++++----
 3 files changed, 21 insertions(+), 11 deletions(-)

-- 
2.47.0

[PATCH v1 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>
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 d2e1b8976c7b..39fe5d81b6a8 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2673,7 +2673,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);
@@ -2843,7 +2843,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 239e5013359d..74834ba15615 100644
--- a/kernel/module/strict_rwx.c
+++ b/kernel/module/strict_rwx.c
@@ -47,7 +47,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;
 
@@ -61,12 +61,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.47.0

[PATCH v1 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>
---
v1: Fixed comment from Petr about __func__
---
 kernel/module/main.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 39fe5d81b6a8..5f922e563fc3 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2675,7 +2675,10 @@ 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: module_enable_rodata_ro_after_init() returned %d, "
+			"ro_after_init data might still be writable\n",
+			mod->name, ret);
+
 	mod_tree_remove_init(mod);
 	module_arch_freeing_init(mod);
 	for_class_mod_mem_type(type, init) {
@@ -2714,8 +2717,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.47.0

[PATCH v1 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-write, continue module loading
and cross fingers so that it still works after module init.

In practice, if it worked once it will work twice:
- On some architecture like powerpc it works on some memory areas and
works on others. If you apply it several times to the same area, either
it always works or it always fails
- On some architecture like ARM, that may apply splitting big pages
into smaller ones, that is what can fails, but once it successed the
pages will remain split so there's no reason to fail on pass two if it
worked on pass one.

Then it should in principle never fail so add a WARN_ON_ONCE() to get
a big fat warning in case it happens anyway. For systems that sets
panic-on-warn, such systems usely care about security and don't want
vulnerable systems, so an implied panic is worth it in that case.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v1: Expanded the commit message based on feedback from RFC series
---
 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 5f922e563fc3..634a4a2aaf8c 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2674,7 +2674,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: module_enable_rodata_ro_after_init() returned %d, "
 			"ro_after_init data might still be writable\n",
 			mod->name, ret);
diff --git a/kernel/module/strict_rwx.c b/kernel/module/strict_rwx.c
index 74834ba15615..1434c48c52ab 100644
--- a/kernel/module/strict_rwx.c
+++ b/kernel/module/strict_rwx.c
@@ -61,7 +61,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.47.0

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

[Luis Chamberlain]

On Thu, Dec 05, 2024 at 08:46:14PM +0100, Christophe Leroy wrote:
> This series reworks module loading to avoid leaving the module in a
> stale state when protecting ro_after_init section fails.
> 
> 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. This is what patch 2 does.
> 
> Then patch 3 tries to go a bit further by testing the ability to write
> protect ro-after-init section prior to initialising the module.

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

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

[Petr Pavlu]

On 12/5/24 20:46, Christophe Leroy wrote:
> This series reworks module loading to avoid leaving the module in a
> stale state when protecting ro_after_init section fails.
> 
> 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. This is what patch 2 does.
> 
> Then patch 3 tries to go a bit further by testing the ability to write
> protect ro-after-init section prior to initialising the module.

I've been holding off on applying this series to modules-next because
there was still some discussion on the previous RFC version [1], and
I wanted to give people more time to potentially comment.

Mike Rapoport also recently posted a series with a patch [2] that
proposes restoring of large pages after fragmentation. Should the last
patch here be then dropped?

[1] https://lore.kernel.org/linux-modules/737f952790c96a09ad5e51689918b97ef9b29174.1731148254.git.christophe.leroy@csgroup.eu/
[2] https://lore.kernel.org/linux-modules/20241227072825.1288491-4-rppt@kernel.org/

-- 
Thanks,
Petr

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

[Christophe Leroy]

Le 03/01/2025 à 17:13, Petr Pavlu a écrit :
> On 12/5/24 20:46, Christophe Leroy wrote:
>> This series reworks module loading to avoid leaving the module in a
>> stale state when protecting ro_after_init section fails.
>>
>> 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. This is what patch 2 does.
>>
>> Then patch 3 tries to go a bit further by testing the ability to write
>> protect ro-after-init section prior to initialising the module.
> 
> I've been holding off on applying this series to modules-next because
> there was still some discussion on the previous RFC version [1], and
> I wanted to give people more time to potentially comment.
> 
> Mike Rapoport also recently posted a series with a patch [2] that
> proposes restoring of large pages after fragmentation. Should the last
> patch here be then dropped?

Indeed, if the large pages are restored when bringing back the 
ro_after_init to RW, it defeats the purpose of patch 3.

So I agree, let's first apply patches 1 and 2 in order to fix the actual 
bug then see how we can improve as a second step.

> 
> [1] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-modules%2F737f952790c96a09ad5e51689918b97ef9b29174.1731148254.git.christophe.leroy%40csgroup.eu%2F&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7Ce1338eec4ee742a40b6208dd2c1192dc%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638715176198708012%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=B7qL4g0NUqBqdREndab5kywoOu2wsNYej6hqnIH10tk%3D&reserved=0
> [2] https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-modules%2F20241227072825.1288491-4-rppt%40kernel.org%2F&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7Ce1338eec4ee742a40b6208dd2c1192dc%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638715176198723794%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=eMxPsu43ByOjr7ny9Xg81ylWS6853dTU5MmU3J9e2hc%3D&reserved=0
> 

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

[Petr Pavlu]

On 1/4/25 08:39, Christophe Leroy wrote:
> Le 03/01/2025 à 17:13, Petr Pavlu a écrit :
>> On 12/5/24 20:46, Christophe Leroy wrote:
>>> This series reworks module loading to avoid leaving the module in a
>>> stale state when protecting ro_after_init section fails.
>>>
>>> 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. This is what patch 2 does.
>>>
>>> Then patch 3 tries to go a bit further by testing the ability to write
>>> protect ro-after-init section prior to initialising the module.
>>
>> I've been holding off on applying this series to modules-next because
>> there was still some discussion on the previous RFC version [1], and
>> I wanted to give people more time to potentially comment.
>>
>> Mike Rapoport also recently posted a series with a patch [2] that
>> proposes restoring of large pages after fragmentation. Should the last
>> patch here be then dropped?
> 
> Indeed, if the large pages are restored when bringing back the 
> ro_after_init to RW, it defeats the purpose of patch 3.
> 
> So I agree, let's first apply patches 1 and 2 in order to fix the actual 
> bug then see how we can improve as a second step.

I've now queued the first two patches on modules-next.

-- 
Thanks,
Petr

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

[Kees Cook]

On Fri, Jan 03, 2025 at 05:13:32PM +0100, Petr Pavlu wrote:
> On 12/5/24 20:46, Christophe Leroy wrote:
> > This series reworks module loading to avoid leaving the module in a
> > stale state when protecting ro_after_init section fails.
> > 
> > Once module init has succeded it is too late to cancel loading.

Is there at least a big WARN about the ro failing? That should let more
sensitive system owners react to the situation if it looks like an
active attack on memory protections.

(And maybe we should set a TAINT flag, but perhaps this is too specific
a failure mode for that?)

Also, why is it too late to cancel? Can we set the module to the
"Unloading" state to stop any dependent modules from loading on top of
it, and then request it unload?

-- 
Kees Cook

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

[Daniel Gomez]

On Mon, Jan 06, 2025 at 04:13:29PM -0800, Kees Cook wrote:
> On Fri, Jan 03, 2025 at 05:13:32PM +0100, Petr Pavlu wrote:
> > On 12/5/24 20:46, Christophe Leroy wrote:
> > > This series reworks module loading to avoid leaving the module in a
> > > stale state when protecting ro_after_init section fails.
> > > 
> > > Once module init has succeded it is too late to cancel loading.
> 
> Is there at least a big WARN about the ro failing? That should let more
> sensitive system owners react to the situation if it looks like an
> active attack on memory protections.

Yes, there is. But I think only the first time a module fails. IIUC,
any subsequent modules failing will not warn anything, is that right?
However, I think this should suffice to know a system is vulnerable but
will not know the full list of the actual vulnerable modules.

> 
> (And maybe we should set a TAINT flag, but perhaps this is too specific
> a failure mode for that?)
> 
> Also, why is it too late to cancel? Can we set the module to the
> "Unloading" state to stop any dependent modules from loading on top of
> it, and then request it unload?

I think Luis summarized it here [1]. Quoting him from that thread:

	Sadly there are a few issues with trying to get to call mod->exit():
	
	- We should try try_stop_module()  and that can fail
	- source_list may not be empty and that would block removal
	- mod->exit may not exist

https://lore.kernel.org/all/Zuv0nmFblHUwuT8v@bombadil.infradead.org/

Module loading goes from UNFORMED to LIVE during the initialization.
And once it's LIVE we do the ro_after_init memory protection. I'm not
sure if an intermediate stage can be added so ro_after_init is performed
and module is unloaded when this fails? I guess init does not necessary
mean LIVE.

Daniel

> 
> -- 
> Kees Cook

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

[Luis Chamberlain]

On Mon, Jan 06, 2025 at 04:13:29PM -0800, Kees Cook wrote:
> On Fri, Jan 03, 2025 at 05:13:32PM +0100, Petr Pavlu wrote:
> > On 12/5/24 20:46, Christophe Leroy wrote:
> > > This series reworks module loading to avoid leaving the module in a
> > > stale state when protecting ro_after_init section fails.
> > > 
> > > Once module init has succeded it is too late to cancel loading.
> 
> Is there at least a big WARN about the ro failing? That should let more
> sensitive system owners react to the situation if it looks like an
> active attack on memory protections.
> 
> (And maybe we should set a TAINT flag, but perhaps this is too specific
> a failure mode for that?)

I don't see a taint flag too far fetched in value. I think its a
sensible compromise, and may be useful for other future set_memory_*()
failures.

 Luis