[PATCH 1/3] module: Use set_memory_rox()

[PATCH 1/3] module: Use set_memory_rox()

[Christophe Leroy]

A couple of architectures seem concerned about calling set_memory_ro()
and set_memory_x() too frequently and have implemented a version of
set_memory_rox(), see commit 60463628c9e0 ("x86/mm: Implement native
set_memory_rox()") and commit 22e99fa56443 ("s390/mm: implement
set_memory_rox()")

Use set_memory_rox() in modules when STRICT_MODULES_RWX is set.

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

diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index c8b7b4dcf782..a647ab17193d 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -324,7 +324,7 @@ static inline struct module *mod_find(unsigned long addr, struct mod_tree_root *
 
 void module_enable_ro(const struct module *mod, bool after_init);
 void module_enable_nx(const struct module *mod);
-void module_enable_x(const struct module *mod);
+void module_enable_rox(const struct module *mod);
 int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
 				char *secstrings, struct module *mod);
 
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 98fedfdb8db5..1c8f328ca015 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2735,7 +2735,7 @@ static int complete_formation(struct module *mod, struct load_info *info)
 
 	module_enable_ro(mod, false);
 	module_enable_nx(mod);
-	module_enable_x(mod);
+	module_enable_rox(mod);
 
 	/*
 	 * Mark state as coming so strong_try_module_get() ignores us,
diff --git a/kernel/module/strict_rwx.c b/kernel/module/strict_rwx.c
index a2b656b4e3d2..9345b09f28a5 100644
--- a/kernel/module/strict_rwx.c
+++ b/kernel/module/strict_rwx.c
@@ -26,10 +26,14 @@ static void module_set_memory(const struct module *mod, enum mod_mem_type type,
  * CONFIG_STRICT_MODULE_RWX because they are needed regardless of whether we
  * are strict.
  */
-void module_enable_x(const struct module *mod)
+void module_enable_rox(const struct module *mod)
 {
-	for_class_mod_mem_type(type, text)
-		module_set_memory(mod, type, set_memory_x);
+	for_class_mod_mem_type(type, text) {
+		if (IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
+			module_set_memory(mod, type, set_memory_rox);
+		else
+			module_set_memory(mod, type, set_memory_x);
+	}
 }
 
 void module_enable_ro(const struct module *mod, bool after_init)
@@ -41,8 +45,6 @@ void module_enable_ro(const struct module *mod, bool after_init)
 		return;
 #endif
 
-	module_set_memory(mod, MOD_TEXT, set_memory_ro);
-	module_set_memory(mod, MOD_INIT_TEXT, set_memory_ro);
 	module_set_memory(mod, MOD_RODATA, set_memory_ro);
 	module_set_memory(mod, MOD_INIT_RODATA, set_memory_ro);
 
-- 
2.41.0

[PATCH 2/3] module: Change module_enable_{nx/x/ro}() to more explicit names

[Christophe Leroy]

It's a bit puzzling to see a call to module_enable_nx() followed by a
call to module_enable_x(). This is because one applies on text while
the other applies on data.

Change name to make that more clear.

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

diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index a647ab17193d..4f1b98f011da 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -322,9 +322,9 @@ static inline struct module *mod_find(unsigned long addr, struct mod_tree_root *
 }
 #endif /* CONFIG_MODULES_TREE_LOOKUP */
 
-void module_enable_ro(const struct module *mod, bool after_init);
-void module_enable_nx(const struct module *mod);
-void module_enable_rox(const struct module *mod);
+void module_enable_rodata_ro(const struct module *mod, bool after_init);
+void module_enable_data_nx(const struct module *mod);
+void module_enable_text_rox(const struct module *mod);
 int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
 				char *secstrings, struct module *mod);
 
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 1c8f328ca015..64662e55e275 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2568,7 +2568,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
-	module_enable_ro(mod, true);
+	module_enable_rodata_ro(mod, true);
 	mod_tree_remove_init(mod);
 	module_arch_freeing_init(mod);
 	for_class_mod_mem_type(type, init) {
@@ -2733,9 +2733,9 @@ 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);
 
-	module_enable_ro(mod, false);
-	module_enable_nx(mod);
-	module_enable_rox(mod);
+	module_enable_rodata_ro(mod, false);
+	module_enable_data_nx(mod);
+	module_enable_text_rox(mod);
 
 	/*
 	 * Mark state as coming so strong_try_module_get() ignores us,
diff --git a/kernel/module/strict_rwx.c b/kernel/module/strict_rwx.c
index 9345b09f28a5..9b2d58a8d59d 100644
--- a/kernel/module/strict_rwx.c
+++ b/kernel/module/strict_rwx.c
@@ -26,7 +26,7 @@ static void module_set_memory(const struct module *mod, enum mod_mem_type type,
  * CONFIG_STRICT_MODULE_RWX because they are needed regardless of whether we
  * are strict.
  */
-void module_enable_rox(const struct module *mod)
+void module_enable_text_rox(const struct module *mod)
 {
 	for_class_mod_mem_type(type, text) {
 		if (IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
@@ -36,7 +36,7 @@ void module_enable_rox(const struct module *mod)
 	}
 }
 
-void module_enable_ro(const struct module *mod, bool after_init)
+void module_enable_rodata_ro(const struct module *mod, bool after_init)
 {
 	if (!IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
 		return;
@@ -52,7 +52,7 @@ void module_enable_ro(const struct module *mod, bool after_init)
 		module_set_memory(mod, MOD_RO_AFTER_INIT, set_memory_ro);
 }
 
-void module_enable_nx(const struct module *mod)
+void module_enable_data_nx(const struct module *mod)
 {
 	if (!IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
 		return;
-- 
2.41.0

[PATCH 3/3] module: Don't ignore errors from set_memory_XX()

[Christophe Leroy]

set_memory_ro(), set_memory_nx(), set_memory_x() and other helps
can fail an return an error. In that case the memory might not be
protected as expected and the module loading has to be aborted to
avoid security issues.

Check return value of all calls to set_memory_XX() and handle
error if any.

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

diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index 4f1b98f011da..2ebece8a789f 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -322,9 +322,9 @@ static inline struct module *mod_find(unsigned long addr, struct mod_tree_root *
 }
 #endif /* CONFIG_MODULES_TREE_LOOKUP */
 
-void module_enable_rodata_ro(const struct module *mod, bool after_init);
-void module_enable_data_nx(const struct module *mod);
-void module_enable_text_rox(const struct module *mod);
+int module_enable_rodata_ro(const struct module *mod, bool after_init);
+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,
 				char *secstrings, struct module *mod);
 
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 64662e55e275..cfe197455d64 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2568,7 +2568,9 @@ 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
-	module_enable_rodata_ro(mod, true);
+	ret = module_enable_rodata_ro(mod, true);
+	if (ret)
+		goto fail_mutex_unlock;
 	mod_tree_remove_init(mod);
 	module_arch_freeing_init(mod);
 	for_class_mod_mem_type(type, init) {
@@ -2606,6 +2608,8 @@ static noinline int do_init_module(struct module *mod)
 
 	return 0;
 
+fail_mutex_unlock:
+	mutex_unlock(&module_mutex);
 fail_free_freeinit:
 	kfree(freeinit);
 fail:
@@ -2733,9 +2737,15 @@ 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);
 
-	module_enable_rodata_ro(mod, false);
-	module_enable_data_nx(mod);
-	module_enable_text_rox(mod);
+	err = module_enable_rodata_ro(mod, false);
+	if (err)
+		goto out;
+	err = module_enable_data_nx(mod);
+	if (err)
+		goto out;
+	err = module_enable_text_rox(mod);
+	if (err)
+		goto out;
 
 	/*
 	 * Mark state as coming so strong_try_module_get() ignores us,
diff --git a/kernel/module/strict_rwx.c b/kernel/module/strict_rwx.c
index 9b2d58a8d59d..a14df9655dbe 100644
--- a/kernel/module/strict_rwx.c
+++ b/kernel/module/strict_rwx.c
@@ -11,13 +11,13 @@
 #include <linux/set_memory.h>
 #include "internal.h"
 
-static void module_set_memory(const struct module *mod, enum mod_mem_type type,
+static int module_set_memory(const struct module *mod, enum mod_mem_type type,
 			      int (*set_memory)(unsigned long start, int num_pages))
 {
 	const struct module_memory *mod_mem = &mod->mem[type];
 
 	set_vm_flush_reset_perms(mod_mem->base);
-	set_memory((unsigned long)mod_mem->base, mod_mem->size >> PAGE_SHIFT);
+	return set_memory((unsigned long)mod_mem->base, mod_mem->size >> PAGE_SHIFT);
 }
 
 /*
@@ -26,39 +26,57 @@ static void module_set_memory(const struct module *mod, enum mod_mem_type type,
  * CONFIG_STRICT_MODULE_RWX because they are needed regardless of whether we
  * are strict.
  */
-void module_enable_text_rox(const struct module *mod)
+int module_enable_text_rox(const struct module *mod)
 {
 	for_class_mod_mem_type(type, text) {
+		int ret;
+
 		if (IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
-			module_set_memory(mod, type, set_memory_rox);
+			ret = module_set_memory(mod, type, set_memory_rox);
 		else
-			module_set_memory(mod, type, set_memory_x);
+			ret = module_set_memory(mod, type, set_memory_x);
+		if (ret)
+			return ret;
 	}
+	return 0;
 }
 
-void module_enable_rodata_ro(const struct module *mod, bool after_init)
+int module_enable_rodata_ro(const struct module *mod, bool after_init)
 {
+	int ret;
+
 	if (!IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
-		return;
+		return 0;
 #ifdef CONFIG_STRICT_MODULE_RWX
 	if (!rodata_enabled)
-		return;
+		return 0;
 #endif
 
-	module_set_memory(mod, MOD_RODATA, set_memory_ro);
-	module_set_memory(mod, MOD_INIT_RODATA, set_memory_ro);
+	ret = module_set_memory(mod, MOD_RODATA, set_memory_ro);
+	if (ret)
+		return ret;
+	ret = module_set_memory(mod, MOD_INIT_RODATA, set_memory_ro);
+	if (ret)
+		return ret;
 
 	if (after_init)
-		module_set_memory(mod, MOD_RO_AFTER_INIT, set_memory_ro);
+		return module_set_memory(mod, MOD_RO_AFTER_INIT, set_memory_ro);
+
+	return 0;
 }
 
-void module_enable_data_nx(const struct module *mod)
+int module_enable_data_nx(const struct module *mod)
 {
 	if (!IS_ENABLED(CONFIG_STRICT_MODULE_RWX))
-		return;
+		return 0;
+
+	for_class_mod_mem_type(type, data) {
+		int ret = module_set_memory(mod, type, set_memory_nx);
 
-	for_class_mod_mem_type(type, data)
-		module_set_memory(mod, type, set_memory_nx);
+		if (ret)
+			return ret;
+	}
+	return 0;
 }
 
 int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
-- 
2.41.0

Re: [PATCH 1/3] module: Use set_memory_rox()

[Luis Chamberlain]

On Thu, Dec 21, 2023 at 08:24:23AM +0100, Christophe Leroy wrote:
> A couple of architectures seem concerned about calling set_memory_ro()
> and set_memory_x() too frequently and have implemented a version of
> set_memory_rox(), see commit 60463628c9e0 ("x86/mm: Implement native
> set_memory_rox()") and commit 22e99fa56443 ("s390/mm: implement
> set_memory_rox()")
> 
> Use set_memory_rox() in modules when STRICT_MODULES_RWX is set.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

Nice simplification. I applied all 3 patches and pushed!

  Luis