| Message ID | 20191101221150.116536-12-samitolvanen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,01/17] arm64: mm: avoid x18 in idmap_kpti_install_ng_mappings | expand |
On Fri, Nov 01, 2019 at 03:11:44PM -0700, Sami Tolvanen wrote: > With CONFIG_FUNCTION_GRAPH_TRACER, function return addresses are > modified in ftrace_graph_caller and prepare_ftrace_return to redirect > control flow to ftrace_return_to_handler. This is incompatible with > SCS. Can you please elaborate on _how_ this is incompatible in the commit message? For example, it's not clear to me if you mean that's functionally incompatible, or if you're trying to remove return-altering gadgets. If there's a functional incompatibility, please spell that out a bit more clearly. Likewise if this is about minimizing the set of places that can mess with control-flow outside of usual function conventions. Thanks, Mark. > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > Reviewed-by: Kees Cook <keescook@chromium.org> > --- > arch/arm64/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index e7b57a8a5531..42867174920f 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -148,7 +148,7 @@ config ARM64 > select HAVE_FTRACE_MCOUNT_RECORD > select HAVE_FUNCTION_TRACER > select HAVE_FUNCTION_ERROR_INJECTION > - select HAVE_FUNCTION_GRAPH_TRACER > + select HAVE_FUNCTION_GRAPH_TRACER if !SHADOW_CALL_STACK > select HAVE_GCC_PLUGINS > select HAVE_HW_BREAKPOINT if PERF_EVENTS > select HAVE_IRQ_TIME_ACCOUNTING > -- > 2.24.0.rc1.363.gb1bccd3e3d-goog >
On Mon, Nov 4, 2019 at 9:11 AM Mark Rutland <mark.rutland@arm.com> wrote: > Can you please elaborate on _how_ this is incompatible in the commit > message? > > For example, it's not clear to me if you mean that's functionally > incompatible, or if you're trying to remove return-altering gadgets. > > If there's a functional incompatibility, please spell that out a bit > more clearly. Likewise if this is about minimizing the set of places > that can mess with control-flow outside of usual function conventions. Sure, I'll add a better description in v5. In this case, the return address is modified in the kernel stack, which means the changes are ignored with SCS. Sami
On Mon, Nov 04, 2019 at 03:44:39PM -0800, Sami Tolvanen wrote: > On Mon, Nov 4, 2019 at 9:11 AM Mark Rutland <mark.rutland@arm.com> wrote: > > Can you please elaborate on _how_ this is incompatible in the commit > > message? > > > > For example, it's not clear to me if you mean that's functionally > > incompatible, or if you're trying to remove return-altering gadgets. > > > > If there's a functional incompatibility, please spell that out a bit > > more clearly. Likewise if this is about minimizing the set of places > > that can mess with control-flow outside of usual function conventions. > > Sure, I'll add a better description in v5. In this case, the return > address is modified in the kernel stack, which means the changes are > ignored with SCS. Ok, that makes sense to me. I'd suggest something like: | 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 the graph tracer when SCS is | enabled. ... as I suspect with some rework of the trampoline and common ftrace code we'd be able to correctly manipulate the shadow stack for this. Similarly, if clang gained -fpatchable-funciton-etnry, we'd get that for free. Thanks, Mark.
On Tue, Nov 5, 2019 at 11:55 AM Mark Rutland <mark.rutland@arm.com> wrote: > Similarly, if clang gained -fpatchable-funciton-etnry, we'd get that for > free. Filed: https://bugs.llvm.org/show_bug.cgi?id=43912
On Tue, Nov 5, 2019 at 11:55 AM Mark Rutland <mark.rutland@arm.com> wrote: > On Mon, Nov 04, 2019 at 03:44:39PM -0800, Sami Tolvanen wrote: > > Sure, I'll add a better description in v5. In this case, the return > > address is modified in the kernel stack, which means the changes are > > ignored with SCS. > > Ok, that makes sense to me. I'd suggest something like: > > | 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 the graph tracer when SCS is > | enabled. > > ... as I suspect with some rework of the trampoline and common ftrace > code we'd be able to correctly manipulate the shadow stack for this. > Similarly, if clang gained -fpatchable-funciton-etnry, we'd get that for > free. That sounds good to me. Thanks, Mark. Sami
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index e7b57a8a5531..42867174920f 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -148,7 +148,7 @@ config ARM64 select HAVE_FTRACE_MCOUNT_RECORD select HAVE_FUNCTION_TRACER select HAVE_FUNCTION_ERROR_INJECTION - select HAVE_FUNCTION_GRAPH_TRACER + select HAVE_FUNCTION_GRAPH_TRACER if !SHADOW_CALL_STACK select HAVE_GCC_PLUGINS select HAVE_HW_BREAKPOINT if PERF_EVENTS select HAVE_IRQ_TIME_ACCOUNTING