Re: [PATCH v6 13/18] arm64: unwind: strip PAC from kernel addresses

[James Morse <james.morse@arm.com>]

Hi Amit,

On 06/03/2020 06:35, Amit Daniel Kachhap wrote:
> From: Mark Rutland <mark.rutland@arm.com>
> 
> When we enable pointer authentication in the kernel, LR values saved to
> the stack will have a PAC which we must strip in order to retrieve the
> real return address.
> 
> Strip PACs when unwinding the stack in order to account for this.

This patch had me looking at the wider pointer-auth + ftrace interaction...


> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index a336cb1..b479df7 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -14,6 +14,7 @@
>  #include <linux/stacktrace.h>
>  
>  #include <asm/irq.h>
> +#include <asm/pointer_auth.h>
>  #include <asm/stack_pointer.h>
>  #include <asm/stacktrace.h>
>  
> @@ -101,6 +102,8 @@ int notrace unwind_frame(struct task_struct *tsk, struct stackframe *frame)

There is an earlier reader of frame->pc:
| #ifdef CONFIG_FUNCTION_GRAPH_TRACER
| 	if (tsk->ret_stack &&
| 			(frame->pc == (unsigned long)return_to_handler)) {


Which leads down the rat-hole of: does this need ptrauth_strip_insn_pac()?

The version of GCC on my desktop supports patchable-function-entry, the function pre-amble
has two nops for use by ftrace[0]. This means if prepare_ftrace_return() re-writes the
saved LR, it does it before the caller paciasp's it.

I think that means if you stack-trace from a function that had been hooked by the
function_graph_tracer, you will see the LR with a PAC, meaning the above == won't match.


The version of LLVM on my desktop however doesn't support patchable-function-entry, it
uses _mcount() to do the ftrace stuff[1]. Here prepare_ftrace_return() overwrites a
paciasp'd LR with one that isn't, which will fail.


Could the ptrauth_strip_insn_pac() call move above the CONFIG_FUNCTION_GRAPH_TRACER block,
and could we add something like:
|	depends on (!FTRACE || HAVE_DYNAMIC_FTRACE_WITH_REGS)

to the Kconfig to prevent both FTRACE and PTR_AUTH being enabled unless the compiler has
support for patchable-function-entry?


>  	}
>  #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
>  
> +	frame->pc = ptrauth_strip_insn_pac(frame->pc);
> +
>  	/*
>  	 * Frames created upon entry from EL0 have NULL FP and PC values, so
>  	 * don't bother reporting these. Frames created by __noreturn functions


Thanks,

James

[0] gcc (Debian 9.2.1-28) 9.2.1 20200203
0000000000000048 <sync_icache_aliases>:
  48:   d503201f        nop
  4c:   d503201f        nop
  50:   90000002        adrp    x2, 0 <__icache_flags>
  54:   d503233f        paciasp
  58:   a9bf7bfd        stp     x29, x30, [sp, #-16]!
  5c:   910003fd        mov     x29, sp
  60:   f9400044        ldr     x4, [x2]
  64:   36000124        tbz     w4, #0, 88 <sync_icache_al


[1] clang version 9.0.0-1 (tags/RELEASE_900/final)
0000000000000000 <sync_icache_aliases>:
   0:   d503233f        paciasp
   4:   a9be4ff4        stp     x20, x19, [sp, #-32]!
   8:   a9017bfd        stp     x29, x30, [sp, #16]
   c:   910043fd        add     x29, sp, #0x10
  10:   aa0103f4        mov     x20, x1
  14:   aa0003f3        mov     x19, x0
  18:   94000000        bl      0 <_mcount>
  1c:   90000008        adrp    x8, 0 <__icache_flags>
  20:   f9400108        ldr     x8, [x8]
  24:   370000a8        tbnz    w8, #0, 38 <sync_icache_aliases+0x38>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel