Skip to contentSkip to conversation

arm64: Use static call trampolines when kCFI is enabled

PATCH Applied Ard Biesheuvel 4 participants 5 messages Mar 31 – Apr 1

Subsystems (2)
arm64 Static branch/call
lore
Share & tools
lore

Ard Biesheuvel

author
#0 lore raw

PATCH v8 1/1 arm64: Use static call trampolines when kCFI is enabled

From: Ard Biesheuvel <[email protected]>

Implement arm64 support for the 'unoptimized' static call variety, which routes all calls through a trampoline that performs a tail call to the chosen function, and wire it up for use when kCFI is enabled. This works around an issue with kCFI and generic static calls, where the prototypes of default handlers such as __static_call_nop() and __static_call_ret0() don't match the expected prototype of the call site, resulting in kCFI false positives [0].

Since static call targets may be located in modules loaded out of direct branching range, this needs a ADRP/ADD pair to load the branch target into R16 and a branch-to-register (BR) instruction to perform an indirect call. This is the exact code sequence that is used by modules when the call target is out of direct branching range.

Unlike on x86, there is no pressing need on arm64 to avoid indirect calls at all cost, but hiding it from the compiler as is done here does have some benefits:
- the literal is located in .rodata, which gives us the same robustness advantage that code patching does;
- no D-cache pollution from fetching hash values from .text sections.

From an execution speed PoV, this is unlikely to make any difference at all.

[0] https://lore.kernel.org/all/[email protected]/

Cc Carlos Llamas <[email protected]>
Cc Sami Tolvanen <[email protected]>
Cc Sean Christopherson <[email protected]>
Cc Kees Cook <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Will McVicker <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>

---
v8: Simplify the trampoline by combining the NULL and RET0 cases, and
dropping the conditional branch and return
v7: https://lore.kernel.org/all/[email protected]/

 arch/arm64/Kconfig                   |  1 +
 arch/arm64/include/asm/static_call.h | 31 ++++++++++++++++++++
 arch/arm64/kernel/Makefile           |  1 +
 arch/arm64/kernel/static_call.c      | 23 +++++++++++++++
 arch/arm64/kernel/vmlinux.lds.S      |  1 +
 5 files changed, 57 insertions(+)
arch/arm64/Kconfig +1 -0
index 38dba5f7e4d2..9ea19b74b6c3 100644
@@ -252,6 +252,7 @@ config ARM64
 	select HAVE_RSEQ
 	select HAVE_RUST if RUSTC_SUPPORTS_ARM64
 	select HAVE_STACKPROTECTOR
+	select HAVE_STATIC_CALL if CFI
 	select HAVE_SYSCALL_TRACEPOINTS
 	select HAVE_KPROBES
 	select HAVE_KRETPROBES
