Received-SPF: pass (google.com: domain of linux-kernel+bounces-68526-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <5cb55af6-5c9f-4a75-8c42-819415aec4d2@samsung.com>
Date: Fri, 16 Feb 2024 12:33:03 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] module: Don't ignore errors from set_memory_XX()
Content-Language: en-US
To: Christophe Leroy <christophe.leroy@csgroup.eu>, Luis Chamberlain
	<mcgrof@kernel.org>, linux-modules@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, Kees Cook <keescook@chromium.org>,
	"linux-hardening @ vger . kernel . org" <linux-hardening@vger.kernel.org>,
	Mark Rutland <mark.rutland@arm.com>
From: Marek Szyprowski <m.szyprowski@samsung.com>
In-Reply-To: <21037bf38438a285f5dff9501668f1675bc45989.1708070781.git.christophe.leroy@csgroup.eu>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <CGME20240216081457eucas1p2c22493d17918753128b39df47e98d8cd@eucas1p2.samsung.com>
	<21037bf38438a285f5dff9501668f1675bc45989.1708070781.git.christophe.leroy@csgroup.eu>

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