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]

Message-ID: <20190627190457.703a486e@gandalf.local.home>
Date:   Thu, 27 Jun 2019 19:04:57 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Josh Poimboeuf <jpoimboe@...hat.com>
Cc:     Petr Mladek <pmladek@...e.com>, Miroslav Benes <mbenes@...e.cz>,
        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 17:47:29 -0500
Josh Poimboeuf <jpoimboe@...hat.com> wrote:

> Thanks a lot for fixing this Petr.
> 
> On Thu, Jun 27, 2019 at 10:13:34AM +0200, Petr Mladek wrote:
> > @@ -35,6 +36,7 @@
> >  
> >  int ftrace_arch_code_modify_prepare(void)
> >  {
> > +	mutex_lock(&text_mutex);
> >  	set_kernel_text_rw();
> >  	set_all_modules_text_rw();
> >  	return 0;
> > @@ -44,6 +46,7 @@ int ftrace_arch_code_modify_post_process(void)
> >  {
> >  	set_all_modules_text_ro();
> >  	set_kernel_text_ro();
> > +	mutex_unlock(&text_mutex);
> >  	return 0;
> >  }  
> 
> Releasing the lock in a separate function seems a bit surprising and
> fragile, would it be possible to do something like this instead?
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index b38c388d1087..89ea1af6fd13 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -37,15 +37,21 @@
>  int ftrace_arch_code_modify_prepare(void)
>  {
>  	mutex_lock(&text_mutex);
> +
>  	set_kernel_text_rw();
>  	set_all_modules_text_rw();
> +
> +	mutex_unlock(&text_mutex);
>  	return 0;
>  }
>  
>  int ftrace_arch_code_modify_post_process(void)
>  {
> +	mutex_lock(&text_mutex);
> +
>  	set_all_modules_text_ro();
>  	set_kernel_text_ro();
> +
>  	mutex_unlock(&text_mutex);
>  	return 0;
>  }

I agree with Josh on this. As the original bug was the race between
ftrace and live patching / modules changing the text from ro to rw and
vice versa. Just protecting the update to the text permissions is more
robust, and should be more self documenting when we need to handle
other architectures for this.

-- Steve