| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 25 May 2022 14:43:55 +0100
From: Mark Rutland <mark.rutland@....com>
To: Xu Kuohai <xukuohai@...wei.com>
Cc: bpf@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
linux-kselftest@...r.kernel.org,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>,
Steven Rostedt <rostedt@...dmis.org>,
Ingo Molnar <mingo@...hat.com>,
Daniel Borkmann <daniel@...earbox.net>,
Alexei Starovoitov <ast@...nel.org>,
Zi Shen Lim <zlim.lnx@...il.com>,
Andrii Nakryiko <andrii@...nel.org>,
Martin KaFai Lau <kafai@...com>,
Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...nel.org>,
"David S . Miller" <davem@...emloft.net>,
Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
David Ahern <dsahern@...nel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
hpa@...or.com, Shuah Khan <shuah@...nel.org>,
Jakub Kicinski <kuba@...nel.org>,
Jesper Dangaard Brouer <hawk@...nel.org>,
Pasha Tatashin <pasha.tatashin@...een.com>,
Ard Biesheuvel <ardb@...nel.org>,
Daniel Kiss <daniel.kiss@....com>,
Steven Price <steven.price@....com>,
Sudeep Holla <sudeep.holla@....com>,
Marc Zyngier <maz@...nel.org>,
Peter Collingbourne <pcc@...gle.com>,
Mark Brown <broonie@...nel.org>,
Delyan Kratunov <delyank@...com>,
Kumar Kartikeya Dwivedi <memxor@...il.com>
Subject: Re: [PATCH bpf-next v5 2/6] ftrace: Fix deadloop caused by direct
call in ftrace selftest
On Wed, May 18, 2022 at 09:16:34AM -0400, Xu Kuohai wrote:
> After direct call is enabled for arm64, ftrace selftest enters a
> dead loop:
IIUC this means that patch 1 alone is broken, and presumably this patch should
have been part of it?
> <trace_selftest_dynamic_test_func>:
> 00 bti c
> 01 mov x9, x30 <trace_direct_tramp>:
> 02 bl <trace_direct_tramp> ----------> ret
> |
> lr/x30 is 03, return to 03
> |
> 03 mov w0, #0x0 <-----------------------------|
> | |
> | dead loop! |
> | |
> 04 ret ---- lr/x30 is still 03, go back to 03 ----|
>
> The reason is that when the direct caller trace_direct_tramp() returns
> to the patched function trace_selftest_dynamic_test_func(), lr is still
> the address after the instrumented instruction in the patched function,
> so when the patched function exits, it returns to itself!
>
> To fix this issue, we need to restore lr before trace_direct_tramp()
> exits, so rewrite a dedicated trace_direct_tramp() for arm64.
As mentioned on patch 1 I'd prefer we solved this through indirection, which
would avoid the need for this and would make things more robust generally by
keeping the unusual calling convention private to the patch-site and regular
trampoline.
Thanks,
Mark.
> Reported-by: Li Huafei <lihuafei1@...wei.com>
> Signed-off-by: Xu Kuohai <xukuohai@...wei.com>
> ---
> arch/arm64/include/asm/ftrace.h | 10 ++++++++++
> arch/arm64/kernel/entry-ftrace.S | 10 ++++++++++
> kernel/trace/trace_selftest.c | 2 ++
> 3 files changed, 22 insertions(+)
>
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index 14a35a5df0a1..6f6b184e72fb 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -126,6 +126,16 @@ static inline bool arch_syscall_match_sym_name(const char *sym,
> */
> return !strcmp(sym + 8, name);
> }
> +
> +#ifdef CONFIG_FTRACE_SELFTEST
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> +
> +#define trace_direct_tramp trace_direct_tramp
> +extern void trace_direct_tramp(void);
> +
> +#endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
> +#endif /* CONFIG_FTRACE_SELFTEST */
> +
> #endif /* ifndef __ASSEMBLY__ */
>
> #endif /* __ASM_FTRACE_H */
> diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
> index dfe62c55e3a2..a47e87d4d3dd 100644
> --- a/arch/arm64/kernel/entry-ftrace.S
> +++ b/arch/arm64/kernel/entry-ftrace.S
> @@ -357,3 +357,13 @@ SYM_CODE_START(return_to_handler)
> ret
> SYM_CODE_END(return_to_handler)
> #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
> +
> +#ifdef CONFIG_FTRACE_SELFTEST
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> +SYM_FUNC_START(trace_direct_tramp)
> + mov x10, x30
> + mov x30, x9
> + ret x10
> +SYM_FUNC_END(trace_direct_tramp)
> +#endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
> +#endif /* CONFIG_FTRACE_SELFTEST */
> diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c
> index abcadbe933bb..e7ccd0d10c39 100644
> --- a/kernel/trace/trace_selftest.c
> +++ b/kernel/trace/trace_selftest.c
> @@ -785,8 +785,10 @@ static struct fgraph_ops fgraph_ops __initdata = {
> };
>
> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> +#ifndef trace_direct_tramp
> noinline __noclone static void trace_direct_tramp(void) { }
> #endif
> +#endif
>
> /*
> * Pretty much the same than for the function tracer from which the selftest
> --
> 2.30.2
>
Powered by blists - more mailing lists