[PATCH] ftrace: fix function type mismatches

[PATCH] ftrace: fix function type mismatches

[Sami Tolvanen]

This change fixes indirect call mismatches with function and function
graph tracing, which trip Control-Flow Integrity (CFI) checking.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 kernel/trace/fgraph.c | 9 ++++++---
 kernel/trace/ftrace.c | 8 +++++---
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
index 7950a0356042..ecfd4a4a106a 100644
--- a/kernel/trace/fgraph.c
+++ b/kernel/trace/fgraph.c
@@ -327,14 +327,17 @@ void ftrace_graph_sleep_time_control(bool enable)
 	fgraph_sleep_time = enable;
 }
 
+void ftrace_graph_return_stub(struct ftrace_graph_ret *trace)
+{
+}
+
 int ftrace_graph_entry_stub(struct ftrace_graph_ent *trace)
 {
 	return 0;
 }
 
 /* The callbacks that hook a function */
-trace_func_graph_ret_t ftrace_graph_return =
-			(trace_func_graph_ret_t)ftrace_stub;
+trace_func_graph_ret_t ftrace_graph_return = ftrace_graph_return_stub;
 trace_func_graph_ent_t ftrace_graph_entry = ftrace_graph_entry_stub;
 static trace_func_graph_ent_t __ftrace_graph_entry = ftrace_graph_entry_stub;
 
@@ -614,7 +617,7 @@ void unregister_ftrace_graph(struct fgraph_ops *gops)
 		goto out;
 
 	ftrace_graph_active--;
-	ftrace_graph_return = (trace_func_graph_ret_t)ftrace_stub;
+	ftrace_graph_return = ftrace_graph_return_stub;
 	ftrace_graph_entry = ftrace_graph_entry_stub;
 	__ftrace_graph_entry = ftrace_graph_entry_stub;
 	ftrace_shutdown(&graph_ops, FTRACE_STOP_FUNC_RET);
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 62a50bf399d6..b68ee130d4a2 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -125,8 +125,9 @@ static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
 				 struct ftrace_ops *op, struct pt_regs *regs);
 #else
 /* See comment below, where ftrace_ops_list_func is defined */
-static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip);
-#define ftrace_ops_list_func ((ftrace_func_t)ftrace_ops_no_ops)
+static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip,
+			      struct ftrace_ops *op, struct pt_regs *regs);
+#define ftrace_ops_list_func ftrace_ops_no_ops
 #endif
 
 static inline void ftrace_ops_init(struct ftrace_ops *ops)
@@ -6325,7 +6326,8 @@ static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
 }
 NOKPROBE_SYMBOL(ftrace_ops_list_func);
 #else
-static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip)
+static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip,
+			      struct ftrace_ops *op, struct pt_regs *regs)
 {
 	__ftrace_ops_list_func(ip, parent_ip, NULL, NULL);
 }
-- 
2.23.0.581.g78d2f28ef7-goog

Re: [PATCH] ftrace: fix function type mismatches

[Kees Cook]

On Mon, Oct 07, 2019 at 02:47:40PM -0700, Sami Tolvanen wrote:
> This change fixes indirect call mismatches with function and function
> graph tracing, which trip Control-Flow Integrity (CFI) checking.
> 
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>

