Messages in this thread Patches in this message | | | Subject | Re: Avoid speculative indirect calls in kernel | Date | Thu, 4 Jan 2018 08:15:53 +0000 | From | "Woodhouse, David" <> |
| |
On Wed, 2018-01-03 at 16:40 -0800, Andi Kleen wrote: > > > > So you say, that we finally need a perl interpreter in the kernel > > to do > > alternative patching? > I don't think perl or objtool makes sense. That would be just > incredibly > fragile because compilers can reorder and mix code. > > It could be done with a gcc change I suppose. That should be > reliable. > > But that would need to be developed first. We don't have it right > now. > > As the first step a compile time approach should be sufficient. > We can add a CONFIG option so people can chose at compile time. > > Then later we can investigate run time patching.
My original code, that Intel had included in the osv_v7.1 patch set, already did runtime patching for all the explicit call sites:
+.macro JMP_THUNK reg:req +#ifdef RETPOLINE + ALTERNATIVE __stringify(jmp __x86.indirect_thunk.\reg), __stringify(jmp *%\reg), X86_FEATURE_IBRS_ATT +#else + jmp *\reg +#endif +.endm
.. and the thunks themselves were also using alternatives:
+ENTRY(__x86.indirect_thunk.\reg) + CFI_STARTPROC + ALTERNATIVE "call 2f", __stringify(jmp *%\reg), X86_FEATURE_IBRS_ATT +1: + lfence + jmp 1b +2: + mov %\reg, (%\sp) + ret + CFI_ENDPROC +ENDPROC(__x86.indirect_thunk.\reg)
So all you were left with was the single static jump in the call sites which GCC emitted, which are basically harmless. It's not clear that it makes sense to post-process GCC output just to eliminate those.
Andi, you seem to have made a lot of changes, some cosmetic and some make it look like you were working on an older version of the code.
For reference, here's my original version.From 970687a4c18ac5adb2124c673e31fbfe079220ca Mon Sep 17 00:00:00 2001 From: David Woodhouse <dwmw2@infradead.org> Date: Sat, 23 Dec 2017 00:51:24 +0000 Subject: [PATCH 1/8] x86: Add initial retpoline support with -mindirect-branch=thunk-extern
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> --- arch/x86/Makefile | 7 +++++++ arch/x86/kernel/Makefile | 1 + arch/x86/kernel/retpoline.S | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+) create mode 100644 arch/x86/kernel/retpoline.S
diff --git a/arch/x86/Makefile b/arch/x86/Makefile index a20eacd..a6d5d39 100644 --- a/arch/x86/Makefile +++ b/arch/x86/Makefile @@ -235,6 +235,13 @@ KBUILD_CFLAGS += -Wno-sign-compare # KBUILD_CFLAGS += -fno-asynchronous-unwind-tables +# +RETPOLINE_CFLAGS += $(call cc-option,-mindirect-branch=thunk-extern -mindirect-branch-register) +ifneq ($(RETPOLINE_CFLAGS),) + KBUILD_CFLAGS += $(RETPOLINE_CFLAGS) -DRETPOLINE + KBUILD_AFLAGS += -DRETPOLINE +endif + archscripts: scripts_basic $(Q)$(MAKE) $(build)=arch/x86/tools relocs diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile index 295abaa..d1af553 100644 --- a/arch/x86/kernel/Makefile +++ b/arch/x86/kernel/Makefile @@ -49,6 +49,7 @@ obj-$(CONFIG_MODIFY_LDT_SYSCALL) += ldt.o obj-y += setup.o x86_init.o i8259.o irqinit.o jump_label.o obj-$(CONFIG_IRQ_WORK) += irq_work.o obj-y += probe_roms.o +obj-y += retpoline.o obj-$(CONFIG_X86_64) += sys_x86_64.o obj-$(CONFIG_X86_ESPFIX64) += espfix_64.o obj-$(CONFIG_SYSFS) += ksysfs.o diff --git a/arch/x86/kernel/retpoline.S b/arch/x86/kernel/retpoline.S new file mode 100644 index 0000000..b2b17b1 --- /dev/null +++ b/arch/x86/kernel/retpoline.S @@ -0,0 +1,37 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifdef RETPOLINE + +#include <linux/stringify.h> +#include <linux/linkage.h> +#include <asm/dwarf2.h> +#include <asm/cpufeatures.h> +#include <asm/alternative-asm.h> + +.macro THUNK sp reg + .section .text.__x86.indirect_thunk.\reg + +ENTRY(__x86.indirect_thunk.\reg) + CFI_STARTPROC + ALTERNATIVE "call 2f", __stringify(jmp *%\reg), X86_FEATURE_IBRS_ATT +1: + lfence + jmp 1b +2: + mov %\reg, (%\sp) + ret + CFI_ENDPROC +ENDPROC(__x86.indirect_thunk.\reg) +.endm + +#ifdef CONFIG_64BIT +.irp reg rax rbx rcx rdx rsi rdi rbp r8 r9 r10 r11 r12 r13 r14 r15 + THUNK rsp \reg +.endr +#else +.irp reg eax ebx ecx edx esi edi ebp + THUNK esp \reg +.endr +#endif + +#endif /* RETPOLINE */ -- 2.7.4 From 68521da88d6d05ed2b9ad5e57a7475e8d3fdc904 Mon Sep 17 00:00:00 2001 From: David Woodhouse <dwmw2@infradead.org> Date: Sat, 23 Dec 2017 01:59:04 +0000 Subject: [PATCH 2/8] x86: Replace indirect branches in asm with CALL_THUNK/JMP_THUNK macro
This uses the same retpoline thunks that the -mindirect-branch= GCC option uses, to avoid speculative branch prediction problems.
Open-code a version of the thunk in the syscall trampoline because we can't just jump to the existing ones.
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> --- arch/x86/crypto/aesni-intel_asm.S | 5 +++-- arch/x86/crypto/camellia-aesni-avx-asm_64.S | 3 ++- arch/x86/crypto/camellia-aesni-avx2-asm_64.S | 3 ++- arch/x86/crypto/crc32c-pcl-intel-asm_64.S | 3 ++- arch/x86/entry/entry_32.S | 4 ++-- arch/x86/entry/entry_64.S | 19 +++++++++++++++---- arch/x86/include/asm/spec_ctrl.h | 16 ++++++++++++++++ arch/x86/kernel/ftrace_32.S | 6 ++++-- arch/x86/kernel/ftrace_64.S | 7 ++++--- arch/x86/lib/checksum_32.S | 5 +++-- 10 files changed, 53 insertions(+), 18 deletions(-)
diff --git a/arch/x86/crypto/aesni-intel_asm.S b/arch/x86/crypto/aesni-intel_asm.S index 16627fe..52160d1 100644 --- a/arch/x86/crypto/aesni-intel_asm.S +++ b/arch/x86/crypto/aesni-intel_asm.S @@ -32,6 +32,7 @@ #include <linux/linkage.h> #include <asm/inst.h> #include <asm/frame.h> +#include <asm/spec_ctrl.h> /* * The following macros are used to move an (un)aligned 16 byte value to/from @@ -2884,7 +2885,7 @@ ENTRY(aesni_xts_crypt8) pxor INC, STATE4 movdqu IV, 0x30(OUTP) - call *%r11 + CALL_THUNK r11 movdqu 0x00(OUTP), INC pxor INC, STATE1 @@ -2929,7 +2930,7 @@ ENTRY(aesni_xts_crypt8) _aesni_gf128mul_x_ble() movups IV, (IVP) - call *%r11 + CALL_THUNK r11 movdqu 0x40(OUTP), INC pxor INC, STATE1 diff --git a/arch/x86/crypto/camellia-aesni-avx-asm_64.S b/arch/x86/crypto/camellia-aesni-avx-asm_64.S index f7c495e..3bf23db 100644 --- a/arch/x86/crypto/camellia-aesni-avx-asm_64.S +++ b/arch/x86/crypto/camellia-aesni-avx-asm_64.S @@ -17,6 +17,7 @@ #include <linux/linkage.h> #include <asm/frame.h> +#include <asm/spec_ctrl.h> #define CAMELLIA_TABLE_BYTE_LEN 272 @@ -1227,7 +1228,7 @@ camellia_xts_crypt_16way: vpxor 14 * 16(%rax), %xmm15, %xmm14; vpxor 15 * 16(%rax), %xmm15, %xmm15; - call *%r9; + CALL_THUNK r9; addq $(16 * 16), %rsp; diff --git a/arch/x86/crypto/camellia-aesni-avx2-asm_64.S b/arch/x86/crypto/camellia-aesni-avx2-asm_64.S index eee5b39..1f06c98 100644 --- a/arch/x86/crypto/camellia-aesni-avx2-asm_64.S +++ b/arch/x86/crypto/camellia-aesni-avx2-asm_64.S @@ -12,6 +12,7 @@ #include <linux/linkage.h> #include <asm/frame.h> +#include <asm/spec_ctrl.h> #define CAMELLIA_TABLE_BYTE_LEN 272 @@ -1343,7 +1344,7 @@ camellia_xts_crypt_32way: vpxor 14 * 32(%rax), %ymm15, %ymm14; vpxor 15 * 32(%rax), %ymm15, %ymm15; - call *%r9; + CALL_THUNK r9; addq $(16 * 32), %rsp; diff --git a/arch/x86/crypto/crc32c-pcl-intel-asm_64.S b/arch/x86/crypto/crc32c-pcl-intel-asm_64.S index 7a7de27..fd89745 100644 --- a/arch/x86/crypto/crc32c-pcl-intel-asm_64.S +++ b/arch/x86/crypto/crc32c-pcl-intel-asm_64.S @@ -45,6 +45,7 @@ #include <asm/inst.h> #include <linux/linkage.h> +#include <asm/spec_ctrl.h> ## ISCSI CRC 32 Implementation with crc32 and pclmulqdq Instruction @@ -172,7 +173,7 @@ continue_block: movzxw (bufp, %rax, 2), len lea crc_array(%rip), bufp lea (bufp, len, 1), bufp - jmp *bufp + JMP_THUNK bufp ################################################################ ## 2a) PROCESS FULL BLOCKS: diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S index ace8f32..608d198 100644 --- a/arch/x86/entry/entry_32.S +++ b/arch/x86/entry/entry_32.S @@ -290,7 +290,7 @@ ENTRY(ret_from_fork) /* kernel thread */ 1: movl %edi, %eax - call *%ebx + CALL_THUNK ebx /* * A kernel thread is allowed to return here after successfully * calling do_execve(). Exit to userspace to complete the execve() @@ -919,7 +919,7 @@ common_exception: movl %ecx, %es TRACE_IRQS_OFF movl %esp, %eax # pt_regs pointer - call *%edi + CALL_THUNK edi jmp ret_from_exception END(common_exception) diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S index c6db10f..0d3ff7f 100644 --- a/arch/x86/entry/entry_64.S +++ b/arch/x86/entry/entry_64.S @@ -189,7 +189,17 @@ ENTRY(entry_SYSCALL_64_trampoline) */ pushq %rdi movq $entry_SYSCALL_64_stage2, %rdi - jmp *%rdi + /* + * Open-code the retpoline from retpoline.S, because we can't + * just jump to it directly. + */ + ALTERNATIVE "call 2f", "jmp *%rdi", X86_FEATURE_IBRS_ATT +1: + lfence + jmp 1b +2: + mov %rdi, (%rsp) + ret END(entry_SYSCALL_64_trampoline) .popsection @@ -276,7 +286,8 @@ entry_SYSCALL_64_fastpath: * It might end up jumping to the slow path. If it jumps, RAX * and all argument registers are clobbered. */ - call *sys_call_table(, %rax, 8) + movq sys_call_table(, %rax, 8), %rax + CALL_THUNK rax .Lentry_SYSCALL_64_after_fastpath_call: movq %rax, RAX(%rsp) @@ -449,7 +460,7 @@ ENTRY(stub_ptregs_64) jmp entry_SYSCALL64_slow_path 1: - jmp *%rax /* Called from C */ + JMP_THUNK rax /* Called from C */ END(stub_ptregs_64) .macro ptregs_stub func @@ -528,7 +539,7 @@ ENTRY(ret_from_fork) 1: /* kernel thread */ movq %r12, %rdi - call *%rbx + CALL_THUNK rbx /* * A kernel thread is allowed to return here after successfully * calling do_execve(). Exit to userspace to complete the execve() diff --git a/arch/x86/include/asm/spec_ctrl.h b/arch/x86/include/asm/spec_ctrl.h index e8ed6fd..59b664e 100644 --- a/arch/x86/include/asm/spec_ctrl.h +++ b/arch/x86/include/asm/spec_ctrl.h @@ -180,5 +180,21 @@ ALTERNATIVE "", __stringify(__ASM_DISABLE_IBRS_CLOBBER), X86_FEATURE_SPEC_CTRL ALTERNATIVE __stringify(__ASM_STUFF_RSB), "", X86_FEATURE_SMEP .endm +.macro JMP_THUNK reg:req +#ifdef RETPOLINE + ALTERNATIVE __stringify(jmp __x86.indirect_thunk.\reg), __stringify(jmp *%\reg), X86_FEATURE_IBRS_ATT +#else + jmp *\reg +#endif +.endm + +.macro CALL_THUNK reg:req +#ifdef RETPOLINE + ALTERNATIVE __stringify(call __x86.indirect_thunk.\reg), __stringify(call *%\reg), X86_FEATURE_IBRS_ATT +#else + jmp *\reg +#endif +.endm + #endif /* __ASSEMBLY__ */ #endif /* _ASM_X86_SPEC_CTRL_H */ diff --git a/arch/x86/kernel/ftrace_32.S b/arch/x86/kernel/ftrace_32.S index b6c6468..e89c6be 100644 --- a/arch/x86/kernel/ftrace_32.S +++ b/arch/x86/kernel/ftrace_32.S @@ -8,6 +8,7 @@ #include <asm/segment.h> #include <asm/export.h> #include <asm/ftrace.h> +#include <asm/spec_ctrl.h> #ifdef CC_USING_FENTRY # define function_hook __fentry__ @@ -197,7 +198,8 @@ ftrace_stub: movl 0x4(%ebp), %edx subl $MCOUNT_INSN_SIZE, %eax - call *ftrace_trace_function + movl ftrace_trace_function, %ecx + CALL_THUNK ecx popl %edx popl %ecx @@ -241,5 +243,5 @@ return_to_handler: movl %eax, %ecx popl %edx popl %eax - jmp *%ecx + JMP_THUNK ecx #endif diff --git a/arch/x86/kernel/ftrace_64.S b/arch/x86/kernel/ftrace_64.S index c832291..6c97463 100644 --- a/arch/x86/kernel/ftrace_64.S +++ b/arch/x86/kernel/ftrace_64.S @@ -7,6 +7,7 @@ #include <asm/ptrace.h> #include <asm/ftrace.h> #include <asm/export.h> +#include <asm/spec_ctrl.h> .code64 @@ -286,8 +287,8 @@ trace: * ip and parent ip are used and the list function is called when * function tracing is enabled. */ - call *ftrace_trace_function - + movq ftrace_trace_function, %r8 + CALL_THUNK r8 restore_mcount_regs jmp fgraph_trace @@ -329,5 +330,5 @@ GLOBAL(return_to_handler) movq 8(%rsp), %rdx movq (%rsp), %rax addq $24, %rsp - jmp *%rdi + JMP_THUNK rdi #endif diff --git a/arch/x86/lib/checksum_32.S b/arch/x86/lib/checksum_32.S index 4d34bb5..86df74c 100644 --- a/arch/x86/lib/checksum_32.S +++ b/arch/x86/lib/checksum_32.S @@ -29,7 +29,8 @@ #include <asm/errno.h> #include <asm/asm.h> #include <asm/export.h> - +#include <asm/spec_ctrl.h> + /* * computes a partial checksum, e.g. for TCP/UDP fragments */ @@ -156,7 +157,7 @@ ENTRY(csum_partial) negl %ebx lea 45f(%ebx,%ebx,2), %ebx testl %esi, %esi - jmp *%ebx + JMP_THUNK ebx # Handle 2-byte-aligned regions 20: addw (%esi), %ax -- 2.7.4 From 0441f6cf7f593502ecfec0da954ef3e15cae7b70 Mon Sep 17 00:00:00 2001 From: David Woodhouse <dwmw2@infradead.org> Date: Sat, 23 Dec 2017 02:11:18 +0000 Subject: [PATCH 3/8] x86: Use retpoline for calls in inline asm
Fix up PV operations and hypercalls to avoid indirect calls.
There is perhaps scope for optimising the pvops case. For ops which aren't going to be changed at runtime (all of them?), we could just patch the code to make it use *direct* jumps instead of indirect.
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> --- arch/x86/include/asm/mshyperv.h | 18 ++++++++++-------- arch/x86/include/asm/paravirt_types.h | 15 ++++++++++++--- arch/x86/include/asm/spec_ctrl.h | 13 +++++++++++++ arch/x86/include/asm/xen/hypercall.h | 5 +++-- 4 files changed, 38 insertions(+), 13 deletions(-)
diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h index 581bb54..ca7463b 100644 --- a/arch/x86/include/asm/mshyperv.h +++ b/arch/x86/include/asm/mshyperv.h @@ -7,6 +7,7 @@ #include <linux/nmi.h> #include <asm/io.h> #include <asm/hyperv.h> +#include <asm/spec_ctrl.h> /* * The below CPUID leaves are present if VersionAndFeatures.HypervisorPresent @@ -186,10 +187,11 @@ static inline u64 hv_do_hypercall(u64 control, void *input, void *output) return U64_MAX; __asm__ __volatile__("mov %4, %%r8\n" - "call *%5" + CALL_THUNK : "=a" (hv_status), ASM_CALL_CONSTRAINT, "+c" (control), "+d" (input_address) - : "r" (output_address), "m" (hv_hypercall_pg) + : "r" (output_address), + THUNK_TARGET(hv_hypercall_pg) : "cc", "memory", "r8", "r9", "r10", "r11"); #else u32 input_address_hi = upper_32_bits(input_address); @@ -200,13 +202,13 @@ static inline u64 hv_do_hypercall(u64 control, void *input, void *output) if (!hv_hypercall_pg) return U64_MAX; - __asm__ __volatile__("call *%7" + __asm__ __volatile__(CALL_THUNK : "=A" (hv_status), "+c" (input_address_lo), ASM_CALL_CONSTRAINT : "A" (control), "b" (input_address_hi), "D"(output_address_hi), "S"(output_address_lo), - "m" (hv_hypercall_pg) + THUNK_TARGET(hv_hypercall_pg) : "cc", "memory"); #endif /* !x86_64 */ return hv_status; @@ -227,10 +229,10 @@ static inline u64 hv_do_fast_hypercall8(u16 code, u64 input1) #ifdef CONFIG_X86_64 { - __asm__ __volatile__("call *%4" + __asm__ __volatile__(CALL_THUNK : "=a" (hv_status), ASM_CALL_CONSTRAINT, "+c" (control), "+d" (input1) - : "m" (hv_hypercall_pg) + : THUNK_TARGET(hv_hypercall_pg) : "cc", "r8", "r9", "r10", "r11"); } #else @@ -238,13 +240,13 @@ static inline u64 hv_do_fast_hypercall8(u16 code, u64 input1) u32 input1_hi = upper_32_bits(input1); u32 input1_lo = lower_32_bits(input1); - __asm__ __volatile__ ("call *%5" + __asm__ __volatile__ (CALL_THUNK : "=A"(hv_status), "+c"(input1_lo), ASM_CALL_CONSTRAINT : "A" (control), "b" (input1_hi), - "m" (hv_hypercall_pg) + THUNK_TARGET(hv_hypercall_pg) : "cc", "edi", "esi"); } #endif diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h index 6ec54d0..dfff11e 100644 --- a/arch/x86/include/asm/paravirt_types.h +++ b/arch/x86/include/asm/paravirt_types.h @@ -336,11 +336,17 @@ extern struct pv_lock_ops pv_lock_ops; #define PARAVIRT_PATCH(x) \ (offsetof(struct paravirt_patch_template, x) / sizeof(void *)) +#define paravirt_clobber(clobber) \ + [paravirt_clobber] "i" (clobber) +#ifdef RETPOLINE +#define paravirt_type(op) \ + [paravirt_typenum] "i" (PARAVIRT_PATCH(op)), \ + [paravirt_opptr] "r" ((op)) +#else #define paravirt_type(op) \ [paravirt_typenum] "i" (PARAVIRT_PATCH(op)), \ [paravirt_opptr] "i" (&(op)) -#define paravirt_clobber(clobber) \ - [paravirt_clobber] "i" (clobber) +#endif /* * Generate some code, and mark it as patchable by the @@ -392,8 +398,11 @@ int paravirt_disable_iospace(void); * offset into the paravirt_patch_template structure, and can therefore be * freely converted back into a structure offset. */ +#ifdef RETPOLINE +#define PARAVIRT_CALL "call __x86.indirect_thunk.%V[paravirt_opptr];" +#else #define PARAVIRT_CALL "call *%c[paravirt_opptr];" - +#endif /* * These macros are intended to wrap calls through one of the paravirt * ops structs, so that they can be later identified and patched at diff --git a/arch/x86/include/asm/spec_ctrl.h b/arch/x86/include/asm/spec_ctrl.h index 59b664e..58a49a2 100644 --- a/arch/x86/include/asm/spec_ctrl.h +++ b/arch/x86/include/asm/spec_ctrl.h @@ -196,5 +196,18 @@ ALTERNATIVE __stringify(__ASM_STUFF_RSB), "", X86_FEATURE_SMEP #endif .endm +#else /* __ASSEMBLY__ */ + +#ifdef RETPOLINE +#define CALL_THUNK ALTERNATIVE( \ + "call __x86.indirect_thunk.%V[thunk_target]", \ + "call *%[thunk_target]", X86_FEATURE_IBRS_ATT) +#define THUNK_TARGET(addr) [thunk_target] "r" (addr) +#else +#define CALL_THUNK "call *%[thunk_target]" +#define THUNK_TARGET(addr) [thunk_target] "rm" (addr) +#endif + #endif /* __ASSEMBLY__ */ + #endif /* _ASM_X86_SPEC_CTRL_H */ diff --git a/arch/x86/include/asm/xen/hypercall.h b/arch/x86/include/asm/xen/hypercall.h index 7cb282e..90df832 100644 --- a/arch/x86/include/asm/xen/hypercall.h +++ b/arch/x86/include/asm/xen/hypercall.h @@ -44,6 +44,7 @@ #include <asm/page.h> #include <asm/pgtable.h> #include <asm/smap.h> +#include <asm/spec_ctrl.h> #include <xen/interface/xen.h> #include <xen/interface/sched.h> @@ -217,9 +218,9 @@ privcmd_call(unsigned call, __HYPERCALL_5ARG(a1, a2, a3, a4, a5); stac(); - asm volatile("call *%[call]" + asm volatile(CALL_THUNK : __HYPERCALL_5PARAM - : [call] "a" (&hypercall_page[call]) + : [thunk_target] "a" (&hypercall_page[call]) : __HYPERCALL_CLOBBER5); clac(); -- 2.7.4 [unhandled content-type:application/x-pkcs7-signature] | |