linux-kernel - Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe() and ftrace_run_update

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 28 Jun 2019 09:32:03 +0200 (CEST)
From:   Miroslav Benes <mbenes@...e.cz>
To:     Petr Mladek <pmladek@...e.com>
cc:     Steven Rostedt <rostedt@...dmis.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Jessica Yu <jeyu@...nel.org>, Jiri Kosina <jikos@...nel.org>,
        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>, mhiramat@...nel.org,
        torvalds@...ux-foundation.org, tglx@...utronix.de
Subject: Re: [PATCH] ftrace: Remove possible deadlock between register_kprobe()
 and ftrace_run_update_code()

On Thu, 27 Jun 2019, Petr Mladek wrote:

> The commit 9f255b632bf12c4dd7 ("module: Fix livepatch/ftrace module text
> permissions race") causes a possible deadlock between register_kprobe()
> and ftrace_run_update_code() when ftrace is using stop_machine().
> 
> The existing dependency chain (in reverse order) is:
> 
> -> #1 (text_mutex){+.+.}:
>        validate_chain.isra.21+0xb32/0xd70
>        __lock_acquire+0x4b8/0x928
>        lock_acquire+0x102/0x230
>        __mutex_lock+0x88/0x908
>        mutex_lock_nested+0x32/0x40
>        register_kprobe+0x254/0x658
>        init_kprobes+0x11a/0x168
>        do_one_initcall+0x70/0x318
>        kernel_init_freeable+0x456/0x508
>        kernel_init+0x22/0x150
>        ret_from_fork+0x30/0x34
>        kernel_thread_starter+0x0/0xc
> 
> -> #0 (cpu_hotplug_lock.rw_sem){++++}:
>        check_prev_add+0x90c/0xde0
>        validate_chain.isra.21+0xb32/0xd70
>        __lock_acquire+0x4b8/0x928
>        lock_acquire+0x102/0x230
>        cpus_read_lock+0x62/0xd0
>        stop_machine+0x2e/0x60
>        arch_ftrace_update_code+0x2e/0x40
>        ftrace_run_update_code+0x40/0xa0
>        ftrace_startup+0xb2/0x168
>        register_ftrace_function+0x64/0x88
>        klp_patch_object+0x1a2/0x290
>        klp_enable_patch+0x554/0x980
>        do_one_initcall+0x70/0x318
>        do_init_module+0x6e/0x250
>        load_module+0x1782/0x1990
>        __s390x_sys_finit_module+0xaa/0xf0
>        system_call+0xd8/0x2d0
> 
>  Possible unsafe locking scenario:
> 
>        CPU0                    CPU1
>        ----                    ----
>   lock(text_mutex);
>                                lock(cpu_hotplug_lock.rw_sem);
>                                lock(text_mutex);
>   lock(cpu_hotplug_lock.rw_sem);
> 
> It is similar problem that has been solved by the commit 2d1e38f56622b9b
> ("kprobes: Cure hotplug lock ordering issues"). Many locks are involved.
> To be on the safe side, text_mutex must become a low level lock taken
> after cpu_hotplug_lock.rw_sem.
> 
> This can't be achieved easily with the current ftrace design.
> For example, arm calls set_all_modules_text_rw() already in
> ftrace_arch_code_modify_prepare(), see arch/arm/kernel/ftrace.c.
> This functions is called:
> 
>   + outside stop_machine() from ftrace_run_update_code()
>   + without stop_machine() from ftrace_module_enable()
> 
> Fortunately, the problematic fix is needed only on x86_64. It is
> the only architecture that calls set_all_modules_text_rw()
> in ftrace path and supports livepatching at the same time.
> 
> Therefore it is enough to move text_mutex handling from the generic
> kernel/trace/ftrace.c into arch/x86/kernel/ftrace.c:
> 
>    ftrace_arch_code_modify_prepare()
>    ftrace_arch_code_modify_post_process()
> 
> This patch basically reverts the ftrace part of the problematic
> commit 9f255b632bf12c4dd7 ("module: Fix livepatch/ftrace module
> text permissions race"). And provides x86_64 specific-fix.
> 
> Some refactoring of the ftrace code will be needed when livepatching
> is implemented for arm or nds32. These architectures call
> set_all_modules_text_rw() and use stop_machine() at the same time.
> 
> Fixes: 9f255b632bf12c4dd7 ("module: Fix livepatch/ftrace module text permissions race")
> Signed-off-by: Petr Mladek <pmladek@...e.com>

Reported-by: Miroslav Benes <mbenes@...e.cz>

> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 38277af44f5c..d3034a4a3fcc 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -34,7 +34,6 @@
>  #include <linux/hash.h>
>  #include <linux/rcupdate.h>
>  #include <linux/kprobes.h>
> -#include <linux/memory.h>
>  
>  #include <trace/events/sched.h>
>  
> @@ -2611,12 +2610,10 @@ 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)
> -		goto out_unlock;
> +		return ret;

Should be just "return;", because the function is "static void".

With that

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

Miroslav