Thanks for sending this! We're getting pretty close to having all the
various CFI issues cleaned up now. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  kernel/trace/fgraph.c | 9 ++++++---
>  kernel/trace/ftrace.c | 8 +++++---
>  2 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index 7950a0356042..ecfd4a4a106a 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -327,14 +327,17 @@ void ftrace_graph_sleep_time_control(bool enable)
>  	fgraph_sleep_time = enable;
>  }
>  
> +void ftrace_graph_return_stub(struct ftrace_graph_ret *trace)
> +{
> +}
> +
>  int ftrace_graph_entry_stub(struct ftrace_graph_ent *trace)
>  {
>  	return 0;
>  }
>  
>  /* The callbacks that hook a function */
> -trace_func_graph_ret_t ftrace_graph_return =
> -			(trace_func_graph_ret_t)ftrace_stub;
> +trace_func_graph_ret_t ftrace_graph_return = ftrace_graph_return_stub;
>  trace_func_graph_ent_t ftrace_graph_entry = ftrace_graph_entry_stub;
>  static trace_func_graph_ent_t __ftrace_graph_entry = ftrace_graph_entry_stub;
>  
> @@ -614,7 +617,7 @@ void unregister_ftrace_graph(struct fgraph_ops *gops)
>  		goto out;
>  
>  	ftrace_graph_active--;
> -	ftrace_graph_return = (trace_func_graph_ret_t)ftrace_stub;
> +	ftrace_graph_return = ftrace_graph_return_stub;
>  	ftrace_graph_entry = ftrace_graph_entry_stub;
>  	__ftrace_graph_entry = ftrace_graph_entry_stub;
>  	ftrace_shutdown(&graph_ops, FTRACE_STOP_FUNC_RET);
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 62a50bf399d6..b68ee130d4a2 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -125,8 +125,9 @@ static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
>  				 struct ftrace_ops *op, struct pt_regs *regs);
>  #else
>  /* See comment below, where ftrace_ops_list_func is defined */
> -static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip);
> -#define ftrace_ops_list_func ((ftrace_func_t)ftrace_ops_no_ops)
> +static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip,
> +			      struct ftrace_ops *op, struct pt_regs *regs);
> +#define ftrace_ops_list_func ftrace_ops_no_ops
>  #endif
>  
>  static inline void ftrace_ops_init(struct ftrace_ops *ops)
> @@ -6325,7 +6326,8 @@ static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
>  }
>  NOKPROBE_SYMBOL(ftrace_ops_list_func);
>  #else
> -static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip)
> +static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip,
> +			      struct ftrace_ops *op, struct pt_regs *regs)
>  {
>  	__ftrace_ops_list_func(ip, parent_ip, NULL, NULL);
>  }
> -- 
> 2.23.0.581.g78d2f28ef7-goog
> 

-- 
Kees Cook

Re: [PATCH] ftrace: fix function type mismatches

[Steven Rostedt]

On Thu, 10 Oct 2019 14:12:24 -0700
Kees Cook <keescook@chromium.org> wrote:

> On Mon, Oct 07, 2019 at 02:47:40PM -0700, Sami Tolvanen wrote:
> > This change fixes indirect call mismatches with function and function
> > graph tracing, which trip Control-Flow Integrity (CFI) checking.
> > 
> > Signed-off-by: Sami Tolvanen <samitolvanen@google.com>  
> 
> Thanks for sending this! We're getting pretty close to having all the
> various CFI issues cleaned up now. :)

Unfortunately, this breaks function graph tracing. There's lots of arch
code that tests if the function graph tracer is set to point to the
ftrace_stub. Which by the way is a nop function written in assembly.

Is there a way to do an alias or something that can fix whatever you
are trying to fix?
 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> -Kees
