| Message ID | 1583476525-13505-14-git-send-email-amit.kachhap@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: return address signing | expand |
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>
Hi James, On 3/10/20 12:33 AM, James Morse wrote: > 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... Thanks for your effort. > > >> 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, This may not be required as we never explicitly sign return_to_handler and frame->pc may store it without any PAC signature for patchable-function-entry. While testing patchable-function-entry, I had an observation regarding WARN messages, [ 541.030265] Hardware name: Foundation-v8A (DT) [ 541.033500] pstate: 60400009 (nZCv daif +PAN -UAO) [ 541.036880] pc : change_pac_parameters+0x40/0x4c [ 541.040279] lr : return_to_handler+0x0/0x3c [ 541.043373] sp : ffff8000126e3d00 Here lr may need some logic to display correct return address although it is unrelated to this ptrauth series. (arch/arm64/kernel/process.c +264) > 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? Yes this is a good condition to have. I feel below condition is more suitable as there is issue only with FUNCTION_GRAPH_TRACER, depends on (!FUNCTION_GRAPH_TRACER || DYNAMIC_FTRACE_WITH_REGS) Thanks, Amit Daniel > > >> } >> #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> >
Hi Amit, On 10/03/2020 12:28, Amit Kachhap wrote: > On 3/10/20 12:33 AM, James Morse wrote: >> 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, > This may not be required as we never explicitly sign return_to_handler Doesn't the original caller sign it? (I agree that assembly is tricky to work out) ftrace_graph_caller passes 'parent' to prepare_ftrace_return() as the LR in regs: | add x1, sp, #S_LR prepare_ftrace_return() may overwrite it with an unsigned value. ftrace_common_return restores this location to x30: | ldr x30, [sp, #S_LR] Then returns to the first real instruction of the original caller: paciasp. (when navigating that assembly, there are two stack frames, each with an LR, and one LR in the regs...) > and frame->pc may > store it without any PAC signature for patchable-function-entry. How does return_to_handler() run? Surely when the original caller pulls the LR off the stack, it runs: | autiasp | ret Wouldn't autiasp transform an unsigned return_to_handler() to be a bogus address? I agree the 'unsigned' case does happen if you're using _mcount(), this will be caught by autiasp, hence we need to depend on HAVE_DYNAMIC_FTRACE_WITH_REGS. > While testing patchable-function-entry, I had an observation regarding WARN messages, > > [ 541.030265] Hardware name: Foundation-v8A (DT) > [ 541.033500] pstate: 60400009 (nZCv daif +PAN -UAO) > [ 541.036880] pc : change_pac_parameters+0x40/0x4c > [ 541.040279] lr : return_to_handler+0x0/0x3c > [ 541.043373] sp : ffff8000126e3d00 (a WARN()ing?, where?! Ah, you mean triggered deliberately to check they look right...) > Here lr may need some logic to display correct return address although it is unrelated to > this ptrauth series. (arch/arm64/kernel/process.c +264) Yes, this happens when a function that has been hooked by ftrace, hits a WARN_ON(), show_regs() will report the real LR. I don't think that's a problem, its helpful to know that ftrace has hooked this call. Presumably return_to_handler() doesn't appear in the call-trace? (that would be a problem) >> 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? > > Yes this is a good condition to have. I feel below condition is more suitable as there is > issue only with FUNCTION_GRAPH_TRACER, Er, yes! Because its callers of prepare_ftrace_return() that have the problem, and that is behind #ifdef FUNCTION_GRAPH_TRACER. Thanks, James
Hi, On 3/10/20 11:07 PM, James Morse wrote: > Hi Amit, > > On 10/03/2020 12:28, Amit Kachhap wrote: >> On 3/10/20 12:33 AM, James Morse wrote: >>> 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, > >> This may not be required as we never explicitly sign return_to_handler > > Doesn't the original caller sign it? (I agree that assembly is tricky to work out) > > ftrace_graph_caller passes 'parent' to prepare_ftrace_return() as the LR in regs: > | add x1, sp, #S_LR > > prepare_ftrace_return() may overwrite it with an unsigned value. > > ftrace_common_return restores this location to x30: > | ldr x30, [sp, #S_LR] > > Then returns to the first real instruction of the original caller: paciasp. > > (when navigating that assembly, there are two stack frames, each with an LR, and one LR in > the regs...) > > >> and frame->pc may >> store it without any PAC signature for patchable-function-entry. > > How does return_to_handler() run? Surely when the original caller pulls the LR off the > stack, it runs: > | autiasp > | ret I used dump_stack() instead of WARN_ON and able to reproduce the issue. Yes ptrauth_strip_insn_pac needs to move up to fix this. Thanks for the details. > > Wouldn't autiasp transform an unsigned return_to_handler() to be a bogus address? > > I agree the 'unsigned' case does happen if you're using _mcount(), this will be caught by > autiasp, hence we need to depend on HAVE_DYNAMIC_FTRACE_WITH_REGS. > > >> While testing patchable-function-entry, I had an observation regarding WARN messages, >> >> [ 541.030265] Hardware name: Foundation-v8A (DT) >> [ 541.033500] pstate: 60400009 (nZCv daif +PAN -UAO) >> [ 541.036880] pc : change_pac_parameters+0x40/0x4c >> [ 541.040279] lr : return_to_handler+0x0/0x3c >> [ 541.043373] sp : ffff8000126e3d00 > > (a WARN()ing?, where?! Ah, you mean triggered deliberately to check they look right...) > > >> Here lr may need some logic to display correct return address although it is unrelated to >> this ptrauth series. (arch/arm64/kernel/process.c +264) > > Yes, this happens when a function that has been hooked by ftrace, hits a WARN_ON(), > show_regs() will report the real LR. I don't think that's a problem, its helpful to know > that ftrace has hooked this call. ok > > Presumably return_to_handler() doesn't appear in the call-trace? (that would be a problem) No it doesn't appear. > > >>> 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? >> >> Yes this is a good condition to have. I feel below condition is more suitable as there is >> issue only with FUNCTION_GRAPH_TRACER, > > Er, yes! > Because its callers of prepare_ftrace_return() that have the problem, and that is behind > #ifdef FUNCTION_GRAPH_TRACER. ok Cheers, Amit > > > Thanks, > > James >
Hi Amit, On 3/11/20 6:07 AM, Amit Kachhap wrote: > On 3/10/20 11:07 PM, James Morse wrote: >> On 10/03/2020 12:28, Amit Kachhap wrote: >>> On 3/10/20 12:33 AM, James Morse wrote: >>>> 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)) { >>>> >>>> Could the ptrauth_strip_insn_pac() call move above the >>>> CONFIG_FUNCTION_GRAPH_TRACER block, >> >>> This may not be required as we never explicitly sign return_to_handler >> >> Doesn't the original caller sign it? (I agree that assembly is tricky >> to work out) > I used dump_stack() instead of WARN_ON and able to reproduce the issue. > Yes ptrauth_strip_insn_pac needs to move up to fix this. Thanks for the > details. Great! >>>> 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? >>> >>> Yes this is a good condition to have. I feel below condition is more >>> suitable as there is >>> issue only with FUNCTION_GRAPH_TRACER, >> >> Er, yes! >> Because its callers of prepare_ftrace_return() that have the problem, >> and that is behind >> #ifdef FUNCTION_GRAPH_TRACER. With the ptrauth_strip_insn_pac() moved, and your better version of that Kconfig suggestion: Reviewed-by: James Morse <james.morse@arm.com> Thanks! James
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) } #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