| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 10 Feb 2022 11:44:52 +0000
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Aaron Tomlin <atomlin@...hat.com>,
"mcgrof@...nel.org" <mcgrof@...nel.org>
CC: "cl@...ux.com" <cl@...ux.com>,
"pmladek@...e.com" <pmladek@...e.com>,
"mbenes@...e.cz" <mbenes@...e.cz>,
"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
"jeyu@...nel.org" <jeyu@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-modules@...r.kernel.org" <linux-modules@...r.kernel.org>,
"live-patching@...r.kernel.org" <live-patching@...r.kernel.org>,
"atomlin@...mlin.com" <atomlin@...mlin.com>,
"ghalat@...hat.com" <ghalat@...hat.com>,
"allen.lkml@...il.com" <allen.lkml@...il.com>,
"void@...ifault.com" <void@...ifault.com>,
"joe@...ches.com" <joe@...ches.com>,
"msuchanek@...e.de" <msuchanek@...e.de>,
"oleksandr@...alenko.name" <oleksandr@...alenko.name>
Subject: Re: [PATCH v5 04/13] module: Move livepatch support to a separate
file
Le 09/02/2022 à 18:03, Aaron Tomlin a écrit :
> No functional change.
>
> This patch migrates livepatch support (i.e. used during module
> add/or load and remove/or deletion) from core module code into
> kernel/module/livepatch.c. At the moment it contains code to
> persist Elf information about a given livepatch module, only.
>
> Signed-off-by: Aaron Tomlin <atomlin@...hat.com>
> ---
> include/linux/module.h | 5 +-
> kernel/module/Makefile | 3 ++
> kernel/module/internal.h | 18 +++++++
> kernel/module/livepatch.c | 80 ++++++++++++++++++++++++++++++
> kernel/module/main.c | 102 ++++----------------------------------
> 5 files changed, 112 insertions(+), 96 deletions(-)
> create mode 100644 kernel/module/livepatch.c
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 1e135fd5c076..680b31ff57fa 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -664,10 +664,7 @@ static inline bool module_requested_async_probing(struct module *module)
> }
>
> #ifdef CONFIG_LIVEPATCH
> -static inline bool is_livepatch_module(struct module *mod)
> -{
> - return mod->klp;
> -}
> +bool is_livepatch_module(struct module *mod);
This change is wrong, build fails with it because is_livepatch_module()
is nowhere defined.
You could move is_livepatch_module() to include/linux/livepatch.h but as
a separate patch.
> #else /* !CONFIG_LIVEPATCH */
> static inline bool is_livepatch_module(struct module *mod)
> {
> diff --git a/kernel/module/Makefile b/kernel/module/Makefile
> index 2902fc7d0ef1..ee20d864ad19 100644
> --- a/kernel/module/Makefile
> +++ b/kernel/module/Makefile
> @@ -7,3 +7,6 @@ obj-$(CONFIG_MODULES) += main.o
> obj-$(CONFIG_MODULE_DECOMPRESS) += decompress.o
> obj-$(CONFIG_MODULE_SIG) += signing.o
> obj-$(CONFIG_MODULE_SIG_FORMAT) += signature.o
> +ifdef CONFIG_MODULES
CONFIG_LIVEPATCH depends on CONFIG_MODULES so this ifdef is not needed
(See kernel/livepatch/Kconfig)
> +obj-$(CONFIG_LIVEPATCH) += livepatch.o
> +endif
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index 1cf5d6dabc97..d252e0af1c54 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -58,6 +58,24 @@ struct load_info {
>
> int mod_verify_sig(const void *mod, struct load_info *info);
>
> +#ifdef CONFIG_LIVEPATCH
> +int copy_module_elf(struct module *mod, struct load_info *info);
> +void free_module_elf(struct module *mod);
> +bool set_livepatch_module(struct module *mod);
> +#else /* !CONFIG_LIVEPATCH */
> +static inline int copy_module_elf(struct module *mod, struct load_info *info)
> +{
> + return 0;
> +}
> +
> +static inline bool set_livepatch_module(struct module *mod)
> +{
> + return false;
> +}
> +
> +static inline void free_module_elf(struct module *mod) { }
> +#endif /* CONFIG_LIVEPATCH */
> +
> #ifdef CONFIG_MODULE_DECOMPRESS
> int module_decompress(struct load_info *info, const void *buf, size_t size);
> void module_decompress_cleanup(struct load_info *info);
> diff --git a/kernel/module/livepatch.c b/kernel/module/livepatch.c
> new file mode 100644
> index 000000000000..7e9cf530c3f0
> --- /dev/null
> +++ b/kernel/module/livepatch.c
Checkpatch reports the following:
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#80:
new file mode 100644
CHECK: Comparison to NULL could be written "!mod->klp_info"
#109: FILE: kernel/module/livepatch.c:25:
+ if (mod->klp_info == NULL)
CHECK: Comparison to NULL could be written "!mod->klp_info->sechdrs"
#119: FILE: kernel/module/livepatch.c:35:
+ if (mod->klp_info->sechdrs == NULL) {
CHECK: Comparison to NULL could be written "!mod->klp_info->secstrings"
#127: FILE: kernel/module/livepatch.c:43:
+ if (mod->klp_info->secstrings == NULL) {
CHECK: No space is necessary after a cast
#142: FILE: kernel/module/livepatch.c:58:
+ mod->klp_info->sechdrs[symndx].sh_addr = (unsigned long)
mod->core_kallsyms.symtab;
> +inline bool set_livepatch_module(struct module *mod)
'inline' keyword is pointless here, as far as this function is in a .c
and is not static, it won't be inlined.
Given how simple this function is, it should be a 'static inline' in
internal.c
> +{
> + mod->klp = true;
> + return true;
> +}
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 750e3ad28679..5f5bd7152b55 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -3091,30 +3016,23 @@ static int copy_chunked_from_user(void *dst, const void __user *usrc, unsigned l
> return 0;
> }
>
> -#ifdef CONFIG_LIVEPATCH
> static int check_modinfo_livepatch(struct module *mod, struct load_info *info)
> {
> - if (get_modinfo(info, "livepatch")) {
> - mod->klp = true;
> + if (!get_modinfo(info, "livepatch"))
> + /* Nothing more to do */
> + return 0;
> +
> + if (set_livepatch_module(mod)) {
> add_taint_module(mod, TAINT_LIVEPATCH, LOCKDEP_STILL_OK);
> pr_notice_once("%s: tainting kernel with TAINT_LIVEPATCH\n",
> - mod->name);
> - }
> -
> - return 0;
> -}
> -#else /* !CONFIG_LIVEPATCH */
> -static int check_modinfo_livepatch(struct module *mod, struct load_info *info)
> -{
> - if (get_modinfo(info, "livepatch")) {
> - pr_err("%s: module is marked as livepatch module, but livepatch support is disabled",
> - mod->name);
> - return -ENOEXEC;
> + mod->name);
This change seems wrong, mod->name must remain aligned to open parenthesis.
> + return 0;
> }
>
> - return 0;
> + pr_err("%s: module is marked as livepatch module, but livepatch support is disabled",
> + mod->name);
CHECK: Alignment should match open parenthesis
#285: FILE: kernel/module/main.c:3033:
+ pr_err("%s: module is marked as livepatch module, but livepatch
support is disabled",
+ mod->name);
> + return -ENOEXEC;
> }
> -#endif /* CONFIG_LIVEPATCH */
>
> static void check_modinfo_retpoline(struct module *mod, struct load_info *info)
> {
Powered by blists - more mailing lists