> 
> > ---
> >  kernel/trace/fgraph.c | 9 ++++++---
> >  kernel/trace/ftrace.c | 8 +++++---
> >  2 files changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> > index 7950a0356042..ecfd4a4a106a 100644
> > --- a/kernel/trace/fgraph.c
> > +++ b/kernel/trace/fgraph.c
> > @@ -327,14 +327,17 @@ void ftrace_graph_sleep_time_control(bool enable)
> >  	fgraph_sleep_time = enable;
> >  }
> >  
> > +void ftrace_graph_return_stub(struct ftrace_graph_ret *trace)
> > +{
> > +}
> > +
> >  int ftrace_graph_entry_stub(struct ftrace_graph_ent *trace)
> >  {
> >  	return 0;
> >  }
> >  
> >  /* The callbacks that hook a function */
> > -trace_func_graph_ret_t ftrace_graph_return =
> > -			(trace_func_graph_ret_t)ftrace_stub;
> > +trace_func_graph_ret_t ftrace_graph_return = ftrace_graph_return_stub;
> >  trace_func_graph_ent_t ftrace_graph_entry = ftrace_graph_entry_stub;
> >  static trace_func_graph_ent_t __ftrace_graph_entry = ftrace_graph_entry_stub;
> >  
> > @@ -614,7 +617,7 @@ void unregister_ftrace_graph(struct fgraph_ops *gops)
> >  		goto out;
> >  
> >  	ftrace_graph_active--;
> > -	ftrace_graph_return = (trace_func_graph_ret_t)ftrace_stub;
> > +	ftrace_graph_return = ftrace_graph_return_stub;
> >  	ftrace_graph_entry = ftrace_graph_entry_stub;
> >  	__ftrace_graph_entry = ftrace_graph_entry_stub;
> >  	ftrace_shutdown(&graph_ops, FTRACE_STOP_FUNC_RET);
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 62a50bf399d6..b68ee130d4a2 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -125,8 +125,9 @@ static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
> >  				 struct ftrace_ops *op, struct pt_regs *regs);
> >  #else
> >  /* See comment below, where ftrace_ops_list_func is defined */
> > -static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip);
> > -#define ftrace_ops_list_func ((ftrace_func_t)ftrace_ops_no_ops)
> > +static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip,
> > +			      struct ftrace_ops *op, struct pt_regs *regs);
> > +#define ftrace_ops_list_func ftrace_ops_no_ops
> >  #endif
> >  
> >  static inline void ftrace_ops_init(struct ftrace_ops *ops)
> > @@ -6325,7 +6326,8 @@ static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
> >  }
> >  NOKPROBE_SYMBOL(ftrace_ops_list_func);
> >  #else
> > -static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip)
> > +static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip,
> > +			      struct ftrace_ops *op, struct pt_regs *regs)

Also, this happens to be done because the arch only passes in the first
two parameters. That's why this is called when ARCH_SUPPORTS_FTRACE_OPS
is net set.

The Linux world is not x86 only!

-- Steve


> >  {
> >  	__ftrace_ops_list_func(ip, parent_ip, NULL, NULL);
> >  }
> > -- 
> > 2.23.0.581.g78d2f28ef7-goog
> >   
> 

Re: [PATCH] ftrace: fix function type mismatches

[Sami Tolvanen]

On Fri, Oct 11, 2019 at 11:26 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> Unfortunately, this breaks function graph tracing.

Thank you for pointing this out. I should have realized this solution
wouldn't work.

> Is there a way to do an alias or something that can fix whatever you
> are trying to fix?

Unfortunately, an alias doesn't work in this case either, because the
compiler still looks at the type of the target function. I'll find
another way to solve the issue.

Sami

Re: [PATCH] ftrace: fix function type mismatches

[Steven Rostedt]

On Mon, 14 Oct 2019 13:00:09 -0700
Sami Tolvanen <samitolvanen@google.com> wrote:

> On Fri, Oct 11, 2019 at 11:26 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> > Unfortunately, this breaks function graph tracing.  
> 
> Thank you for pointing this out. I should have realized this solution
> wouldn't work.
> 
> > Is there a way to do an alias or something that can fix whatever you
> > are trying to fix?  
> 
> Unfortunately, an alias doesn't work in this case either, because the
> compiler still looks at the type of the target function. I'll find
> another way to solve the issue.
> 

Would this work for you?

-- Steve

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index dae64600ccbf..33b36b34721b 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -111,15 +111,22 @@
 
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
 #ifdef CC_USING_PATCHABLE_FUNCTION_ENTRY
+/*
+ * Need to also make ftrace_graph_stub point to ftrace_stub
+ * so that the same stub location may have different protocols
+ * and not mess up with C verifiers.
+ */
 #define MCOUNT_REC()	. = ALIGN(8);				\
 			__start_mcount_loc = .;			\
 			KEEP(*(__patchable_function_entries))	\
