| 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
| ||
|
Message-ID: <20190530115741.GB16012@linux-8ccs>
Date: Thu, 30 May 2019 13:57:41 +0200
From: Jessica Yu <jeyu@...nel.org>
To: Josh Poimboeuf <jpoimboe@...hat.com>
Cc: Steven Rostedt <rostedt@...dmis.org>,
Jiri Kosina <jikos@...nel.org>,
Miroslav Benes <mbenes@...e.cz>,
Petr Mladek <pmladek@...e.com>,
Joe Lawrence <joe.lawrence@...hat.com>,
linux-kernel@...r.kernel.org, live-patching@...r.kernel.org,
Johannes Erdfelt <johannes@...felt.com>,
Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH] livepatch: Fix ftrace module text permissions race
+++ Josh Poimboeuf [29/05/19 14:02 -0500]:
>It's possible for livepatch and ftrace to be toggling a module's text
>permissions at the same time, resulting in the following panic:
>
> BUG: unable to handle page fault for address: ffffffffc005b1d9
> #PF: supervisor write access in kernel mode
> #PF: error_code(0x0003) - permissions violation
> PGD 3ea0c067 P4D 3ea0c067 PUD 3ea0e067 PMD 3cc13067 PTE 3b8a1061
> Oops: 0003 [#1] PREEMPT SMP PTI
> CPU: 1 PID: 453 Comm: insmod Tainted: G O K 5.2.0-rc1-a188339ca5 #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-20181126_142135-anatol 04/01/2014
> RIP: 0010:apply_relocate_add+0xbe/0x14c
> Code: fa 0b 74 21 48 83 fa 18 74 38 48 83 fa 0a 75 40 eb 08 48 83 38 00 74 33 eb 53 83 38 00 75 4e 89 08 89 c8 eb 0a 83 38 00 75 43 <89> 08 48 63 c1 48 39 c8 74 2e eb 48 83 38 00 75 32 48 29 c1 89 08
> RSP: 0018:ffffb223c00dbb10 EFLAGS: 00010246
> RAX: ffffffffc005b1d9 RBX: 0000000000000000 RCX: ffffffff8b200060
> RDX: 000000000000000b RSI: 0000004b0000000b RDI: ffff96bdfcd33000
> RBP: ffffb223c00dbb38 R08: ffffffffc005d040 R09: ffffffffc005c1f0
> R10: ffff96bdfcd33c40 R11: ffff96bdfcd33b80 R12: 0000000000000018
> R13: ffffffffc005c1f0 R14: ffffffffc005e708 R15: ffffffff8b2fbc74
> FS: 00007f5f447beba8(0000) GS:ffff96bdff900000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: ffffffffc005b1d9 CR3: 000000003cedc002 CR4: 0000000000360ea0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> klp_init_object_loaded+0x10f/0x219
> ? preempt_latency_start+0x21/0x57
> klp_enable_patch+0x662/0x809
> ? virt_to_head_page+0x3a/0x3c
> ? kfree+0x8c/0x126
> patch_init+0x2ed/0x1000 [livepatch_test02]
> ? 0xffffffffc0060000
> do_one_initcall+0x9f/0x1c5
> ? kmem_cache_alloc_trace+0xc4/0xd4
> ? do_init_module+0x27/0x210
> do_init_module+0x5f/0x210
> load_module+0x1c41/0x2290
> ? fsnotify_path+0x3b/0x42
> ? strstarts+0x2b/0x2b
> ? kernel_read+0x58/0x65
> __do_sys_finit_module+0x9f/0xc3
> ? __do_sys_finit_module+0x9f/0xc3
> __x64_sys_finit_module+0x1a/0x1c
> do_syscall_64+0x52/0x61
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
>The above panic occurs when loading two modules at the same time with
>ftrace enabled, where at least one of the modules is a livepatch module:
>
>CPU0 CPU1
>klp_enable_patch()
> klp_init_object_loaded()
> module_disable_ro()
> ftrace_module_enable()
> ftrace_arch_code_modify_post_process()
> set_all_modules_text_ro()
> klp_write_object_relocations()
> apply_relocate_add()
> *patches read-only code* - BOOM
>
>A similar race exists when toggling ftrace while loading a livepatch
>module.
>
>Fix it by ensuring that the livepatch and ftrace code patching
>operations -- and their respective permissions changes -- are protected
>by the text_mutex.
>
>Reported-by: Johannes Erdfelt <johannes@...felt.com>
>Signed-off-by: Josh Poimboeuf <jpoimboe@...hat.com>
For module.c:
Acked-by: Jessica Yu <jeyu@...nel.org>
Thanks!
>---
> kernel/livepatch/core.c | 6 ++++++
> kernel/module.c | 21 ++++++++++++++++++---
> kernel/trace/ftrace.c | 10 +++++++++-
> 3 files changed, 33 insertions(+), 4 deletions(-)
>
>diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
>index 2398832947c6..c4ce08f43bd6 100644
>--- a/kernel/livepatch/core.c
>+++ b/kernel/livepatch/core.c
>@@ -18,6 +18,7 @@
> #include <linux/elf.h>
> #include <linux/moduleloader.h>
> #include <linux/completion.h>
>+#include <linux/memory.h>
> #include <asm/cacheflush.h>
> #include "core.h"
> #include "patch.h"
>@@ -718,16 +719,21 @@ static int klp_init_object_loaded(struct klp_patch *patch,
> struct klp_func *func;
> int ret;
>
>+ mutex_lock(&text_mutex);
>+
> module_disable_ro(patch->mod);
> ret = klp_write_object_relocations(patch->mod, obj);
> if (ret) {
> module_enable_ro(patch->mod, true);
>+ mutex_unlock(&text_mutex);
> return ret;
> }
>
> arch_klp_init_object_loaded(patch, obj);
> module_enable_ro(patch->mod, true);
>
>+ mutex_unlock(&text_mutex);
>+
> klp_for_each_func(obj, func) {
> ret = klp_find_object_symbol(obj->name, func->old_name,
> func->old_sympos,
>diff --git a/kernel/module.c b/kernel/module.c
>index 6e6712b3aaf5..3c056b56aefa 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -64,6 +64,7 @@
> #include <linux/bsearch.h>
> #include <linux/dynamic_debug.h>
> #include <linux/audit.h>
>+#include <linux/memory.h>
> #include <uapi/linux/module.h>
> #include "module-internal.h"
>
>@@ -1943,6 +1944,8 @@ static void frob_writable_data(const struct module_layout *layout,
> /* livepatching wants to disable read-only so it can frob module. */
> void module_disable_ro(const struct module *mod)
> {
>+ lockdep_assert_held(&text_mutex);
>+
> if (!rodata_enabled)
> return;
>
>@@ -1953,7 +1956,7 @@ void module_disable_ro(const struct module *mod)
> frob_rodata(&mod->init_layout, set_memory_rw);
> }
>
>-void module_enable_ro(const struct module *mod, bool after_init)
>+static void __module_enable_ro(const struct module *mod, bool after_init)
> {
> if (!rodata_enabled)
> return;
>@@ -1974,6 +1977,13 @@ void module_enable_ro(const struct module *mod, bool after_init)
> frob_ro_after_init(&mod->core_layout, set_memory_ro);
> }
>
>+void module_enable_ro(const struct module *mod, bool after_init)
>+{
>+ lockdep_assert_held(&text_mutex);
>+
>+ __module_enable_ro(mod, after_init);
>+}
>+
> static void module_enable_nx(const struct module *mod)
> {
> frob_rodata(&mod->core_layout, set_memory_nx);
>@@ -1988,6 +1998,8 @@ void set_all_modules_text_rw(void)
> {
> struct module *mod;
>
>+ lockdep_assert_held(&text_mutex);
>+
> if (!rodata_enabled)
> return;
>
>@@ -2007,6 +2019,8 @@ void set_all_modules_text_ro(void)
> {
> struct module *mod;
>
>+ lockdep_assert_held(&text_mutex);
>+
> if (!rodata_enabled)
> return;
>
>@@ -2027,6 +2041,7 @@ void set_all_modules_text_ro(void)
> mutex_unlock(&module_mutex);
> }
> #else
>+static void __module_enable_ro(const struct module *mod, bool after_init) { }
> static void module_enable_nx(const struct module *mod) { }
> #endif
>
>@@ -3519,7 +3534,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_ro(mod, true);
> mod_tree_remove_init(mod);
> module_arch_freeing_init(mod);
> mod->init_layout.base = NULL;
>@@ -3626,7 +3641,7 @@ static int complete_formation(struct module *mod, struct load_info *info)
> /* This relies on module_mutex for list integrity. */
> module_bug_finalize(info->hdr, info->sechdrs, mod);
>
>- module_enable_ro(mod, false);
>+ __module_enable_ro(mod, false);
> module_enable_nx(mod);
>
> /* Mark state as coming so strong_try_module_get() ignores us,
>diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
>index a12aff849c04..8259d4ba8b00 100644
>--- a/kernel/trace/ftrace.c
>+++ b/kernel/trace/ftrace.c
>@@ -34,6 +34,7 @@
> #include <linux/hash.h>
> #include <linux/rcupdate.h>
> #include <linux/kprobes.h>
>+#include <linux/memory.h>
>
> #include <trace/events/sched.h>
>
>@@ -2610,10 +2611,12 @@ static void ftrace_run_update_code(int command)
> {
> int ret;
>
>+ mutex_lock(&text_mutex);
>+
> ret = ftrace_arch_code_modify_prepare();
> FTRACE_WARN_ON(ret);
> if (ret)
>- return;
>+ goto out_unlock;
>
> /*
> * By default we use stop_machine() to modify the code.
>@@ -2625,6 +2628,9 @@ static void ftrace_run_update_code(int command)
>
> ret = ftrace_arch_code_modify_post_process();
> FTRACE_WARN_ON(ret);
>+
>+out_unlock:
>+ mutex_unlock(&text_mutex);
> }
>
> static void ftrace_run_modify_code(struct ftrace_ops *ops, int command,
>@@ -5776,6 +5782,7 @@ void ftrace_module_enable(struct module *mod)
> struct ftrace_page *pg;
>
> mutex_lock(&ftrace_lock);
>+ mutex_lock(&text_mutex);
>
> if (ftrace_disabled)
> goto out_unlock;
>@@ -5837,6 +5844,7 @@ void ftrace_module_enable(struct module *mod)
> ftrace_arch_code_modify_post_process();
>
> out_unlock:
>+ mutex_unlock(&text_mutex);
> mutex_unlock(&ftrace_lock);
>
> process_cached_mods(mod->name);
>--
>2.20.1
>
Powered by blists - more mailing lists