Message-ID: <CAG48ez04Fj=1p61KAxAQWZ3f_z073fVUr8LsQgtKA9c-kcHmDQ@mail.gmail.com>
Date:   Thu, 18 Jun 2020 01:12:37 +0200
From:   Jann Horn <jannh@...gle.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Kernel Hardening <kernel-hardening@...ts.openwall.com>,
        Oscar Carter <oscar.carter@....com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] tracing: Use linker magic instead of recasting ftrace_ops_list_func()

On Thu, Jun 18, 2020 at 12:36 AM Steven Rostedt <rostedt@...dmis.org> wrote:
> On Wed, 17 Jun 2020 23:30:07 +0200
> Jann Horn <jannh@...gle.com> wrote:
> > [...]
> > > +/* Defined by vmlinux.lds.h see the commment above arch_ftrace_ops_list_func for details */
> > > +void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
> > > +                         struct ftrace_ops *op, struct pt_regs *regs);
> > [...]
> > > +void arch_ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip)
> > >  {
> >
> > Well, it's not like the function cast itself is the part that's
> > problematic for CFI; the problematic part is when you actually make a
> > C function call (in particular an indirect one) where the destination
> > is compiled with a prototype that is different from the prototype used
> > at the call site. Doing this linker hackery isn't really any better
> > than shutting up the compiler warning by piling on enough casts or
> > whatever. (There should be some combination of casts that'll shut up
> > this warning, right?)
>
> It's not called by C, it's called by assembly.

Except on nds32 and parisc, right?

> > IIUC the real issue here is that ftrace_func_t is defined as a fixed
> > type, but actually has different types depending on the architecture?
> > If so, it might be cleaner to define ftrace_func_t differently
> > depending on architecture, or something like that?
>
> There's functions that use this type.
>
> When you register a function to be used by the function tracer (that
> will have 4 parameters). If the arch supports it, it will call it
> directly from the trampoline in assembly, but if it does not, then the
> C code will only let assembly call the two parameter version, that will
> call the 4 parameter function (adding NULLs to the extra two arguments).

So essentially there are two types, right? One type for the
registration API, one type for the assembly API? On some
architectures, the types are the same (and assembly calls directly
into the function); on other architectures, the types are not the
same, and assembly calls the NULL-adding helper (with the
assembly-API-type), and then the helper calls the function with the
registration-API-type?

Would it not be possible to have two function types (#define'd as the
same if ARCH_SUPPORTS_FTRACE_OPS), and then ensure that ftrace_func_t
is only used as ftrace_asm_func_t if ARCH_SUPPORTS_FTRACE_OPS?
Something like this (entirely untested)?

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index e339dac91ee62..b08fa393c1fad 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -89,6 +89,11 @@ struct ftrace_ops;

 typedef void (*ftrace_func_t)(unsigned long ip, unsigned long parent_ip,
                              struct ftrace_ops *op, struct pt_regs *regs);
+#if ARCH_SUPPORTS_FTRACE_OPS
+#define ftrace_asm_func_t ftrace_func_t
+#else
+typedef void (*ftrace_asm_func_t)(unsigned long ip, unsigned long parent_ip);
+#endif

 ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops);

@@ -254,8 +259,12 @@ extern enum ftrace_tracing_type_t ftrace_tracing_type;
 int register_ftrace_function(struct ftrace_ops *ops);
 int unregister_ftrace_function(struct ftrace_ops *ops);

+#if ARCH_SUPPORTS_FTRACE_OPS
 extern void ftrace_stub(unsigned long a0, unsigned long a1,
                        struct ftrace_ops *op, struct pt_regs *regs);
+#else
+extern void ftrace_stub(unsigned long a0, unsigned long a1);
+#endif

 #else /* !CONFIG_FUNCTION_TRACER */
 /*
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index c163c3531fafc..abd076cdd84d9 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -116,7 +116,7 @@ static int ftrace_disabled __read_mostly;
 DEFINE_MUTEX(ftrace_lock);

 struct ftrace_ops __rcu *ftrace_ops_list __read_mostly = &ftrace_list_end;
-ftrace_func_t ftrace_trace_function __read_mostly = ftrace_stub;
+ftrace_asm_func_t ftrace_trace_function __read_mostly = ftrace_stub;
 struct ftrace_ops global_ops;

 #if ARCH_SUPPORTS_FTRACE_OPS
@@ -125,7 +125,7 @@ static void ftrace_ops_list_func(unsigned long ip,
unsigned long parent_ip,
 #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)
+#define ftrace_ops_list_func ftrace_ops_no_ops
 #endif

 static inline void ftrace_ops_init(struct ftrace_ops *ops)
@@ -166,22 +166,25 @@ static void ftrace_sync_ipi(void *data)
        smp_rmb();
 }

-static ftrace_func_t ftrace_ops_get_list_func(struct ftrace_ops *ops)
+static ftrace_asm_func_t ftrace_ops_get_list_func(struct ftrace_ops *ops)
 {
+#if FTRACE_FORCE_LIST_FUNC
+       return ftrace_ops_list_func;
+#else
        /*
         * If this is a dynamic, RCU, or per CPU ops, or we force list func,
         * then it needs to call the list anyway.
         */
-       if (ops->flags & (FTRACE_OPS_FL_DYNAMIC | FTRACE_OPS_FL_RCU) ||
-           FTRACE_FORCE_LIST_FUNC)
+       if (ops->flags & (FTRACE_OPS_FL_DYNAMIC | FTRACE_OPS_FL_RCU))
                return ftrace_ops_list_func;

        return ftrace_ops_get_func(ops);
+#endif
 }

 static void update_ftrace_function(void)
 {
-       ftrace_func_t func;
+       ftrace_asm_func_t func;

        /*
         * Prepare the ftrace_ops that the arch callback will use.

Powered by blists - more mailing lists