arch/arm64/include/asm/static_call.h new file +31 -0
new file mode 100644
index 000000000000..b73960c949e4
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_STATIC_CALL_H
+#define _ASM_STATIC_CALL_H
+
+#define __ARCH_DEFINE_STATIC_CALL_TRAMP(name, target)		    \
+	asm("	.pushsection .static_call.text, \"ax\"		\n" \
+	    "	.align	4					\n" \
+	    "	.globl	" name "				\n" \
+	    name ":						\n" \
+	    "	hint	34	/* BTI C */			\n" \
+	    "	adrp	x16, 1f					\n" \
+	    "	ldr	x16, [x16, :lo12:1f]			\n" \
+	    "	br	x16					\n" \
+	    "	.type	" name ", %function			\n" \
+	    "	.size	" name ", . - " name "			\n" \
+	    "	.popsection					\n" \
+	    "	.pushsection .rodata, \"a\"			\n" \
+	    "	.align	3					\n" \
+	    "1:	.quad	" #target "				\n" \
+	    "	.popsection					\n")
+
+#define ARCH_DEFINE_STATIC_CALL_TRAMP(name, func)			\
+	__ARCH_DEFINE_STATIC_CALL_TRAMP(STATIC_CALL_TRAMP_STR(name), #func)
+
+#define ARCH_DEFINE_STATIC_CALL_NULL_TRAMP(name)			\
+	ARCH_DEFINE_STATIC_CALL_TRAMP(name, __static_call_return0)
+
+#define ARCH_DEFINE_STATIC_CALL_RET0_TRAMP(name)			\
+	ARCH_DEFINE_STATIC_CALL_TRAMP(name, __static_call_return0)
+
+#endif /* _ASM_STATIC_CALL_H */
arch/arm64/kernel/Makefile +1 -0
index 76f32e424065..fe627100d199 100644
@@ -46,6 +46,7 @@ obj-$(CONFIG_MODULES)			+= module.o module-plts.o
 obj-$(CONFIG_PERF_EVENTS)		+= perf_regs.o perf_callchain.o
 obj-$(CONFIG_HARDLOCKUP_DETECTOR_PERF)	+= watchdog_hld.o
 obj-$(CONFIG_HAVE_HW_BREAKPOINT)	+= hw_breakpoint.o
+obj-$(CONFIG_HAVE_STATIC_CALL)		+= static_call.o
 obj-$(CONFIG_CPU_PM)			+= sleep.o suspend.o
 obj-$(CONFIG_KGDB)			+= kgdb.o
 obj-$(CONFIG_EFI)			+= efi.o efi-rt-wrapper.o
arch/arm64/kernel/static_call.c new file +23 -0
new file mode 100644
index 000000000000..8b3a19e10871
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/static_call.h>
+#include <linux/memory.h>
+#include <asm/text-patching.h>
+
+void arch_static_call_transform(void *site, void *tramp, void *func, bool tail)
+{
+	u64 literal;
+	int ret;
+
+	if (!func)
+		func = __static_call_return0;
+
+	/* decode the instructions to discover the literal address */
+	literal = ALIGN_DOWN((u64)tramp + 4, SZ_4K) +
+		  aarch64_insn_adrp_get_offset(le32_to_cpup(tramp + 4)) +
+		  8 * aarch64_insn_decode_immediate(AARCH64_INSN_IMM_12,
+						    le32_to_cpup(tramp + 8));
+
+	ret = aarch64_insn_write_literal_u64((void *)literal, (u64)func);
+	WARN_ON_ONCE(ret);
+}
+EXPORT_SYMBOL_GPL(arch_static_call_transform);
arch/arm64/kernel/vmlinux.lds.S +1 -0
index 2964aad0362e..2d1e75263f03 100644
@@ -191,6 +191,7 @@ SECTIONS
 			LOCK_TEXT
 			KPROBES_TEXT
 			HYPERVISOR_TEXT
+			STATIC_CALL_TEXT
 			*(.gnu.warning)
 	}
Signature · 1 line
2.53.0.1018.g2bb0e51243-goog
reviewer
#1 lore raw
Quoted 10 lines · Ard Biesheuvel

From: Ard Biesheuvel <[email protected]>

Implement arm64 support for the 'unoptimized' static call variety, which routes all calls through a trampoline that performs a tail call to the chosen function, and wire it up for use when kCFI is enabled. This works around an issue with kCFI and generic static calls, where the prototypes of default handlers such as __static_call_nop() and __static_call_ret0() don't match the expected prototype of the call site, resulting in kCFI false positives [0].

Quoted 2 lines

Since static call targets may be located in modules loaded out of direct branching range, this needs a ADRP/ADD pair to load the branch target

Sashiko correctly points out that this should say ADRP/LDR rather than ADRP/ADD, and this means that the sequence is in fact different from the one used by modules.

Quoted 4 lines

into R16 and a branch-to-register (BR) instruction to perform an indirect call. This is the exact code sequence that is used by modules when the call target is out of direct branching range.

... so please drop this last sentence when applying.

Quoted 139 lines

Unlike on x86, there is no pressing need on arm64 to avoid indirect calls at all cost, but hiding it from the compiler as is done here does have some benefits:
- the literal is located in .rodata, which gives us the same robustness advantage that code patching does;
- no D-cache pollution from fetching hash values from .text sections.

From an execution speed PoV, this is unlikely to make any difference at all.

[0] https://lore.kernel.org/all/[email protected]/

Cc Carlos Llamas <[email protected]>
Cc Sami Tolvanen <[email protected]>
Cc Sean Christopherson <[email protected]>
Cc Kees Cook <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Will McVicker <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>

