[PATCH] livepatch: Fix ftrace module text permissions race

[PATCH] livepatch: Fix ftrace module text permissions race

[Josh Poimboeuf]

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@erdfelt.com>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 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

Re: [PATCH] livepatch: Fix ftrace module text permissions race

[Jessica Yu]

+++ 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@erdfelt.com>
>Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

For module.c:

Acked-by: Jessica Yu <jeyu@kernel.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
>

Re: [PATCH] livepatch: Fix ftrace module text permissions race

[Petr Mladek]

On Wed 2019-05-29 14:02:24, Josh Poimboeuf wrote:
> 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

This patch looks fine and fixes the race:

Reviewed-by: Petr Mladek <pmladek@suse.com>


That said, the semantic of text_mutex is a bit unclear:

   + It serializes RO/RW setting but not NX

   + Nothing prevents manipulation of the access rights
     by external code before the module is ready-enough.
     I mean before the sections are set RO by the module
     loader itself.

     Most sections are ready in MODULE_STATE_COMMING state.
     Only ro_after_init sections need to stay RW longer,
     see my question below.


> diff --git a/kernel/module.c b/kernel/module.c
> index 6e6712b3aaf5..3c056b56aefa 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -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);

The "true" parameter causes that also ro_after_init section is
set read only. What is the purpose of this section, please?

I ask because module_enable_ro(mod, true) can be called
earlier from klp_init_object_loaded() from do_one_initcall().

For example, it could some MODULE_STATE_LIVE notifier
when it requires write access to ro_after_init section.

Anyway, the above is a separate problem. This patch looks
fine for the original problem.

Best Regards,
Petr

Re: [PATCH] livepatch: Fix ftrace module text permissions race

[Miroslav Benes]

On Wed, 29 May 2019, Josh Poimboeuf wrote:

> 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@erdfelt.com>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>

For the code

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

However, shouldn't the patch be split in two? One adding text_mutex 
protection to livepatch and ftrace. The other adding lockdep_assert_held() 
and __module_enable_ro()? The current changelog does not mention lockdep 
changes at all.

Miroslav

Re: [PATCH] livepatch: Fix ftrace module text permissions race

[Josh Poimboeuf]

On Thu, May 30, 2019 at 03:54:14PM +0200, Petr Mladek wrote:
> On Wed 2019-05-29 14:02:24, Josh Poimboeuf wrote:
> > 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
> 
> This patch looks fine and fixes the race:
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> 
> 
> That said, the semantic of text_mutex is a bit unclear:
> 
>    + It serializes RO/RW setting but not NX

True.  module_enable_nx() is a static function which is only called
internally.  I should probably rename it to __module_enable_nx() so the
locking semantics match the others.

>    + Nothing prevents manipulation of the access rights
>      by external code before the module is ready-enough.
>      I mean before the sections are set RO by the module
>      loader itself.
> 
>      Most sections are ready in MODULE_STATE_COMMING state.
>      Only ro_after_init sections need to stay RW longer,
>      see my question below.
> 
> 
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 6e6712b3aaf5..3c056b56aefa 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -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);
> 
> The "true" parameter causes that also ro_after_init section is
> set read only. What is the purpose of this section, please?
> 
> I ask because module_enable_ro(mod, true) can be called
> earlier from klp_init_object_loaded() from do_one_initcall().
> 
> For example, it could some MODULE_STATE_LIVE notifier
> when it requires write access to ro_after_init section.

Hm, I think you're right.  klp_init_object_loaded() should change the
module_enable_ro() argument to false when it's called from a patch
module's initcall.

Maybe we can instead remove __module_enable_ro()'s after_init argument
and just make it smarter?  It should only do ro_after_init frobbing if
the module state is MODULE_STATE_LIVE.

> Anyway, the above is a separate problem. This patch looks
> fine for the original problem.

Thanks for the review.  I'll post another version, with the above
changes and with the patches split up like Miroslav suggested.

-- 
Josh

Re: [PATCH] livepatch: Fix ftrace module text permissions race

[Josh Poimboeuf]

On Fri, May 31, 2019 at 02:12:56PM -0500, Josh Poimboeuf wrote:
> > Anyway, the above is a separate problem. This patch looks
> > fine for the original problem.
> 
> Thanks for the review.  I'll post another version, with the above
> changes and with the patches split up like Miroslav suggested.

The latest patches are here:

  https://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git/log/?h=fix-livepatch-ftrace-race

If the bot likes them, I'll post them properly soon.

-- 
Josh

Re: [PATCH] livepatch: Fix ftrace module text permissions race

[Steven Rostedt]

On Fri, 31 May 2019 17:25:27 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> On Fri, May 31, 2019 at 02:12:56PM -0500, Josh Poimboeuf wrote:
> > > Anyway, the above is a separate problem. This patch looks
> > > fine for the original problem.  
> > 
> > Thanks for the review.  I'll post another version, with the above
> > changes and with the patches split up like Miroslav suggested.  
> 
> The latest patches are here:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git/log/?h=fix-livepatch-ftrace-race
> 
> If the bot likes them, I'll post them properly soon.
> 

Was this ever posted?

-- Steve

Re: [PATCH] livepatch: Fix ftrace module text permissions race

[Josh Poimboeuf]

On Thu, Jun 13, 2019 at 05:38:04PM -0400, Steven Rostedt wrote:
> On Fri, 31 May 2019 17:25:27 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> > On Fri, May 31, 2019 at 02:12:56PM -0500, Josh Poimboeuf wrote:
> > > > Anyway, the above is a separate problem. This patch looks
> > > > fine for the original problem.  
> > > 
> > > Thanks for the review.  I'll post another version, with the above
> > > changes and with the patches split up like Miroslav suggested.  
> > 
> > The latest patches are here:
> > 
> >   https://git.kernel.org/pub/scm/linux/kernel/git/jpoimboe/linux.git/log/?h=fix-livepatch-ftrace-race
> > 
> > If the bot likes them, I'll post them properly soon.
> > 
> 
> Was this ever posted?

I totally forgot.  I'll post them now.

-- 
Josh