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@xxxxxxxxxx>

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

Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>

-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