---
v8: Simplify the trampoline by combining the NULL and RET0 cases, and
dropping the conditional branch and return
v7: https://lore.kernel.org/all/[email protected]/

 arch/arm64/Kconfig                   |  1 +
 arch/arm64/include/asm/static_call.h | 31 ++++++++++++++++++++
 arch/arm64/kernel/Makefile           |  1 +
 arch/arm64/kernel/static_call.c      | 23 +++++++++++++++
 arch/arm64/kernel/vmlinux.lds.S      |  1 +
 5 files changed, 57 insertions(+)
arch/arm64/Kconfig +1 -0
index 38dba5f7e4d2..9ea19b74b6c3 100644
@@ -252,6 +252,7 @@ config ARM64
 	select HAVE_RSEQ
 	select HAVE_RUST if RUSTC_SUPPORTS_ARM64
 	select HAVE_STACKPROTECTOR
+	select HAVE_STATIC_CALL if CFI
 	select HAVE_SYSCALL_TRACEPOINTS
 	select HAVE_KPROBES
 	select HAVE_KRETPROBES
arch/arm64/include/asm/static_call.h new file +31 -0
new file mode 100644
index 000000000000..b73960c949e4
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_STATIC_CALL_H
+#define _ASM_STATIC_CALL_H
+
+#define __ARCH_DEFINE_STATIC_CALL_TRAMP(name, target)		    \
+	asm("	.pushsection .static_call.text, \"ax\"		\n" \
+	    "	.align	4					\n" \
+	    "	.globl	" name "				\n" \
+	    name ":						\n" \
+	    "	hint	34	/* BTI C */			\n" \
+	    "	adrp	x16, 1f					\n" \
+	    "	ldr	x16, [x16, :lo12:1f]			\n" \
+	    "	br	x16					\n" \
+	    "	.type	" name ", %function			\n" \
+	    "	.size	" name ", . - " name "			\n" \
+	    "	.popsection					\n" \
+	    "	.pushsection .rodata, \"a\"			\n" \
+	    "	.align	3					\n" \
+	    "1:	.quad	" #target "				\n" \
+	    "	.popsection					\n")
+
+#define ARCH_DEFINE_STATIC_CALL_TRAMP(name, func)			\
+	__ARCH_DEFINE_STATIC_CALL_TRAMP(STATIC_CALL_TRAMP_STR(name), #func)
+
+#define ARCH_DEFINE_STATIC_CALL_NULL_TRAMP(name)			\
+	ARCH_DEFINE_STATIC_CALL_TRAMP(name, __static_call_return0)
+
+#define ARCH_DEFINE_STATIC_CALL_RET0_TRAMP(name)			\
+	ARCH_DEFINE_STATIC_CALL_TRAMP(name, __static_call_return0)
+
+#endif /* _ASM_STATIC_CALL_H */
arch/arm64/kernel/Makefile +1 -0
index 76f32e424065..fe627100d199 100644
@@ -46,6 +46,7 @@ obj-$(CONFIG_MODULES)			+= module.o module-plts.o
 obj-$(CONFIG_PERF_EVENTS)		+= perf_regs.o perf_callchain.o
 obj-$(CONFIG_HARDLOCKUP_DETECTOR_PERF)	+= watchdog_hld.o
 obj-$(CONFIG_HAVE_HW_BREAKPOINT)	+= hw_breakpoint.o
+obj-$(CONFIG_HAVE_STATIC_CALL)		+= static_call.o
 obj-$(CONFIG_CPU_PM)			+= sleep.o suspend.o
 obj-$(CONFIG_KGDB)			+= kgdb.o
 obj-$(CONFIG_EFI)			+= efi.o efi-rt-wrapper.o
arch/arm64/kernel/static_call.c new file +6 -0
new file mode 100644
index 000000000000..8b3a19e10871
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/static_call.h>
+#include <linux/memory.h>
+#include <asm/text-patching.h>
+
+void arch_static_call_transform(void *site, void *tramp, void *func,

bool tail)

arch/arm64/kernel/static_call.c new file +17 -0
+{
+	u64 literal;
+	int ret;
+
+	if (!func)
+		func = __static_call_return0;
+
+	/* decode the instructions to discover the literal address */
+	literal = ALIGN_DOWN((u64)tramp + 4, SZ_4K) +
+		  aarch64_insn_adrp_get_offset(le32_to_cpup(tramp + 4)) +
+		  8 * aarch64_insn_decode_immediate(AARCH64_INSN_IMM_12,
+						    le32_to_cpup(tramp + 8));
+
+	ret = aarch64_insn_write_literal_u64((void *)literal, (u64)func);
+	WARN_ON_ONCE(ret);
+}
+EXPORT_SYMBOL_GPL(arch_static_call_transform);
arch/arm64/kernel/vmlinux.lds.S +1 -0
index 2964aad0362e..2d1e75263f03 100644
@@ -191,6 +191,7 @@ SECTIONS
 			LOCK_TEXT
 			KPROBES_TEXT
 			HYPERVISOR_TEXT
+			STATIC_CALL_TEXT
 			*(.gnu.warning)
 	}