-			__stop_mcount_loc = .;
+			__stop_mcount_loc = .;			\
+			ftrace_graph_stub = ftrace_stub;
 #else
 #define MCOUNT_REC()	. = ALIGN(8);				\
 			__start_mcount_loc = .;			\
 			KEEP(*(__mcount_loc))			\
-			__stop_mcount_loc = .;
+			__stop_mcount_loc = .;			\
+			ftrace_graph_stub = ftrace_stub;
 #endif
 #else
 #define MCOUNT_REC()
diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
index 7950a0356042..fa3ce10d0405 100644
--- a/kernel/trace/fgraph.c
+++ b/kernel/trace/fgraph.c
@@ -332,9 +332,14 @@ int ftrace_graph_entry_stub(struct ftrace_graph_ent *trace)
 	return 0;
 }
 
+/*
+ * Simply points to ftrace_stub, but with the proper protocol.
+ * Defined by the linker script in linux/vmlinux.lds.h
+ */
+extern void ftrace_graph_stub(struct ftrace_graph_ret *);
+
 /* The callbacks that hook a function */
-trace_func_graph_ret_t ftrace_graph_return =
-			(trace_func_graph_ret_t)ftrace_stub;
+trace_func_graph_ret_t ftrace_graph_return = ftrace_graph_stub;
 trace_func_graph_ent_t ftrace_graph_entry = ftrace_graph_entry_stub;
 static trace_func_graph_ent_t __ftrace_graph_entry = ftrace_graph_entry_stub;
 
@@ -614,7 +619,7 @@ void unregister_ftrace_graph(struct fgraph_ops *gops)
 		goto out;
 
 	ftrace_graph_active--;
-	ftrace_graph_return = (trace_func_graph_ret_t)ftrace_stub;
+	ftrace_graph_return = ftrace_graph_stub;
 	ftrace_graph_entry = ftrace_graph_entry_stub;
 	__ftrace_graph_entry = ftrace_graph_entry_stub;
 	ftrace_shutdown(&graph_ops, FTRACE_STOP_FUNC_RET);

Re: [PATCH] ftrace: fix function type mismatches

[Steven Rostedt]

On Tue, 15 Oct 2019 09:00:55 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -111,15 +111,22 @@
>  
>  #ifdef CONFIG_FTRACE_MCOUNT_RECORD
>  #ifdef CC_USING_PATCHABLE_FUNCTION_ENTRY
> +/*
> + * Need to also make ftrace_graph_stub point to ftrace_stub
> + * so that the same stub location may have different protocols
> + * and not mess up with C verifiers.
> + */


> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index 7950a0356042..fa3ce10d0405 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -332,9 +332,14 @@ int ftrace_graph_entry_stub(struct ftrace_graph_ent *trace)
>  	return 0;
>  }
>  
> +/*
> + * Simply points to ftrace_stub, but with the proper protocol.
> + * Defined by the linker script in linux/vmlinux.lds.h
> + */
> +extern void ftrace_graph_stub(struct ftrace_graph_ret *);
> +


 s/protocol/prototype/g

Will update.

-- Steve

Re: [PATCH] ftrace: fix function type mismatches

[Sami Tolvanen]

On Tue, Oct 15, 2019 at 6:01 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> Would this work for you?

>  #define MCOUNT_REC()   . = ALIGN(8);                           \
>                         __start_mcount_loc = .;                 \
>                         KEEP(*(__patchable_function_entries))   \
> -                       __stop_mcount_loc = .;
> +                       __stop_mcount_loc = .;                  \
> +                       ftrace_graph_stub = ftrace_stub;

> +extern void ftrace_graph_stub(struct ftrace_graph_ret *);

Yes, it looks like the compiler is actually happy with this approach.
Thank you for the patch!

Sami