[PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()

[PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()

[Peter Zijlstra]

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(-)

--- a/arch/arm64/kernel/entry-ftrace.S
+++ b/arch/arm64/kernel/entry-ftrace.S
@@ -7,6 +7,7 @@
  */
 
 #include <linux/linkage.h>
+#include <linux/cfi_types.h>
 #include <asm/asm-offsets.h>
 #include <asm/assembler.h>
 #include <asm/ftrace.h>
@@ -294,10 +295,14 @@ SYM_FUNC_END(ftrace_graph_caller)
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
 #endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
 
-SYM_FUNC_START(ftrace_stub)
+SYM_TYPED_FUNC_START(ftrace_stub)
 	ret
 SYM_FUNC_END(ftrace_stub)
 
+SYM_TYPED_FUNC_START(ftrace_stub_graph)
+	ret
+SYM_FUNC_END(ftrace_stub_graph)
+
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 /*
  * void return_to_handler(void)
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -4,6 +4,7 @@
  */
 
 #include <linux/linkage.h>
+#include <linux/cfi_types.h>
 #include <asm/ptrace.h>
 #include <asm/ftrace.h>
 #include <asm/export.h>
@@ -129,6 +130,14 @@
 
 	.endm
 
+SYM_TYPED_FUNC_START(ftrace_stub)
+	RET
+SYM_FUNC_END(ftrace_stub)
+
+SYM_TYPED_FUNC_START(ftrace_stub_graph)
+	RET
+SYM_FUNC_END(ftrace_stub_graph)
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 SYM_FUNC_START(__fentry__)
@@ -176,11 +185,6 @@ SYM_INNER_LABEL(ftrace_caller_end, SYM_L
 SYM_FUNC_END(ftrace_caller);
 STACK_FRAME_NON_STANDARD_FP(ftrace_caller)
 
-SYM_FUNC_START(ftrace_stub)
-	UNWIND_HINT_FUNC
-	RET
-SYM_FUNC_END(ftrace_stub)
-
 SYM_FUNC_START(ftrace_regs_caller)
 	/* Save the current flags before any operations that can change them */
 	pushfq
@@ -282,9 +286,6 @@ STACK_FRAME_NON_STANDARD_FP(ftrace_regs_
 SYM_FUNC_START(__fentry__)
 	cmpq $ftrace_stub, ftrace_trace_function
 	jnz trace
-
-SYM_INNER_LABEL(ftrace_stub, SYM_L_GLOBAL)
-	ENDBR
 	RET
 
 trace:
--- 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
+/*
+ * 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
+
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
 /*
  * The ftrace call sites are logged to a section whose name depends on the
@@ -169,10 +179,6 @@
  * FTRACE_CALLSITE_SECTION. We capture all of them here to avoid header
  * dependencies for FTRACE_CALLSITE_SECTION's definition.
  *
- * Need to also make ftrace_stub_graph point to ftrace_stub
- * so that the same stub location may have different protocols
- * and not mess up with C verifiers.
- *
  * ftrace_ops_list_func will be defined as arch_ftrace_ops_list_func
  * as some archs will have a different prototype for that function
  * but ftrace_ops_list_func() will have a single prototype.
@@ -182,11 +188,11 @@
 			KEEP(*(__mcount_loc))			\
 			KEEP_PATCHABLE				\
 			__stop_mcount_loc = .;			\
-			ftrace_stub_graph = ftrace_stub;	\
+			FTRACE_STUB_HACK			\
 			ftrace_ops_list_func = arch_ftrace_ops_list_func;
 #else
 # ifdef CONFIG_FUNCTION_TRACER
-#  define MCOUNT_REC()	ftrace_stub_graph = ftrace_stub;	\
+#  define MCOUNT_REC()	FTRACE_STUB_HACK			\
 			ftrace_ops_list_func = arch_ftrace_ops_list_func;
 # else
 #  define MCOUNT_REC()

Re: [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()

[Peter Zijlstra]

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:

Re: [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()

[Peter Zijlstra]

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

Re: [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()

[Mark Rutland]

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.

Re: [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()

[Steven Rostedt]

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

Re: [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()

[Kees Cook]

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.

-- 
Kees Cook

Re: [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()

[Peter Zijlstra]

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.

Re: [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()

[Peter Zijlstra]

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.

Re: [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()

[Kees Cook]

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?)

-- 
Kees Cook

[tip: x86/urgent] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()

[tip-bot2 for Peter Zijlstra]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     883bbbffa5a4ffd1915f8b42934dab81b7f87226
Gitweb:        https://git.kernel.org/tip/883bbbffa5a4ffd1915f8b42934dab81b7f87226
Author:        Peter Zijlstra <peterz@infradead.org>
AuthorDate:    Tue, 18 Oct 2022 13:49:21 +02:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Thu, 20 Oct 2022 17:10:27 +02:00

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

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

As triggered by the ftrace boot tests:

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

Fixes: 3c516f89e17e ("x86: Add support for CONFIG_CFI_CLANG")
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Link: https://lkml.kernel.org/r/Y06dg4e1xF6JTdQq@hirez.programming.kicks-ass.net
---
 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(-)

diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
index bd5df50..795344a 100644
--- a/arch/arm64/kernel/entry-ftrace.S
+++ b/arch/arm64/kernel/entry-ftrace.S
@@ -7,6 +7,7 @@
  */
 
 #include <linux/linkage.h>
+#include <linux/cfi_types.h>
 #include <asm/asm-offsets.h>
 #include <asm/assembler.h>
 #include <asm/ftrace.h>
@@ -294,10 +295,14 @@ SYM_FUNC_END(ftrace_graph_caller)
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
 #endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
 
-SYM_FUNC_START(ftrace_stub)
+SYM_TYPED_FUNC_START(ftrace_stub)
 	ret
 SYM_FUNC_END(ftrace_stub)
 
+SYM_TYPED_FUNC_START(ftrace_stub_graph)
+	ret
+SYM_FUNC_END(ftrace_stub_graph)
+
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 /*
  * void return_to_handler(void)
diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S
index a90c55a..2a4be92 100644
--- a/arch/x86/kernel/ftrace_64.S
+++ b/arch/x86/kernel/ftrace_64.S
@@ -4,6 +4,7 @@
  */
 
 #include <linux/linkage.h>
+#include <linux/cfi_types.h>
 #include <asm/ptrace.h>
 #include <asm/ftrace.h>
 #include <asm/export.h>
@@ -129,6 +130,14 @@
 
 	.endm
 
+SYM_TYPED_FUNC_START(ftrace_stub)
+	RET
+SYM_FUNC_END(ftrace_stub)
+
+SYM_TYPED_FUNC_START(ftrace_stub_graph)
+	RET
+SYM_FUNC_END(ftrace_stub_graph)
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 SYM_FUNC_START(__fentry__)
@@ -176,11 +185,6 @@ SYM_INNER_LABEL(ftrace_caller_end, SYM_L_GLOBAL)
 SYM_FUNC_END(ftrace_caller);
 STACK_FRAME_NON_STANDARD_FP(ftrace_caller)
 
-SYM_FUNC_START(ftrace_stub)
-	UNWIND_HINT_FUNC
-	RET
-SYM_FUNC_END(ftrace_stub)
-
 SYM_FUNC_START(ftrace_regs_caller)
 	/* Save the current flags before any operations that can change them */
 	pushfq
@@ -282,9 +286,6 @@ STACK_FRAME_NON_STANDARD_FP(ftrace_regs_caller)
 SYM_FUNC_START(__fentry__)
 	cmpq $ftrace_stub, ftrace_trace_function
 	jnz trace
-
-SYM_INNER_LABEL(ftrace_stub, SYM_L_GLOBAL)
-	ENDBR
 	RET
 
 trace:
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index c15de16..d06ada2 100644
--- 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 CONFIG_ARCH_SUPPORTS_CFI_CLANG
+/*
+ * 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
+
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
 /*
  * The ftrace call sites are logged to a section whose name depends on the
@@ -169,10 +179,6 @@
  * FTRACE_CALLSITE_SECTION. We capture all of them here to avoid header
  * dependencies for FTRACE_CALLSITE_SECTION's definition.
  *
- * Need to also make ftrace_stub_graph point to ftrace_stub
- * so that the same stub location may have different protocols
- * and not mess up with C verifiers.
- *
  * ftrace_ops_list_func will be defined as arch_ftrace_ops_list_func
  * as some archs will have a different prototype for that function
  * but ftrace_ops_list_func() will have a single prototype.
@@ -182,11 +188,11 @@
 			KEEP(*(__mcount_loc))			\
 			KEEP_PATCHABLE				\
 			__stop_mcount_loc = .;			\
-			ftrace_stub_graph = ftrace_stub;	\
+			FTRACE_STUB_HACK			\
 			ftrace_ops_list_func = arch_ftrace_ops_list_func;
 #else
 # ifdef CONFIG_FUNCTION_TRACER
-#  define MCOUNT_REC()	ftrace_stub_graph = ftrace_stub;	\
+#  define MCOUNT_REC()	FTRACE_STUB_HACK			\
 			ftrace_ops_list_func = arch_ftrace_ops_list_func;
 # else
 #  define MCOUNT_REC()