* Re: [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()
2022-10-18 12:35 [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph() Peter Zijlstra
@ 2022-10-18 12:36 ` Peter Zijlstra
2022-10-18 13:18 ` Peter Zijlstra
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Peter Zijlstra @ 2022-10-18 12:36 UTC (permalink / raw)
To: Steven Rostedt, x86, linux-kernel, Mark Rutland; +Cc: Kees Cook, Sami Tolvanen
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:
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()
2022-10-18 12:35 [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph() Peter Zijlstra
2022-10-18 12:36 ` Peter Zijlstra
@ 2022-10-18 13:18 ` Peter Zijlstra
2022-10-18 13:26 ` Mark Rutland
2022-10-18 14:28 ` Kees Cook
2022-10-18 14:21 ` Steven Rostedt
2022-10-20 15:17 ` [tip: x86/urgent] " tip-bot2 for Peter Zijlstra
3 siblings, 2 replies; 10+ messages in thread
From: Peter Zijlstra @ 2022-10-18 13:18 UTC (permalink / raw)
To: Steven Rostedt, x86, linux-kernel, Mark Rutland; +Cc: Kees Cook, Sami Tolvanen
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()
2022-10-18 13:18 ` Peter Zijlstra
@ 2022-10-18 13:26 ` Mark Rutland
2022-10-18 14:28 ` Kees Cook
1 sibling, 0 replies; 10+ messages in thread
From: Mark Rutland @ 2022-10-18 13:26 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Steven Rostedt, x86, linux-kernel, Kees Cook, Sami Tolvanen
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()
2022-10-18 13:18 ` Peter Zijlstra
2022-10-18 13:26 ` Mark Rutland
@ 2022-10-18 14:28 ` Kees Cook
2022-10-18 15:00 ` Peter Zijlstra
1 sibling, 1 reply; 10+ messages in thread
From: Kees Cook @ 2022-10-18 14:28 UTC (permalink / raw)
To: Peter Zijlstra, Steven Rostedt, x86, linux-kernel, Mark Rutland
Cc: Kees Cook, Sami Tolvanen
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()
2022-10-18 14:28 ` Kees Cook
@ 2022-10-18 15:00 ` Peter Zijlstra
2022-10-18 18:22 ` Kees Cook
0 siblings, 1 reply; 10+ messages in thread
From: Peter Zijlstra @ 2022-10-18 15:00 UTC (permalink / raw)
To: Kees Cook
Cc: Steven Rostedt, x86, linux-kernel, Mark Rutland, Kees Cook,
Sami Tolvanen
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()
2022-10-18 15:00 ` Peter Zijlstra
@ 2022-10-18 18:22 ` Kees Cook
0 siblings, 0 replies; 10+ messages in thread
From: Kees Cook @ 2022-10-18 18:22 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Steven Rostedt, x86, linux-kernel, Mark Rutland, Sami Tolvanen
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()
2022-10-18 12:35 [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph() Peter Zijlstra
2022-10-18 12:36 ` Peter Zijlstra
2022-10-18 13:18 ` Peter Zijlstra
@ 2022-10-18 14:21 ` Steven Rostedt
2022-10-18 15:02 ` Peter Zijlstra
2022-10-20 15:17 ` [tip: x86/urgent] " tip-bot2 for Peter Zijlstra
3 siblings, 1 reply; 10+ messages in thread
From: Steven Rostedt @ 2022-10-18 14:21 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: x86, linux-kernel, Mark Rutland, Kees Cook, Sami Tolvanen
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()
2022-10-18 14:21 ` Steven Rostedt
@ 2022-10-18 15:02 ` Peter Zijlstra
0 siblings, 0 replies; 10+ messages in thread
From: Peter Zijlstra @ 2022-10-18 15:02 UTC (permalink / raw)
To: Steven Rostedt; +Cc: x86, linux-kernel, Mark Rutland, Kees Cook, Sami Tolvanen
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tip: x86/urgent] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()
2022-10-18 12:35 [PATCH] ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph() Peter Zijlstra
` (2 preceding siblings ...)
2022-10-18 14:21 ` Steven Rostedt
@ 2022-10-20 15:17 ` tip-bot2 for Peter Zijlstra
3 siblings, 0 replies; 10+ messages in thread
From: tip-bot2 for Peter Zijlstra @ 2022-10-20 15:17 UTC (permalink / raw)
To: linux-tip-commits; +Cc: Peter Zijlstra (Intel), Mark Rutland, x86, linux-kernel
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()
^ permalink raw reply related [flat|nested] 10+ messages in thread