ftrace,kcfi: Separate ftrace_stub() and ftrace_stub_graph()
Commit Message
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(-)
Comments
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?)
@@ -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)
@@ -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:
@@ -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()