* [PATCH] arm64: fix the address of syscall in arch_syscall_addr if CFI is enabled @ 2021-12-03 5:29 Ben Dai 2021-12-03 11:12 ` Mark Rutland 0 siblings, 1 reply; 3+ messages in thread From: Ben Dai @ 2021-12-03 5:29 UTC (permalink / raw) To: samitolvanen, ndesaulniers, rostedt, mingo; +Cc: linux-kernel, llvm, Ben Dai With CONFIG_CFI_CLANG, the addresses in sys_call_table[] actually point to jump instructions like "b __arm64_sys_*", and if CONFIG_LTO_CLANG_FULL is enabled, the compiler will not generate a symbol for each jump. It causes syscall tracer can't get symbol name in find_syscall_meta() and fail to initialize. To fix this problem, implement an strong definition of arch_syscall_addr() to get the actual addresses of system calls. Signed-off-by: Ben Dai <ben.dai@unisoc.com> --- arch/arm64/kernel/syscall.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c index 50a0f1a38e84..2b911603966b 100644 --- a/arch/arm64/kernel/syscall.c +++ b/arch/arm64/kernel/syscall.c @@ -12,6 +12,8 @@ #include <asm/debug-monitors.h> #include <asm/exception.h> #include <asm/fpsimd.h> +#include <asm/insn.h> +#include <asm/patching.h> #include <asm/syscall.h> #include <asm/thread_info.h> #include <asm/unistd.h> @@ -19,6 +21,25 @@ long compat_arm_syscall(struct pt_regs *regs, int scno); long sys_ni_syscall(void); +#ifdef CONFIG_CFI_CLANG +unsigned long __init arch_syscall_addr(int nr) +{ + u32 insn; + unsigned long addr = (unsigned long)sys_call_table[nr]; + + /* + * Clang's CFI will replace the address of each system call function + * with the address of a jump table entry. In this case, the jump + * target address is the actual address of the system call. + */ + aarch64_insn_read((void *)addr, &insn); + if (likely(aarch64_insn_is_b(insn))) + addr += aarch64_get_branch_offset(insn); + + return addr; +} +#endif + static long do_ni_syscall(struct pt_regs *regs, int scno) { #ifdef CONFIG_COMPAT -- 2.17.0 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] arm64: fix the address of syscall in arch_syscall_addr if CFI is enabled 2021-12-03 5:29 [PATCH] arm64: fix the address of syscall in arch_syscall_addr if CFI is enabled Ben Dai @ 2021-12-03 11:12 ` Mark Rutland 2021-12-03 16:07 ` Sami Tolvanen 0 siblings, 1 reply; 3+ messages in thread From: Mark Rutland @ 2021-12-03 11:12 UTC (permalink / raw) To: Ben Dai Cc: samitolvanen, ndesaulniers, rostedt, mingo, linux-kernel, llvm, Ben Dai, Catalin Marinas, Will Deacon, Ard Biesheuvel, Peter Zijlstra Hi Ben, Please Cc the arm64 maintainers for arm64 patches, you can find the relevant folk in the MAINTAINERS file, and you can use the get_maintainer.pl script to find them automatically (e.g. with `./scripts/get_maintainer.pl ${PATCH}` or `./scripts/get_maintainer.pl -f ${FILE}`). For arm64 patches, you should also see LAKML (linux-arm-kernel@lists.infradead.org). I've added Will and Catalin now, and Ard and Peter too since they've btoh been fighting with things in this area. On Fri, Dec 03, 2021 at 01:29:08PM +0800, Ben Dai wrote: > With CONFIG_CFI_CLANG, the addresses in sys_call_table[] actually point to > jump instructions like "b __arm64_sys_*", and if CONFIG_LTO_CLANG_FULL is > enabled, the compiler will not generate a symbol for each jump. It causes > syscall tracer can't get symbol name in find_syscall_meta() and fail to > initialize. > > To fix this problem, implement an strong definition of arch_syscall_addr() > to get the actual addresses of system calls. > > Signed-off-by: Ben Dai <ben.dai@unisoc.com> > --- > arch/arm64/kernel/syscall.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c > index 50a0f1a38e84..2b911603966b 100644 > --- a/arch/arm64/kernel/syscall.c > +++ b/arch/arm64/kernel/syscall.c > @@ -12,6 +12,8 @@ > #include <asm/debug-monitors.h> > #include <asm/exception.h> > #include <asm/fpsimd.h> > +#include <asm/insn.h> > +#include <asm/patching.h> > #include <asm/syscall.h> > #include <asm/thread_info.h> > #include <asm/unistd.h> > @@ -19,6 +21,25 @@ > long compat_arm_syscall(struct pt_regs *regs, int scno); > long sys_ni_syscall(void); > > +#ifdef CONFIG_CFI_CLANG > +unsigned long __init arch_syscall_addr(int nr) > +{ > + u32 insn; > + unsigned long addr = (unsigned long)sys_call_table[nr]; > + > + /* > + * Clang's CFI will replace the address of each system call function > + * with the address of a jump table entry. In this case, the jump > + * target address is the actual address of the system call. > + */ > + aarch64_insn_read((void *)addr, &insn); > + if (likely(aarch64_insn_is_b(insn))) > + addr += aarch64_get_branch_offset(insn); The problemn is not your fault, but I absolutely hate this, because we're bodging around the compiler having broken stuff for us in the first place. We've been encountering more of these cases over time, and had we been aware of them to begin with we'd have strongly pushed back on marging the CFI patches until the compiler offered a better solution. My view is that the current clang CFI scheme is broken by design here, and we should mark tracing as incompatible with it for now, and work with the compiler folk to get a scheme that actually works, rather than trying to bodge around it. Thanks, Mark. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] arm64: fix the address of syscall in arch_syscall_addr if CFI is enabled 2021-12-03 11:12 ` Mark Rutland @ 2021-12-03 16:07 ` Sami Tolvanen 0 siblings, 0 replies; 3+ messages in thread From: Sami Tolvanen @ 2021-12-03 16:07 UTC (permalink / raw) To: Mark Rutland Cc: Ben Dai, ndesaulniers, rostedt, mingo, linux-kernel, llvm, Ben Dai, Catalin Marinas, Will Deacon, Ard Biesheuvel, Peter Zijlstra On Fri, Dec 3, 2021 at 3:12 AM Mark Rutland <mark.rutland@arm.com> wrote: > > Hi Ben, > > Please Cc the arm64 maintainers for arm64 patches, you can find the relevant > folk in the MAINTAINERS file, and you can use the get_maintainer.pl script to > find them automatically (e.g. with `./scripts/get_maintainer.pl ${PATCH}` or > `./scripts/get_maintainer.pl -f ${FILE}`). > > For arm64 patches, you should also see LAKML > (linux-arm-kernel@lists.infradead.org). > > I've added Will and Catalin now, and Ard and Peter too since they've btoh been > fighting with things in this area. > > On Fri, Dec 03, 2021 at 01:29:08PM +0800, Ben Dai wrote: > > With CONFIG_CFI_CLANG, the addresses in sys_call_table[] actually point to > > jump instructions like "b __arm64_sys_*", and if CONFIG_LTO_CLANG_FULL is > > enabled, the compiler will not generate a symbol for each jump. It causes > > syscall tracer can't get symbol name in find_syscall_meta() and fail to > > initialize. > > > > To fix this problem, implement an strong definition of arch_syscall_addr() > > to get the actual addresses of system calls. > > > > Signed-off-by: Ben Dai <ben.dai@unisoc.com> I don't think this is the correct solution. I believe Nick fixed the issue with missing symbols in https://reviews.llvm.org/D107934, so we should probably just require a newer compiler version here. > > --- > > arch/arm64/kernel/syscall.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > diff --git a/arch/arm64/kernel/syscall.c b/arch/arm64/kernel/syscall.c > > index 50a0f1a38e84..2b911603966b 100644 > > --- a/arch/arm64/kernel/syscall.c > > +++ b/arch/arm64/kernel/syscall.c > > @@ -12,6 +12,8 @@ > > #include <asm/debug-monitors.h> > > #include <asm/exception.h> > > #include <asm/fpsimd.h> > > +#include <asm/insn.h> > > +#include <asm/patching.h> > > #include <asm/syscall.h> > > #include <asm/thread_info.h> > > #include <asm/unistd.h> > > @@ -19,6 +21,25 @@ > > long compat_arm_syscall(struct pt_regs *regs, int scno); > > long sys_ni_syscall(void); > > > > +#ifdef CONFIG_CFI_CLANG > > +unsigned long __init arch_syscall_addr(int nr) > > +{ > > + u32 insn; > > + unsigned long addr = (unsigned long)sys_call_table[nr]; > > + > > + /* > > + * Clang's CFI will replace the address of each system call function > > + * with the address of a jump table entry. In this case, the jump > > + * target address is the actual address of the system call. > > + */ > > + aarch64_insn_read((void *)addr, &insn); > > + if (likely(aarch64_insn_is_b(insn))) > > + addr += aarch64_get_branch_offset(insn); > > The problemn is not your fault, but I absolutely hate this, because we're > bodging around the compiler having broken stuff for us in the first place. > We've been encountering more of these cases over time, and had we been aware of > them to begin with we'd have strongly pushed back on marging the CFI patches > until the compiler offered a better solution. > > My view is that the current clang CFI scheme is broken by design here, and we > should mark tracing as incompatible with it for now, and work with the compiler > folk to get a scheme that actually works, rather than trying to bodge around > it. We are actively fixing compiler issues when they're discovered. In the meanwhile, Android uses both CFI and tracing without any major issues, so I don't see why we should mark the combination broken. Sami ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-12-03 16:07 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-03 5:29 [PATCH] arm64: fix the address of syscall in arch_syscall_addr if CFI is enabled Ben Dai 2021-12-03 11:12 ` Mark Rutland 2021-12-03 16:07 ` Sami Tolvanen
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.