linux-kernel - Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module text read-write

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:   Thu, 10 Oct 2019 13:20:13 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     LKML <linux-kernel@...r.kernel.org>, Jessica Yu <jeyu@...nel.org>,
        Ingo Molnar <mingo@...nel.org>
Subject: Re: [PATCH] ftrace/module: Allow ftrace to make only loaded module
 text read-write

On Thu, 10 Oct 2019 19:01:11 +0200
Peter Zijlstra <peterz@...radead.org> wrote:

> On Thu, Oct 10, 2019 at 10:55:15AM -0400, Steven Rostedt wrote:
> 
> > OK, so basically this moves the enabling of function tracing from
> > within the ftrace_module_enable() code without releasing the
> > ftrace_lock mutex.
> > 
> > But we have an issue with the state of the module here, as it is still
> > set as MODULE_STATE_UNFORMED. Let's look at what happens if we have:
> > 
> > 
> > 	CPU0				CPU1
> > 	----				----
> >  echo function > current_tracer
> > 				modprobe foo
> > 				  enable foo functions to be traced
> > 				  (foo function records not disabled)
> >  echo nop > current_tracer
> > 
> >    disable all functions being
> >    traced including foo functions
> > 
> >    arch calls set_all_modules_text_rw()
> >     [skips UNFORMED modules, which foo still is ]
> > 
> > 				  set foo's text to read-only
> > 				  foo's state to COMING
> > 
> >    tries to disable foo's functions
> >    foo's text is read-only
> > 
> >    BUG trying to write to ro text!!!
> > 
> > 
> > Like I said, this is very subtle. It may no longer be a bug on x86
> > with your patches, but it will bug on ARM or anything else that still
> > uses set_all_modules_text_rw() in the ftrace prepare code.  
> 
> I can't immediately follow, but I think we really should go there.
> 
> For now, something like this might work:

Hmm, I'm lost at what the below is trying to do with respect to the
above.

-- Steve

> 
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -34,6 +34,8 @@
>  
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  
> +static int ftrace_poke_late = 0;
> +
>  int ftrace_arch_code_modify_prepare(void)
>      __acquires(&text_mutex)
>  {
> @@ -43,12 +45,15 @@ int ftrace_arch_code_modify_prepare(void
>  	 * ftrace has it set to "read/write".
>  	 */
>  	mutex_lock(&text_mutex);
> +	ftrace_poke_late = 1;
>  	return 0;
>  }
>  
>  int ftrace_arch_code_modify_post_process(void)
>      __releases(&text_mutex)
>  {
> +	text_poke_finish();
> +	ftrace_poke_late = 0;
>  	mutex_unlock(&text_mutex);
>  	return 0;
>  }
> @@ -116,7 +121,10 @@ ftrace_modify_code_direct(unsigned long
>  		return ret;
>  
>  	/* replace the text with the new text */
> -	text_poke_early((void *)ip, new_code, MCOUNT_INSN_SIZE);
> +	if (ftrace_poke_late)
> +		text_poke_queue((void *)ip, new_code, MCOUNT_INSN_SIZE, NULL);
> +	else
> +		text_poke_early((void *)ip, new_code, MCOUNT_INSN_SIZE);
>  	return 0;
>  }
>