ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()

  Different function signatures means they needs to be different
functions; otherwise CFI gets upset.

Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---

Notable; this patch depends on eac828eaef29 ("x86/ftrace: Remove
ftrace_epilogue()") which can be cleanly picked on top of -rc1.

Since kCFI is upstream this should go into some /urgent tree.

 arch/arm64/kernel/entry-ftrace.S  |    7 ++++++-
 arch/x86/kernel/ftrace_64.S       |   17 +++++++++--------
 include/asm-generic/vmlinux.lds.h |   18 ++++++++++++------
 3 files changed, 27 insertions(+), 15 deletions(-)
  

On Tue, Oct 18, 2022 at 02:35:16PM +0200, Peter Zijlstra wrote:
> 
> Different function signatures means they needs to be different
> functions; otherwise CFI gets upset.
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> 
> Notable; this patch depends on eac828eaef29 ("x86/ftrace: Remove
> ftrace_epilogue()") which can be cleanly picked on top of -rc1.
> 
> Since kCFI is upstream this should go into some /urgent tree.

Combined (eac828eaef29 + this patch) generate a conflict against
tip/x86/core. Resolution looks like:

diff --cc arch/x86/kernel/ftrace_64.S
index 2a4be92fd144,6a7e6d666a12..000000000000
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@@ -4,7 -4,7 +4,8 @@@
   */
  
  #include <linux/linkage.h>
 +#include <linux/cfi_types.h>
+ #include <asm/asm-offsets.h>
  #include <asm/ptrace.h>
  #include <asm/ftrace.h>
  #include <asm/export.h>
@@@ -130,14 -130,6 +131,16 @@@
  
  	.endm
  
 +SYM_TYPED_FUNC_START(ftrace_stub)
++	CALL_DEPTH_ACCOUNT
 +	RET
 +SYM_FUNC_END(ftrace_stub)
 +
 +SYM_TYPED_FUNC_START(ftrace_stub_graph)
++	CALL_DEPTH_ACCOUNT
 +	RET
 +SYM_FUNC_END(ftrace_stub_graph)
 +
  #ifdef CONFIG_DYNAMIC_FTRACE
  
  SYM_FUNC_START(__fentry__)
@@@ -284,8 -305,13 +311,10 @@@ STACK_FRAME_NON_STANDARD_FP(ftrace_regs
  #else /* ! CONFIG_DYNAMIC_FTRACE */
  
  SYM_FUNC_START(__fentry__)
+ 	CALL_DEPTH_ACCOUNT
+ 
  	cmpq $ftrace_stub, ftrace_trace_function
  	jnz trace
 -
 -SYM_INNER_LABEL(ftrace_stub, SYM_L_GLOBAL)
 -	ENDBR
  	RET
  
  trace:
  

On Tue, Oct 18, 2022 at 02:35:16PM +0200, Peter Zijlstra wrote:
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -162,6 +162,16 @@
>  #define PATCHABLE_DISCARDS	*(__patchable_function_entries)
>  #endif
>  
> +#ifndef ARCH_SUPPORTS_CFI_CLANG

#ifndef CONFIG_ARCH_..

works much better as we found.

> +/*
> + * Simply points to ftrace_stub, but with the proper protocol.
> + * Defined by the linker script in linux/vmlinux.lds.h
> + */
> +#define	FTRACE_STUB_HACK	ftrace_stub_graph = ftrace_stub;
> +#else
> +#define FTRACE_STUB_HACK
> +#endif

Fixed up version available at:

  git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/urgent
  

On Tue, Oct 18, 2022 at 03:18:46PM +0200, Peter Zijlstra wrote:
> On Tue, Oct 18, 2022 at 02:35:16PM +0200, Peter Zijlstra wrote:
> > --- a/include/asm-generic/vmlinux.lds.h
> > +++ b/include/asm-generic/vmlinux.lds.h
> > @@ -162,6 +162,16 @@
> >  #define PATCHABLE_DISCARDS	*(__patchable_function_entries)
> >  #endif
> >  
> > +#ifndef ARCH_SUPPORTS_CFI_CLANG
> 
> #ifndef CONFIG_ARCH_..
> 
> works much better as we found.
> 
> > +/*
> > + * Simply points to ftrace_stub, but with the proper protocol.
> > + * Defined by the linker script in linux/vmlinux.lds.h
> > + */
> > +#define	FTRACE_STUB_HACK	ftrace_stub_graph = ftrace_stub;
> > +#else
> > +#define FTRACE_STUB_HACK
> > +#endif
> 
> Fixed up version available at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/urgent

Thanks for this!

I've just tested that branch, (HEAD commit 586edc9c16e747ce). With kCFI
enabled I can run the ftrace boot tests and ftrace selftests without
issues.

FWIW:

  Reviewed-by: Mark Rutland <mark.rutland@arm.com>
  Tested-by: Mark Rutland <mark.rutland@arm.com>

Mark.
  

On Tue, 18 Oct 2022 14:35:15 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> Different function signatures means they needs to be different
> functions; otherwise CFI gets upset.

This is due to this commit:

commit b83b43ffc6e4b514ca034a0fbdee01322e2f7022
Author: Steven Rostedt (VMware) <rostedt@goodmis.org>
Date:   Tue Oct 15 09:00:55 2019 -0400

    fgraph: Fix function type mismatches of ftrace_graph_return using ftrace_stub
    
    The C compiler is allowing more checks to make sure that function pointers
    are assigned to the correct prototype function. Unfortunately, the function
    graph tracer uses a special name with its assigned ftrace_graph_return
    function pointer that maps to a stub function used by the function tracer
    (ftrace_stub). The ftrace_graph_return variable is compared to the
    ftrace_stub in some archs to know if the function graph tracer is enabled or
    not. This means we can not just simply create a new function stub that
    compares it without modifying all the archs.
    
    Instead, have the linker script create a function_graph_stub that maps to
    ftrace_stub, and this way we can define the prototype for it to match the
    prototype of ftrace_graph_return, and make the compiler checks all happy!


Perhaps its time to just modify all the archs and get rid of that hack.

Ideally, all the archs should be switched over to the FTRACE_WITH_ARGS and
we can get rid of all of this. But that may be asking for too many ponies.

At the very least, perhaps we should just make all the archs define a
ftrace_stub_graph that need it and add a new CONFIG_HAVE.. that allows us to
remove some of this from the core code.

-- Steve
  

On October 18, 2022 6:18:46 AM PDT, Peter Zijlstra <peterz@infradead.org> wrote:
>On Tue, Oct 18, 2022 at 02:35:16PM +0200, Peter Zijlstra wrote:
>> --- a/include/asm-generic/vmlinux.lds.h
>> +++ b/include/asm-generic/vmlinux.lds.h
>> @@ -162,6 +162,16 @@
>>  #define PATCHABLE_DISCARDS	*(__patchable_function_entries)
>>  #endif
>>  
>> +#ifndef ARCH_SUPPORTS_CFI_CLANG
>
>#ifndef CONFIG_ARCH_..
>
>works much better as we found.

I nearly did this with an IS_ENABLED() recently. Saved by checkpatch! I wonder if it has similar checks for #ifdefs...

>
>> +/*
>> + * Simply points to ftrace_stub, but with the proper protocol.
>> + * Defined by the linker script in linux/vmlinux.lds.h
>> + */
>> +#define	FTRACE_STUB_HACK	ftrace_stub_graph = ftrace_stub;
>> +#else
>> +#define FTRACE_STUB_HACK
>> +#endif
>
>Fixed up version available at:
>
>  git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git x86/urgent

Thanks for solving this! Just for future archeology, can you include the splat (I assume you hit a CFI splat) in the commit log, and/or how you triggered the problem? I usually find it helpful in trying to fix similar issues later, etc.
  

On Tue, Oct 18, 2022 at 07:28:29AM -0700, Kees Cook wrote:

> Thanks for solving this! Just for future archeology, can you include
> the splat (I assume you hit a CFI splat) in the commit log, and/or how
> you triggered the problem? I usually find it helpful in trying to fix
> similar issues later, etc.

Unfortunately I didn't save it; it was a while ago (I sorta lost track
of this fix for a while since it was stuffed in my fineibt queue).

But Mark ran it today to confirm on arm64 and there it looks like
(harvested from IRC):

  [    3.153082] CFI failure at ftrace_return_to_handler+0xac/0x16c (target: ftrace_stub+0x0/0x14; expected type: 0x0a5d5347)

I think simply enabling the ftrace boot time tests is enough to trigger
this.
  

On Tue, Oct 18, 2022 at 10:21:00AM -0400, Steven Rostedt wrote:
> On Tue, 18 Oct 2022 14:35:15 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > Different function signatures means they needs to be different
> > functions; otherwise CFI gets upset.
> 
> This is due to this commit:
> 
> commit b83b43ffc6e4b514ca034a0fbdee01322e2f7022
> Author: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Date:   Tue Oct 15 09:00:55 2019 -0400
> 
>     fgraph: Fix function type mismatches of ftrace_graph_return using ftrace_stub
>     
>     The C compiler is allowing more checks to make sure that function pointers
>     are assigned to the correct prototype function. Unfortunately, the function
>     graph tracer uses a special name with its assigned ftrace_graph_return
>     function pointer that maps to a stub function used by the function tracer
>     (ftrace_stub). The ftrace_graph_return variable is compared to the
>     ftrace_stub in some archs to know if the function graph tracer is enabled or
>     not. This means we can not just simply create a new function stub that
>     compares it without modifying all the archs.
>     
>     Instead, have the linker script create a function_graph_stub that maps to
>     ftrace_stub, and this way we can define the prototype for it to match the
>     prototype of ftrace_graph_return, and make the compiler checks all happy!
> 
> 
> Perhaps its time to just modify all the archs and get rid of that hack.

Ideally yes, but given kCFI is in Linus' tree now, I didn't feel like
fixing up all archs in a hurry.

Mark mentioned that most archs can probably move to a common empty C
function for each stub.
  

On Tue, Oct 18, 2022 at 05:00:33PM +0200, Peter Zijlstra wrote:
> On Tue, Oct 18, 2022 at 07:28:29AM -0700, Kees Cook wrote:
> 
> > Thanks for solving this! Just for future archeology, can you include
> > the splat (I assume you hit a CFI splat) in the commit log, and/or how
> > you triggered the problem? I usually find it helpful in trying to fix
> > similar issues later, etc.
> 
> Unfortunately I didn't save it; it was a while ago (I sorta lost track
> of this fix for a while since it was stuffed in my fineibt queue).
> 
> But Mark ran it today to confirm on arm64 and there it looks like
> (harvested from IRC):
> 
>   [    3.153082] CFI failure at ftrace_return_to_handler+0xac/0x16c (target: ftrace_stub+0x0/0x14; expected type: 0x0a5d5347)
> 
> I think simply enabling the ftrace boot time tests is enough to trigger
> this.

Ah-ha, perfect. Thanks! (If it's not yet in tip, can you update the
commit log?)