Signature · 1 line
2.53.0.1018.g2bb0e51243-goog
maintainer Applied
#2 lore raw
In reply to Ard Biesheuvel
Quoted 9 lines · Ard Biesheuvel

Implement arm64 support for the 'unoptimized' static call variety, which routes all calls through a trampoline that performs a tail call to the chosen function, and wire it up for use when kCFI is enabled. This works around an issue with kCFI and generic static calls, where the prototypes of default handlers such as __static_call_nop() and __static_call_ret0() don't match the expected prototype of the call site, resulting in kCFI false positives [0].

[...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64: Use static call trampolines when kCFI is enabled

      https://git.kernel.org/arm64/c/e70c2335f889

Cheers,

Signature · 5 lines
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev
reviewer
#3 lore raw
Quoted 9 lines · Will Deacon
Quoted 8 lines · Ard Biesheuvel

Implement arm64 support for the 'unoptimized' static call variety, which routes all calls through a trampoline that performs a tail call to the chosen function, and wire it up for use when kCFI is enabled. This works around an issue with kCFI and generic static calls, where the prototypes of default handlers such as __static_call_nop() and __static_call_ret0() don't match the expected prototype of the call site, resulting in kCFI false positives [0].

Quoted 7 lines
Quoted 1 line

[...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64: Use static call trampolines when kCFI is enabled

      https://git.kernel.org/arm64/c/e70c2335f889

Thanks,

I just spotted that the function name gets stringified twice inadvertently.

E.g., the assembler may see

.quad "__static_call_return0"

rather than the intended

.quad __static_call_return0

The assembler does not seem to care, and still emits an ABS64 relocation against the correct symbol, but it is definitely unintentional.

I can send a follow-up fix if you prefer, or you could just tweak the patch in place:

arch/arm64/include/asm/static_call.h +1 -1
@@ -16,7 +16,7 @@
            "   .popsection                                     \n" \
            "   .pushsection .rodata, \"a\"                     \n" \
            "   .align  3                                       \n" \
-           "1: .quad   " #target "                             \n" \
+           "1: .quad   " target "                              \n" \
            "   .popsection                                     \n")
maintainer
#4 lore raw
Quoted 38 lines · Ard Biesheuvel
Quoted 9 lines · Will Deacon
Quoted 8 lines · Ard Biesheuvel

Implement arm64 support for the 'unoptimized' static call variety, which routes all calls through a trampoline that performs a tail call to the chosen function, and wire it up for use when kCFI is enabled. This works around an issue with kCFI and generic static calls, where the prototypes of default handlers such as __static_call_nop() and __static_call_ret0() don't match the expected prototype of the call site, resulting in kCFI false positives [0].

Quoted 7 lines
Quoted 1 line

[...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64: Use static call trampolines when kCFI is enabled

      https://git.kernel.org/arm64/c/e70c2335f889

Thanks,

I just spotted that the function name gets stringified twice inadvertently.

E.g., the assembler may see

.quad "__static_call_return0"

rather than the intended

.quad __static_call_return0

The assembler does not seem to care, and still emits an ABS64 relocation against the correct symbol, but it is definitely unintentional.

I can send a follow-up fix if you prefer, or you could just tweak the patch in place:

arch/arm64/include/asm/static_call.h line 20

Quoted 6 lines
arch/arm64/include/asm/static_call.h +1 -1
@@ -16,7 +16,7 @@
            "   .popsection                                     \n" \
            "   .pushsection .rodata, \"a\"                     \n" \
            "   .align  3                                       \n" \
-           "1: .quad   " #target "                             \n" \
+           "1: .quad   " target "                              \n" \
            "   .popsection                                     \n")

Not sure Will is going to look at this until next week. I folded it into the arm64 for-next/fixes branch and pushed out.

Signature · 1 line
Catalin