Message-ID: <20200726155148.GA9341@ubuntu>
Date:   Sun, 26 Jul 2020 17:52:42 +0200
From:   Oscar Carter <oscar.carter@....com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Oscar Carter <oscar.carter@....com>,
        Ingo Molnar <mingo@...hat.com>,
        Kees Cook <keescook@...omium.org>,
        linux-kernel@...r.kernel.org, kernel-hardening@...ts.openwall.com,
        Jann Horn <jannh@...gle.com>
Subject: Re: [PATCH v2 2/2] kernel/trace: Remove function callback casts

Hi Steven,

On Sat, Jul 25, 2020 at 05:09:14PM +0200, Oscar Carter wrote:
> Hi Steven,
>
> On Fri, Jul 24, 2020 at 02:34:57PM -0400, Steven Rostedt wrote:
> > On Fri, 24 Jul 2020 19:55:00 +0200
> > Oscar Carter <oscar.carter@....com> wrote:
> >
> > > > Which one of the above is this patch set for?
> > >
> > > This patch is the result of a warning obtained with the following:
> > >
> > > make allmodconfig ARCH=powerpc
> > > make ARCH=powerpc CROSS_COMPILE=powerpc-linux-gnu- -j4
> > >
> > > And with the -Wcast-function-type enabled in the top level makefile.
> >
> > Looking into powerpc I found this:
> >
> > arch/powerpc/include/asm/ftrace.h:
> >
> >   #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> >   #define ARCH_SUPPORTS_FTRACE_OPS 1
> >   #endif
> >
> >
> > arch/powerpc/Kconfig:
> >
> > 	select HAVE_DYNAMIC_FTRACE_WITH_REGS	if MPROFILE_KERNEL
> > [..]
> >
> >   config MPROFILE_KERNEL
> > 	depends on PPC64 && CPU_LITTLE_ENDIAN && FUNCTION_TRACER
> > 	def_bool $(success,$(srctree)/arch/powerpc/tools/gcc-check-mprofile-kernel.sh $(CC) -I$(srctree)/include -D__KERNEL__)
> >
> > So, it looks like you need to be 64bit PowerPC, Little Endian, and gcc
> > needs to support -mprofile.
> >
> > Otherwise, it falls back to the old way that does the type casting.
> >
> > If you are really concerned about this, I would recommend adding
> > support to the architecture you care about, and then this will no
> > longer be an issue.
> >
> > The funny part is, you can still add support for ftrace_ops, without
> > adding support for DYNAMIC_FTRACE_WITH_REGS, if you only care about not
> > having to do that typecast.
>
> I agree with you. I will try to add the support for ftrace_ops without
> adding support for DYNAMIC_FTRACE_WITH_REGS to the powerpc architecture.
>
> It's a great solution to allow a clean CFI build and so, protect this
> arch (powerpc) against attacks that try to modify the normal control
> flow.
>
> I take a look at the kernel documentation about port ftrace to other
> architectures [1] but it is out of date.
>
> If I try to do this I will need some help. Some info that point me to the
> right direction would be greatly appreciated. Some advice about what
> functions I will need to implement would be really helpfull. Or point me
> to the right piece of code that I can pick as base point.

I've been searching and reading the code as much as possible. I've found
two patches that I think can be useful to guide me. One [1] adds support
for ftrace_ops to the riscv architecture. The other one [2] adds support
for ftrace_ops to the parisc architecture.

[1] commit 71e736a7d655 ("riscv/ftrace: Add ARCH_SUPPORTS_FTRACE_OPS support")
[2] commit d562aca37a54 ("parisc/ftrace: Add ARCH_SUPPORTS_FTRACE_OPS support")

Due to powerpc arch calls the needed functions from assembly, I based my
idea on the patch for the RISCV arch.

Can something like the following work?

diff --git a/arch/powerpc/include/asm/ftrace.h b/arch/powerpc/include/asm/ftrace.h
index bc76970b6ee5..1c51ff5afae1 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -61,9 +61,8 @@ struct dyn_arch_ftrace {
 };
 #endif /* __ASSEMBLY__ */

-#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
 #define ARCH_SUPPORTS_FTRACE_OPS 1
-#endif
+
 #endif /* CONFIG_FUNCTION_TRACER */

 #ifndef __ASSEMBLY__
diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
index e023ae59c429..e69a4e945986 100644
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ b/arch/powerpc/kernel/trace/ftrace_32.S
@@ -29,6 +29,10 @@ _GLOBAL(ftrace_caller)
        MCOUNT_SAVE_FRAME
        /* r3 ends up with link register */
        subi    r3, r3, MCOUNT_INSN_SIZE
+
+       /* Set ftrace_ops (r5) to the global variable function_trace_op */
+       /* Set pt_regs (r6) to NULL */
+
 .globl ftrace_call
 ftrace_call:
        bl      ftrace_stub
diff --git a/arch/powerpc/kernel/trace/ftrace_64_pg.S b/arch/powerpc/kernel/trace/ftrace_64_pg.S
index 6708e24db0ab..a741448b1df9 100644
--- a/arch/powerpc/kernel/trace/ftrace_64_pg.S
+++ b/arch/powerpc/kernel/trace/ftrace_64_pg.S
@@ -22,6 +22,10 @@ _GLOBAL_TOC(ftrace_caller)
        std     r3, 128(r1)
        ld      r4, 16(r11)
        subi    r3, r3, MCOUNT_INSN_SIZE
+
+       /* Set ftrace_ops (r5) to the global variable function_trace_op */
+       /* Set pt_regs (r6) to NULL */
+
 .globl ftrace_call
 ftrace_call:
        bl      ftrace_stub

To add support for ftrace_ops to the powerpc architecture is only necessary
to fill the r5 and r6 registers before the call to ftrace_stub in all the
cases. The register r5 is a pointer to ftrace_ops struct and the register
r6 is a pointer to pt_regs struct. These registers are the third and fourth
parameters of a function with the following prototype. The first and second
ones are yet set.

void func(unsigned long ip, unsigned long parent_ip,
	  struct ftrace_ops *ops, struct pt_regs *regs);

Am I in the right direction? or am I totally wrong?

Thanks for your time and patience.
Oscar Carter.

> > My NAK still stands. I wont let an intrusive patch be added to the
> > ftrace core code to deal with an unsupported feature in an architecture.
>
> Don't worry. I agree with you and thanks for finding a better way to
> accomplish the initial purpose.
>
> > I would be will to add that linker trick to remove the warning. Or we
> > just use that warning as incentive to get architecture developers to
> > implement this feature ;-)
>
> In my opinion it would be better to leave the code as it an make the warning
> visible until this feature has been added.
>
> > -- Steve
>
> [1] https://www.kernel.org/doc/html/latest/trace/ftrace-design.html
>
> Thanks again,
> Oscar Carter

Powered by blists - more mailing lists