* [PATCH v8 00/17] x86: Mitigations for SP2/CVE-2017-5715/Branch Target Injection
@ 2018-01-12 18:00 Andrew Cooper
2018-01-12 18:00 ` [PATCH v8 01/17] x86: Support compiling with indirect branch thunks Andrew Cooper
` (16 more replies)
0 siblings, 17 replies; 55+ messages in thread
From: Andrew Cooper @ 2018-01-12 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
This series is availabe in git form from:
http://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/sp2-mitigations-v8
In addition to this software series, you will need the following:
1) A compiler which understands -mindirect-branch=thunk-external and
-mindirect-branch-register. A GCC patch series implementing this should
be available imminently. In the meantime, a development branch can be
obtained from:
https://github.com/hjl-tools/gcc/commits/hjl/indirect/gcc-7-branch/master
2) New microcode from Intel and AMD. These provide new MSRs for Xen to use,
and virtualise for guest kernels to use.
There are some limitations, even with the work presented here.
1) vCPU-to-vCPU SP2 attacks can only be mitigated at the hypervisor level
with IBPB support, which for internal pipeline reasons, we do not expect
to be made available on older processors. For now, I will leave these
details to the hardware vendors.
2) Hardware lacking SMEP is in a worse position than hardware with SMEP. If
you have SMEP (Intel IvyBridge and later, Some AMD Fam16h and all Fam17h
and later), make absolutely sure it is enabled in the BIOS and working.
3) On hardware lacking SMEP support, it is still an open question how to
protect against RSB-to-SMM speculation. Native operating systems can fix
this by prohibiting userspace from mmap()'ing addresses which alias the
SMM range, but Xen has no feasible way of enforcing this restriction on
PV guests, even if we could tolerate the ABI breakage. (However, see the
forthcoming SP3 mitigation series for alternatives for un trusted PV
guests).
~Andrew
Changes from v7:
* Spelling fixes
* Rebase over upstream fixes to IO emulation handling
* Tweak the RSB overwriting algorithm to be smaller
Andrew Cooper (17):
x86: Support compiling with indirect branch thunks
x86: Support indirect thunks from assembly code
x86/boot: Report details of speculative mitigations
x86/amd: Try to set lfence as being Dispatch Serialising
x86: Introduce alternative indirect thunks
x86/feature: Definitions for Indirect Branch Controls
x86/cmdline: Introduce a command line option to disable IBRS/IBPB,
STIBP and IBPB
x86/msr: Emulation of MSR_{SPEC_CTRL,PRED_CMD} for guests
x86/migrate: Move MSR_SPEC_CTRL on migrate
x86/hvm: Permit guests direct access to MSR_{SPEC_CTRL,PRED_CMD}
x86: Protect unaware domains from meddling hyperthreads
x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point
x86/boot: Calculate the most appropriate BTI mitigation to use
x86/entry: Clobber the Return Stack Buffer/Return Address Stack on
entry to Xen
x86/ctxt: Issue a speculation barrier between vcpu contexts
x86/cpuid: Offer Indirect Branch Controls to guests
x86/idle: Clear SPEC_CTRL while idle
docs/misc/xen-command-line.markdown | 39 ++++
tools/libxc/xc_cpuid_x86.c | 4 +-
tools/libxl/libxl_cpuid.c | 3 +
tools/misc/xen-cpuid.c | 12 +-
xen/Rules.mk | 4 +-
xen/arch/x86/Makefile | 2 +
xen/arch/x86/Rules.mk | 13 ++
xen/arch/x86/acpi/cpu_idle.c | 21 ++
xen/arch/x86/boot/trampoline.S | 24 ++-
xen/arch/x86/cpu/amd.c | 35 +++-
xen/arch/x86/cpu/mwait-idle.c | 7 +
xen/arch/x86/cpuid.c | 43 ++++
xen/arch/x86/domain.c | 19 ++
xen/arch/x86/domctl.c | 21 ++
xen/arch/x86/extable.c | 4 +-
xen/arch/x86/hvm/hvm.c | 2 +
xen/arch/x86/hvm/svm/entry.S | 8 +-
xen/arch/x86/hvm/svm/svm.c | 5 +
xen/arch/x86/hvm/vmx/entry.S | 11 ++
xen/arch/x86/hvm/vmx/vmx.c | 18 ++
xen/arch/x86/indirect-thunk.S | 49 +++++
xen/arch/x86/msr.c | 37 ++++
xen/arch/x86/pv/emul-priv-op.c | 41 ++--
xen/arch/x86/setup.c | 4 +
xen/arch/x86/smpboot.c | 2 +
xen/arch/x86/spec_ctrl.c | 295 ++++++++++++++++++++++++++++
xen/arch/x86/x86_64/asm-offsets.c | 6 +
xen/arch/x86/x86_64/compat/entry.S | 12 ++
xen/arch/x86/x86_64/entry.S | 39 +++-
xen/arch/x86/x86_emulate/x86_emulate.c | 4 +-
xen/arch/x86/xen.lds.S | 1 +
xen/common/kernel.c | 23 +++
xen/common/wait.c | 8 +-
xen/include/asm-x86/asm_defns.h | 11 ++
xen/include/asm-x86/cpufeature.h | 4 +
xen/include/asm-x86/cpufeatures.h | 8 +
xen/include/asm-x86/current.h | 6 +
xen/include/asm-x86/indirect_thunk_asm.h | 41 ++++
xen/include/asm-x86/msr-index.h | 9 +
xen/include/asm-x86/msr.h | 15 ++
xen/include/asm-x86/nops.h | 7 +
xen/include/asm-x86/spec_ctrl.h | 101 ++++++++++
xen/include/asm-x86/spec_ctrl_asm.h | 270 +++++++++++++++++++++++++
xen/include/public/arch-x86/cpufeatureset.h | 3 +
xen/include/xen/lib.h | 7 +
xen/tools/gen-cpuid.py | 5 +
46 files changed, 1273 insertions(+), 30 deletions(-)
create mode 100644 xen/arch/x86/indirect-thunk.S
create mode 100644 xen/arch/x86/spec_ctrl.c
create mode 100644 xen/include/asm-x86/indirect_thunk_asm.h
create mode 100644 xen/include/asm-x86/spec_ctrl.h
create mode 100644 xen/include/asm-x86/spec_ctrl_asm.h
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v8 01/17] x86: Support compiling with indirect branch thunks
2018-01-12 18:00 [PATCH v8 00/17] x86: Mitigations for SP2/CVE-2017-5715/Branch Target Injection Andrew Cooper
@ 2018-01-12 18:00 ` Andrew Cooper
2018-01-14 19:48 ` David Woodhouse
2018-01-15 10:14 ` Jan Beulich
2018-01-12 18:00 ` [PATCH v8 02/17] x86: Support indirect thunks from assembly code Andrew Cooper
` (15 subsequent siblings)
16 siblings, 2 replies; 55+ messages in thread
From: Andrew Cooper @ 2018-01-12 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
Use -mindirect-branch=thunk-extern/-mindirect-branch-register when available.
To begin with, use the retpoline thunk. Later work will add alternative
thunks which can be selected at boot time.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v7:
* Alter thunk symbols to match the latested GCC changes proposed upstream.
* Rename indirect_thunk.S to indirect-thunk.S
* Use a single parameter now that we don't need to split reg and name.
* Sort registers by encoding.
v8:
* Add a header comment and .file directive for indirect-thunk.S
---
xen/arch/x86/Makefile | 1 +
xen/arch/x86/Rules.mk | 7 +++++++
xen/arch/x86/indirect-thunk.S | 38 ++++++++++++++++++++++++++++++++++++++
xen/arch/x86/xen.lds.S | 1 +
4 files changed, 47 insertions(+)
create mode 100644 xen/arch/x86/indirect-thunk.S
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index d5d58a2..b334366 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -36,6 +36,7 @@ obj-y += io_apic.o
obj-$(CONFIG_LIVEPATCH) += alternative.o livepatch.o
obj-y += msi.o
obj-y += msr.o
+obj-$(CONFIG_INDIRECT_THUNK) += indirect-thunk.o
obj-y += ioport_emulate.o
obj-y += irq.o
obj-$(CONFIG_KEXEC) += machine_kexec.o
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index 568657e..abcc4d4 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -30,3 +30,10 @@ CFLAGS += -fno-asynchronous-unwind-tables
ifneq ($(call cc-option,$(CC),-fvisibility=hidden,n),n)
CFLAGS += -DGCC_HAS_VISIBILITY_ATTRIBUTE
endif
+
+# Compile with thunk-extern, indirect-branch-register if avaiable.
+ifneq ($(call cc-option,$(CC),-mindirect-branch-register,n),n)
+CFLAGS += -mindirect-branch=thunk-extern -mindirect-branch-register
+CFLAGS += -DCONFIG_INDIRECT_THUNK
+export CONFIG_INDIRECT_THUNK=y
+endif
diff --git a/xen/arch/x86/indirect-thunk.S b/xen/arch/x86/indirect-thunk.S
new file mode 100644
index 0000000..3eaf505
--- /dev/null
+++ b/xen/arch/x86/indirect-thunk.S
@@ -0,0 +1,38 @@
+/*
+ * Implement __x86_indirect_thunk_* symbols for use with compatbile compilers
+ * and the -mindirect-branch=thunk-extern -mindirect-branch-register options.
+ *
+ * Copyright (c) 2017-2018 Citrix Systems Ltd.
+ *
+ * This source code is licensed under the GNU General Public License,
+ * Version 2. See the file COPYING for more details.
+ */
+ .file __FILE__
+
+#include <asm/asm_defns.h>
+
+.macro IND_THUNK_RETPOLINE reg:req
+ call 2f
+1:
+ lfence
+ jmp 1b
+2:
+ mov %\reg, (%rsp)
+ ret
+.endm
+
+/*
+ * Build the __x86_indirect_thunk_* symbols. Currently implement the
+ * retpoline thunk only.
+ */
+.macro GEN_INDIRECT_THUNK reg:req
+ .section .text.__x86_indirect_thunk_\reg, "ax", @progbits
+
+ENTRY(__x86_indirect_thunk_\reg)
+ IND_THUNK_RETPOLINE \reg
+.endm
+
+/* Instantiate GEN_INDIRECT_THUNK for each register except %rsp. */
+.irp reg, ax, cx, dx, bx, bp, si, di, 8, 9, 10, 11, 12, 13, 14, 15
+ GEN_INDIRECT_THUNK reg=r\reg
+.endr
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index d5e8821..d3c984a 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -59,6 +59,7 @@ SECTIONS
.text : {
_stext = .; /* Text and read-only data */
*(.text)
+ *(.text.__x86_indirect_thunk_*)
*(.text.cold)
*(.text.unlikely)
*(.fixup)
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v8 02/17] x86: Support indirect thunks from assembly code
2018-01-12 18:00 [PATCH v8 00/17] x86: Mitigations for SP2/CVE-2017-5715/Branch Target Injection Andrew Cooper
2018-01-12 18:00 ` [PATCH v8 01/17] x86: Support compiling with indirect branch thunks Andrew Cooper
@ 2018-01-12 18:00 ` Andrew Cooper
2018-01-15 10:28 ` Jan Beulich
2018-02-04 10:57 ` David Woodhouse
2018-01-12 18:00 ` [PATCH v8 03/17] x86/boot: Report details of speculative mitigations Andrew Cooper
` (14 subsequent siblings)
16 siblings, 2 replies; 55+ messages in thread
From: Andrew Cooper @ 2018-01-12 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
Introduce INDIRECT_CALL and INDIRECT_JMP which either degrade to a normal
indirect branch, or dispatch to the __x86_indirect_thunk_* symbols.
Update all the manual indirect branches in to use the new thunks. The
indirect branches in the early boot and kexec path are left intact as we can't
use the compiled-in thunks at those points.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v7:
* Protect the jmp from the trampoline into the high mappings on the AP boot
path, and the hand-crafted indirect jump in the PV IO emulation stubs.
* Rebase over compiler changes
* Spelling fixes
* Fix clang build
v8:
* Spelling/grammar fixes
---
xen/Rules.mk | 4 ++--
xen/arch/x86/Rules.mk | 6 +++++
xen/arch/x86/boot/trampoline.S | 24 +++++++++++++++++--
xen/arch/x86/extable.c | 4 ++--
xen/arch/x86/pv/emul-priv-op.c | 41 ++++++++++++++++++++++----------
xen/arch/x86/x86_64/entry.S | 6 +++--
xen/arch/x86/x86_emulate/x86_emulate.c | 4 ++--
xen/common/wait.c | 8 ++++---
xen/include/asm-x86/asm_defns.h | 8 +++++++
xen/include/asm-x86/indirect_thunk_asm.h | 41 ++++++++++++++++++++++++++++++++
10 files changed, 121 insertions(+), 25 deletions(-)
create mode 100644 xen/include/asm-x86/indirect_thunk_asm.h
diff --git a/xen/Rules.mk b/xen/Rules.mk
index 2659f8a..3cf4075 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -66,8 +66,8 @@ endif
AFLAGS-y += -D__ASSEMBLY__
-# Clang's built-in assembler can't handle .code16/.code32/.code64 yet
-AFLAGS-$(clang) += -no-integrated-as
+# Clang's built-in assembler can't handle embedded .include's
+CFLAGS-$(clang) += -no-integrated-as
ALL_OBJS := $(ALL_OBJS-y)
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index abcc4d4..70e9d8f 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -37,3 +37,9 @@ CFLAGS += -mindirect-branch=thunk-extern -mindirect-branch-register
CFLAGS += -DCONFIG_INDIRECT_THUNK
export CONFIG_INDIRECT_THUNK=y
endif
+
+# Set up the assembler include path properly for older GCC toolchains. Clang
+# objects to the agument being passed however.
+ifneq ($(clang),y)
+CFLAGS += -Wa,-I$(BASEDIR)/include
+endif
diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index 4d640f3..f70d913 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -153,8 +153,28 @@ trampoline_protmode_entry:
.code64
start64:
/* Jump to high mappings. */
- movabs $__high_start,%rax
- jmpq *%rax
+ movabs $__high_start, %rdi
+
+#ifdef CONFIG_INDIRECT_THUNK
+ /*
+ * If booting virtualised, or hot-onlining a CPU, sibling threads can
+ * attempt Branch Target Injection against this jmp.
+ *
+ * We've got no usable stack so can't use a RETPOLINE thunk, and are
+ * further than disp32 from the high mappings so couldn't use
+ * JUMP_THUNK even if it was a non-RETPOLINE thunk. Furthermore, an
+ * LFENCE isn't necessarily safe to use at this point.
+ *
+ * As this isn't a hotpath, use a fully serialising event to reduce
+ * the speculation window as much as possible. %ebx needs preserving
+ * for __high_start.
+ */
+ mov %ebx, %esi
+ cpuid
+ mov %esi, %ebx
+#endif
+
+ jmpq *%rdi
#include "wakeup.S"
diff --git a/xen/arch/x86/extable.c b/xen/arch/x86/extable.c
index 6fffe05..72f30d9 100644
--- a/xen/arch/x86/extable.c
+++ b/xen/arch/x86/extable.c
@@ -158,7 +158,7 @@ static int __init stub_selftest(void)
memcpy(ptr, tests[i].opc, ARRAY_SIZE(tests[i].opc));
unmap_domain_page(ptr);
- asm volatile ( "call *%[stb]\n"
+ asm volatile ( "INDIRECT_CALL %[stb]\n"
".Lret%=:\n\t"
".pushsection .fixup,\"ax\"\n"
".Lfix%=:\n\t"
@@ -167,7 +167,7 @@ static int __init stub_selftest(void)
".popsection\n\t"
_ASM_EXTABLE(.Lret%=, .Lfix%=)
: [exn] "+m" (res)
- : [stb] "rm" (addr), "a" (tests[i].rax));
+ : [stb] "r" (addr), "a" (tests[i].rax));
ASSERT(res == tests[i].res.raw);
}
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index ebd6dc1..2b2a58b 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -73,46 +73,63 @@ void (*pv_post_outb_hook)(unsigned int port, u8 value);
typedef void io_emul_stub_t(struct cpu_user_regs *);
+void __x86_indirect_thunk_rcx(void);
+
static io_emul_stub_t *io_emul_stub_setup(struct priv_op_ctxt *ctxt, u8 opcode,
unsigned int port, unsigned int bytes)
{
+ struct stubs *this_stubs = &this_cpu(stubs);
+ unsigned long stub_va = this_stubs->addr + STUB_BUF_SIZE / 2;
bool use_quirk_stub = false;
if ( !ctxt->io_emul_stub )
- ctxt->io_emul_stub = map_domain_page(_mfn(this_cpu(stubs.mfn))) +
- (this_cpu(stubs.addr) &
- ~PAGE_MASK) +
- STUB_BUF_SIZE / 2;
+ ctxt->io_emul_stub =
+ map_domain_page(_mfn(this_stubs->mfn)) + (stub_va & ~PAGE_MASK);
/* movq $host_to_guest_gpr_switch,%rcx */
ctxt->io_emul_stub[0] = 0x48;
ctxt->io_emul_stub[1] = 0xb9;
*(void **)&ctxt->io_emul_stub[2] = (void *)host_to_guest_gpr_switch;
+
+#ifdef CONFIG_INDIRECT_THUNK
+ /* callq __x86_indirect_thunk_rcx */
+ ctxt->io_emul_stub[10] = 0xe8;
+ *(int32_t *)&ctxt->io_emul_stub[11] =
+ (unsigned long)__x86_indirect_thunk_rcx - (stub_va + 11 + 4);
+
+#else
/* callq *%rcx */
ctxt->io_emul_stub[10] = 0xff;
ctxt->io_emul_stub[11] = 0xd1;
+ /*
+ * 3 bytes of P6_NOPS.
+ * TODO: untangle ideal_nops from init/livepatch Kconfig options.
+ */
+ memcpy(&ctxt->io_emul_stub[12], "\x0f\x1f\x00", 3);
+#endif
+
if ( unlikely(ioemul_handle_quirk) )
- use_quirk_stub = ioemul_handle_quirk(opcode, &ctxt->io_emul_stub[12],
+ use_quirk_stub = ioemul_handle_quirk(opcode, &ctxt->io_emul_stub[15],
ctxt->ctxt.regs);
if ( !use_quirk_stub )
{
/* data16 or nop */
- ctxt->io_emul_stub[12] = (bytes != 2) ? 0x90 : 0x66;
+ ctxt->io_emul_stub[15] = (bytes != 2) ? 0x90 : 0x66;
/* <io-access opcode> */
- ctxt->io_emul_stub[13] = opcode;
+ ctxt->io_emul_stub[16] = opcode;
/* imm8 or nop */
- ctxt->io_emul_stub[14] = !(opcode & 8) ? port : 0x90;
+ ctxt->io_emul_stub[17] = !(opcode & 8) ? port : 0x90;
/* ret (jumps to guest_to_host_gpr_switch) */
- ctxt->io_emul_stub[15] = 0xc3;
+ ctxt->io_emul_stub[18] = 0xc3;
}
- BUILD_BUG_ON(STUB_BUF_SIZE / 2 < MAX(16, /* Default emul stub */
- 12 + IOEMUL_QUIRK_STUB_BYTES));
+ BUILD_BUG_ON(STUB_BUF_SIZE / 2 < MAX(19, /* Default emul stub */
+ 15 + IOEMUL_QUIRK_STUB_BYTES));
/* Handy function-typed pointer to the stub. */
- return (void *)(this_cpu(stubs.addr) + STUB_BUF_SIZE / 2);
+ return (void *)stub_va;
}
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 1dd9ccf..cbd73f6 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -474,7 +474,8 @@ handle_exception_saved:
movzbl UREGS_entry_vector(%rsp),%eax
leaq exception_table(%rip),%rdx
PERFC_INCR(exceptions, %rax, %rbx)
- callq *(%rdx,%rax,8)
+ mov (%rdx, %rax, 8), %rdx
+ INDIRECT_CALL %rdx
testb $3,UREGS_cs(%rsp)
jz restore_all_xen
leaq VCPU_trap_bounce(%rbx),%rdx
@@ -615,7 +616,8 @@ handle_ist_exception:
1: movq %rsp,%rdi
movzbl UREGS_entry_vector(%rsp),%eax
leaq exception_table(%rip),%rdx
- callq *(%rdx,%rax,8)
+ mov (%rdx, %rax, 8), %rdx
+ INDIRECT_CALL %rdx
cmpb $TRAP_nmi,UREGS_entry_vector(%rsp)
jne ret_from_intr
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 820495f..ff0a003 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -867,7 +867,7 @@ static inline int mkec(uint8_t e, int32_t ec, ...)
#ifdef __XEN__
# define invoke_stub(pre, post, constraints...) do { \
union stub_exception_token res_ = { .raw = ~0 }; \
- asm volatile ( pre "\n\tcall *%[stub]\n\t" post "\n" \
+ asm volatile ( pre "\n\tINDIRECT_CALL %[stub]\n\t" post "\n" \
".Lret%=:\n\t" \
".pushsection .fixup,\"ax\"\n" \
".Lfix%=:\n\t" \
@@ -876,7 +876,7 @@ static inline int mkec(uint8_t e, int32_t ec, ...)
".popsection\n\t" \
_ASM_EXTABLE(.Lret%=, .Lfix%=) \
: [exn] "+g" (res_), constraints, \
- [stub] "rm" (stub.func), \
+ [stub] "r" (stub.func), \
"m" (*(uint8_t(*)[MAX_INST_LEN + 1])stub.ptr) ); \
if ( unlikely(~res_.raw) ) \
{ \
diff --git a/xen/common/wait.c b/xen/common/wait.c
index 3d3d9fe..a57bc10 100644
--- a/xen/common/wait.c
+++ b/xen/common/wait.c
@@ -203,12 +203,14 @@ void check_wakeup_from_wait(void)
/*
* Hand-rolled longjmp(). Returns to the pointer on the top of
- * wqv->stack, and lands on a `rep movs` instruction.
+ * wqv->stack, and lands on a `rep movs` instruction. All other GPRs are
+ * restored from the stack, so are available for use here.
*/
asm volatile (
- "mov %1,%%"__OP"sp; jmp *(%0)"
+ "mov %1,%%"__OP"sp; INDIRECT_JMP %[ip]"
: : "S" (wqv->stack), "D" (wqv->esp),
- "c" ((char *)get_cpu_info() - (char *)wqv->esp)
+ "c" ((char *)get_cpu_info() - (char *)wqv->esp),
+ [ip] "r" (*(unsigned long *)wqv->stack)
: "memory" );
unreachable();
}
diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index 7e8838e..40b0250 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -13,6 +13,14 @@
#include <asm/cpufeature.h>
#include <asm/alternative.h>
+#ifdef __ASSEMBLY__
+# include <asm/indirect_thunk_asm.h>
+#else
+asm ( "\t.equ CONFIG_INDIRECT_THUNK, "
+ __stringify(IS_ENABLED(CONFIG_INDIRECT_THUNK)) );
+asm ( "\t.include \"asm/indirect_thunk_asm.h\"" );
+#endif
+
#ifndef __ASSEMBLY__
void ret_from_intr(void);
#endif
diff --git a/xen/include/asm-x86/indirect_thunk_asm.h b/xen/include/asm-x86/indirect_thunk_asm.h
new file mode 100644
index 0000000..96bcc25
--- /dev/null
+++ b/xen/include/asm-x86/indirect_thunk_asm.h
@@ -0,0 +1,41 @@
+/*
+ * Warning! This file is included at an assembler level for .c files, causing
+ * usual #ifdef'ary to turn into comments.
+ */
+
+.macro INDIRECT_BRANCH insn:req arg:req
+/*
+ * Create an indirect branch. insn is one of call/jmp, arg is a single
+ * register.
+ *
+ * With no compiler support, this degrades into a plain indirect call/jmp.
+ * With compiler support, dispatch to the correct __x86_indirect_thunk_*
+ */
+ .if CONFIG_INDIRECT_THUNK == 1
+
+ $done = 0
+ .irp reg, ax, cx, dx, bx, bp, si, di, 8, 9, 10, 11, 12, 13, 14, 15
+ .ifeqs "\arg", "%r\reg"
+ \insn __x86_indirect_thunk_r\reg
+ $done = 1
+ .exitm
+ .endif
+ .endr
+
+ .if $done != 1
+ .error "Bad register arg \arg"
+ .endif
+
+ .else
+ \insn *\arg
+ .endif
+.endm
+
+/* Convenience wrappers. */
+.macro INDIRECT_CALL arg:req
+ INDIRECT_BRANCH call \arg
+.endm
+
+.macro INDIRECT_JMP arg:req
+ INDIRECT_BRANCH jmp \arg
+.endm
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v8 03/17] x86/boot: Report details of speculative mitigations
2018-01-12 18:00 [PATCH v8 00/17] x86: Mitigations for SP2/CVE-2017-5715/Branch Target Injection Andrew Cooper
2018-01-12 18:00 ` [PATCH v8 01/17] x86: Support compiling with indirect branch thunks Andrew Cooper
2018-01-12 18:00 ` [PATCH v8 02/17] x86: Support indirect thunks from assembly code Andrew Cooper
@ 2018-01-12 18:00 ` Andrew Cooper
2018-01-12 18:00 ` [PATCH v8 04/17] x86/amd: Try to set lfence as being Dispatch Serialising Andrew Cooper
` (13 subsequent siblings)
16 siblings, 0 replies; 55+ messages in thread
From: Andrew Cooper @ 2018-01-12 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
Nothing very interesting at the moment, but the logic will grow as new
mitigations are added.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/Makefile | 1 +
xen/arch/x86/setup.c | 3 ++
xen/arch/x86/spec_ctrl.c | 75 +++++++++++++++++++++++++++++++++++++++++
xen/include/asm-x86/spec_ctrl.h | 35 +++++++++++++++++++
4 files changed, 114 insertions(+)
create mode 100644 xen/arch/x86/spec_ctrl.c
create mode 100644 xen/include/asm-x86/spec_ctrl.h
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index b334366..e8c4963 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -57,6 +57,7 @@ obj-y += setup.o
obj-y += shutdown.o
obj-y += smp.o
obj-y += smpboot.o
+obj-y += spec_ctrl.o
obj-y += srat.o
obj-y += string.o
obj-y += sysctl.o
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 65170fe..9426760 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -51,6 +51,7 @@
#include <asm/alternative.h>
#include <asm/mc146818rtc.h>
#include <asm/cpuid.h>
+#include <asm/spec_ctrl.h>
/* opt_nosmp: If true, secondary processors are ignored. */
static bool __initdata opt_nosmp;
@@ -1502,6 +1503,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
if ( cpu_has_fsgsbase )
set_in_cr4(X86_CR4_FSGSBASE);
+ init_speculation_mitigations();
+
init_idle_domain();
this_cpu(stubs.addr) = alloc_stub_page(smp_processor_id(),
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
new file mode 100644
index 0000000..256701a
--- /dev/null
+++ b/xen/arch/x86/spec_ctrl.c
@@ -0,0 +1,75 @@
+/******************************************************************************
+ * arch/x86/spec_ctrl.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Copyright (c) 2017-2018 Citrix Systems Ltd.
+ */
+#include <xen/init.h>
+#include <xen/lib.h>
+
+#include <asm/processor.h>
+#include <asm/spec_ctrl.h>
+
+enum ind_thunk {
+ THUNK_DEFAULT, /* Decide which thunk to use at boot time. */
+ THUNK_NONE, /* Missing compiler support for thunks. */
+
+ THUNK_RETPOLINE,
+};
+
+static void __init print_details(enum ind_thunk thunk)
+{
+ printk(XENLOG_DEBUG "Speculative mitigation facilities:\n");
+
+ /* Compiled-in support which pertains to BTI mitigations. */
+ if ( IS_ENABLED(CONFIG_INDIRECT_THUNK) )
+ printk(XENLOG_DEBUG " Compiled-in support: INDIRECT_THUNK\n");
+
+ printk(XENLOG_INFO
+ "BTI mitigations: Thunk %s\n",
+ thunk == THUNK_NONE ? "N/A" :
+ thunk == THUNK_RETPOLINE ? "RETPOLINE" : "?");
+}
+
+void __init init_speculation_mitigations(void)
+{
+ enum ind_thunk thunk = THUNK_DEFAULT;
+
+ /*
+ * Supplimentary minor adjustments. Without compiler support, there are
+ * no thunks.
+ */
+ if ( !IS_ENABLED(CONFIG_INDIRECT_THUNK) )
+ thunk = THUNK_NONE;
+
+ /*
+ * If there are still no thunk preferences, the compiled default is
+ * actually retpoline, and it is better than nothing.
+ */
+ if ( thunk == THUNK_DEFAULT )
+ thunk = THUNK_RETPOLINE;
+
+ print_details(thunk);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-x86/spec_ctrl.h b/xen/include/asm-x86/spec_ctrl.h
new file mode 100644
index 0000000..e088a55
--- /dev/null
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -0,0 +1,35 @@
+/******************************************************************************
+ * include/asm-x86/spec_ctrl.h
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Copyright (c) 2017-2018 Citrix Systems Ltd.
+ */
+
+#ifndef __X86_SPEC_CTRL_H__
+#define __X86_SPEC_CTRL_H__
+
+void init_speculation_mitigations(void);
+
+#endif /* !__X86_SPEC_CTRL_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v8 04/17] x86/amd: Try to set lfence as being Dispatch Serialising
2018-01-12 18:00 [PATCH v8 00/17] x86: Mitigations for SP2/CVE-2017-5715/Branch Target Injection Andrew Cooper
` (2 preceding siblings ...)
2018-01-12 18:00 ` [PATCH v8 03/17] x86/boot: Report details of speculative mitigations Andrew Cooper
@ 2018-01-12 18:00 ` Andrew Cooper
2018-01-12 18:00 ` [PATCH v8 05/17] x86: Introduce alternative indirect thunks Andrew Cooper
` (12 subsequent siblings)
16 siblings, 0 replies; 55+ messages in thread
From: Andrew Cooper @ 2018-01-12 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
This property is required for the AMD's recommended mitigation for Branch
Target Injection, but Xen needs to cope with being unable to detect or modify
the MSR.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/cpu/amd.c | 35 ++++++++++++++++++++++++++++++++++-
xen/include/asm-x86/cpufeature.h | 1 +
xen/include/asm-x86/cpufeatures.h | 1 +
xen/include/asm-x86/msr-index.h | 1 +
4 files changed, 37 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
index 5f36ac7..40c0bac 100644
--- a/xen/arch/x86/cpu/amd.c
+++ b/xen/arch/x86/cpu/amd.c
@@ -558,8 +558,41 @@ static void init_amd(struct cpuinfo_x86 *c)
wrmsr_amd_safe(0xc001100d, l, h & ~1);
}
+ /*
+ * Attempt to set lfence to be Dispatch Serialising. This MSR almost
+ * certainly isn't virtualised (and Xen at least will leak the real
+ * value in but silently discard writes), as well as being per-core
+ * rather than per-thread, so do a full safe read/write/readback cycle
+ * in the worst case.
+ */
+ if (c->x86 == 0x0f || c->x86 == 0x11)
+ /* Always dispatch serialising on this hardare. */
+ __set_bit(X86_FEATURE_LFENCE_DISPATCH, c->x86_capability);
+ else /* Implicily "== 0x10 || >= 0x12" by being 64bit. */ {
+ if (rdmsr_safe(MSR_AMD64_DE_CFG, value))
+ /* Unable to read. Assume the safer default. */
+ __clear_bit(X86_FEATURE_LFENCE_DISPATCH,
+ c->x86_capability);
+ else if (value & AMD64_DE_CFG_LFENCE_SERIALISE)
+ /* Already dispatch serialising. */
+ __set_bit(X86_FEATURE_LFENCE_DISPATCH,
+ c->x86_capability);
+ else if (wrmsr_safe(MSR_AMD64_DE_CFG,
+ value | AMD64_DE_CFG_LFENCE_SERIALISE) ||
+ rdmsr_safe(MSR_AMD64_DE_CFG, value) ||
+ !(value & AMD64_DE_CFG_LFENCE_SERIALISE))
+ /* Attempt to set failed. Assume the safer default. */
+ __clear_bit(X86_FEATURE_LFENCE_DISPATCH,
+ c->x86_capability);
+ else
+ /* Successfully enabled! */
+ __set_bit(X86_FEATURE_LFENCE_DISPATCH,
+ c->x86_capability);
+ }
+
/* MFENCE stops RDTSC speculation */
- __set_bit(X86_FEATURE_MFENCE_RDTSC, c->x86_capability);
+ if (!cpu_has_lfence_dispatch)
+ __set_bit(X86_FEATURE_MFENCE_RDTSC, c->x86_capability);
switch(c->x86)
{
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index 84cc51d..adc333f 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -104,6 +104,7 @@
#define cpu_has_arch_perfmon boot_cpu_has(X86_FEATURE_ARCH_PERFMON)
#define cpu_has_cpuid_faulting boot_cpu_has(X86_FEATURE_CPUID_FAULTING)
#define cpu_has_aperfmperf boot_cpu_has(X86_FEATURE_APERFMPERF)
+#define cpu_has_lfence_dispatch boot_cpu_has(X86_FEATURE_LFENCE_DISPATCH)
enum _cache_type {
CACHE_TYPE_NULL = 0,
diff --git a/xen/include/asm-x86/cpufeatures.h b/xen/include/asm-x86/cpufeatures.h
index bc98227..58b37d6 100644
--- a/xen/include/asm-x86/cpufeatures.h
+++ b/xen/include/asm-x86/cpufeatures.h
@@ -22,3 +22,4 @@ XEN_CPUFEATURE(APERFMPERF, (FSCAPINTS+0)*32+ 8) /* APERFMPERF */
XEN_CPUFEATURE(MFENCE_RDTSC, (FSCAPINTS+0)*32+ 9) /* MFENCE synchronizes RDTSC */
XEN_CPUFEATURE(XEN_SMEP, (FSCAPINTS+0)*32+10) /* SMEP gets used by Xen itself */
XEN_CPUFEATURE(XEN_SMAP, (FSCAPINTS+0)*32+11) /* SMAP gets used by Xen itself */
+XEN_CPUFEATURE(LFENCE_DISPATCH, (FSCAPINTS+0)*32+12) /* lfence set as Dispatch Serialising */
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index a834f3b..56f5359 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -207,6 +207,7 @@
#define MSR_AMD64_IC_CFG 0xc0011021
#define MSR_AMD64_DC_CFG 0xc0011022
#define MSR_AMD64_DE_CFG 0xc0011029
+#define AMD64_DE_CFG_LFENCE_SERIALISE (_AC(1, ULL) << 1)
#define MSR_AMD64_DR0_ADDRESS_MASK 0xc0011027
#define MSR_AMD64_DR1_ADDRESS_MASK 0xc0011019
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v8 05/17] x86: Introduce alternative indirect thunks
2018-01-12 18:00 [PATCH v8 00/17] x86: Mitigations for SP2/CVE-2017-5715/Branch Target Injection Andrew Cooper
` (3 preceding siblings ...)
2018-01-12 18:00 ` [PATCH v8 04/17] x86/amd: Try to set lfence as being Dispatch Serialising Andrew Cooper
@ 2018-01-12 18:00 ` Andrew Cooper
2018-01-15 10:53 ` Jan Beulich
2018-01-12 18:00 ` [PATCH v8 06/17] x86/feature: Definitions for Indirect Branch Controls Andrew Cooper
` (11 subsequent siblings)
16 siblings, 1 reply; 55+ messages in thread
From: Andrew Cooper @ 2018-01-12 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
Depending on hardware and microcode availability, we will want to replace
IND_THUNK_REPOLINE with other implementations.
For AMD hardware, choose IND_THUNK_LFENCE in preference to retpoline if lfence
is known to be (or was successfully made) dispatch serialising.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v7:
* Rebase over compiler changes
* Spelling/grammar fixes
* Make opt_thunk static
---
docs/misc/xen-command-line.markdown | 16 ++++++++
xen/arch/x86/indirect-thunk.S | 17 +++++++--
xen/arch/x86/spec_ctrl.c | 75 +++++++++++++++++++++++++++++++++++--
xen/include/asm-x86/cpufeatures.h | 2 +
4 files changed, 104 insertions(+), 6 deletions(-)
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 781110d..96e57c2 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -245,6 +245,22 @@ and not running softirqs. Reduce this if softirqs are not being run frequently
enough. Setting this to a high value may cause boot failure, particularly if
the NMI watchdog is also enabled.
+### bti (x86)
+> `= List of [ thunk=retpoline|lfence|jmp ]`
+
+Branch Target Injection controls. By default, Xen will pick the most
+appropriate BTI mitigations based on compiled in support, loaded microcode,
+and hardware details.
+
+**WARNING: Any use of this option may interfere with heuristics. Use with
+extreme care.**
+
+If Xen was compiled with INDIRECT_THUNK support, `thunk=` can be used to
+select which of the thunks gets patched into the `__x86_indirect_thunk_%reg`
+locations. The default thunk is `retpoline` (generally preferred for Intel
+hardware), with the alternatives being `jmp` (a `jmp *%reg` gadget, minimal
+overhead), and `lfence` (an `lfence; jmp *%reg` gadget, preferred for AMD).
+
### xenheap\_megabytes (arm32)
> `= <size>`
diff --git a/xen/arch/x86/indirect-thunk.S b/xen/arch/x86/indirect-thunk.S
index 3eaf505..7d34707 100644
--- a/xen/arch/x86/indirect-thunk.S
+++ b/xen/arch/x86/indirect-thunk.S
@@ -21,15 +21,26 @@
ret
.endm
+.macro IND_THUNK_LFENCE reg:req
+ lfence
+ jmp *%\reg
+.endm
+
+.macro IND_THUNK_JMP reg:req
+ jmp *%\reg
+.endm
+
/*
- * Build the __x86_indirect_thunk_* symbols. Currently implement the
- * retpoline thunk only.
+ * Build the __x86.indirect_thunk.* symbols. Execution lands on an
+ * alternative patch point which implements one of the above THUNK_*'s
*/
.macro GEN_INDIRECT_THUNK reg:req
.section .text.__x86_indirect_thunk_\reg, "ax", @progbits
ENTRY(__x86_indirect_thunk_\reg)
- IND_THUNK_RETPOLINE \reg
+ ALTERNATIVE_2 __stringify(IND_THUNK_RETPOLINE \reg), \
+ __stringify(IND_THUNK_LFENCE \reg), X86_FEATURE_IND_THUNK_LFENCE, \
+ __stringify(IND_THUNK_JMP \reg), X86_FEATURE_IND_THUNK_JMP
.endm
/* Instantiate GEN_INDIRECT_THUNK for each register except %rsp. */
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 256701a..d601c02 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -16,18 +16,54 @@
*
* Copyright (c) 2017-2018 Citrix Systems Ltd.
*/
+#include <xen/errno.h>
#include <xen/init.h>
#include <xen/lib.h>
#include <asm/processor.h>
#include <asm/spec_ctrl.h>
-enum ind_thunk {
+static enum ind_thunk {
THUNK_DEFAULT, /* Decide which thunk to use at boot time. */
THUNK_NONE, /* Missing compiler support for thunks. */
THUNK_RETPOLINE,
-};
+ THUNK_LFENCE,
+ THUNK_JMP,
+} opt_thunk __initdata = THUNK_DEFAULT;
+
+static int __init parse_bti(const char *s)
+{
+ const char *ss;
+ int rc = 0;
+
+ do {
+ ss = strchr(s, ',');
+ if ( !ss )
+ ss = strchr(s, '\0');
+
+ if ( !strncmp(s, "thunk=", 6) )
+ {
+ s += 6;
+
+ if ( !strncmp(s, "retpoline", ss - s) )
+ opt_thunk = THUNK_RETPOLINE;
+ else if ( !strncmp(s, "lfence", ss - s) )
+ opt_thunk = THUNK_LFENCE;
+ else if ( !strncmp(s, "jmp", ss - s) )
+ opt_thunk = THUNK_JMP;
+ else
+ rc = -EINVAL;
+ }
+ else
+ rc = -EINVAL;
+
+ s = ss + 1;
+ } while ( *ss );
+
+ return rc;
+}
+custom_param("bti", parse_bti);
static void __init print_details(enum ind_thunk thunk)
{
@@ -40,7 +76,9 @@ static void __init print_details(enum ind_thunk thunk)
printk(XENLOG_INFO
"BTI mitigations: Thunk %s\n",
thunk == THUNK_NONE ? "N/A" :
- thunk == THUNK_RETPOLINE ? "RETPOLINE" : "?");
+ thunk == THUNK_RETPOLINE ? "RETPOLINE" :
+ thunk == THUNK_LFENCE ? "LFENCE" :
+ thunk == THUNK_JMP ? "JMP" : "?");
}
void __init init_speculation_mitigations(void)
@@ -48,6 +86,31 @@ void __init init_speculation_mitigations(void)
enum ind_thunk thunk = THUNK_DEFAULT;
/*
+ * Has the user specified any custom BTI mitigations? If so, follow their
+ * instructions exactly and disable all heuristics.
+ */
+ if ( opt_thunk != THUNK_DEFAULT )
+ {
+ thunk = opt_thunk;
+ }
+ else
+ {
+ /*
+ * Evaluate the safest Branch Target Injection mitigations to use.
+ * First, begin with compiler-aided mitigations.
+ */
+ if ( IS_ENABLED(CONFIG_INDIRECT_THUNK) )
+ {
+ /*
+ * AMD's recommended mitigation is to set lfence as being dispatch
+ * serialising, and to use IND_THUNK_LFENCE.
+ */
+ if ( cpu_has_lfence_dispatch )
+ thunk = THUNK_LFENCE;
+ }
+ }
+
+ /*
* Supplimentary minor adjustments. Without compiler support, there are
* no thunks.
*/
@@ -61,6 +124,12 @@ void __init init_speculation_mitigations(void)
if ( thunk == THUNK_DEFAULT )
thunk = THUNK_RETPOLINE;
+ /* Apply the chosen settings. */
+ if ( thunk == THUNK_LFENCE )
+ setup_force_cpu_cap(X86_FEATURE_IND_THUNK_LFENCE);
+ else if ( thunk == THUNK_JMP )
+ setup_force_cpu_cap(X86_FEATURE_IND_THUNK_JMP);
+
print_details(thunk);
}
diff --git a/xen/include/asm-x86/cpufeatures.h b/xen/include/asm-x86/cpufeatures.h
index 58b37d6..ba1771b 100644
--- a/xen/include/asm-x86/cpufeatures.h
+++ b/xen/include/asm-x86/cpufeatures.h
@@ -23,3 +23,5 @@ XEN_CPUFEATURE(MFENCE_RDTSC, (FSCAPINTS+0)*32+ 9) /* MFENCE synchronizes RDTS
XEN_CPUFEATURE(XEN_SMEP, (FSCAPINTS+0)*32+10) /* SMEP gets used by Xen itself */
XEN_CPUFEATURE(XEN_SMAP, (FSCAPINTS+0)*32+11) /* SMAP gets used by Xen itself */
XEN_CPUFEATURE(LFENCE_DISPATCH, (FSCAPINTS+0)*32+12) /* lfence set as Dispatch Serialising */
+XEN_CPUFEATURE(IND_THUNK_LFENCE,(FSCAPINTS+0)*32+13) /* Use IND_THUNK_LFENCE */
+XEN_CPUFEATURE(IND_THUNK_JMP, (FSCAPINTS+0)*32+14) /* Use IND_THUNK_JMP */
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v8 06/17] x86/feature: Definitions for Indirect Branch Controls
2018-01-12 18:00 [PATCH v8 00/17] x86: Mitigations for SP2/CVE-2017-5715/Branch Target Injection Andrew Cooper
` (4 preceding siblings ...)
2018-01-12 18:00 ` [PATCH v8 05/17] x86: Introduce alternative indirect thunks Andrew Cooper
@ 2018-01-12 18:00 ` Andrew Cooper
2018-01-12 18:00 ` [PATCH v8 07/17] x86/cmdline: Introduce a command line option to disable IBRS/IBPB, STIBP and IBPB Andrew Cooper
` (10 subsequent siblings)
16 siblings, 0 replies; 55+ messages in thread
From: Andrew Cooper @ 2018-01-12 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
Contemporary processors are gaining Indirect Branch Controls via microcode
updates. Intel are introducing one bit to indicate IBRS and IBPB support, and
a second bit for STIBP. AMD are introducing IBPB only, so enumerate it with a
separate bit.
Furthermore, depending on compiler and microcode availability, we may want to
run Xen with IBRS set, or clear.
To use these facilities, we synthesise separate IBRS and IBPB bits for
internal use. A lot of infrastructure is required before these features are
safe to offer to guests.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
v7:
* Spelling fixes in the commit message.
---
tools/libxl/libxl_cpuid.c | 3 +++
tools/misc/xen-cpuid.c | 12 ++++++++++--
xen/arch/x86/spec_ctrl.c | 17 +++++++++++++++++
xen/include/asm-x86/cpufeatures.h | 3 +++
xen/include/asm-x86/msr-index.h | 8 ++++++++
xen/include/public/arch-x86/cpufeatureset.h | 3 +++
xen/tools/gen-cpuid.py | 5 +++++
7 files changed, 49 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index e692b61..81ba961 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -202,6 +202,8 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
{"avx512-4vnniw",0x00000007, 0, CPUID_REG_EDX, 2, 1},
{"avx512-4fmaps",0x00000007, 0, CPUID_REG_EDX, 3, 1},
+ {"ibrsb", 0x00000007, 0, CPUID_REG_EDX, 26, 1},
+ {"stibp", 0x00000007, 0, CPUID_REG_EDX, 27, 1},
{"lahfsahf", 0x80000001, NA, CPUID_REG_ECX, 0, 1},
{"cmplegacy", 0x80000001, NA, CPUID_REG_ECX, 1, 1},
@@ -239,6 +241,7 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
{"invtsc", 0x80000007, NA, CPUID_REG_EDX, 8, 1},
+ {"ibpb", 0x80000008, NA, CPUID_REG_EBX, 12, 1},
{"nc", 0x80000008, NA, CPUID_REG_ECX, 0, 8},
{"apicidsize", 0x80000008, NA, CPUID_REG_ECX, 12, 4},
diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index 0831f75..8c3dac0 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -149,7 +149,11 @@ static const char *str_e8b[32] =
{
[ 0] = "clzero",
- [1 ... 31] = "REZ",
+ [1 ... 11] = "REZ",
+
+ [12] = "ibpb",
+
+ [13 ... 31] = "REZ",
};
static const char *str_7d0[32] =
@@ -158,7 +162,11 @@ static const char *str_7d0[32] =
[ 2] = "avx512_4vnniw", [ 3] = "avx512_4fmaps",
- [4 ... 31] = "REZ",
+ [4 ... 25] = "REZ",
+
+ [26] = "ibrsb", [27] = "stibp",
+
+ [28 ... 31] = "REZ",
};
static struct {
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index d601c02..89e7287 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -67,8 +67,25 @@ custom_param("bti", parse_bti);
static void __init print_details(enum ind_thunk thunk)
{
+ unsigned int _7d0 = 0, e8b = 0, tmp;
+
+ /* Collect diagnostics about available mitigations. */
+ if ( boot_cpu_data.cpuid_level >= 7 )
+ cpuid_count(7, 0, &tmp, &tmp, &tmp, &_7d0);
+ if ( boot_cpu_data.extended_cpuid_level >= 0x80000008 )
+ cpuid(0x80000008, &tmp, &e8b, &tmp, &tmp);
+
printk(XENLOG_DEBUG "Speculative mitigation facilities:\n");
+ /* Hardware features which pertain to speculative mitigations. */
+ if ( (_7d0 & (cpufeat_mask(X86_FEATURE_IBRSB) |
+ cpufeat_mask(X86_FEATURE_STIBP))) ||
+ (e8b & cpufeat_mask(X86_FEATURE_IBPB)) )
+ printk(XENLOG_DEBUG " Hardware features:%s%s%s\n",
+ (_7d0 & cpufeat_mask(X86_FEATURE_IBRSB)) ? " IBRS/IBPB" : "",
+ (_7d0 & cpufeat_mask(X86_FEATURE_STIBP)) ? " STIBP" : "",
+ (e8b & cpufeat_mask(X86_FEATURE_IBPB)) ? " IBPB" : "");
+
/* Compiled-in support which pertains to BTI mitigations. */
if ( IS_ENABLED(CONFIG_INDIRECT_THUNK) )
printk(XENLOG_DEBUG " Compiled-in support: INDIRECT_THUNK\n");
diff --git a/xen/include/asm-x86/cpufeatures.h b/xen/include/asm-x86/cpufeatures.h
index ba1771b..dd2388f 100644
--- a/xen/include/asm-x86/cpufeatures.h
+++ b/xen/include/asm-x86/cpufeatures.h
@@ -25,3 +25,6 @@ XEN_CPUFEATURE(XEN_SMAP, (FSCAPINTS+0)*32+11) /* SMAP gets used by Xen it
XEN_CPUFEATURE(LFENCE_DISPATCH, (FSCAPINTS+0)*32+12) /* lfence set as Dispatch Serialising */
XEN_CPUFEATURE(IND_THUNK_LFENCE,(FSCAPINTS+0)*32+13) /* Use IND_THUNK_LFENCE */
XEN_CPUFEATURE(IND_THUNK_JMP, (FSCAPINTS+0)*32+14) /* Use IND_THUNK_JMP */
+XEN_CPUFEATURE(XEN_IBPB, (FSCAPINTS+0)*32+15) /* IBRSB || IBPB */
+XEN_CPUFEATURE(XEN_IBRS_SET, (FSCAPINTS+0)*32+16) /* IBRSB && IRBS set in Xen */
+XEN_CPUFEATURE(XEN_IBRS_CLEAR, (FSCAPINTS+0)*32+17) /* IBRSB && IBRS clear in Xen */
diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h
index 56f5359..a0aacfa 100644
--- a/xen/include/asm-x86/msr-index.h
+++ b/xen/include/asm-x86/msr-index.h
@@ -31,6 +31,14 @@
#define EFER_LMSLE (1<<_EFER_LMSLE)
#define EFER_FFXSE (1<<_EFER_FFXSE)
+/* Speculation Controls. */
+#define MSR_SPEC_CTRL 0x00000048
+#define SPEC_CTRL_IBRS (_AC(1, ULL) << 0)
+#define SPEC_CTRL_STIBP (_AC(1, ULL) << 1)
+
+#define MSR_PRED_CMD 0x00000049
+#define PRED_CMD_IBPB (_AC(1, ULL) << 0)
+
/* Intel MSRs. Some also available on other CPUs */
#define MSR_IA32_PERFCTR0 0x000000c1
#define MSR_IA32_A_PERFCTR0 0x000004c1
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index be6da8e..e148755 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -237,10 +237,13 @@ XEN_CPUFEATURE(EFRO, 7*32+10) /* APERF/MPERF Read Only interface */
/* AMD-defined CPU features, CPUID level 0x80000008.ebx, word 8 */
XEN_CPUFEATURE(CLZERO, 8*32+ 0) /*A CLZERO instruction */
+XEN_CPUFEATURE(IBPB, 8*32+12) /* IBPB support only (no IBRS, used by AMD) */
/* Intel-defined CPU features, CPUID level 0x00000007:0.edx, word 9 */
XEN_CPUFEATURE(AVX512_4VNNIW, 9*32+ 2) /*A AVX512 Neural Network Instructions */
XEN_CPUFEATURE(AVX512_4FMAPS, 9*32+ 3) /*A AVX512 Multiply Accumulation Single Precision */
+XEN_CPUFEATURE(IBRSB, 9*32+26) /* IBRS and IBPB support (used by Intel) */
+XEN_CPUFEATURE(STIBP, 9*32+27) /* STIBP */
#endif /* XEN_CPUFEATURE */
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index 9ec4486..613b909 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -256,6 +256,11 @@ def crunch_numbers(state):
AVX512F: [AVX512DQ, AVX512IFMA, AVX512PF, AVX512ER, AVX512CD,
AVX512BW, AVX512VL, AVX512VBMI, AVX512_4VNNIW,
AVX512_4FMAPS, AVX512_VPOPCNTDQ],
+
+ # Single Thread Indirect Branch Predictors enumerates a new bit in the
+ # MSR enumerated by Indirect Branch Restricted Speculation/Indirect
+ # Branch Prediction Barrier enumeration.
+ IBRSB: [STIBP],
}
deep_features = tuple(sorted(deps.keys()))
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v8 07/17] x86/cmdline: Introduce a command line option to disable IBRS/IBPB, STIBP and IBPB
2018-01-12 18:00 [PATCH v8 00/17] x86: Mitigations for SP2/CVE-2017-5715/Branch Target Injection Andrew Cooper
` (5 preceding siblings ...)
2018-01-12 18:00 ` [PATCH v8 06/17] x86/feature: Definitions for Indirect Branch Controls Andrew Cooper
@ 2018-01-12 18:00 ` Andrew Cooper
2018-01-12 18:00 ` [PATCH v8 08/17] x86/msr: Emulation of MSR_{SPEC_CTRL, PRED_CMD} for guests Andrew Cooper
` (9 subsequent siblings)
16 siblings, 0 replies; 55+ messages in thread
From: Andrew Cooper @ 2018-01-12 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
Instead of gaining yet another top level boolean, introduce a more generic
cpuid= option. Also introduce a helper function to parse a generic boolean
value.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
docs/misc/xen-command-line.markdown | 12 ++++++++++++
xen/arch/x86/cpuid.c | 35 +++++++++++++++++++++++++++++++++++
xen/common/kernel.c | 23 +++++++++++++++++++++++
xen/include/xen/lib.h | 7 +++++++
4 files changed, 77 insertions(+)
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 96e57c2..b42abc6 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -471,6 +471,18 @@ choice of `dom0-kernel` is deprecated and not supported by all Dom0 kernels.
respectively.
* `verbose` option can be included as a string or also as `verbose=<integer>`
+### cpuid (x86)
+> `= List of comma separated booleans`
+
+This option allows for fine tuning of the facilities Xen will use, after
+accounting for hardware capabilities as enumerated via CPUID.
+
+Currently accepted:
+
+The Speculation Control hardware features `ibrsb`, `stibp`, `ibpb` are used by
+default if avaiable. They can be ignored, e.g. `no-ibrsb`, at which point Xen
+won't use them itself, and won't offer them to guests.
+
### cpuid\_mask\_cpu (AMD only)
> `= fam_0f_rev_c | fam_0f_rev_d | fam_0f_rev_e | fam_0f_rev_f | fam_0f_rev_g | fam_10_rev_b | fam_10_rev_c | fam_11_rev_b`
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 5ee82d3..2ef71d2 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -18,6 +18,41 @@ static const uint32_t hvm_shadow_featuremask[] = INIT_HVM_SHADOW_FEATURES;
static const uint32_t hvm_hap_featuremask[] = INIT_HVM_HAP_FEATURES;
static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
+static int __init parse_xen_cpuid(const char *s)
+{
+ const char *ss;
+ int val, rc = 0;
+
+ do {
+ ss = strchr(s, ',');
+ if ( !ss )
+ ss = strchr(s, '\0');
+
+ if ( (val = parse_boolean("ibpb", s, ss)) >= 0 )
+ {
+ if ( !val )
+ setup_clear_cpu_cap(X86_FEATURE_IBPB);
+ }
+ else if ( (val = parse_boolean("ibrsb", s, ss)) >= 0 )
+ {
+ if ( !val )
+ setup_clear_cpu_cap(X86_FEATURE_IBRSB);
+ }
+ else if ( (val = parse_boolean("stibp", s, ss)) >= 0 )
+ {
+ if ( !val )
+ setup_clear_cpu_cap(X86_FEATURE_STIBP);
+ }
+ else
+ rc = -EINVAL;
+
+ s = ss + 1;
+ } while ( *ss );
+
+ return rc;
+}
+custom_param("cpuid", parse_xen_cpuid);
+
#define EMPTY_LEAF ((struct cpuid_leaf){})
static void zero_leaves(struct cpuid_leaf *l,
unsigned int first, unsigned int last)
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 8d137c5..19f9bad 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -244,6 +244,29 @@ int parse_bool(const char *s, const char *e)
return -1;
}
+int parse_boolean(const char *name, const char *s, const char *e)
+{
+ size_t slen, nlen;
+ int val = !!strncmp(s, "no-", 3);
+
+ if ( !val )
+ s += 3;
+
+ slen = e ? ({ ASSERT(e >= s); e - s; }) : strlen(s);
+ nlen = strlen(name);
+
+ /* Does s now start with name? */
+ if ( slen < nlen || strncmp(s, name, nlen) )
+ return -1;
+
+ switch ( s[nlen] )
+ {
+ case '\0': return val;
+ case '=': return parse_bool(&s[nlen + 1], e);
+ default: return -1;
+ }
+}
+
unsigned int tainted;
/**
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index ed00ae1..1d97713 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -74,6 +74,13 @@ void cmdline_parse(const char *cmdline);
int runtime_parse(const char *line);
int parse_bool(const char *s, const char *e);
+/**
+ * Given a specific name, parses a string of the form:
+ * [no-]$NAME[=...]
+ * returning 0 or 1 for a recognised boolean, or -1 for an error.
+ */
+int parse_boolean(const char *name, const char *s, const char *e);
+
/*#define DEBUG_TRACE_DUMP*/
#ifdef DEBUG_TRACE_DUMP
extern void debugtrace_dump(void);
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v8 08/17] x86/msr: Emulation of MSR_{SPEC_CTRL, PRED_CMD} for guests
2018-01-12 18:00 [PATCH v8 00/17] x86: Mitigations for SP2/CVE-2017-5715/Branch Target Injection Andrew Cooper
` (6 preceding siblings ...)
2018-01-12 18:00 ` [PATCH v8 07/17] x86/cmdline: Introduce a command line option to disable IBRS/IBPB, STIBP and IBPB Andrew Cooper
@ 2018-01-12 18:00 ` Andrew Cooper
2018-01-16 11:10 ` David Woodhouse
2018-01-12 18:00 ` [PATCH v8 09/17] x86/migrate: Move MSR_SPEC_CTRL on migrate Andrew Cooper
` (8 subsequent siblings)
16 siblings, 1 reply; 55+ messages in thread
From: Andrew Cooper @ 2018-01-12 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/msr.c | 35 +++++++++++++++++++++++++++++++++++
xen/include/asm-x86/msr.h | 12 ++++++++++++
2 files changed, 47 insertions(+)
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 31983ed..02a7b49 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -119,11 +119,22 @@ int init_vcpu_msr_policy(struct vcpu *v)
int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
{
+ const struct cpuid_policy *cp = v->domain->arch.cpuid;
const struct msr_domain_policy *dp = v->domain->arch.msr;
const struct msr_vcpu_policy *vp = v->arch.msr;
switch ( msr )
{
+ case MSR_PRED_CMD:
+ /* Write-only */
+ goto gp_fault;
+
+ case MSR_SPEC_CTRL:
+ if ( !cp->feat.ibrsb )
+ goto gp_fault;
+ *val = vp->spec_ctrl.guest;
+ break;
+
case MSR_INTEL_PLATFORM_INFO:
if ( !dp->plaform_info.available )
goto gp_fault;
@@ -152,14 +163,38 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
{
const struct vcpu *curr = current;
struct domain *d = v->domain;
+ const struct cpuid_policy *cp = d->arch.cpuid;
struct msr_domain_policy *dp = d->arch.msr;
struct msr_vcpu_policy *vp = v->arch.msr;
switch ( msr )
{
case MSR_INTEL_PLATFORM_INFO:
+ /* Read-only */
goto gp_fault;
+ case MSR_SPEC_CTRL:
+ if ( !cp->feat.ibrsb )
+ goto gp_fault; /* MSR available? */
+ if ( val & ~(SPEC_CTRL_IBRS |
+ (cp->feat.stibp ? SPEC_CTRL_STIBP : 0)) )
+ goto gp_fault; /* Rsvd bit set? */
+ vp->spec_ctrl.guest = val;
+ vp->spec_ctrl.host = val;
+ break;
+
+ case MSR_PRED_CMD:
+ if ( !cp->feat.ibrsb && !cp->extd.ibpb )
+ goto gp_fault; /* MSR available? */
+
+ /*
+ * The only defined behaviour is when writing PRED_CMD_IBPB. In
+ * practice, real hardware accepts any value without faulting.
+ */
+ if ( v == curr && (val & PRED_CMD_IBPB) )
+ wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
+ break;
+
case MSR_INTEL_MISC_FEATURES_ENABLES:
{
uint64_t rsvd = ~0ull;
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index 2fbed02..3d0012d 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -223,6 +223,18 @@ struct msr_domain_policy
/* MSR policy object for per-vCPU MSRs */
struct msr_vcpu_policy
{
+ /* 0x00000048 - MSR_SPEC_CTRL */
+ struct {
+ /*
+ * Only the bottom two bits are defined, so no need to waste space
+ * with uint64_t at the moment. We maintain the guests idea of the
+ * value it wrote, and a value to install into hardware (extended to
+ * uint32_t to simplify the asm) which might be different.
+ */
+ uint32_t host;
+ uint8_t guest;
+ } spec_ctrl;
+
/* 0x00000140 MSR_INTEL_MISC_FEATURES_ENABLES */
struct {
bool available; /* This MSR is non-architectural */
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v8 09/17] x86/migrate: Move MSR_SPEC_CTRL on migrate
2018-01-12 18:00 [PATCH v8 00/17] x86: Mitigations for SP2/CVE-2017-5715/Branch Target Injection Andrew Cooper
` (7 preceding siblings ...)
2018-01-12 18:00 ` [PATCH v8 08/17] x86/msr: Emulation of MSR_{SPEC_CTRL, PRED_CMD} for guests Andrew Cooper
@ 2018-01-12 18:00 ` Andrew Cooper
2018-01-12 18:01 ` [PATCH v8 10/17] x86/hvm: Permit guests direct access to MSR_{SPEC_CTRL, PRED_CMD} Andrew Cooper
` (7 subsequent siblings)
16 siblings, 0 replies; 55+ messages in thread
From: Andrew Cooper @ 2018-01-12 18:00 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/domctl.c | 2 ++
xen/arch/x86/hvm/hvm.c | 2 ++
2 files changed, 4 insertions(+)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 5973d9f..72b4489 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1290,6 +1290,7 @@ long arch_do_domctl(
struct xen_domctl_vcpu_msr msr = {};
struct vcpu *v;
static const uint32_t msrs_to_send[] = {
+ MSR_SPEC_CTRL,
MSR_INTEL_MISC_FEATURES_ENABLES,
};
uint32_t nr_msrs = ARRAY_SIZE(msrs_to_send);
@@ -1413,6 +1414,7 @@ long arch_do_domctl(
switch ( msr.index )
{
+ case MSR_SPEC_CTRL:
case MSR_INTEL_MISC_FEATURES_ENABLES:
if ( guest_wrmsr(v, msr.index, msr.value) != X86EMUL_OKAY )
break;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index db282b5..ed36598 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1323,6 +1323,7 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
#define HVM_CPU_MSR_SIZE(cnt) offsetof(struct hvm_msr, msr[cnt])
static const uint32_t msrs_to_send[] = {
+ MSR_SPEC_CTRL,
MSR_INTEL_MISC_FEATURES_ENABLES,
};
static unsigned int __read_mostly msr_count_max = ARRAY_SIZE(msrs_to_send);
@@ -1458,6 +1459,7 @@ static int hvm_load_cpu_msrs(struct domain *d, hvm_domain_context_t *h)
{
int rc;
+ case MSR_SPEC_CTRL:
case MSR_INTEL_MISC_FEATURES_ENABLES:
rc = guest_wrmsr(v, ctxt->msr[i].index, ctxt->msr[i].val);
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v8 10/17] x86/hvm: Permit guests direct access to MSR_{SPEC_CTRL, PRED_CMD}
2018-01-12 18:00 [PATCH v8 00/17] x86: Mitigations for SP2/CVE-2017-5715/Branch Target Injection Andrew Cooper
` (8 preceding siblings ...)
2018-01-12 18:00 ` [PATCH v8 09/17] x86/migrate: Move MSR_SPEC_CTRL on migrate Andrew Cooper
@ 2018-01-12 18:01 ` Andrew Cooper
2018-01-15 11:11 ` Jan Beulich
2018-01-12 18:01 ` [PATCH v8 11/17] x86: Protect unaware domains from meddling hyperthreads Andrew Cooper
` (6 subsequent siblings)
16 siblings, 1 reply; 55+ messages in thread
From: Andrew Cooper @ 2018-01-12 18:01 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
For performance reasons, HVM guests should have direct access to these MSRs
when possible.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v7:
* Drop excess brackets
---
xen/arch/x86/domctl.c | 19 +++++++++++++++++++
xen/arch/x86/hvm/svm/svm.c | 5 +++++
xen/arch/x86/hvm/vmx/vmx.c | 18 ++++++++++++++++++
xen/arch/x86/msr.c | 3 ++-
xen/include/asm-x86/msr.h | 5 ++++-
5 files changed, 48 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 72b4489..e5fdde7 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -53,6 +53,7 @@ static int update_domain_cpuid_info(struct domain *d,
struct cpuid_policy *p = d->arch.cpuid;
const struct cpuid_leaf leaf = { ctl->eax, ctl->ebx, ctl->ecx, ctl->edx };
int old_vendor = p->x86_vendor;
+ unsigned int old_7d0 = p->feat.raw[0].d, old_e8b = p->extd.raw[8].b;
bool call_policy_changed = false; /* Avoid for_each_vcpu() unnecessarily */
/*
@@ -218,6 +219,14 @@ static int update_domain_cpuid_info(struct domain *d,
d->arch.pv_domain.cpuidmasks->_7ab0 = mask;
}
+
+ /*
+ * If the IBSRB/STIBP policy has changed, we need to recalculate the
+ * MSR interception bitmaps and STIBP protection default.
+ */
+ call_policy_changed = ((old_7d0 ^ p->feat.raw[0].d) &
+ (cpufeat_mask(X86_FEATURE_IBRSB) |
+ cpufeat_mask(X86_FEATURE_STIBP)));
break;
case 0xa:
@@ -292,6 +301,16 @@ static int update_domain_cpuid_info(struct domain *d,
d->arch.pv_domain.cpuidmasks->e1cd = mask;
}
break;
+
+ case 0x80000008:
+ /*
+ * If the IBRB policy has changed, we need to recalculate the MSR
+ * interception bitmaps.
+ */
+ call_policy_changed = (is_hvm_domain(d) &&
+ ((old_e8b ^ p->extd.raw[8].b) &
+ cpufeat_mask(X86_FEATURE_IBPB)));
+ break;
}
if ( call_policy_changed )
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index c48fdfa..ee47508 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -617,6 +617,7 @@ static void svm_cpuid_policy_changed(struct vcpu *v)
{
struct arch_svm_struct *arch_svm = &v->arch.hvm_svm;
struct vmcb_struct *vmcb = arch_svm->vmcb;
+ const struct cpuid_policy *cp = v->domain->arch.cpuid;
u32 bitmap = vmcb_get_exception_intercepts(vmcb);
if ( opt_hvm_fep ||
@@ -626,6 +627,10 @@ static void svm_cpuid_policy_changed(struct vcpu *v)
bitmap &= ~(1U << TRAP_invalid_op);
vmcb_set_exception_intercepts(vmcb, bitmap);
+
+ /* Give access to MSR_PRED_CMD if the guest has been told about it. */
+ svm_intercept_msr(v, MSR_PRED_CMD,
+ cp->extd.ibpb ? MSR_INTERCEPT_NONE : MSR_INTERCEPT_RW);
}
static void svm_sync_vmcb(struct vcpu *v)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index e036303..8609de3 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -656,6 +656,8 @@ void vmx_update_exception_bitmap(struct vcpu *v)
static void vmx_cpuid_policy_changed(struct vcpu *v)
{
+ const struct cpuid_policy *cp = v->domain->arch.cpuid;
+
if ( opt_hvm_fep ||
(v->domain->arch.cpuid->x86_vendor != boot_cpu_data.x86_vendor) )
v->arch.hvm_vmx.exception_bitmap |= (1U << TRAP_invalid_op);
@@ -665,6 +667,22 @@ static void vmx_cpuid_policy_changed(struct vcpu *v)
vmx_vmcs_enter(v);
vmx_update_exception_bitmap(v);
vmx_vmcs_exit(v);
+
+ /*
+ * We can only pass though MSR_SPEC_CTRL if the guest knows about all bits
+ * in it. Otherwise, Xen may be fixing up in the background.
+ */
+ v->arch.msr->spec_ctrl.direct_access = cp->feat.ibrsb && cp->feat.stibp;
+ if ( v->arch.msr->spec_ctrl.direct_access )
+ vmx_clear_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
+ else
+ vmx_set_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
+
+ /* MSR_PRED_CMD is safe to pass through if the guest knows about it. */
+ if ( cp->feat.ibrsb || cp->extd.ibpb )
+ vmx_clear_msr_intercept(v, MSR_PRED_CMD, VMX_MSR_RW);
+ else
+ vmx_set_msr_intercept(v, MSR_PRED_CMD, VMX_MSR_RW);
}
int vmx_guest_x86_mode(struct vcpu *v)
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 02a7b49..697cc6e 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -132,7 +132,8 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
case MSR_SPEC_CTRL:
if ( !cp->feat.ibrsb )
goto gp_fault;
- *val = vp->spec_ctrl.guest;
+ *val = (vp->spec_ctrl.direct_access
+ ? vp->spec_ctrl.host : vp->spec_ctrl.guest);
break;
case MSR_INTEL_PLATFORM_INFO:
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index 3d0012d..007e966 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -229,10 +229,13 @@ struct msr_vcpu_policy
* Only the bottom two bits are defined, so no need to waste space
* with uint64_t at the moment. We maintain the guests idea of the
* value it wrote, and a value to install into hardware (extended to
- * uint32_t to simplify the asm) which might be different.
+ * uint32_t to simplify the asm) which might be different. HVM guests
+ * might be given direct access to the MSRs, at which point the
+ * 'guest' value becomes stale.
*/
uint32_t host;
uint8_t guest;
+ bool direct_access;
} spec_ctrl;
/* 0x00000140 MSR_INTEL_MISC_FEATURES_ENABLES */
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v8 11/17] x86: Protect unaware domains from meddling hyperthreads
2018-01-12 18:00 [PATCH v8 00/17] x86: Mitigations for SP2/CVE-2017-5715/Branch Target Injection Andrew Cooper
` (9 preceding siblings ...)
2018-01-12 18:01 ` [PATCH v8 10/17] x86/hvm: Permit guests direct access to MSR_{SPEC_CTRL, PRED_CMD} Andrew Cooper
@ 2018-01-12 18:01 ` Andrew Cooper
2018-01-15 11:26 ` Jan Beulich
2018-01-12 18:01 ` [PATCH v8 12/17] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point Andrew Cooper
` (5 subsequent siblings)
16 siblings, 1 reply; 55+ messages in thread
From: Andrew Cooper @ 2018-01-12 18:01 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
Set STIBP behind the guests back if it knows about IBRS but not STIBP, and no
MSR_SPEC_CTRL protection active.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v7:
* Move logic into a static inline helper.
---
xen/arch/x86/domain.c | 8 ++++++++
xen/arch/x86/msr.c | 3 ++-
xen/include/asm-x86/cpufeature.h | 3 +++
xen/include/asm-x86/spec_ctrl.h | 21 +++++++++++++++++++++
4 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index da1bf1a..8849e3f 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -65,6 +65,7 @@
#include <asm/psr.h>
#include <asm/pv/domain.h>
#include <asm/pv/mm.h>
+#include <asm/spec_ctrl.h>
DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
@@ -2030,6 +2031,13 @@ int domain_relinquish_resources(struct domain *d)
*/
void cpuid_policy_updated(struct vcpu *v)
{
+ const struct cpuid_policy *cp = v->domain->arch.cpuid;
+ struct msr_vcpu_policy *vp = v->arch.msr;
+
+ /* Calculate a safe host default. */
+ if ( cp->feat.ibrsb )
+ vp->spec_ctrl.host = spec_ctrl_host_val(v->domain, vp->spec_ctrl.guest);
+
if ( is_hvm_vcpu(v) )
hvm_cpuid_policy_changed(v);
}
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 697cc6e..45c4d78 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -23,6 +23,7 @@
#include <xen/lib.h>
#include <xen/sched.h>
#include <asm/msr.h>
+#include <asm/spec_ctrl.h>
struct msr_domain_policy __read_mostly hvm_max_msr_domain_policy,
__read_mostly pv_max_msr_domain_policy;
@@ -181,7 +182,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
(cp->feat.stibp ? SPEC_CTRL_STIBP : 0)) )
goto gp_fault; /* Rsvd bit set? */
vp->spec_ctrl.guest = val;
- vp->spec_ctrl.host = val;
+ vp->spec_ctrl.host = spec_ctrl_host_val(d, val);
break;
case MSR_PRED_CMD:
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index adc333f..988a834 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -100,6 +100,9 @@
/* CPUID level 0x80000007.edx */
#define cpu_has_itsc boot_cpu_has(X86_FEATURE_ITSC)
+/* CPUID level 0x00000007:0.edx */
+#define cpu_has_stibp boot_cpu_has(X86_FEATURE_STIBP)
+
/* Synthesized. */
#define cpu_has_arch_perfmon boot_cpu_has(X86_FEATURE_ARCH_PERFMON)
#define cpu_has_cpuid_faulting boot_cpu_has(X86_FEATURE_CPUID_FAULTING)
diff --git a/xen/include/asm-x86/spec_ctrl.h b/xen/include/asm-x86/spec_ctrl.h
index e088a55..77f7c60 100644
--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -20,8 +20,29 @@
#ifndef __X86_SPEC_CTRL_H__
#define __X86_SPEC_CTRL_H__
+#include <xen/sched.h>
+
void init_speculation_mitigations(void);
+/*
+ * For guests which know about IBRS but are not told about STIBP running on
+ * hardware supporting hyperthreading, the guest doesn't know to protect
+ * itself fully. (Such a guest won't be permitted direct access to the MSR.)
+ * Have Xen fill in the gaps, so an unaware guest can't be interfered with by
+ * a meddling guest on an adjacent hyperthread.
+ */
+static inline unsigned int spec_ctrl_host_val(const struct domain *d,
+ unsigned int guest_val)
+{
+ const struct cpuid_policy *cp = d->arch.cpuid;
+
+ if ( !cp->feat.stibp && cpu_has_stibp &&
+ !(guest_val & (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP)) )
+ return SPEC_CTRL_STIBP;
+ else
+ return guest_val;
+}
+
#endif /* !__X86_SPEC_CTRL_H__ */
/*
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v8 12/17] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point
2018-01-12 18:00 [PATCH v8 00/17] x86: Mitigations for SP2/CVE-2017-5715/Branch Target Injection Andrew Cooper
` (10 preceding siblings ...)
2018-01-12 18:01 ` [PATCH v8 11/17] x86: Protect unaware domains from meddling hyperthreads Andrew Cooper
@ 2018-01-12 18:01 ` Andrew Cooper
2018-01-15 12:09 ` Jan Beulich
2018-01-12 18:01 ` [PATCH v8 13/17] x86/boot: Calculate the most appropriate BTI mitigation to use Andrew Cooper
` (4 subsequent siblings)
16 siblings, 1 reply; 55+ messages in thread
From: Andrew Cooper @ 2018-01-12 18:01 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
We need to be able to either set or clear IBRS in Xen context, as well as
restore appropriate guest values in guest context. See the documentation in
asm-x86/spec_ctrl_asm.h for details.
There is a semi-unrelated bugfix, where various asm_defn.h macros have a
hidden dependency on PAGE_SIZE, which results in an assembler error if used in
a .macro definition.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v7:
* Spelling fixes
* Reposition the semicolon fix.
---
xen/arch/x86/hvm/svm/entry.S | 8 +-
xen/arch/x86/hvm/vmx/entry.S | 11 ++
xen/arch/x86/setup.c | 1 +
xen/arch/x86/smpboot.c | 2 +
xen/arch/x86/x86_64/asm-offsets.c | 6 +
xen/arch/x86/x86_64/compat/entry.S | 12 ++
xen/arch/x86/x86_64/entry.S | 33 ++++++
xen/include/asm-x86/asm_defns.h | 3 +
xen/include/asm-x86/current.h | 6 +
xen/include/asm-x86/nops.h | 6 +
xen/include/asm-x86/spec_ctrl.h | 9 ++
xen/include/asm-x86/spec_ctrl_asm.h | 230 ++++++++++++++++++++++++++++++++++++
12 files changed, 326 insertions(+), 1 deletion(-)
create mode 100644 xen/include/asm-x86/spec_ctrl_asm.h
diff --git a/xen/arch/x86/hvm/svm/entry.S b/xen/arch/x86/hvm/svm/entry.S
index df86da0..fb1048b 100644
--- a/xen/arch/x86/hvm/svm/entry.S
+++ b/xen/arch/x86/hvm/svm/entry.S
@@ -79,6 +79,9 @@ UNLIKELY_END(svm_trace)
or $X86_EFLAGS_MBS,%rax
mov %rax,VMCB_rflags(%rcx)
+ /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
+ SPEC_CTRL_EXIT_TO_GUEST /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
+
pop %r15
pop %r14
pop %r13
@@ -101,8 +104,11 @@ UNLIKELY_END(svm_trace)
SAVE_ALL
GET_CURRENT(bx)
- mov VCPU_svm_vmcb(%rbx),%rcx
+ SPEC_CTRL_ENTRY_FROM_VMEXIT /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
+ /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
+
+ mov VCPU_svm_vmcb(%rbx),%rcx
movb $0,VCPU_svm_vmcb_in_sync(%rbx)
mov VMCB_rax(%rcx),%rax
mov %rax,UREGS_rax(%rsp)
diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
index b2f98be..21e959f 100644
--- a/xen/arch/x86/hvm/vmx/entry.S
+++ b/xen/arch/x86/hvm/vmx/entry.S
@@ -38,6 +38,9 @@ ENTRY(vmx_asm_vmexit_handler)
movb $1,VCPU_vmx_launched(%rbx)
mov %rax,VCPU_hvm_guest_cr2(%rbx)
+ SPEC_CTRL_ENTRY_FROM_VMEXIT /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
+ /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
+
mov %rsp,%rdi
call vmx_vmexit_handler
@@ -68,6 +71,10 @@ UNLIKELY_END(realmode)
call vmx_vmenter_helper
test %al, %al
jz .Lvmx_vmentry_restart
+
+ /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
+ SPEC_CTRL_EXIT_TO_GUEST /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
+
mov VCPU_hvm_guest_cr2(%rbx),%rax
pop %r15
@@ -99,6 +106,10 @@ UNLIKELY_END(realmode)
.Lvmx_vmentry_fail:
sti
SAVE_ALL
+
+ SPEC_CTRL_ENTRY_FROM_PV /* Req: %rsp=regs/cpuinfo Clob: acd */
+ /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
+
call vmx_vmentry_failure
BUG /* vmx_vmentry_failure() shouldn't return. */
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 9426760..d31f771 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -668,6 +668,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
set_processor_id(0);
set_current(INVALID_VCPU); /* debug sanity. */
idle_vcpu[0] = current;
+ init_shadow_spec_ctrl_state();
percpu_init_areas();
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 7b97ff8..a695d12 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -40,6 +40,7 @@
#include <asm/flushtlb.h>
#include <asm/msr.h>
#include <asm/mtrr.h>
+#include <asm/spec_ctrl.h>
#include <asm/time.h>
#include <asm/tboot.h>
#include <mach_apic.h>
@@ -308,6 +309,7 @@ void start_secondary(void *unused)
set_current(idle_vcpu[cpu]);
this_cpu(curr_vcpu) = idle_vcpu[cpu];
rdmsrl(MSR_EFER, this_cpu(efer));
+ init_shadow_spec_ctrl_state();
/*
* Just as during early bootstrap, it is convenient here to disable
diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c
index e136af6..7d36185 100644
--- a/xen/arch/x86/x86_64/asm-offsets.c
+++ b/xen/arch/x86/x86_64/asm-offsets.c
@@ -88,6 +88,7 @@ void __dummy__(void)
OFFSET(VCPU_kernel_ss, struct vcpu, arch.pv_vcpu.kernel_ss);
OFFSET(VCPU_iopl, struct vcpu, arch.pv_vcpu.iopl);
OFFSET(VCPU_guest_context_flags, struct vcpu, arch.vgc_flags);
+ OFFSET(VCPU_arch_msr, struct vcpu, arch.msr);
OFFSET(VCPU_nmi_pending, struct vcpu, nmi_pending);
OFFSET(VCPU_mce_pending, struct vcpu, mce_pending);
OFFSET(VCPU_nmi_old_mask, struct vcpu, nmi_state.old_mask);
@@ -137,6 +138,8 @@ void __dummy__(void)
OFFSET(CPUINFO_processor_id, struct cpu_info, processor_id);
OFFSET(CPUINFO_current_vcpu, struct cpu_info, current_vcpu);
OFFSET(CPUINFO_cr4, struct cpu_info, cr4);
+ OFFSET(CPUINFO_shadow_spec_ctrl, struct cpu_info, shadow_spec_ctrl);
+ OFFSET(CPUINFO_use_shadow_spec_ctrl, struct cpu_info, use_shadow_spec_ctrl);
DEFINE(CPUINFO_sizeof, sizeof(struct cpu_info));
BLANK();
@@ -152,6 +155,9 @@ void __dummy__(void)
OFFSET(TRAPBOUNCE_eip, struct trap_bounce, eip);
BLANK();
+ OFFSET(VCPUMSR_spec_ctrl_host, struct msr_vcpu_policy, spec_ctrl.host);
+ BLANK();
+
#ifdef CONFIG_PERF_COUNTERS
DEFINE(ASM_PERFC_exceptions, PERFC_exceptions);
BLANK();
diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
index 3fea54e..422c25d 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -18,6 +18,10 @@ ENTRY(entry_int82)
pushq $0
movl $HYPERCALL_VECTOR, 4(%rsp)
SAVE_ALL compat=1 /* DPL1 gate, restricted to 32bit PV guests only. */
+
+ SPEC_CTRL_ENTRY_FROM_PV /* Req: %rsp=regs/cpuinfo Clob: acd */
+ /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
+
CR4_PV32_RESTORE
GET_CURRENT(bx)
@@ -142,6 +146,10 @@ ENTRY(compat_restore_all_guest)
.popsection
or $X86_EFLAGS_IF,%r11
mov %r11d,UREGS_eflags(%rsp)
+
+ /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
+ SPEC_CTRL_EXIT_TO_GUEST /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
+
RESTORE_ALL adj=8 compat=1
.Lft0: iretq
_ASM_PRE_EXTABLE(.Lft0, handle_exception)
@@ -199,6 +207,10 @@ ENTRY(cstar_enter)
pushq $0
movl $TRAP_syscall, 4(%rsp)
SAVE_ALL
+
+ SPEC_CTRL_ENTRY_FROM_PV /* Req: %rsp=regs/cpuinfo Clob: acd */
+ /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
+
GET_CURRENT(bx)
movq VCPU_domain(%rbx),%rcx
cmpb $0,DOMAIN_is_32bit_pv(%rcx)
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index cbd73f6..e4e2193 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -37,6 +37,10 @@ ENTRY(switch_to_kernel)
/* %rbx: struct vcpu, interrupts disabled */
restore_all_guest:
ASSERT_INTERRUPTS_DISABLED
+
+ /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
+ SPEC_CTRL_EXIT_TO_GUEST /* Req: b=curr %rsp=regs/cpuinfo, Clob: acd */
+
RESTORE_ALL
testw $TRAP_syscall,4(%rsp)
jz iret_exit_to_guest
@@ -71,6 +75,8 @@ iret_exit_to_guest:
ALIGN
/* No special register assumptions. */
restore_all_xen:
+ /* WARNING! `ret`, `call *`, `jmp *` not safe beyond this point. */
+ SPEC_CTRL_EXIT_TO_XEN /* Req: nothing Clob: acd */
RESTORE_ALL adj=8
iretq
@@ -100,6 +106,10 @@ ENTRY(lstar_enter)
pushq $0
movl $TRAP_syscall, 4(%rsp)
SAVE_ALL
+
+ SPEC_CTRL_ENTRY_FROM_PV /* Req: %rsp=regs/cpuinfo Clob: acd */
+ /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
+
GET_CURRENT(bx)
testb $TF_kernel_mode,VCPU_thread_flags(%rbx)
jz switch_to_kernel
@@ -192,6 +202,10 @@ GLOBAL(sysenter_eflags_saved)
pushq $0
movl $TRAP_syscall, 4(%rsp)
SAVE_ALL
+
+ SPEC_CTRL_ENTRY_FROM_PV /* Req: %rsp=regs/cpuinfo Clob: acd */
+ /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
+
GET_CURRENT(bx)
cmpb $0,VCPU_sysenter_disables_events(%rbx)
movq VCPU_sysenter_addr(%rbx),%rax
@@ -228,6 +242,9 @@ ENTRY(int80_direct_trap)
movl $0x80, 4(%rsp)
SAVE_ALL
+ SPEC_CTRL_ENTRY_FROM_PV /* Req: %rsp=regs/cpuinfo Clob: acd */
+ /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
+
cmpb $0,untrusted_msi(%rip)
UNLIKELY_START(ne, msi_check)
movl $0x80,%edi
@@ -391,6 +408,10 @@ ENTRY(dom_crash_sync_extable)
ENTRY(common_interrupt)
SAVE_ALL CLAC
+
+ SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs Clob: acd */
+ /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
+
CR4_PV32_RESTORE
movq %rsp,%rdi
callq do_IRQ
@@ -411,6 +432,10 @@ ENTRY(page_fault)
/* No special register assumptions. */
GLOBAL(handle_exception)
SAVE_ALL CLAC
+
+ SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs Clob: acd */
+ /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
+
handle_exception_saved:
GET_CURRENT(bx)
testb $X86_EFLAGS_IF>>8,UREGS_eflags+1(%rsp)
@@ -586,6 +611,10 @@ ENTRY(double_fault)
movl $TRAP_double_fault,4(%rsp)
/* Set AC to reduce chance of further SMAP faults */
SAVE_ALL STAC
+
+ SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs Clob: acd */
+ /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
+
movq %rsp,%rdi
call do_double_fault
BUG /* do_double_fault() shouldn't return. */
@@ -604,6 +633,10 @@ ENTRY(nmi)
movl $TRAP_nmi,4(%rsp)
handle_ist_exception:
SAVE_ALL CLAC
+
+ SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs Clob: acd */
+ /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
+
CR4_PV32_RESTORE
testb $3,UREGS_cs(%rsp)
jz 1f
diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index 40b0250..bc10167 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -7,6 +7,7 @@
#include <asm/asm-offsets.h>
#endif
#include <asm/bug.h>
+#include <asm/page.h>
#include <asm/processor.h>
#include <asm/percpu.h>
#include <xen/stringify.h>
@@ -344,4 +345,6 @@ static always_inline void stac(void)
#define REX64_PREFIX "rex64/"
#endif
+#include <asm/spec_ctrl_asm.h>
+
#endif /* __X86_ASM_DEFNS_H__ */
diff --git a/xen/include/asm-x86/current.h b/xen/include/asm-x86/current.h
index 8984992..749d7aa 100644
--- a/xen/include/asm-x86/current.h
+++ b/xen/include/asm-x86/current.h
@@ -41,6 +41,12 @@ struct cpu_info {
struct vcpu *current_vcpu;
unsigned long per_cpu_offset;
unsigned long cr4;
+
+ /* See asm-x86/spec_ctrl_asm.h for usage. */
+ unsigned int shadow_spec_ctrl;
+ bool use_shadow_spec_ctrl;
+
+ unsigned long __pad;
/* get_stack_bottom() must be 16-byte aligned */
};
diff --git a/xen/include/asm-x86/nops.h b/xen/include/asm-x86/nops.h
index 1a46b97..1738ba6 100644
--- a/xen/include/asm-x86/nops.h
+++ b/xen/include/asm-x86/nops.h
@@ -65,6 +65,12 @@
#define ASM_NOP8 _ASM_MK_NOP(P6_NOP8)
#define ASM_NOP9 _ASM_MK_NOP(P6_NOP9)
+#define ASM_NOP22 ASM_NOP8; ASM_NOP8; ASM_NOP6
+#define ASM_NOP26 ASM_NOP8; ASM_NOP8; ASM_NOP8; ASM_NOP2
+#define ASM_NOP32 ASM_NOP8; ASM_NOP8; ASM_NOP8; ASM_NOP8
+#define ASM_NOP33 ASM_NOP8; ASM_NOP8; ASM_NOP8; ASM_NOP7; ASM_NOP2
+#define ASM_NOP38 ASM_NOP8; ASM_NOP8; ASM_NOP8; ASM_NOP8; ASM_NOP6
+
#define ASM_NOP_MAX 9
#endif /* __X86_ASM_NOPS_H__ */
diff --git a/xen/include/asm-x86/spec_ctrl.h b/xen/include/asm-x86/spec_ctrl.h
index 77f7c60..45814d0 100644
--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -22,6 +22,8 @@
#include <xen/sched.h>
+#include <asm/current.h>
+
void init_speculation_mitigations(void);
/*
@@ -43,6 +45,13 @@ static inline unsigned int spec_ctrl_host_val(const struct domain *d,
return guest_val;
}
+static inline void init_shadow_spec_ctrl_state(void)
+{
+ struct cpu_info *info = get_cpu_info();
+
+ info->shadow_spec_ctrl = info->use_shadow_spec_ctrl = 0;
+}
+
#endif /* !__X86_SPEC_CTRL_H__ */
/*
diff --git a/xen/include/asm-x86/spec_ctrl_asm.h b/xen/include/asm-x86/spec_ctrl_asm.h
new file mode 100644
index 0000000..c380a3d
--- /dev/null
+++ b/xen/include/asm-x86/spec_ctrl_asm.h
@@ -0,0 +1,230 @@
+/******************************************************************************
+ * include/asm-x86/spec_ctrl.h
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Copyright (c) 2017-2018 Citrix Systems Ltd.
+ */
+
+#ifndef __X86_SPEC_CTRL_ASM_H__
+#define __X86_SPEC_CTRL_ASM_H__
+
+#ifdef __ASSEMBLY__
+#include <asm/msr.h>
+
+/*
+ * Saving and restoring MSR_SPEC_CTRL state is a little tricky.
+ *
+ * We want the guests choice of SPEC_CTRL while in guest context, and IBRS
+ * (set or clear, depending on the hardware) while running in Xen context.
+ * Therefore, a simplistic algorithm is:
+ *
+ * - Set/clear IBRS on entry to Xen
+ * - Set the guests' choice on exit to guest
+ * - Leave SPEC_CTRL unchanged on exit to xen
+ *
+ * There are two complicating factors:
+ * 1) HVM guests can have direct access to the MSR, so it can change
+ * behind Xen's back.
+ * 2) An NMI or MCE can interrupt at any point, including early in the entry
+ * path, or late in the exit path after restoring the guest value. This
+ * will corrupt the guest value.
+ *
+ * Factor 1 is dealt with by relying on NMIs/MCEs being blocked immediately
+ * after VMEXIT. The VMEXIT-specific code reads MSR_SPEC_CTRL and updates
+ * current before loading Xen's MSR_SPEC_CTRL setting.
+ *
+ * Factor 2 is harder. We maintain a shadow_spec_ctrl value, and
+ * use_shadow_spec_ctrl boolean per cpu. The synchronous use is:
+ *
+ * 1) Store guest value in shadow_spec_ctrl
+ * 2) Set use_shadow_spec_ctrl boolean
+ * 3) Load guest value into MSR_SPEC_CTRL
+ * 4) Exit to guest
+ * 5) Entry from guest
+ * 6) Clear use_shadow_spec_ctrl boolean
+ *
+ * The asynchronous use for interrupts/exceptions is:
+ * - Set/clear IBRS on entry to Xen
+ * - On exit to Xen, check use_shadow_spec_ctrl
+ * - If set, load shadow_spec_ctrl
+ *
+ * Therefore, an interrupt/exception which hits the synchronous path between
+ * steps 2 and 6 will restore the shadow value rather than leaving Xen's value
+ * loaded and corrupting the value used in guest context.
+ *
+ * The following ASM fragments implement this algorithm. See their local
+ * comments for further details.
+ * - SPEC_CTRL_ENTRY_FROM_VMEXIT
+ * - SPEC_CTRL_ENTRY_FROM_PV
+ * - SPEC_CTRL_ENTRY_FROM_INTR
+ * - SPEC_CTRL_EXIT_TO_XEN
+ * - SPEC_CTRL_EXIT_TO_GUEST
+ */
+
+.macro DO_SPEC_CTRL_ENTRY_FROM_VMEXIT ibrs_val:req
+/*
+ * Requires %rbx=current, %rsp=regs/cpuinfo
+ * Clobbers %rax, %rcx, %rdx
+ *
+ * The common case is that a guest has direct access to MSR_SPEC_CTRL, at
+ * which point we need to save the guest value before setting IBRS for Xen.
+ * Unilaterally saving the guest value is shorter and faster than checking.
+ */
+ mov $MSR_SPEC_CTRL, %ecx
+ rdmsr
+
+ /* Stash the value from hardware. */
+ mov VCPU_arch_msr(%rbx), %rdx
+ mov %al, VCPUMSR_spec_ctrl_host(%rdx)
+ xor %edx, %edx
+
+ /* Clear SPEC_CTRL shadowing *before* loading Xen's value. */
+ movb %dl, CPUINFO_use_shadow_spec_ctrl(%rsp)
+
+ /* Load Xen's intended value. */
+ mov $\ibrs_val, %eax
+ wrmsr
+.endm
+
+.macro DO_SPEC_CTRL_ENTRY maybexen:req ibrs_val:req
+/*
+ * Requires %rsp=regs (also cpuinfo if !maybexen)
+ * Clobbers %rax, %rcx, %rdx
+ *
+ * PV guests can't update MSR_SPEC_CTRL behind Xen's back, so no need to read
+ * it back. Entries from guest context need to clear SPEC_CTRL shadowing,
+ * while entries from Xen must leave shadowing in its current state.
+ */
+ mov $MSR_SPEC_CTRL, %ecx
+
+ .if \maybexen
+ cmpw $__HYPERVISOR_CS, UREGS_cs(%rsp)
+ je .Lentry_from_xen\@
+ .endif
+
+ /*
+ * Clear SPEC_CTRL shadowing *before* loading Xen's value. If entering
+ * from a possibly-xen context, %rsp doesn't necessarily alias the cpuinfo
+ * block so calculate the position directly.
+ */
+ .if \maybexen
+ GET_STACK_END(dx)
+ movb $0, STACK_CPUINFO_FIELD(use_shadow_spec_ctrl)(%rdx)
+ .else
+ movb $0, CPUINFO_use_shadow_spec_ctrl(%rsp)
+ .endif
+
+.Lentry_from_xen\@:
+ /* Load Xen's intended value. */
+ mov $\ibrs_val, %eax
+ xor %edx, %edx
+ wrmsr
+.endm
+
+.macro DO_SPEC_CTRL_EXIT_TO_XEN
+/*
+ * Requires nothing
+ * Clobbers %rax, %rcx, %rdx
+ *
+ * When returning to Xen context, look to see whether SPEC_CTRL shadowing is
+ * in effect, and reload the shadow value. This covers race conditions which
+ * exist with an NMI/MCE/etc hitting late in the return-to-guest path.
+ */
+ GET_STACK_END(dx)
+ cmpb $0, STACK_CPUINFO_FIELD(use_shadow_spec_ctrl)(%rdx)
+ je .Lend_\@
+
+ mov STACK_CPUINFO_FIELD(shadow_spec_ctrl)(%rdx), %eax
+ mov $MSR_SPEC_CTRL, %ecx
+ xor %edx, %edx
+ wrmsr
+
+.Lend_\@:
+.endm
+
+.macro DO_SPEC_CTRL_EXIT_TO_GUEST
+/*
+ * Requires %rbx=current, %rsp=regs/cpuinfo
+ * Clobbers %rax, %rcx, %rdx
+ *
+ * When returning to guest context, set up SPEC_CTRL shadowing and load the
+ * guest value.
+ */
+ mov VCPU_arch_msr(%rbx), %rdx
+ mov VCPUMSR_spec_ctrl_host(%rdx), %eax
+
+ /* Set up shadow value *before* enabling shadowing. */
+ mov %eax, CPUINFO_shadow_spec_ctrl(%rsp)
+
+ /* Set SPEC_CTRL shadowing *before* loading the guest value. */
+ movb $1, CPUINFO_use_shadow_spec_ctrl(%rsp)
+
+ mov $MSR_SPEC_CTRL, %ecx
+ xor %edx, %edx
+ wrmsr
+.endm
+
+/* Use after a VMEXIT from an HVM guest. */
+#define SPEC_CTRL_ENTRY_FROM_VMEXIT \
+ ALTERNATIVE_2 __stringify(ASM_NOP32), \
+ __stringify(DO_SPEC_CTRL_ENTRY_FROM_VMEXIT \
+ ibrs_val=SPEC_CTRL_IBRS), \
+ X86_FEATURE_XEN_IBRS_SET, \
+ __stringify(DO_SPEC_CTRL_ENTRY_FROM_VMEXIT \
+ ibrs_val=0), \
+ X86_FEATURE_XEN_IBRS_CLEAR
+
+/* Use after an entry from PV context (syscall/sysenter/int80/int82/etc). */
+#define SPEC_CTRL_ENTRY_FROM_PV \
+ ALTERNATIVE_2 __stringify(ASM_NOP22), \
+ __stringify(DO_SPEC_CTRL_ENTRY maybexen=0 \
+ ibrs_val=SPEC_CTRL_IBRS), \
+ X86_FEATURE_XEN_IBRS_SET, \
+ __stringify(DO_SPEC_CTRL_ENTRY maybexen=0 ibrs_val=0), \
+ X86_FEATURE_XEN_IBRS_CLEAR
+
+/* Use in interrupt/exception context. May interrupt Xen or PV context. */
+#define SPEC_CTRL_ENTRY_FROM_INTR \
+ ALTERNATIVE_2 __stringify(ASM_NOP38), \
+ __stringify(DO_SPEC_CTRL_ENTRY maybexen=1 \
+ ibrs_val=SPEC_CTRL_IBRS), \
+ X86_FEATURE_XEN_IBRS_SET, \
+ __stringify(DO_SPEC_CTRL_ENTRY maybexen=1 ibrs_val=0), \
+ X86_FEATURE_XEN_IBRS_CLEAR
+
+/* Use when exiting to Xen context. */
+#define SPEC_CTRL_EXIT_TO_XEN \
+ ALTERNATIVE_2 __stringify(ASM_NOP26), \
+ DO_SPEC_CTRL_EXIT_TO_XEN, X86_FEATURE_XEN_IBRS_SET, \
+ DO_SPEC_CTRL_EXIT_TO_XEN, X86_FEATURE_XEN_IBRS_CLEAR
+
+/* Use when exiting to guest context. */
+#define SPEC_CTRL_EXIT_TO_GUEST \
+ ALTERNATIVE_2 __stringify(ASM_NOP33), \
+ DO_SPEC_CTRL_EXIT_TO_GUEST, X86_FEATURE_XEN_IBRS_SET, \
+ DO_SPEC_CTRL_EXIT_TO_GUEST, X86_FEATURE_XEN_IBRS_CLEAR
+
+#endif /* __ASSEMBLY__ */
+#endif /* !__X86_SPEC_CTRL_ASM_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v8 13/17] x86/boot: Calculate the most appropriate BTI mitigation to use
2018-01-12 18:00 [PATCH v8 00/17] x86: Mitigations for SP2/CVE-2017-5715/Branch Target Injection Andrew Cooper
` (11 preceding siblings ...)
2018-01-12 18:01 ` [PATCH v8 12/17] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point Andrew Cooper
@ 2018-01-12 18:01 ` Andrew Cooper
2018-01-16 14:10 ` Boris Ostrovsky
2018-01-12 18:01 ` [PATCH v8 14/17] x86/entry: Clobber the Return Stack Buffer/Return Address Stack on entry to Xen Andrew Cooper
` (3 subsequent siblings)
16 siblings, 1 reply; 55+ messages in thread
From: Andrew Cooper @ 2018-01-12 18:01 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v7:
* static, and tweak comment
---
docs/misc/xen-command-line.markdown | 6 ++-
xen/arch/x86/spec_ctrl.c | 104 ++++++++++++++++++++++++++++++++++--
2 files changed, 105 insertions(+), 5 deletions(-)
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index b42abc6..1aa1225 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -246,7 +246,7 @@ enough. Setting this to a high value may cause boot failure, particularly if
the NMI watchdog is also enabled.
### bti (x86)
-> `= List of [ thunk=retpoline|lfence|jmp ]`
+> `= List of [ thunk=retpoline|lfence|jmp, ibrs=<bool> ]`
Branch Target Injection controls. By default, Xen will pick the most
appropriate BTI mitigations based on compiled in support, loaded microcode,
@@ -261,6 +261,10 @@ locations. The default thunk is `retpoline` (generally preferred for Intel
hardware), with the alternatives being `jmp` (a `jmp *%reg` gadget, minimal
overhead), and `lfence` (an `lfence; jmp *%reg` gadget, preferred for AMD).
+On hardware supporting IBRS, the `ibrs=` option can be used to force or
+prevent Xen using the feature itself. If Xen is not using IBRS itself,
+functionality is still set up so IBRS can be virtualised for guests.
+
### xenheap\_megabytes (arm32)
> `= <size>`
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 89e7287..7b0daaf 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -20,6 +20,7 @@
#include <xen/init.h>
#include <xen/lib.h>
+#include <asm/microcode.h>
#include <asm/processor.h>
#include <asm/spec_ctrl.h>
@@ -31,11 +32,12 @@ static enum ind_thunk {
THUNK_LFENCE,
THUNK_JMP,
} opt_thunk __initdata = THUNK_DEFAULT;
+static int opt_ibrs __initdata = -1;
static int __init parse_bti(const char *s)
{
const char *ss;
- int rc = 0;
+ int val, rc = 0;
do {
ss = strchr(s, ',');
@@ -55,6 +57,8 @@ static int __init parse_bti(const char *s)
else
rc = -EINVAL;
}
+ else if ( (val = parse_boolean("ibrs", s, ss)) >= 0 )
+ opt_ibrs = val;
else
rc = -EINVAL;
@@ -91,24 +95,82 @@ static void __init print_details(enum ind_thunk thunk)
printk(XENLOG_DEBUG " Compiled-in support: INDIRECT_THUNK\n");
printk(XENLOG_INFO
- "BTI mitigations: Thunk %s\n",
+ "BTI mitigations: Thunk %s, Others:%s\n",
thunk == THUNK_NONE ? "N/A" :
thunk == THUNK_RETPOLINE ? "RETPOLINE" :
thunk == THUNK_LFENCE ? "LFENCE" :
- thunk == THUNK_JMP ? "JMP" : "?");
+ thunk == THUNK_JMP ? "JMP" : "?",
+ boot_cpu_has(X86_FEATURE_XEN_IBRS_SET) ? " IBRS+" :
+ boot_cpu_has(X86_FEATURE_XEN_IBRS_CLEAR) ? " IBRS-" : "");
+}
+
+/* Calculate whether Retpoline is known-safe on this CPU. */
+static bool __init retpoline_safe(void)
+{
+ unsigned int ucode_rev = this_cpu(ucode_cpu_info).cpu_sig.rev;
+
+ if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
+ return true;
+
+ if ( boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ||
+ boot_cpu_data.x86 != 6 )
+ return false;
+
+ switch ( boot_cpu_data.x86_model )
+ {
+ case 0x17: /* Penryn */
+ case 0x1d: /* Dunnington */
+ case 0x1e: /* Nehalem */
+ case 0x1f: /* Auburndale / Havendale */
+ case 0x1a: /* Nehalem EP */
+ case 0x2e: /* Nehalem EX */
+ case 0x25: /* Westmere */
+ case 0x2c: /* Westmere EP */
+ case 0x2f: /* Westmere EX */
+ case 0x2a: /* SandyBridge */
+ case 0x2d: /* SandyBridge EP/EX */
+ case 0x3a: /* IvyBridge */
+ case 0x3e: /* IvyBridge EP/EX */
+ case 0x3c: /* Haswell */
+ case 0x3f: /* Haswell EX/EP */
+ case 0x45: /* Haswell D */
+ case 0x46: /* Haswell H */
+ return true;
+
+ /*
+ * Broadwell processors are retpoline-safe after specific microcode
+ * versions.
+ */
+ case 0x3d: /* Broadwell */
+ return ucode_rev >= 0x28;
+ case 0x47: /* Broadwell H */
+ return ucode_rev >= 0x1b;
+ case 0x4f: /* Broadwell EP/EX */
+ return ucode_rev >= 0xb000025;
+ case 0x56: /* Broadwell D */
+ return false; /* TBD. */
+
+ /*
+ * Skylake and later processors are not retpoline-safe.
+ */
+ default:
+ return false;
+ }
}
void __init init_speculation_mitigations(void)
{
enum ind_thunk thunk = THUNK_DEFAULT;
+ bool ibrs = false;
/*
* Has the user specified any custom BTI mitigations? If so, follow their
* instructions exactly and disable all heuristics.
*/
- if ( opt_thunk != THUNK_DEFAULT )
+ if ( opt_thunk != THUNK_DEFAULT || opt_ibrs != -1 )
{
thunk = opt_thunk;
+ ibrs = !!opt_ibrs;
}
else
{
@@ -124,7 +186,21 @@ void __init init_speculation_mitigations(void)
*/
if ( cpu_has_lfence_dispatch )
thunk = THUNK_LFENCE;
+ /*
+ * On Intel hardware, we'd like to use retpoline in preference to
+ * IBRS, but only if it is safe on this hardware.
+ */
+ else if ( boot_cpu_has(X86_FEATURE_IBRSB) )
+ {
+ if ( retpoline_safe() )
+ thunk = THUNK_RETPOLINE;
+ else
+ ibrs = true;
+ }
}
+ /* Without compiler thunk support, use IBRS if available. */
+ else if ( boot_cpu_has(X86_FEATURE_IBRSB) )
+ ibrs = true;
}
/*
@@ -135,6 +211,13 @@ void __init init_speculation_mitigations(void)
thunk = THUNK_NONE;
/*
+ * If IBRS is in use and thunks are compiled in, there is no point
+ * suffering extra overhead. Switch to the least-overhead thunk.
+ */
+ if ( ibrs && thunk == THUNK_DEFAULT )
+ thunk = THUNK_JMP;
+
+ /*
* If there are still no thunk preferences, the compiled default is
* actually retpoline, and it is better than nothing.
*/
@@ -147,6 +230,19 @@ void __init init_speculation_mitigations(void)
else if ( thunk == THUNK_JMP )
setup_force_cpu_cap(X86_FEATURE_IND_THUNK_JMP);
+ if ( boot_cpu_has(X86_FEATURE_IBRSB) )
+ {
+ /*
+ * Even if we've chosen to not have IBRS set in Xen context, we still
+ * need the IBRS entry/exit logic to virtualise IBRS support for
+ * guests.
+ */
+ if ( ibrs )
+ setup_force_cpu_cap(X86_FEATURE_XEN_IBRS_SET);
+ else
+ setup_force_cpu_cap(X86_FEATURE_XEN_IBRS_CLEAR);
+ }
+
print_details(thunk);
}
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v8 14/17] x86/entry: Clobber the Return Stack Buffer/Return Address Stack on entry to Xen
2018-01-12 18:00 [PATCH v8 00/17] x86: Mitigations for SP2/CVE-2017-5715/Branch Target Injection Andrew Cooper
` (12 preceding siblings ...)
2018-01-12 18:01 ` [PATCH v8 13/17] x86/boot: Calculate the most appropriate BTI mitigation to use Andrew Cooper
@ 2018-01-12 18:01 ` Andrew Cooper
2018-01-12 18:01 ` [PATCH v8 15/17] x86/ctxt: Issue a speculation barrier between vcpu contexts Andrew Cooper
` (2 subsequent siblings)
16 siblings, 0 replies; 55+ messages in thread
From: Andrew Cooper @ 2018-01-12 18:01 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
ret instructions are speculated directly to values recorded in the RSB/RAS, as
there is no uncertainty in well-formed code. Guests can take advantage of
this in two ways:
1) If they can find a path in Xen which executes more ret instructions than
call instructions. (At least one in the waitqueue infrastructure,
probably others.)
2) Use the fact that the RSB/RAS in hardware is actually a circular stack
without a concept of empty. (When it logically empties, stale values
will start being used.)
To mitigate, unconditionally overwrite the RSB on entry to Xen with gadgets
which will capture and contain rogue speculation.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v7:
* Rewritten almost from scratch. See code comments for details.
v8:
* Use jmp rather than call to contain speculation. It doesn't affect the
correctness of containment, but removes 6 bytes.
---
docs/misc/xen-command-line.markdown | 6 +++++-
xen/arch/x86/spec_ctrl.c | 34 +++++++++++++++++++++++++++++--
xen/include/asm-x86/cpufeatures.h | 2 ++
xen/include/asm-x86/nops.h | 1 +
xen/include/asm-x86/spec_ctrl_asm.h | 40 +++++++++++++++++++++++++++++++++++++
5 files changed, 80 insertions(+), 3 deletions(-)
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 1aa1225..8510e47 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -246,7 +246,7 @@ enough. Setting this to a high value may cause boot failure, particularly if
the NMI watchdog is also enabled.
### bti (x86)
-> `= List of [ thunk=retpoline|lfence|jmp, ibrs=<bool> ]`
+> `= List of [ thunk=retpoline|lfence|jmp, ibrs=<bool>, rsb_{vmexit,native}=<bool> ]`
Branch Target Injection controls. By default, Xen will pick the most
appropriate BTI mitigations based on compiled in support, loaded microcode,
@@ -265,6 +265,10 @@ On hardware supporting IBRS, the `ibrs=` option can be used to force or
prevent Xen using the feature itself. If Xen is not using IBRS itself,
functionality is still set up so IBRS can be virtualised for guests.
+The `rsb_vmexit=` and `rsb_native=` options can be used to fine tune when the
+RSB gets overwritten. There are individual controls for an entry from HVM
+context, and an entry from a native (PV or Xen) context.
+
### xenheap\_megabytes (arm32)
> `= <size>`
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 7b0daaf..680fabe 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -33,6 +33,7 @@ static enum ind_thunk {
THUNK_JMP,
} opt_thunk __initdata = THUNK_DEFAULT;
static int opt_ibrs __initdata = -1;
+static bool opt_rsb_native __initdata = true, opt_rsb_vmexit __initdata = true;
static int __init parse_bti(const char *s)
{
@@ -59,6 +60,10 @@ static int __init parse_bti(const char *s)
}
else if ( (val = parse_boolean("ibrs", s, ss)) >= 0 )
opt_ibrs = val;
+ else if ( (val = parse_boolean("rsb_native", s, ss)) >= 0 )
+ opt_rsb_native = val;
+ else if ( (val = parse_boolean("rsb_vmexit", s, ss)) >= 0 )
+ opt_rsb_vmexit = val;
else
rc = -EINVAL;
@@ -95,13 +100,15 @@ static void __init print_details(enum ind_thunk thunk)
printk(XENLOG_DEBUG " Compiled-in support: INDIRECT_THUNK\n");
printk(XENLOG_INFO
- "BTI mitigations: Thunk %s, Others:%s\n",
+ "BTI mitigations: Thunk %s, Others:%s%s%s\n",
thunk == THUNK_NONE ? "N/A" :
thunk == THUNK_RETPOLINE ? "RETPOLINE" :
thunk == THUNK_LFENCE ? "LFENCE" :
thunk == THUNK_JMP ? "JMP" : "?",
boot_cpu_has(X86_FEATURE_XEN_IBRS_SET) ? " IBRS+" :
- boot_cpu_has(X86_FEATURE_XEN_IBRS_CLEAR) ? " IBRS-" : "");
+ boot_cpu_has(X86_FEATURE_XEN_IBRS_CLEAR) ? " IBRS-" : "",
+ boot_cpu_has(X86_FEATURE_RSB_NATIVE) ? " RSB_NATIVE" : "",
+ boot_cpu_has(X86_FEATURE_RSB_VMEXIT) ? " RSB_VMEXIT" : "");
}
/* Calculate whether Retpoline is known-safe on this CPU. */
@@ -243,6 +250,29 @@ void __init init_speculation_mitigations(void)
setup_force_cpu_cap(X86_FEATURE_XEN_IBRS_CLEAR);
}
+ /*
+ * PV guests can poison the RSB to any virtual address from which
+ * they can execute a call instruction. This is necessarily outside
+ * of the Xen supervisor mappings.
+ *
+ * With SMEP enabled, the processor won't speculate into user
+ * mappings. Therefore, we don't need to worry about poisoned
+ * entries from 64bit PV guests.
+ *
+ * 32bit PV guest kernels run in ring 1, so use supervisor mappings.
+ * If a processors speculates to 32bit PV guest kernel mappings, it is
+ * speculating in 64bit supervisor mode, and can leak data.
+ */
+ if ( opt_rsb_native )
+ setup_force_cpu_cap(X86_FEATURE_RSB_NATIVE);
+
+ /*
+ * HVM guests can always poison the RSB to point at Xen supervisor
+ * mappings.
+ */
+ if ( opt_rsb_vmexit )
+ setup_force_cpu_cap(X86_FEATURE_RSB_VMEXIT);
+
print_details(thunk);
}
diff --git a/xen/include/asm-x86/cpufeatures.h b/xen/include/asm-x86/cpufeatures.h
index dd2388f..0ee4a1f 100644
--- a/xen/include/asm-x86/cpufeatures.h
+++ b/xen/include/asm-x86/cpufeatures.h
@@ -28,3 +28,5 @@ XEN_CPUFEATURE(IND_THUNK_JMP, (FSCAPINTS+0)*32+14) /* Use IND_THUNK_JMP */
XEN_CPUFEATURE(XEN_IBPB, (FSCAPINTS+0)*32+15) /* IBRSB || IBPB */
XEN_CPUFEATURE(XEN_IBRS_SET, (FSCAPINTS+0)*32+16) /* IBRSB && IRBS set in Xen */
XEN_CPUFEATURE(XEN_IBRS_CLEAR, (FSCAPINTS+0)*32+17) /* IBRSB && IBRS clear in Xen */
+XEN_CPUFEATURE(RSB_NATIVE, (FSCAPINTS+0)*32+18) /* RSB overwrite needed for native */
+XEN_CPUFEATURE(RSB_VMEXIT, (FSCAPINTS+0)*32+20) /* RSB overwrite needed for vmexit */
diff --git a/xen/include/asm-x86/nops.h b/xen/include/asm-x86/nops.h
index 1738ba6..e7de02b 100644
--- a/xen/include/asm-x86/nops.h
+++ b/xen/include/asm-x86/nops.h
@@ -69,6 +69,7 @@
#define ASM_NOP26 ASM_NOP8; ASM_NOP8; ASM_NOP8; ASM_NOP2
#define ASM_NOP32 ASM_NOP8; ASM_NOP8; ASM_NOP8; ASM_NOP8
#define ASM_NOP33 ASM_NOP8; ASM_NOP8; ASM_NOP8; ASM_NOP7; ASM_NOP2
+#define ASM_NOP34 ASM_NOP8; ASM_NOP8; ASM_NOP8; ASM_NOP8; ASM_NOP2
#define ASM_NOP38 ASM_NOP8; ASM_NOP8; ASM_NOP8; ASM_NOP8; ASM_NOP6
#define ASM_NOP_MAX 9
diff --git a/xen/include/asm-x86/spec_ctrl_asm.h b/xen/include/asm-x86/spec_ctrl_asm.h
index c380a3d..5293b65 100644
--- a/xen/include/asm-x86/spec_ctrl_asm.h
+++ b/xen/include/asm-x86/spec_ctrl_asm.h
@@ -73,6 +73,40 @@
* - SPEC_CTRL_EXIT_TO_GUEST
*/
+.macro DO_OVERWRITE_RSB
+/*
+ * Req: %rsp=regs
+ * Clobbers %rax, %rcx
+ *
+ * Requires 256 bytes of stack space, but %rsp has no net change. Based on
+ * Google's performance numbers, the loop is unrolled to 16 iterations and two
+ * calls per iteration.
+ *
+ * The call filling the RSB needs a nonzero displacement, but we use "1:
+ * pause, jmp 1b" to safely contains any ret-based speculation, even if the
+ * loop is speculatively executed prematurely.
+ *
+ * %rsp is preserved by using an extra GPR because a) we've got plenty spare,
+ * b) the two movs are shorter to encode than `add $32*8, %rsp`, and c) can be
+ * optimised with mov-elimination in modern cores.
+ */
+ mov $16, %ecx /* 16 iterations, two calls per loop */
+ mov %rsp, %rax /* Store the current %rsp */
+
+.Lloop_\@:
+
+ .rept 2 /* Unrolled twice. */
+ call 2f /* Create an RSB entry. */
+1: pause
+ jmp 1b /* Capture rogue speculation. */
+2:
+ .endr
+
+ sub $1, %ecx
+ jnz .Lloop_\@
+ mov %rax, %rsp /* Retore old %rsp */
+.endm
+
.macro DO_SPEC_CTRL_ENTRY_FROM_VMEXIT ibrs_val:req
/*
* Requires %rbx=current, %rsp=regs/cpuinfo
@@ -178,6 +212,8 @@
/* Use after a VMEXIT from an HVM guest. */
#define SPEC_CTRL_ENTRY_FROM_VMEXIT \
+ ALTERNATIVE __stringify(ASM_NOP34), \
+ DO_OVERWRITE_RSB, X86_FEATURE_RSB_VMEXIT; \
ALTERNATIVE_2 __stringify(ASM_NOP32), \
__stringify(DO_SPEC_CTRL_ENTRY_FROM_VMEXIT \
ibrs_val=SPEC_CTRL_IBRS), \
@@ -188,6 +224,8 @@
/* Use after an entry from PV context (syscall/sysenter/int80/int82/etc). */
#define SPEC_CTRL_ENTRY_FROM_PV \
+ ALTERNATIVE __stringify(ASM_NOP34), \
+ DO_OVERWRITE_RSB, X86_FEATURE_RSB_NATIVE; \
ALTERNATIVE_2 __stringify(ASM_NOP22), \
__stringify(DO_SPEC_CTRL_ENTRY maybexen=0 \
ibrs_val=SPEC_CTRL_IBRS), \
@@ -197,6 +235,8 @@
/* Use in interrupt/exception context. May interrupt Xen or PV context. */
#define SPEC_CTRL_ENTRY_FROM_INTR \
+ ALTERNATIVE __stringify(ASM_NOP34), \
+ DO_OVERWRITE_RSB, X86_FEATURE_RSB_NATIVE; \
ALTERNATIVE_2 __stringify(ASM_NOP38), \
__stringify(DO_SPEC_CTRL_ENTRY maybexen=1 \
ibrs_val=SPEC_CTRL_IBRS), \
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v8 15/17] x86/ctxt: Issue a speculation barrier between vcpu contexts
2018-01-12 18:00 [PATCH v8 00/17] x86: Mitigations for SP2/CVE-2017-5715/Branch Target Injection Andrew Cooper
` (13 preceding siblings ...)
2018-01-12 18:01 ` [PATCH v8 14/17] x86/entry: Clobber the Return Stack Buffer/Return Address Stack on entry to Xen Andrew Cooper
@ 2018-01-12 18:01 ` Andrew Cooper
2018-01-15 12:54 ` David Woodhouse
2018-01-12 18:01 ` [PATCH v8 16/17] x86/cpuid: Offer Indirect Branch Controls to guests Andrew Cooper
2018-01-12 18:01 ` [PATCH v8 17/17] x86/idle: Clear SPEC_CTRL while idle Andrew Cooper
16 siblings, 1 reply; 55+ messages in thread
From: Andrew Cooper @ 2018-01-12 18:01 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
v7:
* Use the opt_ibpb boolean rather than using a cpufeature flag.
---
docs/misc/xen-command-line.markdown | 5 ++++-
xen/arch/x86/domain.c | 3 +++
xen/arch/x86/spec_ctrl.c | 10 +++++++++-
xen/include/asm-x86/spec_ctrl.h | 2 ++
4 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 8510e47..a24d585 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -246,7 +246,7 @@ enough. Setting this to a high value may cause boot failure, particularly if
the NMI watchdog is also enabled.
### bti (x86)
-> `= List of [ thunk=retpoline|lfence|jmp, ibrs=<bool>, rsb_{vmexit,native}=<bool> ]`
+> `= List of [ thunk=retpoline|lfence|jmp, ibrs=<bool>, ibpb=<bool>, rsb_{vmexit,native}=<bool> ]`
Branch Target Injection controls. By default, Xen will pick the most
appropriate BTI mitigations based on compiled in support, loaded microcode,
@@ -265,6 +265,9 @@ On hardware supporting IBRS, the `ibrs=` option can be used to force or
prevent Xen using the feature itself. If Xen is not using IBRS itself,
functionality is still set up so IBRS can be virtualised for guests.
+On hardware supporting IBPB, the `ibpb=` option can be used to prevent Xen
+from issuing Branch Prediction Barriers on vcpu context switches.
+
The `rsb_vmexit=` and `rsb_native=` options can be used to fine tune when the
RSB gets overwritten. There are individual controls for an entry from HVM
context, and an entry from a native (PV or Xen) context.
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 8849e3f..ba10ed9 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1736,6 +1736,9 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
}
ctxt_switch_levelling(next);
+
+ if ( opt_ibpb )
+ wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
}
context_saved(prev);
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 680fabe..ae3e7d7 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -33,6 +33,7 @@ static enum ind_thunk {
THUNK_JMP,
} opt_thunk __initdata = THUNK_DEFAULT;
static int opt_ibrs __initdata = -1;
+bool __read_mostly opt_ibpb = true;
static bool opt_rsb_native __initdata = true, opt_rsb_vmexit __initdata = true;
static int __init parse_bti(const char *s)
@@ -60,6 +61,8 @@ static int __init parse_bti(const char *s)
}
else if ( (val = parse_boolean("ibrs", s, ss)) >= 0 )
opt_ibrs = val;
+ else if ( (val = parse_boolean("ibpb", s, ss)) >= 0 )
+ opt_ibpb = val;
else if ( (val = parse_boolean("rsb_native", s, ss)) >= 0 )
opt_rsb_native = val;
else if ( (val = parse_boolean("rsb_vmexit", s, ss)) >= 0 )
@@ -100,13 +103,14 @@ static void __init print_details(enum ind_thunk thunk)
printk(XENLOG_DEBUG " Compiled-in support: INDIRECT_THUNK\n");
printk(XENLOG_INFO
- "BTI mitigations: Thunk %s, Others:%s%s%s\n",
+ "BTI mitigations: Thunk %s, Others:%s%s%s%s\n",
thunk == THUNK_NONE ? "N/A" :
thunk == THUNK_RETPOLINE ? "RETPOLINE" :
thunk == THUNK_LFENCE ? "LFENCE" :
thunk == THUNK_JMP ? "JMP" : "?",
boot_cpu_has(X86_FEATURE_XEN_IBRS_SET) ? " IBRS+" :
boot_cpu_has(X86_FEATURE_XEN_IBRS_CLEAR) ? " IBRS-" : "",
+ opt_ibpb ? " IBPB" : "",
boot_cpu_has(X86_FEATURE_RSB_NATIVE) ? " RSB_NATIVE" : "",
boot_cpu_has(X86_FEATURE_RSB_VMEXIT) ? " RSB_VMEXIT" : "");
}
@@ -273,6 +277,10 @@ void __init init_speculation_mitigations(void)
if ( opt_rsb_vmexit )
setup_force_cpu_cap(X86_FEATURE_RSB_VMEXIT);
+ /* Check we have hardware IBPB support before using it... */
+ if ( !boot_cpu_has(X86_FEATURE_IBRSB) && !boot_cpu_has(X86_FEATURE_IBPB) )
+ opt_ibpb = false;
+
print_details(thunk);
}
diff --git a/xen/include/asm-x86/spec_ctrl.h b/xen/include/asm-x86/spec_ctrl.h
index 45814d0..f139581 100644
--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -26,6 +26,8 @@
void init_speculation_mitigations(void);
+extern bool opt_ibpb;
+
/*
* For guests which know about IBRS but are not told about STIBP running on
* hardware supporting hyperthreading, the guest doesn't know to protect
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v8 16/17] x86/cpuid: Offer Indirect Branch Controls to guests
2018-01-12 18:00 [PATCH v8 00/17] x86: Mitigations for SP2/CVE-2017-5715/Branch Target Injection Andrew Cooper
` (14 preceding siblings ...)
2018-01-12 18:01 ` [PATCH v8 15/17] x86/ctxt: Issue a speculation barrier between vcpu contexts Andrew Cooper
@ 2018-01-12 18:01 ` Andrew Cooper
2018-01-12 18:01 ` [PATCH v8 17/17] x86/idle: Clear SPEC_CTRL while idle Andrew Cooper
16 siblings, 0 replies; 55+ messages in thread
From: Andrew Cooper @ 2018-01-12 18:01 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
With all infrastructure in place, it is now safe to let guests see and use
these features. Allow AMD's IBPB to be set even on Intel hardware, so the
toolstack can express "IBPB only" to guests.
This also requires updating the libxc logic to understand the e8b feature
leaf, which has the side effect of also offering CLZERO on applicable
hardware.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
tools/libxc/xc_cpuid_x86.c | 4 +++-
xen/arch/x86/cpuid.c | 8 ++++++++
xen/include/public/arch-x86/cpufeatureset.h | 6 +++---
3 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 25b922e..9fa2f7c 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -465,7 +465,9 @@ static void xc_cpuid_hvm_policy(xc_interface *xch,
case 0x80000008:
regs[0] &= 0x0000ffffu;
- regs[1] = regs[3] = 0;
+ regs[1] = info->featureset[featureword_of(X86_FEATURE_CLZERO)];
+ /* regs[2] handled in the per-vendor logic. */
+ regs[3] = 0;
break;
case 0x00000002: /* Intel cache info (dumped by AMD policy) */
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 2ef71d2..e1b8c7a 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -354,6 +354,14 @@ static void __init calculate_host_policy(void)
recalculate_xstate(p);
recalculate_misc(p);
+ /*
+ * AMD's IPBP is a subset of Intel's IBRS/IBPB. For performance reasons,
+ * we may want to offer a guest only IBPB and not IBRS, so allow the AMD
+ * CPUID bit to be used whenever the hardware supports IBPB.
+ */
+ if ( p->feat.ibrsb )
+ p->extd.ibpb = true;
+
if ( p->extd.svm )
{
/* Clamp to implemented features which require hardware support. */
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index e148755..aaeb149 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -237,13 +237,13 @@ XEN_CPUFEATURE(EFRO, 7*32+10) /* APERF/MPERF Read Only interface */
/* AMD-defined CPU features, CPUID level 0x80000008.ebx, word 8 */
XEN_CPUFEATURE(CLZERO, 8*32+ 0) /*A CLZERO instruction */
-XEN_CPUFEATURE(IBPB, 8*32+12) /* IBPB support only (no IBRS, used by AMD) */
+XEN_CPUFEATURE(IBPB, 8*32+12) /*A IBPB support only (no IBRS, used by AMD) */
/* Intel-defined CPU features, CPUID level 0x00000007:0.edx, word 9 */
XEN_CPUFEATURE(AVX512_4VNNIW, 9*32+ 2) /*A AVX512 Neural Network Instructions */
XEN_CPUFEATURE(AVX512_4FMAPS, 9*32+ 3) /*A AVX512 Multiply Accumulation Single Precision */
-XEN_CPUFEATURE(IBRSB, 9*32+26) /* IBRS and IBPB support (used by Intel) */
-XEN_CPUFEATURE(STIBP, 9*32+27) /* STIBP */
+XEN_CPUFEATURE(IBRSB, 9*32+26) /*A IBRS and IBPB support (used by Intel) */
+XEN_CPUFEATURE(STIBP, 9*32+27) /*A STIBP */
#endif /* XEN_CPUFEATURE */
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v8 17/17] x86/idle: Clear SPEC_CTRL while idle
2018-01-12 18:00 [PATCH v8 00/17] x86: Mitigations for SP2/CVE-2017-5715/Branch Target Injection Andrew Cooper
` (15 preceding siblings ...)
2018-01-12 18:01 ` [PATCH v8 16/17] x86/cpuid: Offer Indirect Branch Controls to guests Andrew Cooper
@ 2018-01-12 18:01 ` Andrew Cooper
16 siblings, 0 replies; 55+ messages in thread
From: Andrew Cooper @ 2018-01-12 18:01 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper
On contemporary hardware, setting IBRS/STIBP has a performance impact on
adjacent hyperthreads. It is therefore recommended to clear the setting
before becoming idle, to avoid an idle core preventing adjacent userspace
execution from running at full performance.
Care must be taken to ensure there are no ret or indirect branch instructions
between spec_ctrl_{enter,exit}_idle() invocations, which are forced always
inline. Care must also be taken to avoid using spec_ctrl_enter_idle() between
flushing caches and becoming idle, in cases where that matters.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/acpi/cpu_idle.c | 21 +++++++++++++++++++++
xen/arch/x86/cpu/mwait-idle.c | 7 +++++++
xen/arch/x86/domain.c | 8 ++++++++
xen/include/asm-x86/spec_ctrl.h | 34 ++++++++++++++++++++++++++++++++++
4 files changed, 70 insertions(+)
diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
index cb1c5da..3f72bda 100644
--- a/xen/arch/x86/acpi/cpu_idle.c
+++ b/xen/arch/x86/acpi/cpu_idle.c
@@ -55,6 +55,7 @@
#include <asm/mwait.h>
#include <xen/notifier.h>
#include <xen/cpu.h>
+#include <asm/spec_ctrl.h>
/*#define DEBUG_PM_CX*/
@@ -414,8 +415,14 @@ void mwait_idle_with_hints(unsigned int eax, unsigned int ecx)
*/
if ( (expires > NOW() || expires == 0) && !softirq_pending(cpu) )
{
+ struct cpu_info *info = get_cpu_info();
+
cpumask_set_cpu(cpu, &cpuidle_mwait_flags);
+
+ spec_ctrl_enter_idle(info);
__mwait(eax, ecx);
+ spec_ctrl_exit_idle(info);
+
cpumask_clear_cpu(cpu, &cpuidle_mwait_flags);
}
@@ -430,6 +437,8 @@ static void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cx)
static void acpi_idle_do_entry(struct acpi_processor_cx *cx)
{
+ struct cpu_info *info = get_cpu_info();
+
switch ( cx->entry_method )
{
case ACPI_CSTATE_EM_FFH:
@@ -437,15 +446,19 @@ static void acpi_idle_do_entry(struct acpi_processor_cx *cx)
acpi_processor_ffh_cstate_enter(cx);
return;
case ACPI_CSTATE_EM_SYSIO:
+ spec_ctrl_enter_idle(info);
/* IO port based C-state */
inb(cx->address);
/* Dummy wait op - must do something useless after P_LVL2 read
because chipsets cannot guarantee that STPCLK# signal
gets asserted in time to freeze execution properly. */
inl(pmtmr_ioport);
+ spec_ctrl_exit_idle(info);
return;
case ACPI_CSTATE_EM_HALT:
+ spec_ctrl_enter_idle(info);
safe_halt();
+ spec_ctrl_exit_idle(info);
local_irq_disable();
return;
}
@@ -573,7 +586,13 @@ static void acpi_processor_idle(void)
if ( pm_idle_save )
pm_idle_save();
else
+ {
+ struct cpu_info *info = get_cpu_info();
+
+ spec_ctrl_enter_idle(info);
safe_halt();
+ spec_ctrl_exit_idle(info);
+ }
return;
}
@@ -752,6 +771,7 @@ void acpi_dead_idle(void)
* Otherwise, CPU may still hold dirty data, breaking cache coherency,
* leading to strange errors.
*/
+ spec_ctrl_enter_idle(get_cpu_info());
wbinvd();
while ( 1 )
@@ -781,6 +801,7 @@ void acpi_dead_idle(void)
u32 address = cx->address;
u32 pmtmr_ioport_local = pmtmr_ioport;
+ spec_ctrl_enter_idle(get_cpu_info());
wbinvd();
while ( 1 )
diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index 762dff1..e357f29 100644
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -58,6 +58,7 @@
#include <asm/hpet.h>
#include <asm/mwait.h>
#include <asm/msr.h>
+#include <asm/spec_ctrl.h>
#include <acpi/cpufreq/cpufreq.h>
#define MWAIT_IDLE_VERSION "0.4.1"
@@ -736,7 +737,13 @@ static void mwait_idle(void)
if (pm_idle_save)
pm_idle_save();
else
+ {
+ struct cpu_info *info = get_cpu_info();
+
+ spec_ctrl_enter_idle(info);
safe_halt();
+ spec_ctrl_exit_idle(info);
+ }
return;
}
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index ba10ed9..04e9902 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -55,6 +55,7 @@
#include <asm/hvm/viridian.h>
#include <asm/debugreg.h>
#include <asm/msr.h>
+#include <asm/spec_ctrl.h>
#include <asm/traps.h>
#include <asm/nmi.h>
#include <asm/mce.h>
@@ -75,9 +76,15 @@ void (*dead_idle) (void) __read_mostly = default_dead_idle;
static void default_idle(void)
{
+ struct cpu_info *info = get_cpu_info();
+
local_irq_disable();
if ( cpu_is_haltable(smp_processor_id()) )
+ {
+ spec_ctrl_enter_idle(info);
safe_halt();
+ spec_ctrl_exit_idle(info);
+ }
else
local_irq_enable();
}
@@ -89,6 +96,7 @@ void default_dead_idle(void)
* held by the CPUs spinning here indefinitely, and get discarded by
* a subsequent INIT.
*/
+ spec_ctrl_enter_idle(get_cpu_info());
wbinvd();
for ( ; ; )
halt();
diff --git a/xen/include/asm-x86/spec_ctrl.h b/xen/include/asm-x86/spec_ctrl.h
index f139581..11c40c4 100644
--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -22,7 +22,9 @@
#include <xen/sched.h>
+#include <asm/alternative.h>
#include <asm/current.h>
+#include <asm/msr-index.h>
void init_speculation_mitigations(void);
@@ -54,6 +56,38 @@ static inline void init_shadow_spec_ctrl_state(void)
info->shadow_spec_ctrl = info->use_shadow_spec_ctrl = 0;
}
+/* WARNING! `ret`, `call *`, `jmp *` not safe after this call. */
+static always_inline void spec_ctrl_enter_idle(struct cpu_info *info)
+{
+ uint32_t val = 0;
+
+ /*
+ * Latch the new shadow value, then enable shadowing, then update the MSR.
+ * There are no SMP issues here; only local processor ordering concerns.
+ */
+ info->shadow_spec_ctrl = val;
+ barrier();
+ info->use_shadow_spec_ctrl = true;
+ barrier();
+ asm volatile ( ALTERNATIVE(ASM_NOP3, "wrmsr", X86_FEATURE_XEN_IBRS_SET)
+ :: "a" (val), "c" (MSR_SPEC_CTRL), "d" (0) : "memory" );
+}
+
+/* WARNING! `ret`, `call *`, `jmp *` not safe before this call. */
+static always_inline void spec_ctrl_exit_idle(struct cpu_info *info)
+{
+ uint32_t val = SPEC_CTRL_IBRS;
+
+ /*
+ * Disable shadowing before updating the MSR. There are no SMP issues
+ * here; only local processor ordering concerns.
+ */
+ info->use_shadow_spec_ctrl = false;
+ barrier();
+ asm volatile ( ALTERNATIVE(ASM_NOP3, "wrmsr", X86_FEATURE_XEN_IBRS_SET)
+ :: "a" (val), "c" (MSR_SPEC_CTRL), "d" (0) : "memory" );
+}
+
#endif /* !__X86_SPEC_CTRL_H__ */
/*
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v8 01/17] x86: Support compiling with indirect branch thunks
2018-01-12 18:00 ` [PATCH v8 01/17] x86: Support compiling with indirect branch thunks Andrew Cooper
@ 2018-01-14 19:48 ` David Woodhouse
2018-01-15 0:00 ` Andrew Cooper
2018-01-15 4:11 ` Konrad Rzeszutek Wilk
2018-01-15 10:14 ` Jan Beulich
1 sibling, 2 replies; 55+ messages in thread
From: David Woodhouse @ 2018-01-14 19:48 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 316 bytes --]
On Fri, 2018-01-12 at 18:00 +0000, Andrew Cooper wrote:
>
> +.macro IND_THUNK_RETPOLINE reg:req
> + call 2f
> +1:
Linux and GCC have now settled on 'pause;lfence;jmp' here.
> + lfence
> + jmp 1b
> +2:
> + mov %\reg, (%rsp)
> + ret
> +.endm
> +
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 01/17] x86: Support compiling with indirect branch thunks
2018-01-14 19:48 ` David Woodhouse
@ 2018-01-15 0:00 ` Andrew Cooper
2018-01-15 4:11 ` Konrad Rzeszutek Wilk
1 sibling, 0 replies; 55+ messages in thread
From: Andrew Cooper @ 2018-01-15 0:00 UTC (permalink / raw)
To: David Woodhouse, Xen-devel
On 14/01/2018 19:48, David Woodhouse wrote:
> On Fri, 2018-01-12 at 18:00 +0000, Andrew Cooper wrote:
>> +.macro IND_THUNK_RETPOLINE reg:req
>> + call 2f
>> +1:
> Linux and GCC have now settled on 'pause;lfence;jmp' here.
And elsewhere. It is probably worth taking, but it takes us to 17 bytes
which is frustratingly over one cache line.
~Andrew
>
>> + lfence
>> + jmp 1b
>> +2:
>> + mov %\reg, (%rsp)
>> + ret
>> +.endm
>> +
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 01/17] x86: Support compiling with indirect branch thunks
2018-01-14 19:48 ` David Woodhouse
2018-01-15 0:00 ` Andrew Cooper
@ 2018-01-15 4:11 ` Konrad Rzeszutek Wilk
1 sibling, 0 replies; 55+ messages in thread
From: Konrad Rzeszutek Wilk @ 2018-01-15 4:11 UTC (permalink / raw)
To: David Woodhouse; +Cc: Andrew Cooper, Xen-devel
On Sun, Jan 14, 2018 at 08:48:26PM +0100, David Woodhouse wrote:
> On Fri, 2018-01-12 at 18:00 +0000, Andrew Cooper wrote:
> >
> > +.macro IND_THUNK_RETPOLINE reg:req
> > + call 2f
> > +1:
>
> Linux and GCC have now settled on 'pause;lfence;jmp' here.
What is the benefit of the 'pause' in that?
>
> > + lfence
> > + jmp 1b
> > +2:
> > + mov %\reg, (%rsp)
> > + ret
> > +.endm
> > +
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 01/17] x86: Support compiling with indirect branch thunks
2018-01-12 18:00 ` [PATCH v8 01/17] x86: Support compiling with indirect branch thunks Andrew Cooper
2018-01-14 19:48 ` David Woodhouse
@ 2018-01-15 10:14 ` Jan Beulich
2018-01-15 10:40 ` Andrew Cooper
1 sibling, 1 reply; 55+ messages in thread
From: Jan Beulich @ 2018-01-15 10:14 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 12.01.18 at 19:00, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/Rules.mk
> +++ b/xen/arch/x86/Rules.mk
> @@ -30,3 +30,10 @@ CFLAGS += -fno-asynchronous-unwind-tables
> ifneq ($(call cc-option,$(CC),-fvisibility=hidden,n),n)
> CFLAGS += -DGCC_HAS_VISIBILITY_ATTRIBUTE
> endif
> +
> +# Compile with thunk-extern, indirect-branch-register if avaiable.
> +ifneq ($(call cc-option,$(CC),-mindirect-branch-register,n),n)
> +CFLAGS += -mindirect-branch=thunk-extern -mindirect-branch-register
> +CFLAGS += -DCONFIG_INDIRECT_THUNK
> +export CONFIG_INDIRECT_THUNK=y
> +endif
Still not a proper config option?
> --- /dev/null
> +++ b/xen/arch/x86/indirect-thunk.S
> @@ -0,0 +1,38 @@
> +/*
> + * Implement __x86_indirect_thunk_* symbols for use with compatbile compilers
> + * and the -mindirect-branch=thunk-extern -mindirect-branch-register options.
> + *
> + * Copyright (c) 2017-2018 Citrix Systems Ltd.
> + *
> + * This source code is licensed under the GNU General Public License,
> + * Version 2. See the file COPYING for more details.
> + */
> + .file __FILE__
> +
> +#include <asm/asm_defns.h>
> +
> +.macro IND_THUNK_RETPOLINE reg:req
> + call 2f
> +1:
> + lfence
> + jmp 1b
> +2:
As noted in a couple of other places, I'd prefer if numeric labels
weren't used in macros (and especially new ones), in favor of
.L ones utilizing \@.
> + mov %\reg, (%rsp)
> + ret
> +.endm
> +
> +/*
> + * Build the __x86_indirect_thunk_* symbols. Currently implement the
> + * retpoline thunk only.
> + */
> +.macro GEN_INDIRECT_THUNK reg:req
> + .section .text.__x86_indirect_thunk_\reg, "ax", @progbits
> +
> +ENTRY(__x86_indirect_thunk_\reg)
> + IND_THUNK_RETPOLINE \reg
> +.endm
Still unnecessary leading underscores in the section name?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 02/17] x86: Support indirect thunks from assembly code
2018-01-12 18:00 ` [PATCH v8 02/17] x86: Support indirect thunks from assembly code Andrew Cooper
@ 2018-01-15 10:28 ` Jan Beulich
2018-01-16 13:55 ` Andrew Cooper
2018-02-04 10:57 ` David Woodhouse
1 sibling, 1 reply; 55+ messages in thread
From: Jan Beulich @ 2018-01-15 10:28 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 12.01.18 at 19:00, <andrew.cooper3@citrix.com> wrote:
> Introduce INDIRECT_CALL and INDIRECT_JMP which either degrade to a normal
> indirect branch, or dispatch to the __x86_indirect_thunk_* symbols.
>
> Update all the manual indirect branches in to use the new thunks. The
> indirect branches in the early boot and kexec path are left intact as we
> can't
> use the compiled-in thunks at those points.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with a couple of cosmetic remarks:
> --- a/xen/arch/x86/pv/emul-priv-op.c
> +++ b/xen/arch/x86/pv/emul-priv-op.c
> @@ -73,46 +73,63 @@ void (*pv_post_outb_hook)(unsigned int port, u8 value);
>
> typedef void io_emul_stub_t(struct cpu_user_regs *);
>
> +void __x86_indirect_thunk_rcx(void);
> +
> static io_emul_stub_t *io_emul_stub_setup(struct priv_op_ctxt *ctxt, u8 opcode,
> unsigned int port, unsigned int bytes)
> {
> + struct stubs *this_stubs = &this_cpu(stubs);
> + unsigned long stub_va = this_stubs->addr + STUB_BUF_SIZE / 2;
> bool use_quirk_stub = false;
>
> if ( !ctxt->io_emul_stub )
> - ctxt->io_emul_stub = map_domain_page(_mfn(this_cpu(stubs.mfn))) +
> - (this_cpu(stubs.addr) &
> - ~PAGE_MASK) +
> - STUB_BUF_SIZE / 2;
> + ctxt->io_emul_stub =
> + map_domain_page(_mfn(this_stubs->mfn)) + (stub_va & ~PAGE_MASK);
>
> /* movq $host_to_guest_gpr_switch,%rcx */
> ctxt->io_emul_stub[0] = 0x48;
> ctxt->io_emul_stub[1] = 0xb9;
> *(void **)&ctxt->io_emul_stub[2] = (void *)host_to_guest_gpr_switch;
> +
> +#ifdef CONFIG_INDIRECT_THUNK
> + /* callq __x86_indirect_thunk_rcx */
> + ctxt->io_emul_stub[10] = 0xe8;
> + *(int32_t *)&ctxt->io_emul_stub[11] =
> + (unsigned long)__x86_indirect_thunk_rcx - (stub_va + 11 + 4);
Given the context I think a cast to signed long would be more
appropriate here.
> +
> +#else
> /* callq *%rcx */
Given the rest of the conditional you add, please either remove
the blank line above or add three more immediately inside the
preprocessor directives.
> ctxt->io_emul_stub[10] = 0xff;
> ctxt->io_emul_stub[11] = 0xd1;
>
> + /*
> + * 3 bytes of P6_NOPS.
> + * TODO: untangle ideal_nops from init/livepatch Kconfig options.
> + */
> + memcpy(&ctxt->io_emul_stub[12], "\x0f\x1f\x00", 3);
Perhaps better "P6_NOP3" in the comment? And perhaps
__stringify(P6_NOP3) in the memcpy() invocation, which may then
make unnecessary that part of the comment?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 01/17] x86: Support compiling with indirect branch thunks
2018-01-15 10:14 ` Jan Beulich
@ 2018-01-15 10:40 ` Andrew Cooper
2018-01-15 10:48 ` Jan Beulich
0 siblings, 1 reply; 55+ messages in thread
From: Andrew Cooper @ 2018-01-15 10:40 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 15/01/18 10:14, Jan Beulich wrote:
>>>> On 12.01.18 at 19:00, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/Rules.mk
>> +++ b/xen/arch/x86/Rules.mk
>> @@ -30,3 +30,10 @@ CFLAGS += -fno-asynchronous-unwind-tables
>> ifneq ($(call cc-option,$(CC),-fvisibility=hidden,n),n)
>> CFLAGS += -DGCC_HAS_VISIBILITY_ATTRIBUTE
>> endif
>> +
>> +# Compile with thunk-extern, indirect-branch-register if avaiable.
>> +ifneq ($(call cc-option,$(CC),-mindirect-branch-register,n),n)
>> +CFLAGS += -mindirect-branch=thunk-extern -mindirect-branch-register
>> +CFLAGS += -DCONFIG_INDIRECT_THUNK
>> +export CONFIG_INDIRECT_THUNK=y
>> +endif
> Still not a proper config option?
No, for backportability.
I'm preparing a post-SP2 series with some cleanup for staging.
>
>> --- /dev/null
>> +++ b/xen/arch/x86/indirect-thunk.S
>> @@ -0,0 +1,38 @@
>> +/*
>> + * Implement __x86_indirect_thunk_* symbols for use with compatbile compilers
>> + * and the -mindirect-branch=thunk-extern -mindirect-branch-register options.
>> + *
>> + * Copyright (c) 2017-2018 Citrix Systems Ltd.
>> + *
>> + * This source code is licensed under the GNU General Public License,
>> + * Version 2. See the file COPYING for more details.
>> + */
>> + .file __FILE__
>> +
>> +#include <asm/asm_defns.h>
>> +
>> +.macro IND_THUNK_RETPOLINE reg:req
>> + call 2f
>> +1:
>> + lfence
>> + jmp 1b
>> +2:
> As noted in a couple of other places, I'd prefer if numeric labels
> weren't used in macros (and especially new ones), in favor of
> .L ones utilizing \@.
For macros in header files, I can see this rational.
However, this is a local macro which only gets expanded in this file,
and isn't likely to move elsewhere. I don't see the value in reducing
the readability.
>
>> + mov %\reg, (%rsp)
>> + ret
>> +.endm
>> +
>> +/*
>> + * Build the __x86_indirect_thunk_* symbols. Currently implement the
>> + * retpoline thunk only.
>> + */
>> +.macro GEN_INDIRECT_THUNK reg:req
>> + .section .text.__x86_indirect_thunk_\reg, "ax", @progbits
>> +
>> +ENTRY(__x86_indirect_thunk_\reg)
>> + IND_THUNK_RETPOLINE \reg
>> +.endm
> Still unnecessary leading underscores in the section name?
Having the section names different to the symbols in them is far worse
than using double underscores.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 01/17] x86: Support compiling with indirect branch thunks
2018-01-15 10:40 ` Andrew Cooper
@ 2018-01-15 10:48 ` Jan Beulich
0 siblings, 0 replies; 55+ messages in thread
From: Jan Beulich @ 2018-01-15 10:48 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 15.01.18 at 11:40, <andrew.cooper3@citrix.com> wrote:
> On 15/01/18 10:14, Jan Beulich wrote:
>>>>> On 12.01.18 at 19:00, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/Rules.mk
>>> +++ b/xen/arch/x86/Rules.mk
>>> @@ -30,3 +30,10 @@ CFLAGS += -fno-asynchronous-unwind-tables
>>> ifneq ($(call cc-option,$(CC),-fvisibility=hidden,n),n)
>>> CFLAGS += -DGCC_HAS_VISIBILITY_ATTRIBUTE
>>> endif
>>> +
>>> +# Compile with thunk-extern, indirect-branch-register if avaiable.
>>> +ifneq ($(call cc-option,$(CC),-mindirect-branch-register,n),n)
>>> +CFLAGS += -mindirect-branch=thunk-extern -mindirect-branch-register
>>> +CFLAGS += -DCONFIG_INDIRECT_THUNK
>>> +export CONFIG_INDIRECT_THUNK=y
>>> +endif
>> Still not a proper config option?
>
> No, for backportability.
>
> I'm preparing a post-SP2 series with some cleanup for staging.
>
>>
>>> --- /dev/null
>>> +++ b/xen/arch/x86/indirect-thunk.S
>>> @@ -0,0 +1,38 @@
>>> +/*
>>> + * Implement __x86_indirect_thunk_* symbols for use with compatbile
> compilers
>>> + * and the -mindirect-branch=thunk-extern -mindirect-branch-register
> options.
>>> + *
>>> + * Copyright (c) 2017-2018 Citrix Systems Ltd.
>>> + *
>>> + * This source code is licensed under the GNU General Public License,
>>> + * Version 2. See the file COPYING for more details.
>>> + */
>>> + .file __FILE__
>>> +
>>> +#include <asm/asm_defns.h>
>>> +
>>> +.macro IND_THUNK_RETPOLINE reg:req
>>> + call 2f
>>> +1:
>>> + lfence
>>> + jmp 1b
>>> +2:
>> As noted in a couple of other places, I'd prefer if numeric labels
>> weren't used in macros (and especially new ones), in favor of
>> .L ones utilizing \@.
>
> For macros in header files, I can see this rational.
>
> However, this is a local macro which only gets expanded in this file,
> and isn't likely to move elsewhere. I don't see the value in reducing
> the readability.
>
>>
>>> + mov %\reg, (%rsp)
>>> + ret
>>> +.endm
>>> +
>>> +/*
>>> + * Build the __x86_indirect_thunk_* symbols. Currently implement the
>>> + * retpoline thunk only.
>>> + */
>>> +.macro GEN_INDIRECT_THUNK reg:req
>>> + .section .text.__x86_indirect_thunk_\reg, "ax", @progbits
>>> +
>>> +ENTRY(__x86_indirect_thunk_\reg)
>>> + IND_THUNK_RETPOLINE \reg
>>> +.endm
>> Still unnecessary leading underscores in the section name?
>
> Having the section names different to the symbols in them is far worse
> than using double underscores.
Well, okay then:
Acked-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 05/17] x86: Introduce alternative indirect thunks
2018-01-12 18:00 ` [PATCH v8 05/17] x86: Introduce alternative indirect thunks Andrew Cooper
@ 2018-01-15 10:53 ` Jan Beulich
0 siblings, 0 replies; 55+ messages in thread
From: Jan Beulich @ 2018-01-15 10:53 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 12.01.18 at 19:00, <andrew.cooper3@citrix.com> wrote:
> Depending on hardware and microcode availability, we will want to replace
> IND_THUNK_REPOLINE with other implementations.
>
> For AMD hardware, choose IND_THUNK_LFENCE in preference to retpoline if lfence
> is known to be (or was successfully made) dispatch serialising.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 10/17] x86/hvm: Permit guests direct access to MSR_{SPEC_CTRL, PRED_CMD}
2018-01-12 18:01 ` [PATCH v8 10/17] x86/hvm: Permit guests direct access to MSR_{SPEC_CTRL, PRED_CMD} Andrew Cooper
@ 2018-01-15 11:11 ` Jan Beulich
2018-01-15 16:02 ` Boris Ostrovsky
2018-01-16 0:39 ` Tian, Kevin
0 siblings, 2 replies; 55+ messages in thread
From: Jan Beulich @ 2018-01-15 11:11 UTC (permalink / raw)
To: Andrew Cooper
Cc: Boris Ostrovsky, Kevin Tian, Jun Nakajima, Suravee Suthikulpanit,
Xen-devel
>>> On 12.01.18 at 19:01, <andrew.cooper3@citrix.com> wrote:
> For performance reasons, HVM guests should have direct access to these MSRs
> when possible.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one spelling fix (see below)
As these are non-trivial changes to VMX and SVM code I think you
should have Cc-ed the maintainers (now added, and leaving the
full patch in context for them.
> ---
> v7:
> * Drop excess brackets
> ---
> xen/arch/x86/domctl.c | 19 +++++++++++++++++++
> xen/arch/x86/hvm/svm/svm.c | 5 +++++
> xen/arch/x86/hvm/vmx/vmx.c | 18 ++++++++++++++++++
> xen/arch/x86/msr.c | 3 ++-
> xen/include/asm-x86/msr.h | 5 ++++-
> 5 files changed, 48 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 72b4489..e5fdde7 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -53,6 +53,7 @@ static int update_domain_cpuid_info(struct domain *d,
> struct cpuid_policy *p = d->arch.cpuid;
> const struct cpuid_leaf leaf = { ctl->eax, ctl->ebx, ctl->ecx, ctl->edx };
> int old_vendor = p->x86_vendor;
> + unsigned int old_7d0 = p->feat.raw[0].d, old_e8b = p->extd.raw[8].b;
> bool call_policy_changed = false; /* Avoid for_each_vcpu() unnecessarily */
>
> /*
> @@ -218,6 +219,14 @@ static int update_domain_cpuid_info(struct domain *d,
>
> d->arch.pv_domain.cpuidmasks->_7ab0 = mask;
> }
> +
> + /*
> + * If the IBSRB/STIBP policy has changed, we need to recalculate the
> + * MSR interception bitmaps and STIBP protection default.
> + */
> + call_policy_changed = ((old_7d0 ^ p->feat.raw[0].d) &
> + (cpufeat_mask(X86_FEATURE_IBRSB) |
> + cpufeat_mask(X86_FEATURE_STIBP)));
> break;
>
> case 0xa:
> @@ -292,6 +301,16 @@ static int update_domain_cpuid_info(struct domain *d,
> d->arch.pv_domain.cpuidmasks->e1cd = mask;
> }
> break;
> +
> + case 0x80000008:
> + /*
> + * If the IBRB policy has changed, we need to recalculate the MSR
> + * interception bitmaps.
> + */
> + call_policy_changed = (is_hvm_domain(d) &&
> + ((old_e8b ^ p->extd.raw[8].b) &
> + cpufeat_mask(X86_FEATURE_IBPB)));
> + break;
> }
>
> if ( call_policy_changed )
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index c48fdfa..ee47508 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -617,6 +617,7 @@ static void svm_cpuid_policy_changed(struct vcpu *v)
> {
> struct arch_svm_struct *arch_svm = &v->arch.hvm_svm;
> struct vmcb_struct *vmcb = arch_svm->vmcb;
> + const struct cpuid_policy *cp = v->domain->arch.cpuid;
> u32 bitmap = vmcb_get_exception_intercepts(vmcb);
>
> if ( opt_hvm_fep ||
> @@ -626,6 +627,10 @@ static void svm_cpuid_policy_changed(struct vcpu *v)
> bitmap &= ~(1U << TRAP_invalid_op);
>
> vmcb_set_exception_intercepts(vmcb, bitmap);
> +
> + /* Give access to MSR_PRED_CMD if the guest has been told about it. */
> + svm_intercept_msr(v, MSR_PRED_CMD,
> + cp->extd.ibpb ? MSR_INTERCEPT_NONE : MSR_INTERCEPT_RW);
> }
>
> static void svm_sync_vmcb(struct vcpu *v)
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index e036303..8609de3 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -656,6 +656,8 @@ void vmx_update_exception_bitmap(struct vcpu *v)
>
> static void vmx_cpuid_policy_changed(struct vcpu *v)
> {
> + const struct cpuid_policy *cp = v->domain->arch.cpuid;
> +
> if ( opt_hvm_fep ||
> (v->domain->arch.cpuid->x86_vendor != boot_cpu_data.x86_vendor) )
> v->arch.hvm_vmx.exception_bitmap |= (1U << TRAP_invalid_op);
> @@ -665,6 +667,22 @@ static void vmx_cpuid_policy_changed(struct vcpu *v)
> vmx_vmcs_enter(v);
> vmx_update_exception_bitmap(v);
> vmx_vmcs_exit(v);
> +
> + /*
> + * We can only pass though MSR_SPEC_CTRL if the guest knows about all bits
"through"
Jan
> + * in it. Otherwise, Xen may be fixing up in the background.
> + */
> + v->arch.msr->spec_ctrl.direct_access = cp->feat.ibrsb && cp->feat.stibp;
> + if ( v->arch.msr->spec_ctrl.direct_access )
> + vmx_clear_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
> + else
> + vmx_set_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
> +
> + /* MSR_PRED_CMD is safe to pass through if the guest knows about it. */
> + if ( cp->feat.ibrsb || cp->extd.ibpb )
> + vmx_clear_msr_intercept(v, MSR_PRED_CMD, VMX_MSR_RW);
> + else
> + vmx_set_msr_intercept(v, MSR_PRED_CMD, VMX_MSR_RW);
> }
>
> int vmx_guest_x86_mode(struct vcpu *v)
> diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> index 02a7b49..697cc6e 100644
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -132,7 +132,8 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
> case MSR_SPEC_CTRL:
> if ( !cp->feat.ibrsb )
> goto gp_fault;
> - *val = vp->spec_ctrl.guest;
> + *val = (vp->spec_ctrl.direct_access
> + ? vp->spec_ctrl.host : vp->spec_ctrl.guest);
> break;
>
> case MSR_INTEL_PLATFORM_INFO:
> diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
> index 3d0012d..007e966 100644
> --- a/xen/include/asm-x86/msr.h
> +++ b/xen/include/asm-x86/msr.h
> @@ -229,10 +229,13 @@ struct msr_vcpu_policy
> * Only the bottom two bits are defined, so no need to waste space
> * with uint64_t at the moment. We maintain the guests idea of the
> * value it wrote, and a value to install into hardware (extended to
> - * uint32_t to simplify the asm) which might be different.
> + * uint32_t to simplify the asm) which might be different. HVM guests
> + * might be given direct access to the MSRs, at which point the
> + * 'guest' value becomes stale.
> */
> uint32_t host;
> uint8_t guest;
> + bool direct_access;
> } spec_ctrl;
>
> /* 0x00000140 MSR_INTEL_MISC_FEATURES_ENABLES */
> --
> 2.1.4
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 11/17] x86: Protect unaware domains from meddling hyperthreads
2018-01-12 18:01 ` [PATCH v8 11/17] x86: Protect unaware domains from meddling hyperthreads Andrew Cooper
@ 2018-01-15 11:26 ` Jan Beulich
2018-01-16 21:11 ` Andrew Cooper
0 siblings, 1 reply; 55+ messages in thread
From: Jan Beulich @ 2018-01-15 11:26 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 12.01.18 at 19:01, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/asm-x86/spec_ctrl.h
> +++ b/xen/include/asm-x86/spec_ctrl.h
> @@ -20,8 +20,29 @@
> #ifndef __X86_SPEC_CTRL_H__
> #define __X86_SPEC_CTRL_H__
>
> +#include <xen/sched.h>
> +
> void init_speculation_mitigations(void);
>
> +/*
> + * For guests which know about IBRS but are not told about STIBP running on
> + * hardware supporting hyperthreading, the guest doesn't know to protect
> + * itself fully. (Such a guest won't be permitted direct access to the MSR.)
> + * Have Xen fill in the gaps, so an unaware guest can't be interfered with by
> + * a meddling guest on an adjacent hyperthread.
> + */
> +static inline unsigned int spec_ctrl_host_val(const struct domain *d,
> + unsigned int guest_val)
> +{
> + const struct cpuid_policy *cp = d->arch.cpuid;
> +
> + if ( !cp->feat.stibp && cpu_has_stibp &&
> + !(guest_val & (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP)) )
> + return SPEC_CTRL_STIBP;
> + else
> + return guest_val;
> +}
The comment's "won't be permitted direct access" can be verified
by looking at patch 10, but where's the HT dependency here (or
in another patch)? For now it looks to me as if you enabled this
behind-the-back protection even in the non-HT case.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 12/17] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point
2018-01-12 18:01 ` [PATCH v8 12/17] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point Andrew Cooper
@ 2018-01-15 12:09 ` Jan Beulich
2018-01-16 21:24 ` Andrew Cooper
0 siblings, 1 reply; 55+ messages in thread
From: Jan Beulich @ 2018-01-15 12:09 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 12.01.18 at 19:01, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -668,6 +668,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> set_processor_id(0);
> set_current(INVALID_VCPU); /* debug sanity. */
> idle_vcpu[0] = current;
> + init_shadow_spec_ctrl_state();
Considering the strict need to fill struct cpu_info fields early on (also
in my SP3 band-aid) I wonder whether we wouldn't be better off
uniformly memset()-ing the whole structure first thing here and in
start_secondary(). But this is just a remark, not necessarily
something to be done in this patch.
> @@ -586,6 +611,10 @@ ENTRY(double_fault)
> movl $TRAP_double_fault,4(%rsp)
> /* Set AC to reduce chance of further SMAP faults */
> SAVE_ALL STAC
> +
> + SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs Clob: acd */
> + /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
Is it actually useful to do _anything_ in the double fault handler?
> +.macro DO_SPEC_CTRL_ENTRY maybexen:req ibrs_val:req
> +/*
> + * Requires %rsp=regs (also cpuinfo if !maybexen)
> + * Clobbers %rax, %rcx, %rdx
> + *
> + * PV guests can't update MSR_SPEC_CTRL behind Xen's back, so no need to read
> + * it back. Entries from guest context need to clear SPEC_CTRL shadowing,
> + * while entries from Xen must leave shadowing in its current state.
> + */
> + mov $MSR_SPEC_CTRL, %ecx
> +
> + .if \maybexen
> + cmpw $__HYPERVISOR_CS, UREGS_cs(%rsp)
I see you've changed to cmpw here. To eliminate your length-
changing-prefix concern, how about using testb instead to
just evaluate RPL or the selector, as I'm doing in the SP3
band-aid?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 15/17] x86/ctxt: Issue a speculation barrier between vcpu contexts
2018-01-12 18:01 ` [PATCH v8 15/17] x86/ctxt: Issue a speculation barrier between vcpu contexts Andrew Cooper
@ 2018-01-15 12:54 ` David Woodhouse
2018-01-15 13:02 ` Andrew Cooper
0 siblings, 1 reply; 55+ messages in thread
From: David Woodhouse @ 2018-01-15 12:54 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 833 bytes --]
On Fri, 2018-01-12 at 18:01 +0000, Andrew Cooper wrote:
>
> @@ -1736,6 +1736,9 @@ void context_switch(struct vcpu *prev, struct
> vcpu *next)
> }
>
> ctxt_switch_levelling(next);
> +
> + if ( opt_ibpb )
> + wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
> }
>
If you're doing that without an 'else lfence' don't you need to include
a comment with your proof that it's safe to do so, and the CPU can't
speculate a taken conditional branch and all the way to a problematic
instruction?
Also... if you're doing that in context_switch() does it do the right
thing with idle? If a CPU switches to the idle domain and then back
again to the same vCPU, does that do the IBPB twice?
For vmx we only really need IBPB when we do VMPTRLD, right?
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 15/17] x86/ctxt: Issue a speculation barrier between vcpu contexts
2018-01-15 12:54 ` David Woodhouse
@ 2018-01-15 13:02 ` Andrew Cooper
2018-01-15 13:23 ` David Woodhouse
0 siblings, 1 reply; 55+ messages in thread
From: Andrew Cooper @ 2018-01-15 13:02 UTC (permalink / raw)
To: David Woodhouse, Xen-devel
On 15/01/18 12:54, David Woodhouse wrote:
> On Fri, 2018-01-12 at 18:01 +0000, Andrew Cooper wrote:
>> @@ -1736,6 +1736,9 @@ void context_switch(struct vcpu *prev, struct
>> vcpu *next)
>> }
>>
>> ctxt_switch_levelling(next);
>> +
>> + if ( opt_ibpb )
>> + wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
>> }
>>
> If you're doing that without an 'else lfence' don't you need to include
> a comment with your proof that it's safe to do so, and the CPU can't
> speculate a taken conditional branch and all the way to a problematic
> instruction?
What would that gain? A malicious guest could speculate around it, but
speculation will catch up (at the very latest) when we return to guest,
which is a serialising event.
There is nothing a guest can usefully achieve by attacking this branch,
because it can't prevent the WRMSR from happening before we leave
hypervisor context.
> Also... if you're doing that in context_switch() does it do the right
> thing with idle? If a CPU switches to the idle domain and then back
> again to the same vCPU, does that do the IBPB twice?
Context switches to idle will skip the IBPB because it isn't needed, but
any switch to non-idle need it. In your example, we should execute just
a single IBPB.
> For vmx we only really need IBPB when we do VMPTRLD, right?
That is part of IBRS_ATT is it not?
At which point it pertains to reuse of a VMCS for a new security
context, but that should be covered by the context switch IBPB.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 15/17] x86/ctxt: Issue a speculation barrier between vcpu contexts
2018-01-15 13:02 ` Andrew Cooper
@ 2018-01-15 13:23 ` David Woodhouse
2018-01-15 21:39 ` David Woodhouse
0 siblings, 1 reply; 55+ messages in thread
From: David Woodhouse @ 2018-01-15 13:23 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 2516 bytes --]
On Mon, 2018-01-15 at 13:02 +0000, Andrew Cooper wrote:
> On 15/01/18 12:54, David Woodhouse wrote:
> >
> > On Fri, 2018-01-12 at 18:01 +0000, Andrew Cooper wrote:
> > >
> > > @@ -1736,6 +1736,9 @@ void context_switch(struct vcpu *prev, struct
> > > vcpu *next)
> > > }
> > >
> > > ctxt_switch_levelling(next);
> > > +
> > > + if ( opt_ibpb )
> > > + wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
> > > }
> > >
> > If you're doing that without an 'else lfence' don't you need to include
> > a comment with your proof that it's safe to do so, and the CPU can't
> > speculate a taken conditional branch and all the way to a problematic
> > instruction?
>
> What would that gain? A malicious guest could speculate around it, but
> speculation will catch up (at the very latest) when we return to guest,
> which is a serialising event.
There's your proof. I'd just be happier with a blanket policy of
*including* that proof in all cases where we do this kind of runtime
conditional branch around setting IBRS or IBPB. Because if we're in the
habit of doing the 'if (foo) wrmsrl()' without it, we *might* miss a
case where it's not actually OK.
(Aside: Is VMLAUNCH actually architecturally guaranteed to be
serialising? That doesn't seem consistent with a long-term goal of
designing hardware to make VMs go faster. Or have we merely extracted a
promise from Intel that *current* hardware will stop speculative
execution when it hits a VMLAUNCH?)
> >
> > Also... if you're doing that in context_switch() does it do the right
> > thing with idle? If a CPU switches to the idle domain and then back
> > again to the same vCPU, does that do the IBPB twice?
>
> Context switches to idle will skip the IBPB because it isn't needed, but
> any switch to non-idle need it. In your example, we should execute just
> a single IBPB.
In my example I think we should not execute IBPB at all. We come from a
given VMCS, sleep for a while, and go back to it. No need for any
flushing whatsoever.
> >
> > For vmx we only really need IBPB when we do VMPTRLD, right?
>
> That is part of IBRS_ATT is it not?
It doesn't go away with IBRS_ALL (as it's now called), but it's the
same for the existing IBRS *and* retpoline. Doing it on VMPTRLD is what
Linux is doing.
(cf. https://lkml.org/lkml/2018/1/13/40 and
https://lkml.org/lkml/2018/1/15/146 and note the AMD SVM caveat.)
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 10/17] x86/hvm: Permit guests direct access to MSR_{SPEC_CTRL, PRED_CMD}
2018-01-15 11:11 ` Jan Beulich
@ 2018-01-15 16:02 ` Boris Ostrovsky
2018-01-16 0:39 ` Tian, Kevin
1 sibling, 0 replies; 55+ messages in thread
From: Boris Ostrovsky @ 2018-01-15 16:02 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper
Cc: Kevin Tian, Jun Nakajima, Suravee Suthikulpanit, Xen-devel
On 01/15/2018 06:11 AM, Jan Beulich wrote:
>>>> On 12.01.18 at 19:01, <andrew.cooper3@citrix.com> wrote:
>> For performance reasons, HVM guests should have direct access to these MSRs
>> when possible.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with one spelling fix (see below)
>
> As these are non-trivial changes to VMX and SVM code I think you
> should have Cc-ed the maintainers (now added, and leaving the
> full patch in context for them.
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 15/17] x86/ctxt: Issue a speculation barrier between vcpu contexts
2018-01-15 13:23 ` David Woodhouse
@ 2018-01-15 21:39 ` David Woodhouse
2018-01-17 17:26 ` David Woodhouse
0 siblings, 1 reply; 55+ messages in thread
From: David Woodhouse @ 2018-01-15 21:39 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: Matt Wilson
[-- Attachment #1.1: Type: text/plain, Size: 1044 bytes --]
On Mon, 2018-01-15 at 14:23 +0100, David Woodhouse wrote:
>
> > >
> > > Also... if you're doing that in context_switch() does it do the right
> > > thing with idle? If a CPU switches to the idle domain and then back
> > > again to the same vCPU, does that do the IBPB twice?
> >
> > Context switches to idle will skip the IBPB because it isn't needed, but
> > any switch to non-idle need it. In your example, we should execute just
> > a single IBPB.
>
> In my example I think we should not execute IBPB at all. We come from a
> given VMCS, sleep for a while, and go back to it. No need for any
> flushing whatsoever.
msw points out that in my example we *don't* execute IBPB at all, I
think.
In both switching to idle, and back to the vCPU, we should hit this
case and not the 'else' case that does the IBPB:
1710 if ( (per_cpu(curr_vcpu, cpu) == next) ||
1711 (is_idle_domain(nextd) && cpu_online(cpu)) )
1712 {
1713 local_irq_enable();
1714 }
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 10/17] x86/hvm: Permit guests direct access to MSR_{SPEC_CTRL, PRED_CMD}
2018-01-15 11:11 ` Jan Beulich
2018-01-15 16:02 ` Boris Ostrovsky
@ 2018-01-16 0:39 ` Tian, Kevin
1 sibling, 0 replies; 55+ messages in thread
From: Tian, Kevin @ 2018-01-16 0:39 UTC (permalink / raw)
To: Jan Beulich, Andrew Cooper
Cc: Boris Ostrovsky, Nakajima, Jun, Suravee Suthikulpanit, Xen-devel
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Monday, January 15, 2018 7:11 PM
>
> >>> On 12.01.18 at 19:01, <andrew.cooper3@citrix.com> wrote:
> > For performance reasons, HVM guests should have direct access to these
> MSRs
> > when possible.
> >
> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with one spelling fix (see below)
>
> As these are non-trivial changes to VMX and SVM code I think you
> should have Cc-ed the maintainers (now added, and leaving the
> full patch in context for them.
>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>, with other
two spelling fixes.
> > ---
> > v7:
> > * Drop excess brackets
> > ---
> > xen/arch/x86/domctl.c | 19 +++++++++++++++++++
> > xen/arch/x86/hvm/svm/svm.c | 5 +++++
> > xen/arch/x86/hvm/vmx/vmx.c | 18 ++++++++++++++++++
> > xen/arch/x86/msr.c | 3 ++-
> > xen/include/asm-x86/msr.h | 5 ++++-
> > 5 files changed, 48 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> > index 72b4489..e5fdde7 100644
> > --- a/xen/arch/x86/domctl.c
> > +++ b/xen/arch/x86/domctl.c
> > @@ -53,6 +53,7 @@ static int update_domain_cpuid_info(struct domain
> *d,
> > struct cpuid_policy *p = d->arch.cpuid;
> > const struct cpuid_leaf leaf = { ctl->eax, ctl->ebx, ctl->ecx, ctl->edx };
> > int old_vendor = p->x86_vendor;
> > + unsigned int old_7d0 = p->feat.raw[0].d, old_e8b = p->extd.raw[8].b;
> > bool call_policy_changed = false; /* Avoid for_each_vcpu()
> unnecessarily */
> >
> > /*
> > @@ -218,6 +219,14 @@ static int update_domain_cpuid_info(struct
> domain *d,
> >
> > d->arch.pv_domain.cpuidmasks->_7ab0 = mask;
> > }
> > +
> > + /*
> > + * If the IBSRB/STIBP policy has changed, we need to recalculate the
IBRSB
> > + * MSR interception bitmaps and STIBP protection default.
> > + */
> > + call_policy_changed = ((old_7d0 ^ p->feat.raw[0].d) &
> > + (cpufeat_mask(X86_FEATURE_IBRSB) |
> > + cpufeat_mask(X86_FEATURE_STIBP)));
> > break;
> >
> > case 0xa:
> > @@ -292,6 +301,16 @@ static int update_domain_cpuid_info(struct
> domain *d,
> > d->arch.pv_domain.cpuidmasks->e1cd = mask;
> > }
> > break;
> > +
> > + case 0x80000008:
> > + /*
> > + * If the IBRB policy has changed, we need to recalculate the MSR
IBPB
> > + * interception bitmaps.
> > + */
> > + call_policy_changed = (is_hvm_domain(d) &&
> > + ((old_e8b ^ p->extd.raw[8].b) &
> > + cpufeat_mask(X86_FEATURE_IBPB)));
> > + break;
> > }
> >
> > if ( call_policy_changed )
> > diff --git a/xen/arch/x86/hvm/svm/svm.c
> b/xen/arch/x86/hvm/svm/svm.c
> > index c48fdfa..ee47508 100644
> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -617,6 +617,7 @@ static void svm_cpuid_policy_changed(struct vcpu
> *v)
> > {
> > struct arch_svm_struct *arch_svm = &v->arch.hvm_svm;
> > struct vmcb_struct *vmcb = arch_svm->vmcb;
> > + const struct cpuid_policy *cp = v->domain->arch.cpuid;
> > u32 bitmap = vmcb_get_exception_intercepts(vmcb);
> >
> > if ( opt_hvm_fep ||
> > @@ -626,6 +627,10 @@ static void svm_cpuid_policy_changed(struct
> vcpu *v)
> > bitmap &= ~(1U << TRAP_invalid_op);
> >
> > vmcb_set_exception_intercepts(vmcb, bitmap);
> > +
> > + /* Give access to MSR_PRED_CMD if the guest has been told about it.
> */
> > + svm_intercept_msr(v, MSR_PRED_CMD,
> > + cp->extd.ibpb ? MSR_INTERCEPT_NONE :
> MSR_INTERCEPT_RW);
> > }
> >
> > static void svm_sync_vmcb(struct vcpu *v)
> > diff --git a/xen/arch/x86/hvm/vmx/vmx.c
> b/xen/arch/x86/hvm/vmx/vmx.c
> > index e036303..8609de3 100644
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -656,6 +656,8 @@ void vmx_update_exception_bitmap(struct vcpu
> *v)
> >
> > static void vmx_cpuid_policy_changed(struct vcpu *v)
> > {
> > + const struct cpuid_policy *cp = v->domain->arch.cpuid;
> > +
> > if ( opt_hvm_fep ||
> > (v->domain->arch.cpuid->x86_vendor !=
> boot_cpu_data.x86_vendor) )
> > v->arch.hvm_vmx.exception_bitmap |= (1U << TRAP_invalid_op);
> > @@ -665,6 +667,22 @@ static void vmx_cpuid_policy_changed(struct
> vcpu *v)
> > vmx_vmcs_enter(v);
> > vmx_update_exception_bitmap(v);
> > vmx_vmcs_exit(v);
> > +
> > + /*
> > + * We can only pass though MSR_SPEC_CTRL if the guest knows about
> all bits
>
> "through"
>
> Jan
>
> > + * in it. Otherwise, Xen may be fixing up in the background.
> > + */
> > + v->arch.msr->spec_ctrl.direct_access = cp->feat.ibrsb && cp-
> >feat.stibp;
> > + if ( v->arch.msr->spec_ctrl.direct_access )
> > + vmx_clear_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
> > + else
> > + vmx_set_msr_intercept(v, MSR_SPEC_CTRL, VMX_MSR_RW);
> > +
> > + /* MSR_PRED_CMD is safe to pass through if the guest knows about it.
> */
> > + if ( cp->feat.ibrsb || cp->extd.ibpb )
> > + vmx_clear_msr_intercept(v, MSR_PRED_CMD, VMX_MSR_RW);
> > + else
> > + vmx_set_msr_intercept(v, MSR_PRED_CMD, VMX_MSR_RW);
> > }
> >
> > int vmx_guest_x86_mode(struct vcpu *v)
> > diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
> > index 02a7b49..697cc6e 100644
> > --- a/xen/arch/x86/msr.c
> > +++ b/xen/arch/x86/msr.c
> > @@ -132,7 +132,8 @@ int guest_rdmsr(const struct vcpu *v, uint32_t
> msr, uint64_t *val)
> > case MSR_SPEC_CTRL:
> > if ( !cp->feat.ibrsb )
> > goto gp_fault;
> > - *val = vp->spec_ctrl.guest;
> > + *val = (vp->spec_ctrl.direct_access
> > + ? vp->spec_ctrl.host : vp->spec_ctrl.guest);
> > break;
> >
> > case MSR_INTEL_PLATFORM_INFO:
> > diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
> > index 3d0012d..007e966 100644
> > --- a/xen/include/asm-x86/msr.h
> > +++ b/xen/include/asm-x86/msr.h
> > @@ -229,10 +229,13 @@ struct msr_vcpu_policy
> > * Only the bottom two bits are defined, so no need to waste space
> > * with uint64_t at the moment. We maintain the guests idea of the
> > * value it wrote, and a value to install into hardware (extended to
> > - * uint32_t to simplify the asm) which might be different.
> > + * uint32_t to simplify the asm) which might be different. HVM
> guests
> > + * might be given direct access to the MSRs, at which point the
> > + * 'guest' value becomes stale.
> > */
> > uint32_t host;
> > uint8_t guest;
> > + bool direct_access;
> > } spec_ctrl;
> >
> > /* 0x00000140 MSR_INTEL_MISC_FEATURES_ENABLES */
> > --
> > 2.1.4
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xenproject.org
> > https://lists.xenproject.org/mailman/listinfo/xen-devel
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 08/17] x86/msr: Emulation of MSR_{SPEC_CTRL, PRED_CMD} for guests
2018-01-12 18:00 ` [PATCH v8 08/17] x86/msr: Emulation of MSR_{SPEC_CTRL, PRED_CMD} for guests Andrew Cooper
@ 2018-01-16 11:10 ` David Woodhouse
2018-01-16 16:58 ` Andrew Cooper
0 siblings, 1 reply; 55+ messages in thread
From: David Woodhouse @ 2018-01-16 11:10 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 1344 bytes --]
On Fri, 2018-01-12 at 18:00 +0000, Andrew Cooper wrote:
>
> @@ -152,14 +163,38 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr,
> uint64_t val)
> {
> const struct vcpu *curr = current;
> struct domain *d = v->domain;
> + const struct cpuid_policy *cp = d->arch.cpuid;
> struct msr_domain_policy *dp = d->arch.msr;
> struct msr_vcpu_policy *vp = v->arch.msr;
>
> switch ( msr )
> {
> case MSR_INTEL_PLATFORM_INFO:
> + /* Read-only */
> goto gp_fault;
>
> + case MSR_SPEC_CTRL:
> + if ( !cp->feat.ibrsb )
> + goto gp_fault; /* MSR available? */
> + if ( val & ~(SPEC_CTRL_IBRS |
> + (cp->feat.stibp ? SPEC_CTRL_STIBP : 0)) )
Intel defines the STIBP bit as non-faulting and ignored, even when
STIBP isn't advertised. So you should probably just mask it out
if !cp->feat.stibp.
> + goto gp_fault; /* Rsvd bit set? */
> + vp->spec_ctrl.guest = val;
> + vp->spec_ctrl.host = val;
> + break;
> +
There's no actual wrmsr there. This is fine, because you're going to do
it on the way back to the guest (albeit in a later patch in the
series). But it probably warrants a comment?
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 02/17] x86: Support indirect thunks from assembly code
2018-01-15 10:28 ` Jan Beulich
@ 2018-01-16 13:55 ` Andrew Cooper
2018-01-16 14:00 ` Jan Beulich
0 siblings, 1 reply; 55+ messages in thread
From: Andrew Cooper @ 2018-01-16 13:55 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 15/01/18 10:28, Jan Beulich wrote:
>> ctxt->io_emul_stub[10] = 0xff;
>> ctxt->io_emul_stub[11] = 0xd1;
>>
>> + /*
>> + * 3 bytes of P6_NOPS.
>> + * TODO: untangle ideal_nops from init/livepatch Kconfig options.
>> + */
>> + memcpy(&ctxt->io_emul_stub[12], "\x0f\x1f\x00", 3);
> Perhaps better "P6_NOP3" in the comment? And perhaps
> __stringify(P6_NOP3) in the memcpy() invocation, which may then
> make unnecessary that part of the comment?
__stringify(P6_NOP3) really doesn't do what you expect here. This is
the preprocessed result:
__builtin_memcpy(&ctxt->io_emul_stub[12], "0x0f,0x1f,0x00", 3);
I've fixed up the other points, but left this one opencoded.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 02/17] x86: Support indirect thunks from assembly code
2018-01-16 13:55 ` Andrew Cooper
@ 2018-01-16 14:00 ` Jan Beulich
0 siblings, 0 replies; 55+ messages in thread
From: Jan Beulich @ 2018-01-16 14:00 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 16.01.18 at 14:55, <andrew.cooper3@citrix.com> wrote:
> On 15/01/18 10:28, Jan Beulich wrote:
>>> ctxt->io_emul_stub[10] = 0xff;
>>> ctxt->io_emul_stub[11] = 0xd1;
>>>
>>> + /*
>>> + * 3 bytes of P6_NOPS.
>>> + * TODO: untangle ideal_nops from init/livepatch Kconfig options.
>>> + */
>>> + memcpy(&ctxt->io_emul_stub[12], "\x0f\x1f\x00", 3);
>> Perhaps better "P6_NOP3" in the comment? And perhaps
>> __stringify(P6_NOP3) in the memcpy() invocation, which may then
>> make unnecessary that part of the comment?
>
> __stringify(P6_NOP3) really doesn't do what you expect here. This is
> the preprocessed result:
>
> __builtin_memcpy(&ctxt->io_emul_stub[12], "0x0f,0x1f,0x00", 3);
Oops, of course - I'm sorry for the noise.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 13/17] x86/boot: Calculate the most appropriate BTI mitigation to use
2018-01-12 18:01 ` [PATCH v8 13/17] x86/boot: Calculate the most appropriate BTI mitigation to use Andrew Cooper
@ 2018-01-16 14:10 ` Boris Ostrovsky
2018-01-16 14:13 ` Andrew Cooper
0 siblings, 1 reply; 55+ messages in thread
From: Boris Ostrovsky @ 2018-01-16 14:10 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
On 01/12/2018 01:01 PM, Andrew Cooper wrote:
>
> + if ( boot_cpu_has(X86_FEATURE_IBRSB) )
> + {
> + /*
> + * Even if we've chosen to not have IBRS set in Xen context, we still
> + * need the IBRS entry/exit logic to virtualise IBRS support for
> + * guests.
> + */
> + if ( ibrs )
> + setup_force_cpu_cap(X86_FEATURE_XEN_IBRS_SET);
> + else
> + setup_force_cpu_cap(X86_FEATURE_XEN_IBRS_CLEAR);
> + }
>
Are you going to add support for Intel's "Enhanced IBRS" (I think that's
what they call the "always on" mode")?
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 13/17] x86/boot: Calculate the most appropriate BTI mitigation to use
2018-01-16 14:10 ` Boris Ostrovsky
@ 2018-01-16 14:13 ` Andrew Cooper
2018-01-16 14:25 ` Boris Ostrovsky
0 siblings, 1 reply; 55+ messages in thread
From: Andrew Cooper @ 2018-01-16 14:13 UTC (permalink / raw)
To: Boris Ostrovsky, Xen-devel
On 16/01/18 14:10, Boris Ostrovsky wrote:
> On 01/12/2018 01:01 PM, Andrew Cooper wrote:
>>
>> + if ( boot_cpu_has(X86_FEATURE_IBRSB) )
>> + {
>> + /*
>> + * Even if we've chosen to not have IBRS set in Xen context, we still
>> + * need the IBRS entry/exit logic to virtualise IBRS support for
>> + * guests.
>> + */
>> + if ( ibrs )
>> + setup_force_cpu_cap(X86_FEATURE_XEN_IBRS_SET);
>> + else
>> + setup_force_cpu_cap(X86_FEATURE_XEN_IBRS_CLEAR);
>> + }
>>
> Are you going to add support for Intel's "Enhanced IBRS" (I think that's
> what they call the "always on" mode")?
I'm not going to touch IBRS_ATT mode until I've got an SDP to develop
against.
Given how many times the IBRS_ATT spec has changed already, I have
little confidence that it will remain unchanged right to the eventual
hardware arrives.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 13/17] x86/boot: Calculate the most appropriate BTI mitigation to use
2018-01-16 14:13 ` Andrew Cooper
@ 2018-01-16 14:25 ` Boris Ostrovsky
2018-01-16 15:12 ` Andrew Cooper
0 siblings, 1 reply; 55+ messages in thread
From: Boris Ostrovsky @ 2018-01-16 14:25 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
On 01/16/2018 09:13 AM, Andrew Cooper wrote:
> On 16/01/18 14:10, Boris Ostrovsky wrote:
>> On 01/12/2018 01:01 PM, Andrew Cooper wrote:
>>>
>>> + if ( boot_cpu_has(X86_FEATURE_IBRSB) )
>>> + {
>>> + /*
>>> + * Even if we've chosen to not have IBRS set in Xen context, we still
>>> + * need the IBRS entry/exit logic to virtualise IBRS support for
>>> + * guests.
>>> + */
>>> + if ( ibrs )
>>> + setup_force_cpu_cap(X86_FEATURE_XEN_IBRS_SET);
>>> + else
>>> + setup_force_cpu_cap(X86_FEATURE_XEN_IBRS_CLEAR);
>>> + }
>>>
>> Are you going to add support for Intel's "Enhanced IBRS" (I think that's
>> what they call the "always on" mode")?
> I'm not going to touch IBRS_ATT mode until I've got an SDP to develop
> against.
>
> Given how many times the IBRS_ATT spec has changed already, I have
> little confidence that it will remain unchanged right to the eventual
> hardware arrives.
I don't know if you are aware of it (I learned about this doc on Sunday) but
https://software.intel.com/sites/default/files/managed/c5/63/336996-Speculative-Execution-Side-Channel-Mitigations.pdf
(Not part of the SDM but still, an official specification. For a change.)
-boris
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 13/17] x86/boot: Calculate the most appropriate BTI mitigation to use
2018-01-16 14:25 ` Boris Ostrovsky
@ 2018-01-16 15:12 ` Andrew Cooper
0 siblings, 0 replies; 55+ messages in thread
From: Andrew Cooper @ 2018-01-16 15:12 UTC (permalink / raw)
To: Boris Ostrovsky, Xen-devel
On 16/01/18 14:25, Boris Ostrovsky wrote:
> On 01/16/2018 09:13 AM, Andrew Cooper wrote:
>> On 16/01/18 14:10, Boris Ostrovsky wrote:
>>> On 01/12/2018 01:01 PM, Andrew Cooper wrote:
>>>>
>>>> + if ( boot_cpu_has(X86_FEATURE_IBRSB) )
>>>> + {
>>>> + /*
>>>> + * Even if we've chosen to not have IBRS set in Xen context, we still
>>>> + * need the IBRS entry/exit logic to virtualise IBRS support for
>>>> + * guests.
>>>> + */
>>>> + if ( ibrs )
>>>> + setup_force_cpu_cap(X86_FEATURE_XEN_IBRS_SET);
>>>> + else
>>>> + setup_force_cpu_cap(X86_FEATURE_XEN_IBRS_CLEAR);
>>>> + }
>>>>
>>> Are you going to add support for Intel's "Enhanced IBRS" (I think that's
>>> what they call the "always on" mode")?
>> I'm not going to touch IBRS_ATT mode until I've got an SDP to develop
>> against.
>>
>> Given how many times the IBRS_ATT spec has changed already, I have
>> little confidence that it will remain unchanged right to the eventual
>> hardware arrives.
> I don't know if you are aware of it (I learned about this doc on Sunday) but
>
> https://software.intel.com/sites/default/files/managed/c5/63/336996-Speculative-Execution-Side-Channel-Mitigations.pdf
>
> (Not part of the SDM but still, an official specification. For a change.)
Wow - the published 1.0 has far more than the prerelease versions.
Still, ARCH_CAPS is only going to appear in new hardware, which is still
probably years away. There are more important things to worry about at
the moment.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 08/17] x86/msr: Emulation of MSR_{SPEC_CTRL, PRED_CMD} for guests
2018-01-16 11:10 ` David Woodhouse
@ 2018-01-16 16:58 ` Andrew Cooper
2018-01-17 9:11 ` Jan Beulich
0 siblings, 1 reply; 55+ messages in thread
From: Andrew Cooper @ 2018-01-16 16:58 UTC (permalink / raw)
To: David Woodhouse, Xen-devel
On 16/01/18 11:10, David Woodhouse wrote:
> On Fri, 2018-01-12 at 18:00 +0000, Andrew Cooper wrote:
>> @@ -152,14 +163,38 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr,
>> uint64_t val)
>> {
>> const struct vcpu *curr = current;
>> struct domain *d = v->domain;
>> + const struct cpuid_policy *cp = d->arch.cpuid;
>> struct msr_domain_policy *dp = d->arch.msr;
>> struct msr_vcpu_policy *vp = v->arch.msr;
>>
>> switch ( msr )
>> {
>> case MSR_INTEL_PLATFORM_INFO:
>> + /* Read-only */
>> goto gp_fault;
>>
>> + case MSR_SPEC_CTRL:
>> + if ( !cp->feat.ibrsb )
>> + goto gp_fault; /* MSR available? */
>> + if ( val & ~(SPEC_CTRL_IBRS |
>> + (cp->feat.stibp ? SPEC_CTRL_STIBP : 0)) )
> Intel defines the STIBP bit as non-faulting and ignored, even when
> STIBP isn't advertised. So you should probably just mask it out
> if !cp->feat.stibp.
Well - this published spec finally answers several several-month-old
outstanding questions of mine.
Time for some rewriting. /sigh
>
>> + goto gp_fault; /* Rsvd bit set? */
>> + vp->spec_ctrl.guest = val;
>> + vp->spec_ctrl.host = val;
>> + break;
>> +
> There's no actual wrmsr there. This is fine, because you're going to do
> it on the way back to the guest (albeit in a later patch in the
> series). But it probably warrants a comment?
Actually, IBPB being a wrmsr() here is going to be the rare
circustances. Most of guest_wrmsr() will only be updating internal
hypervisor state, and/or the VMCS/VMCB when I move some more of the HVM
logic here.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 11/17] x86: Protect unaware domains from meddling hyperthreads
2018-01-15 11:26 ` Jan Beulich
@ 2018-01-16 21:11 ` Andrew Cooper
2018-01-17 8:40 ` Jan Beulich
0 siblings, 1 reply; 55+ messages in thread
From: Andrew Cooper @ 2018-01-16 21:11 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 15/01/18 11:26, Jan Beulich wrote:
>>>> On 12.01.18 at 19:01, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/include/asm-x86/spec_ctrl.h
>> +++ b/xen/include/asm-x86/spec_ctrl.h
>> @@ -20,8 +20,29 @@
>> #ifndef __X86_SPEC_CTRL_H__
>> #define __X86_SPEC_CTRL_H__
>>
>> +#include <xen/sched.h>
>> +
>> void init_speculation_mitigations(void);
>>
>> +/*
>> + * For guests which know about IBRS but are not told about STIBP running on
>> + * hardware supporting hyperthreading, the guest doesn't know to protect
>> + * itself fully. (Such a guest won't be permitted direct access to the MSR.)
>> + * Have Xen fill in the gaps, so an unaware guest can't be interfered with by
>> + * a meddling guest on an adjacent hyperthread.
>> + */
>> +static inline unsigned int spec_ctrl_host_val(const struct domain *d,
>> + unsigned int guest_val)
>> +{
>> + const struct cpuid_policy *cp = d->arch.cpuid;
>> +
>> + if ( !cp->feat.stibp && cpu_has_stibp &&
>> + !(guest_val & (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP)) )
>> + return SPEC_CTRL_STIBP;
>> + else
>> + return guest_val;
>> +}
> The comment's "won't be permitted direct access" can be verified
> by looking at patch 10, but where's the HT dependency here (or
> in another patch)? For now it looks to me as if you enabled this
> behind-the-back protection even in the non-HT case.
The problem is that this is all speculative programming (pardon the
pun), without answers to several questions, and without microcode to
actually try it out on. I guess I will have to unwind the changes, and
hope there isn't some leaky record of this train of thought somewhere...
(Sorry - I couldn't resist!)
I've guessed since the very first spec I saw that STIBP was expected not
always to be advertised, and the fact that there are two CPUID bits
means that, whether the situation exists in practice, it is possible to
level a VM into such a state.
Therefore, Xen should provide a defence against such a levelling so an
unassuming guest doesn't get caught out.
The latest intel spec (published since I posted v8) says:
"These include processors on which Intel Hyper-Threading Technology is
not enabled and those that do not share indirect branch predictors
between logical processors. To simplify software enabling and enhance
workload migration, STIBP may be enumerated (and setting
IA32_SPEC_CTRL.STIBP allowed) on such processors."
which I take to mean "for microcode simplicity, we will always advertise
it on HT-capable hardware", and
"A processor may enumerate support for the IA32_SPEC_CTRL MSR (e.g., by
enumerating CPUID.(EAX=7H,ECX=0):EDX[26] as 1) but not for STIBP
(CPUID.(EAX=7H,ECX=0):EDX[27] is enumerated as 0). On such processors,
execution of WRMSR to IA32_SPEC_CTRL ignores the value of bit 1 (STIBP)
and does not cause a general-protection exception (#GP) if bit 1 of the
source operand is set. It is expected that this fact will simplify
virtualization in some cases."
which I take to mean "non-HT hardware won't enumerate STIBP, but will
cope with an OS trying to use it".
An alternative to the current levelling logic would be to treat STIBP as
a Special Bit (in CPUID terms, like OSXSAVE/etc) and unconditionally set
it equal to IBRS, irrespective of the toolstack setting. That way,
migration between HT and non-HT hardware is safe and a VM which chooses
to use STIBP will work even on non-HT hardware which simply ignores the
request.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 12/17] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point
2018-01-15 12:09 ` Jan Beulich
@ 2018-01-16 21:24 ` Andrew Cooper
2018-01-17 8:47 ` Jan Beulich
0 siblings, 1 reply; 55+ messages in thread
From: Andrew Cooper @ 2018-01-16 21:24 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 15/01/18 12:09, Jan Beulich wrote:
>>>> On 12.01.18 at 19:01, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -668,6 +668,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>> set_processor_id(0);
>> set_current(INVALID_VCPU); /* debug sanity. */
>> idle_vcpu[0] = current;
>> + init_shadow_spec_ctrl_state();
> Considering the strict need to fill struct cpu_info fields early on (also
> in my SP3 band-aid) I wonder whether we wouldn't be better off
> uniformly memset()-ing the whole structure first thing here and in
> start_secondary(). But this is just a remark, not necessarily
> something to be done in this patch.
One thing I didn't quite get to in my KAISER series actually switched to
having BSP fill in the entire top-of-stack block for APs when the stack
was allocated.
I think that would be a good change in a future patch.
>
>> @@ -586,6 +611,10 @@ ENTRY(double_fault)
>> movl $TRAP_double_fault,4(%rsp)
>> /* Set AC to reduce chance of further SMAP faults */
>> SAVE_ALL STAC
>> +
>> + SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs Clob: acd */
>> + /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
> Is it actually useful to do _anything_ in the double fault handler?
Typically no, but then again we hope never to execute this code.
OTOH, we would need to do this if we ever get around to doing espfix64.
>
>> +.macro DO_SPEC_CTRL_ENTRY maybexen:req ibrs_val:req
>> +/*
>> + * Requires %rsp=regs (also cpuinfo if !maybexen)
>> + * Clobbers %rax, %rcx, %rdx
>> + *
>> + * PV guests can't update MSR_SPEC_CTRL behind Xen's back, so no need to read
>> + * it back. Entries from guest context need to clear SPEC_CTRL shadowing,
>> + * while entries from Xen must leave shadowing in its current state.
>> + */
>> + mov $MSR_SPEC_CTRL, %ecx
>> +
>> + .if \maybexen
>> + cmpw $__HYPERVISOR_CS, UREGS_cs(%rsp)
> I see you've changed to cmpw here. To eliminate your length-
> changing-prefix concern, how about using testb instead to
> just evaluate RPL or the selector, as I'm doing in the SP3
> band-aid?
That would be better still.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 11/17] x86: Protect unaware domains from meddling hyperthreads
2018-01-16 21:11 ` Andrew Cooper
@ 2018-01-17 8:40 ` Jan Beulich
2018-01-17 8:43 ` Andrew Cooper
0 siblings, 1 reply; 55+ messages in thread
From: Jan Beulich @ 2018-01-17 8:40 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 16.01.18 at 22:11, <andrew.cooper3@citrix.com> wrote:
> An alternative to the current levelling logic would be to treat STIBP as
> a Special Bit (in CPUID terms, like OSXSAVE/etc) and unconditionally set
> it equal to IBRS, irrespective of the toolstack setting. That way,
> migration between HT and non-HT hardware is safe and a VM which chooses
> to use STIBP will work even on non-HT hardware which simply ignores the
> request.
Wouldn't that overall result in simpler code anyway?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 11/17] x86: Protect unaware domains from meddling hyperthreads
2018-01-17 8:40 ` Jan Beulich
@ 2018-01-17 8:43 ` Andrew Cooper
0 siblings, 0 replies; 55+ messages in thread
From: Andrew Cooper @ 2018-01-17 8:43 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
On 17/01/2018 08:40, Jan Beulich wrote:
>>>> On 16.01.18 at 22:11, <andrew.cooper3@citrix.com> wrote:
>> An alternative to the current levelling logic would be to treat STIBP as
>> a Special Bit (in CPUID terms, like OSXSAVE/etc) and unconditionally set
>> it equal to IBRS, irrespective of the toolstack setting. That way,
>> migration between HT and non-HT hardware is safe and a VM which chooses
>> to use STIBP will work even on non-HT hardware which simply ignores the
>> request.
> Wouldn't that overall result in simpler code anyway?
Much, except its only become an option since yesterday.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 12/17] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point
2018-01-16 21:24 ` Andrew Cooper
@ 2018-01-17 8:47 ` Jan Beulich
2018-01-17 9:25 ` Andrew Cooper
0 siblings, 1 reply; 55+ messages in thread
From: Jan Beulich @ 2018-01-17 8:47 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel
>>> On 16.01.18 at 22:24, <andrew.cooper3@citrix.com> wrote:
> On 15/01/18 12:09, Jan Beulich wrote:
>>>>> On 12.01.18 at 19:01, <andrew.cooper3@citrix.com> wrote:
>>> @@ -586,6 +611,10 @@ ENTRY(double_fault)
>>> movl $TRAP_double_fault,4(%rsp)
>>> /* Set AC to reduce chance of further SMAP faults */
>>> SAVE_ALL STAC
>>> +
>>> + SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs Clob: acd */
>>> + /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
>> Is it actually useful to do _anything_ in the double fault handler?
>
> Typically no, but then again we hope never to execute this code.
>
> OTOH, we would need to do this if we ever get around to doing espfix64.
Could I get you to omit the change to the handler until then, to keep
it as straightforward as possible?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 08/17] x86/msr: Emulation of MSR_{SPEC_CTRL, PRED_CMD} for guests
2018-01-16 16:58 ` Andrew Cooper
@ 2018-01-17 9:11 ` Jan Beulich
2018-01-17 9:39 ` Andrew Cooper
0 siblings, 1 reply; 55+ messages in thread
From: Jan Beulich @ 2018-01-17 9:11 UTC (permalink / raw)
To: Andrew Cooper; +Cc: David Woodhouse, Xen-devel
>>> On 16.01.18 at 17:58, <andrew.cooper3@citrix.com> wrote:
> On 16/01/18 11:10, David Woodhouse wrote:
>> On Fri, 2018-01-12 at 18:00 +0000, Andrew Cooper wrote:
>>> @@ -152,14 +163,38 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr,
>>> uint64_t val)
>>> {
>>> const struct vcpu *curr = current;
>>> struct domain *d = v->domain;
>>> + const struct cpuid_policy *cp = d->arch.cpuid;
>>> struct msr_domain_policy *dp = d->arch.msr;
>>> struct msr_vcpu_policy *vp = v->arch.msr;
>>>
>>> switch ( msr )
>>> {
>>> case MSR_INTEL_PLATFORM_INFO:
>>> + /* Read-only */
>>> goto gp_fault;
>>>
>>> + case MSR_SPEC_CTRL:
>>> + if ( !cp->feat.ibrsb )
>>> + goto gp_fault; /* MSR available? */
>>> + if ( val & ~(SPEC_CTRL_IBRS |
>>> + (cp->feat.stibp ? SPEC_CTRL_STIBP : 0)) )
>> Intel defines the STIBP bit as non-faulting and ignored, even when
>> STIBP isn't advertised. So you should probably just mask it out
>> if !cp->feat.stibp.
>
> Well - this published spec finally answers several several-month-old
> outstanding questions of mine.
>
> Time for some rewriting. /sigh
In light of this, is there actually much point in me looking at the two
remaining v8 patches (13 and 14)?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 12/17] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point
2018-01-17 8:47 ` Jan Beulich
@ 2018-01-17 9:25 ` Andrew Cooper
0 siblings, 0 replies; 55+ messages in thread
From: Andrew Cooper @ 2018-01-17 9:25 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel
No, not really. Omitting it on the grounds of "we don't expect to take a double fault" don't beat uniformally altering all the entrypoints in a consistent manor.
The only thing which can go wrong is that we forget to do it when it is needed.
~Andrew
________________________________________
From: Jan Beulich [JBeulich@suse.com]
Sent: 17 January 2018 08:47
To: Andrew Cooper
Cc: Xen-devel
Subject: Re: [Xen-devel] [PATCH v8 12/17] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point
>>> On 16.01.18 at 22:24, <andrew.cooper3@citrix.com> wrote:
> On 15/01/18 12:09, Jan Beulich wrote:
>>>>> On 12.01.18 at 19:01, <andrew.cooper3@citrix.com> wrote:
>>> @@ -586,6 +611,10 @@ ENTRY(double_fault)
>>> movl $TRAP_double_fault,4(%rsp)
>>> /* Set AC to reduce chance of further SMAP faults */
>>> SAVE_ALL STAC
>>> +
>>> + SPEC_CTRL_ENTRY_FROM_INTR /* Req: %rsp=regs Clob: acd */
>>> + /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
>> Is it actually useful to do _anything_ in the double fault handler?
>
> Typically no, but then again we hope never to execute this code.
>
> OTOH, we would need to do this if we ever get around to doing espfix64.
Could I get you to omit the change to the handler until then, to keep
it as straightforward as possible?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 08/17] x86/msr: Emulation of MSR_{SPEC_CTRL, PRED_CMD} for guests
2018-01-17 9:11 ` Jan Beulich
@ 2018-01-17 9:39 ` Andrew Cooper
0 siblings, 0 replies; 55+ messages in thread
From: Andrew Cooper @ 2018-01-17 9:39 UTC (permalink / raw)
To: Jan Beulich; +Cc: David Woodhouse, Xen-devel
________________________________________
From: Jan Beulich [JBeulich@suse.com]
Sent: 17 January 2018 09:11
To: Andrew Cooper
Cc: David Woodhouse; Xen-devel
Subject: Re: [Xen-devel] [PATCH v8 08/17] x86/msr: Emulation of MSR_{SPEC_CTRL, PRED_CMD} for guests
>>> On 16.01.18 at 17:58, <andrew.cooper3@citrix.com> wrote:
> On 16/01/18 11:10, David Woodhouse wrote:
>> On Fri, 2018-01-12 at 18:00 +0000, Andrew Cooper wrote:
>>> @@ -152,14 +163,38 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr,
>>> uint64_t val)
>>> {
>>> const struct vcpu *curr = current;
>>> struct domain *d = v->domain;
>>> + const struct cpuid_policy *cp = d->arch.cpuid;
>>> struct msr_domain_policy *dp = d->arch.msr;
>>> struct msr_vcpu_policy *vp = v->arch.msr;
>>>
>>> switch ( msr )
>>> {
>>> case MSR_INTEL_PLATFORM_INFO:
>>> + /* Read-only */
>>> goto gp_fault;
>>>
>>> + case MSR_SPEC_CTRL:
>>> + if ( !cp->feat.ibrsb )
>>> + goto gp_fault; /* MSR available? */
>>> + if ( val & ~(SPEC_CTRL_IBRS |
>>> + (cp->feat.stibp ? SPEC_CTRL_STIBP : 0)) )
>> Intel defines the STIBP bit as non-faulting and ignored, even when
>> STIBP isn't advertised. So you should probably just mask it out
>> if !cp->feat.stibp.
>
> Well - this published spec finally answers several several-month-old
> outstanding questions of mine.
>
> Time for some rewriting. /sigh
In light of this, is there actually much point in me looking at the two
remaining v8 patches (13 and 14)?
Not really. They are going to change completely.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 15/17] x86/ctxt: Issue a speculation barrier between vcpu contexts
2018-01-15 21:39 ` David Woodhouse
@ 2018-01-17 17:26 ` David Woodhouse
2018-01-18 9:12 ` David Woodhouse
0 siblings, 1 reply; 55+ messages in thread
From: David Woodhouse @ 2018-01-17 17:26 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: Matt Wilson
[-- Attachment #1.1: Type: text/plain, Size: 1571 bytes --]
On Mon, 2018-01-15 at 22:39 +0100, David Woodhouse wrote:
> On Mon, 2018-01-15 at 14:23 +0100, David Woodhouse wrote:
> >
> >
> > >
> > > >
> > > >
> > > > Also... if you're doing that in context_switch() does it do the right
> > > > thing with idle? If a CPU switches to the idle domain and then back
> > > > again to the same vCPU, does that do the IBPB twice?
> > >
> > > Context switches to idle will skip the IBPB because it isn't needed, but
> > > any switch to non-idle need it. In your example, we should execute just
> > > a single IBPB.
>
> > In my example I think we should not execute IBPB at all. We come from a
> > given VMCS, sleep for a while, and go back to it. No need for any
> > flushing whatsoever.
>
> msw points out that in my example we *don't* execute IBPB at all, I
> think.
>
> In both switching to idle, and back to the vCPU, we should hit this
> case and not the 'else' case that does the IBPB:
>
> 1710 if ( (per_cpu(curr_vcpu, cpu) == next) ||
> 1711 (is_idle_domain(nextd) && cpu_online(cpu)) )
> 1712 {
> 1713 local_irq_enable();
> 1714 }
I tested that; it doesn't seem to be the case. We end up here with prev
being the idle thread, next being the actual vCPU, and
per_cpu(curr_vcpu, cpu) being the idle thread too. So we still do the
IBPB even when we've just switch from a given vCPU to idle and back
again.
That's not actually tested on Xen master, but the code here looks very
much the same as what I actually did test.
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 15/17] x86/ctxt: Issue a speculation barrier between vcpu contexts
2018-01-17 17:26 ` David Woodhouse
@ 2018-01-18 9:12 ` David Woodhouse
0 siblings, 0 replies; 55+ messages in thread
From: David Woodhouse @ 2018-01-18 9:12 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: Matt Wilson
[-- Attachment #1.1: Type: text/plain, Size: 1954 bytes --]
On Wed, 2018-01-17 at 18:26 +0100, David Woodhouse wrote:
>
> > In both switching to idle, and back to the vCPU, we should hit this
> > case and not the 'else' case that does the IBPB:
> >
> > 1710 if ( (per_cpu(curr_vcpu, cpu) == next) ||
> > 1711 (is_idle_domain(nextd) && cpu_online(cpu)) )
> > 1712 {
> > 1713 local_irq_enable();
> > 1714 }
>
> I tested that; it doesn't seem to be the case. We end up here with prev
> being the idle thread, next being the actual vCPU, and
> per_cpu(curr_vcpu, cpu) being the idle thread too. So we still do the
> IBPB even when we've just switch from a given vCPU to idle and back
> again.
>
> That's not actually tested on Xen master, but the code here looks very
> much the same as what I actually did test.
This appears to make the excessive IBPBs go away. There might be better
approaches.
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 04e9902..b8a9d54 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -68,6 +68,7 @@
#include <asm/pv/mm.h>
#include <asm/spec_ctrl.h>
+DEFINE_PER_CPU(struct vcpu *, last_vcpu); /* Last non-idle vCPU */
DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
static void default_idle(void);
@@ -1745,8 +1746,14 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
ctxt_switch_levelling(next);
- if ( opt_ibpb )
+ /* IBPB on switching to a non-idle vCPU, if that vCPU was not
+ * the last one to be scheduled on this pCPU */
+ if ( opt_ibpb && !is_idle_cpu(next) &&
+ per_cpu(last_vcpu, cpu) != next )
+ {
+ per_cpu(last_vcpu, cpu) = next;
wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
+ }
}
context_saved(prev);
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v8 02/17] x86: Support indirect thunks from assembly code
2018-01-12 18:00 ` [PATCH v8 02/17] x86: Support indirect thunks from assembly code Andrew Cooper
2018-01-15 10:28 ` Jan Beulich
@ 2018-02-04 10:57 ` David Woodhouse
2018-02-05 8:56 ` Jan Beulich
1 sibling, 1 reply; 55+ messages in thread
From: David Woodhouse @ 2018-02-04 10:57 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 536 bytes --]
On Fri, 2018-01-12 at 18:00 +0000, Andrew Cooper wrote:
> +#ifdef CONFIG_INDIRECT_THUNK
> + /* callq __x86_indirect_thunk_rcx */
> + ctxt->io_emul_stub[10] = 0xe8;
> + *(int32_t *)&ctxt->io_emul_stub[11] =
> + (unsigned long)__x86_indirect_thunk_rcx - (stub_va + 11 + 4);
> +
> +#else
Is that always guaranteed to be within a 32-bit offset? It's from the
stack, isn't it? Even if it's true now, do we need a sanity check just
to make *sure* things never get changed around and make it untrue?
[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v8 02/17] x86: Support indirect thunks from assembly code
2018-02-04 10:57 ` David Woodhouse
@ 2018-02-05 8:56 ` Jan Beulich
0 siblings, 0 replies; 55+ messages in thread
From: Jan Beulich @ 2018-02-05 8:56 UTC (permalink / raw)
To: David Woodhouse; +Cc: Andrew Cooper, Xen-devel
>>> On 04.02.18 at 11:57, <dwmw2@infradead.org> wrote:
> On Fri, 2018-01-12 at 18:00 +0000, Andrew Cooper wrote:
>> +#ifdef CONFIG_INDIRECT_THUNK
>> + /* callq __x86_indirect_thunk_rcx */
>> + ctxt->io_emul_stub[10] = 0xe8;
>> + *(int32_t *)&ctxt->io_emul_stub[11] =
>> + (unsigned long)__x86_indirect_thunk_rcx - (stub_va + 11 + 4);
>> +
>> +#else
>
> Is that always guaranteed to be within a 32-bit offset? It's from the
> stack, isn't it? Even if it's true now, do we need a sanity check just
> to make *sure* things never get changed around and make it untrue?
No, it's not from the stack - we've specifically switched away from
having stubs on the stack quite some time ago. The stub placement
is specifically so that they are within reach.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 55+ messages in thread
end of thread, other threads:[~2018-02-05 8:56 UTC | newest]
Thread overview: 55+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-12 18:00 [PATCH v8 00/17] x86: Mitigations for SP2/CVE-2017-5715/Branch Target Injection Andrew Cooper
2018-01-12 18:00 ` [PATCH v8 01/17] x86: Support compiling with indirect branch thunks Andrew Cooper
2018-01-14 19:48 ` David Woodhouse
2018-01-15 0:00 ` Andrew Cooper
2018-01-15 4:11 ` Konrad Rzeszutek Wilk
2018-01-15 10:14 ` Jan Beulich
2018-01-15 10:40 ` Andrew Cooper
2018-01-15 10:48 ` Jan Beulich
2018-01-12 18:00 ` [PATCH v8 02/17] x86: Support indirect thunks from assembly code Andrew Cooper
2018-01-15 10:28 ` Jan Beulich
2018-01-16 13:55 ` Andrew Cooper
2018-01-16 14:00 ` Jan Beulich
2018-02-04 10:57 ` David Woodhouse
2018-02-05 8:56 ` Jan Beulich
2018-01-12 18:00 ` [PATCH v8 03/17] x86/boot: Report details of speculative mitigations Andrew Cooper
2018-01-12 18:00 ` [PATCH v8 04/17] x86/amd: Try to set lfence as being Dispatch Serialising Andrew Cooper
2018-01-12 18:00 ` [PATCH v8 05/17] x86: Introduce alternative indirect thunks Andrew Cooper
2018-01-15 10:53 ` Jan Beulich
2018-01-12 18:00 ` [PATCH v8 06/17] x86/feature: Definitions for Indirect Branch Controls Andrew Cooper
2018-01-12 18:00 ` [PATCH v8 07/17] x86/cmdline: Introduce a command line option to disable IBRS/IBPB, STIBP and IBPB Andrew Cooper
2018-01-12 18:00 ` [PATCH v8 08/17] x86/msr: Emulation of MSR_{SPEC_CTRL, PRED_CMD} for guests Andrew Cooper
2018-01-16 11:10 ` David Woodhouse
2018-01-16 16:58 ` Andrew Cooper
2018-01-17 9:11 ` Jan Beulich
2018-01-17 9:39 ` Andrew Cooper
2018-01-12 18:00 ` [PATCH v8 09/17] x86/migrate: Move MSR_SPEC_CTRL on migrate Andrew Cooper
2018-01-12 18:01 ` [PATCH v8 10/17] x86/hvm: Permit guests direct access to MSR_{SPEC_CTRL, PRED_CMD} Andrew Cooper
2018-01-15 11:11 ` Jan Beulich
2018-01-15 16:02 ` Boris Ostrovsky
2018-01-16 0:39 ` Tian, Kevin
2018-01-12 18:01 ` [PATCH v8 11/17] x86: Protect unaware domains from meddling hyperthreads Andrew Cooper
2018-01-15 11:26 ` Jan Beulich
2018-01-16 21:11 ` Andrew Cooper
2018-01-17 8:40 ` Jan Beulich
2018-01-17 8:43 ` Andrew Cooper
2018-01-12 18:01 ` [PATCH v8 12/17] x86/entry: Organise the use of MSR_SPEC_CTRL at each entry/exit point Andrew Cooper
2018-01-15 12:09 ` Jan Beulich
2018-01-16 21:24 ` Andrew Cooper
2018-01-17 8:47 ` Jan Beulich
2018-01-17 9:25 ` Andrew Cooper
2018-01-12 18:01 ` [PATCH v8 13/17] x86/boot: Calculate the most appropriate BTI mitigation to use Andrew Cooper
2018-01-16 14:10 ` Boris Ostrovsky
2018-01-16 14:13 ` Andrew Cooper
2018-01-16 14:25 ` Boris Ostrovsky
2018-01-16 15:12 ` Andrew Cooper
2018-01-12 18:01 ` [PATCH v8 14/17] x86/entry: Clobber the Return Stack Buffer/Return Address Stack on entry to Xen Andrew Cooper
2018-01-12 18:01 ` [PATCH v8 15/17] x86/ctxt: Issue a speculation barrier between vcpu contexts Andrew Cooper
2018-01-15 12:54 ` David Woodhouse
2018-01-15 13:02 ` Andrew Cooper
2018-01-15 13:23 ` David Woodhouse
2018-01-15 21:39 ` David Woodhouse
2018-01-17 17:26 ` David Woodhouse
2018-01-18 9:12 ` David Woodhouse
2018-01-12 18:01 ` [PATCH v8 16/17] x86/cpuid: Offer Indirect Branch Controls to guests Andrew Cooper
2018-01-12 18:01 ` [PATCH v8 17/17] x86/idle: Clear SPEC_CTRL while idle Andrew Cooper
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.