[PATCH] arm64: fix the address of syscall in arch_syscall_addr if CFI is enabled

[PATCH] arm64: fix the address of syscall in arch_syscall_addr if CFI is enabled

[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

Re: [PATCH] arm64: fix the address of syscall in arch_syscall_addr if CFI is enabled

[Mark Rutland]

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.

Re: [PATCH] arm64: fix the address of syscall in arch_syscall_addr if CFI is enabled

[Sami Tolvanen]

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