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

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

[Christophe Leroy]

set_memory_ro(), set_memory_nx(), set_memory_x() and other helpers
can fail and 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.

Add a check to not call set_memory_XX() on NULL pointers as some
architectures may not like it allthough numpages is always 0 in that
case. This also avoid a useless call to set_vm_flush_reset_perms().

Link: https://github.com/KSPP/linux/issues/7
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v2:
- Bail out early from module_set_memory() when address is NULL.
- Properly clear bug list when module_set_memory() fails in complete_formation().

This patch applies on top of modules/modules-next branch
---
 kernel/module/internal.h   |  6 ++---
 kernel/module/main.c       | 20 ++++++++++++---
 kernel/module/strict_rwx.c | 51 +++++++++++++++++++++++++++-----------
 3 files changed, 55 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 a9a4a4885102..689def7676c4 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2571,7 +2571,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) {
@@ -2609,6 +2611,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:
@@ -2736,9 +2740,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_strict_rwx;
+	err = module_enable_data_nx(mod);
+	if (err)
+		goto out_strict_rwx;
+	err = module_enable_text_rox(mod);
+	if (err)
+		goto out_strict_rwx;
 
 	/*
 	 * Mark state as coming so strong_try_module_get() ignores us,
@@ -2749,6 +2759,8 @@ static int complete_formation(struct module *mod, struct load_info *info)
 
 	return 0;
 
+out_strict_rwx:
+	module_bug_cleanup(mod);
 out:
 	mutex_unlock(&module_mutex);
 	return err;
diff --git a/kernel/module/strict_rwx.c b/kernel/module/strict_rwx.c
index b36d93983465..c45caa4690e5 100644
--- a/kernel/module/strict_rwx.c
+++ b/kernel/module/strict_rwx.c
@@ -11,13 +11,16 @@
 #include <linux/set_memory.h>
 #include "internal.h"
 
-static void module_set_memory(const struct module *mod, enum mod_mem_type type,
-			      int (*set_memory)(unsigned long start, int num_pages))
+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];
 
+	if (!mod_mem->base)
+		return 0;
+
 	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,35 +29,53 @@ 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) || !rodata_enabled)
-		return;
+		return 0;
 
-	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)
-		module_set_memory(mod, type, set_memory_nx);
+	for_class_mod_mem_type(type, data) {
+		int ret = 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.43.0

Re: [PATCH v2] module: Don't ignore errors from set_memory_XX()

[Marek Szyprowski]

On 16.02.2024 09:14, Christophe Leroy wrote:
> set_memory_ro(), set_memory_nx(), set_memory_x() and other helpers
> can fail and 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.
>
> Add a check to not call set_memory_XX() on NULL pointers as some
> architectures may not like it allthough numpages is always 0 in that
> case. This also avoid a useless call to set_vm_flush_reset_perms().
>
> Link:https://github.com/KSPP/linux/issues/7
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---
> v2:
> - Bail out early from module_set_memory() when address is NULL.
> - Properly clear bug list when module_set_memory() fails in complete_formation().
>
> This patch applies on top of modules/modules-next branch
> ---
>   kernel/module/internal.h   |  6 ++---
>   kernel/module/main.c       | 20 ++++++++++++---
>   kernel/module/strict_rwx.c | 51 +++++++++++++++++++++++++++-----------
>   3 files changed, 55 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 a9a4a4885102..689def7676c4 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -2571,7 +2571,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) {
> @@ -2609,6 +2611,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:
> @@ -2736,9 +2740,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_strict_rwx;
> +	err = module_enable_data_nx(mod);
> +	if (err)
> +		goto out_strict_rwx;
> +	err = module_enable_text_rox(mod);
> +	if (err)
> +		goto out_strict_rwx;
>   
>   	/*
>   	 * Mark state as coming so strong_try_module_get() ignores us,
> @@ -2749,6 +2759,8 @@ static int complete_formation(struct module *mod, struct load_info *info)
>   
>   	return 0;
>   
> +out_strict_rwx:
> +	module_bug_cleanup(mod);
>   out:
>   	mutex_unlock(&module_mutex);
>   	return err;
> diff --git a/kernel/module/strict_rwx.c b/kernel/module/strict_rwx.c
> index b36d93983465..c45caa4690e5 100644
> --- a/kernel/module/strict_rwx.c
> +++ b/kernel/module/strict_rwx.c
> @@ -11,13 +11,16 @@
>   #include <linux/set_memory.h>
>   #include "internal.h"
>   
> -static void module_set_memory(const struct module *mod, enum mod_mem_type type,
> -			      int (*set_memory)(unsigned long start, int num_pages))
> +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];
>   
> +	if (!mod_mem->base)
> +		return 0;
> +
>   	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,35 +29,53 @@ 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) || !rodata_enabled)
> -		return;
> +		return 0;
>   
> -	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)
> -		module_set_memory(mod, type, set_memory_nx);
> +	for_class_mod_mem_type(type, data) {
> +		int ret = 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,

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2] module: Don't ignore errors from set_memory_XX()

[Luis Chamberlain]

On Fri, Feb 16, 2024 at 09:14:27AM +0100, Christophe Leroy wrote:
> set_memory_ro(), set_memory_nx(), set_memory_x() and other helpers
> can fail and 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.
> 
> Add a check to not call set_memory_XX() on NULL pointers as some
> architectures may not like it allthough numpages is always 0 in that
> case. This also avoid a useless call to set_vm_flush_reset_perms().
> 
> Link: https://github.com/KSPP/linux/issues/7
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

Applied and pushed, thanks!

  Luis

Re: [PATCH v2] module: Don't ignore errors from set_memory_XX()

[Kees Cook]

On Fri, Feb 16, 2024 at 09:14:27AM +0100, Christophe Leroy wrote:
> set_memory_ro(), set_memory_nx(), set_memory_x() and other helpers
> can fail and 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.
> 
> Add a check to not call set_memory_XX() on NULL pointers as some
> architectures may not like it allthough numpages is always 0 in that
> case. This also avoid a useless call to set_vm_flush_reset_perms().
> 
> Link: https://github.com/KSPP/linux/issues/7
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

Yay! Glad to see this happening. Universal __must_check for set_memory_*()
starts to appear on the horizon. ;)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v2] module: Don't ignore errors from set_memory_XX()

[Christophe Leroy]

Le 16/02/2024 à 20:28, Kees Cook a écrit :
> On Fri, Feb 16, 2024 at 09:14:27AM +0100, Christophe Leroy wrote:
>> set_memory_ro(), set_memory_nx(), set_memory_x() and other helpers
>> can fail and 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.
>>
>> Add a check to not call set_memory_XX() on NULL pointers as some
>> architectures may not like it allthough numpages is always 0 in that
>> case. This also avoid a useless call to set_vm_flush_reset_perms().
>>
>> Link: https://github.com/KSPP/linux/issues/7
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> 
> Yay! Glad to see this happening. Universal __must_check for set_memory_*()
> starts to appear on the horizon. ;)
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 

Thanks

I also sent 3 patches to powerpc, I forgot to copy you but I put the 
links in https://github.com/KSPP/linux/issues/7

Christophe