| Message ID | 20200416161245.148813-5-samitolvanen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add support for Clang's Shadow Call Stack | expand |
On Thu, Apr 16, 2020 at 09:12:37AM -0700, Sami Tolvanen wrote: > The graph tracer hooks returns by modifying frame records on the > (regular) stack, but with SCS the return address is taken from the > shadow stack, and the value in the frame record has no effect. As we > don't currently have a mechanism to determine the corresponding slot > on the shadow stack (and to pass this through the ftrace > infrastructure), for now let's disable SCS when the graph tracer is > enabled. > > With SCS the return address is taken from the shadow stack and the > value in the frame record has no effect. The mcount based graph tracer > hooks returns by modifying frame records on the (regular) stack, and > thus is not compatible. The patchable-function-entry graph tracer > used for DYNAMIC_FTRACE_WITH_REGS modifies the LR before it is saved > to the shadow stack, and is compatible. > > Modifying the mcount based graph tracer to work with SCS would require > a mechanism to determine the corresponding slot on the shadow stack > (and to pass this through the ftrace infrastructure), and we expect > that everyone will eventually move to the patchable-function-entry > based graph tracer anyway, so for now let's disable SCS when the > mcount-based graph tracer is enabled. > > SCS and patchable-function-entry are both supported from LLVM 10.x. SCS would actually provide another way to do return hooking. An arguably much saner model at that. The 'normal' way is to (temporary) replace the on-stack return value, and then replace it back in the return handler. This is because we can't simply push a fake return on the stack, because that would wreck the expected stack layout of the regular function. But there is nothing that would stop us from pushing an extra entry on the SCS. It would in fact be a much cleaner solution. The entry hook sticks an extra entry on the SCS, the function ignores what's on the normal stack and pops from the SCS, we return to the exit handler, which in turn pops from the SCS stack at which point we're back to regular. The only 'funny' is that the exit handler itself should not push to the SCS, or we should frob the return-to-exit-handler such that it lands after the push. > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > Reviewed-by: Kees Cook <keescook@chromium.org> > Reviewed-by: Mark Rutland <mark.rutland@arm.com> > --- > arch/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/Kconfig b/arch/Kconfig > index 691a552c2cc3..c53cb9025ad2 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -542,6 +542,7 @@ config ARCH_SUPPORTS_SHADOW_CALL_STACK > > config SHADOW_CALL_STACK > bool "Clang Shadow Call Stack" > + depends on DYNAMIC_FTRACE_WITH_REGS || !FUNCTION_GRAPH_TRACER > depends on ARCH_SUPPORTS_SHADOW_CALL_STACK > help > This option enables Clang's Shadow Call Stack, which uses a AFAICT you also need to kill KRETPROBES, which plays similar games. And doesn't BPF also do stuff like this?
Hi Peter, On Fri, Apr 17, 2020 at 12:00:39PM +0200, Peter Zijlstra wrote: > On Thu, Apr 16, 2020 at 09:12:37AM -0700, Sami Tolvanen wrote: > > The graph tracer hooks returns by modifying frame records on the > > (regular) stack, but with SCS the return address is taken from the > > shadow stack, and the value in the frame record has no effect. As we > > don't currently have a mechanism to determine the corresponding slot > > on the shadow stack (and to pass this through the ftrace > > infrastructure), for now let's disable SCS when the graph tracer is > > enabled. > > > > With SCS the return address is taken from the shadow stack and the > > value in the frame record has no effect. The mcount based graph tracer > > hooks returns by modifying frame records on the (regular) stack, and > > thus is not compatible. The patchable-function-entry graph tracer > > used for DYNAMIC_FTRACE_WITH_REGS modifies the LR before it is saved > > to the shadow stack, and is compatible. > > > > Modifying the mcount based graph tracer to work with SCS would require > > a mechanism to determine the corresponding slot on the shadow stack > > (and to pass this through the ftrace infrastructure), and we expect > > that everyone will eventually move to the patchable-function-entry > > based graph tracer anyway, so for now let's disable SCS when the > > mcount-based graph tracer is enabled. > > > > SCS and patchable-function-entry are both supported from LLVM 10.x. > > SCS would actually provide another way to do return hooking. An arguably > much saner model at that. > > The 'normal' way is to (temporary) replace the on-stack return value, > and then replace it back in the return handler. This is because we can't > simply push a fake return on the stack, because that would wreck the > expected stack layout of the regular function. > > But there is nothing that would stop us from pushing an extra entry on > the SCS. It would in fact be a much cleaner solution. The entry hook > sticks an extra entry on the SCS, the function ignores what's on the > normal stack and pops from the SCS, we return to the exit handler, which > in turn pops from the SCS stack at which point we're back to regular. For background: on arm64 we wanted to use DYNAMIC_FTRACE_WITH_REGS since we already have to use that to handle pointer authentication, and didn't want to gain more ways of implementing ftrace. Arguably we should move the dependency into the arm64 Kconfig for ARCH_SUPPORTS_SHADOW_CALL_STACK. > The only 'funny' is that the exit handler itself should not push to the > SCS, or we should frob the return-to-exit-handler such that it lands > after the push. > > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > > Reviewed-by: Kees Cook <keescook@chromium.org> > > Reviewed-by: Mark Rutland <mark.rutland@arm.com> > > --- > > arch/Kconfig | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/Kconfig b/arch/Kconfig > > index 691a552c2cc3..c53cb9025ad2 100644 > > --- a/arch/Kconfig > > +++ b/arch/Kconfig > > @@ -542,6 +542,7 @@ config ARCH_SUPPORTS_SHADOW_CALL_STACK > > > > config SHADOW_CALL_STACK > > bool "Clang Shadow Call Stack" > > + depends on DYNAMIC_FTRACE_WITH_REGS || !FUNCTION_GRAPH_TRACER > > depends on ARCH_SUPPORTS_SHADOW_CALL_STACK > > help > > This option enables Clang's Shadow Call Stack, which uses a > AFAICT you also need to kill KRETPROBES, which plays similar games. Hmm... how does KREPROBES work? If you can only mess with the return address when probing the first instruction in the function, it'll just work for SCS or pointer authentication, as the LR is used at that instant. If KRETPROBES tries to mess with the return address elsewhere it'd be broken today... > And doesn't BPF also do stuff like this? Can BPF mess with return addresses now!? Thanks, Mark.
On Fri, Apr 17, 2020 at 03:46:21PM +0100, Mark Rutland wrote: > > > diff --git a/arch/Kconfig b/arch/Kconfig > > > index 691a552c2cc3..c53cb9025ad2 100644 > > > --- a/arch/Kconfig > > > +++ b/arch/Kconfig > > > @@ -542,6 +542,7 @@ config ARCH_SUPPORTS_SHADOW_CALL_STACK > > > > > > config SHADOW_CALL_STACK > > > bool "Clang Shadow Call Stack" > > > + depends on DYNAMIC_FTRACE_WITH_REGS || !FUNCTION_GRAPH_TRACER > > > depends on ARCH_SUPPORTS_SHADOW_CALL_STACK > > > help > > > This option enables Clang's Shadow Call Stack, which uses a > > > AFAICT you also need to kill KRETPROBES, which plays similar games. > > Hmm... how does KREPROBES work? If you can only mess with the return > address when probing the first instruction in the function, it'll just > work for SCS or pointer authentication, as the LR is used at that > instant. If KRETPROBES tries to mess with the return address elsewhere > it'd be broken today... To be fair, I've not looked at the arm64 implementation. x86 does gross things like ftrace does. On x86 ftrace_graph and kretprobe also can't be on at the same time for the same function, there's some yuck around there. Rostedt was recently talking about cleaning some of that up. But if kretprobe can work on arm64, then ftrace_graph can too, but I think that links back to what you said earlier, you didn't want more ftrace variants or something. > > And doesn't BPF also do stuff like this? > > Can BPF mess with return addresses now!? At least on x86 I think it does. But what do I know, I can't operate that stuff. Rostedt might know.
On Fri, Apr 17, 2020 at 05:26:45PM +0200, Peter Zijlstra wrote: > On Fri, Apr 17, 2020 at 03:46:21PM +0100, Mark Rutland wrote: > > > > diff --git a/arch/Kconfig b/arch/Kconfig > > > > index 691a552c2cc3..c53cb9025ad2 100644 > > > > --- a/arch/Kconfig > > > > +++ b/arch/Kconfig > > > > @@ -542,6 +542,7 @@ config ARCH_SUPPORTS_SHADOW_CALL_STACK > > > > > > > > config SHADOW_CALL_STACK > > > > bool "Clang Shadow Call Stack" > > > > + depends on DYNAMIC_FTRACE_WITH_REGS || !FUNCTION_GRAPH_TRACER > > > > depends on ARCH_SUPPORTS_SHADOW_CALL_STACK > > > > help > > > > This option enables Clang's Shadow Call Stack, which uses a > > > > > AFAICT you also need to kill KRETPROBES, which plays similar games. > > > > Hmm... how does KREPROBES work? If you can only mess with the return > > address when probing the first instruction in the function, it'll just > > work for SCS or pointer authentication, as the LR is used at that > > instant. If KRETPROBES tries to mess with the return address elsewhere > > it'd be broken today... > > To be fair, I've not looked at the arm64 implementation. x86 does gross > things like ftrace does. On x86 ftrace_graph and kretprobe also can't > be on at the same time for the same function, there's some yuck around > there. I can imagine the same holds true for us there. > Rostedt was recently talking about cleaning some of that up. > > But if kretprobe can work on arm64, then ftrace_graph can too, but I > think that links back to what you said earlier, you didn't want more > ftrace variants or something. I just want to avoid yet another implementation of the underlying mechanism. For DYNAMIC_FTRACE_WITH_REGS we can mess with the LR before pauth or SCS sees it, so those definitely work. If KRETPROBES works by messing with the LR at the instnat the function is entered, that should work similarly. If it works by replacing the RET it should also work out since any pauth/SCS work will have been undone by that point. If it attempts to mess with the return address in the middle of a function then it's not reliable today. I'll take a look, since > > > And doesn't BPF also do stuff like this? > > > > Can BPF mess with return addresses now!? > > At least on x86 I think it does. But what do I know, I can't operate > that stuff. Rostedt might know. Sounds like I might need to do some digging... Mark.
On Fri, Apr 17, 2020 at 04:46:14PM +0100, Mark Rutland wrote: > If KRETPROBES works by messing with the LR at the instnat the function > is entered, that should work similarly. If it works by replacing the > RET it should also work out since any pauth/SCS work will have been > undone by that point. If it attempts to mess with the return address in > the middle of a function then it's not reliable today. I did initially have a patch to disable kretprobes (until v5), but as Mark pointed out back then, the return address is modified before it gets pushed to the shadow stack, so there was no conflict with SCS. I confirmed this on arm64, but haven't looked at other architectures. Sami
On Fri, 17 Apr 2020 16:46:14 +0100 Mark Rutland <mark.rutland@arm.com> wrote: > > > > And doesn't BPF also do stuff like this? > > > > > > Can BPF mess with return addresses now!? > > > > At least on x86 I think it does. But what do I know, I can't operate > > that stuff. Rostedt might know. > > Sounds like I might need to do some digging... May want to ping Alexei. It appears that if BPF adds a direct hook to a function, it will prevent the function graph tracer from tracing it. :-/ -- Steve
diff --git a/arch/Kconfig b/arch/Kconfig index 691a552c2cc3..c53cb9025ad2 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -542,6 +542,7 @@ config ARCH_SUPPORTS_SHADOW_CALL_STACK config SHADOW_CALL_STACK bool "Clang Shadow Call Stack" + depends on DYNAMIC_FTRACE_WITH_REGS || !FUNCTION_GRAPH_TRACER depends on ARCH_SUPPORTS_SHADOW_CALL_STACK help This option enables Clang's Shadow Call Stack, which uses a