Re: [PATCH 01/65] x86: Introduce support for CET-IBT
On 10.12.2021 15:20, Andrew Cooper wrote: > On 29/11/2021 09:21, Jan Beulich wrote: >> On 26.11.2021 16:21, Andrew Cooper wrote: >>> On 26/11/2021 14:10, Jan Beulich wrote: On 26.11.2021 13:33, Andrew Cooper wrote: > @@ -124,6 +129,18 @@ config XEN_SHSTK > When CET-SS is active, 32bit PV guests cannot be used. Backwards > compatiblity can be provided via the PV Shim mechanism. > > +config XEN_IBT > + bool "Supervisor Indirect Branch Tracking" > + depends on HAS_CC_CET_IBT > + default y > + help > + Control-flow Enforcement Technology (CET) is a set of features in > + hardware designed to combat Return-oriented Programming (ROP, also > + call/jump COP/JOP) attacks. Indirect Branch Tracking is one CET > + feature designed to provide function pointer protection. > + > + This option arranges for Xen to use CET-IBT for its own protection. Shouldn't this depend on BROKEN until it's actually functional? >>> It compiles fine right from now, and making it BROKEN would inhibit >>> bisection through the series. >>> >>> Nothing actually matters until patch 65 turns on MSR_S_CET.ENDBR_EN. >> "Nothing" except that until then the promised extra security isn't >> there. > > The series is very likely to be committed in one fell swoop, but even > that aside, it really doesn't matter until 4.17-rc1 > > As it stands, this is ~65 patches of incremental changes to the binary, > and oughtn't to be 65 nops and a massive switch at the end. Well, I'm not convinced, but I can live with it being the way you have it. Jan
Re: [PATCH 01/65] x86: Introduce support for CET-IBT
On 29/11/2021 09:21, Jan Beulich wrote: > On 26.11.2021 16:21, Andrew Cooper wrote: >> On 26/11/2021 14:10, Jan Beulich wrote: >>> On 26.11.2021 13:33, Andrew Cooper wrote: @@ -124,6 +129,18 @@ config XEN_SHSTK When CET-SS is active, 32bit PV guests cannot be used. Backwards compatiblity can be provided via the PV Shim mechanism. +config XEN_IBT + bool "Supervisor Indirect Branch Tracking" + depends on HAS_CC_CET_IBT + default y + help +Control-flow Enforcement Technology (CET) is a set of features in +hardware designed to combat Return-oriented Programming (ROP, also +call/jump COP/JOP) attacks. Indirect Branch Tracking is one CET +feature designed to provide function pointer protection. + +This option arranges for Xen to use CET-IBT for its own protection. >>> Shouldn't this depend on BROKEN until it's actually functional? >> It compiles fine right from now, and making it BROKEN would inhibit >> bisection through the series. >> >> Nothing actually matters until patch 65 turns on MSR_S_CET.ENDBR_EN. > "Nothing" except that until then the promised extra security isn't > there. The series is very likely to be committed in one fell swoop, but even that aside, it really doesn't matter until 4.17-rc1 As it stands, this is ~65 patches of incremental changes to the binary, and oughtn't to be 65 nops and a massive switch at the end. ~Andrew
Re: [PATCH 01/65] x86: Introduce support for CET-IBT
On 29/11/2021 09:27, Jan Beulich wrote: > On 26.11.2021 13:33, Andrew Cooper wrote: >> --- a/xen/arch/x86/arch.mk >> +++ b/xen/arch/x86/arch.mk >> @@ -46,6 +46,12 @@ CFLAGS-$(CONFIG_INDIRECT_THUNK) += >> -mindirect-branch=thunk-extern >> CFLAGS-$(CONFIG_INDIRECT_THUNK) += -mindirect-branch-register >> CFLAGS-$(CONFIG_INDIRECT_THUNK) += -fno-jump-tables >> >> +ifdef CONFIG_HAS_CC_CET_IBT >> +CFLAGS += -fcf-protection=branch -mmanual-endbr > Don't you mean to check XEN_IBT here rather than the underlying > compiler capability? I did, and elsewhere in the patch (already fixed up). I added CONFIG_XEN_IBT rather late in the dev cycle, and missed a few conversions. ~Andrew
Re: [PATCH 01/65] x86: Introduce support for CET-IBT
On 26.11.2021 13:33, Andrew Cooper wrote: > --- a/xen/arch/x86/arch.mk > +++ b/xen/arch/x86/arch.mk > @@ -46,6 +46,12 @@ CFLAGS-$(CONFIG_INDIRECT_THUNK) += > -mindirect-branch=thunk-extern > CFLAGS-$(CONFIG_INDIRECT_THUNK) += -mindirect-branch-register > CFLAGS-$(CONFIG_INDIRECT_THUNK) += -fno-jump-tables > > +ifdef CONFIG_HAS_CC_CET_IBT > +CFLAGS += -fcf-protection=branch -mmanual-endbr Don't you mean to check XEN_IBT here rather than the underlying compiler capability? Jan
Re: [PATCH 01/65] x86: Introduce support for CET-IBT
On 26.11.2021 16:21, Andrew Cooper wrote: > On 26/11/2021 14:10, Jan Beulich wrote: >> On 26.11.2021 13:33, Andrew Cooper wrote: >>> @@ -124,6 +129,18 @@ config XEN_SHSTK >>> When CET-SS is active, 32bit PV guests cannot be used. Backwards >>> compatiblity can be provided via the PV Shim mechanism. >>> >>> +config XEN_IBT >>> + bool "Supervisor Indirect Branch Tracking" >>> + depends on HAS_CC_CET_IBT >>> + default y >>> + help >>> + Control-flow Enforcement Technology (CET) is a set of features in >>> + hardware designed to combat Return-oriented Programming (ROP, also >>> + call/jump COP/JOP) attacks. Indirect Branch Tracking is one CET >>> + feature designed to provide function pointer protection. >>> + >>> + This option arranges for Xen to use CET-IBT for its own protection. >> Shouldn't this depend on BROKEN until it's actually functional? > > It compiles fine right from now, and making it BROKEN would inhibit > bisection through the series. > > Nothing actually matters until patch 65 turns on MSR_S_CET.ENDBR_EN. "Nothing" except that until then the promised extra security isn't there. >>> --- a/xen/arch/x86/x86_emulate/x86_emulate.h >>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h >>> @@ -35,6 +35,11 @@ >>> # error Unknown compilation width >>> #endif >>> >>> +#ifndef cf_check >>> +/* Cope with userspace build not knowing about CET-IBT */ >>> +#define cf_check >>> +#endif >> Imo this shouldn't go here, but in tools/tests/x86_emulator/x86-emulate.h, >> and then presumably without #ifdef. > > I considered that, but the test harness isn't the only userspace > harness. There is the fuzzing harness too, and I'm not sure we want to > force every userspace harness to provide the same workaround. But that's the idea of putting it where I suggested: This header gets re-used by the fuzzing harness: x86-emulate.c x86-emulate.h wrappers.c: %: [ -L $* ] || ln -sf $(XEN_ROOT)/tools/tests/x86_emulator/$* Jan
Re: [PATCH 01/65] x86: Introduce support for CET-IBT
On 26/11/2021 14:10, Jan Beulich wrote: > On 26.11.2021 13:33, Andrew Cooper wrote: >> @@ -124,6 +129,18 @@ config XEN_SHSTK >>When CET-SS is active, 32bit PV guests cannot be used. Backwards >>compatiblity can be provided via the PV Shim mechanism. >> >> +config XEN_IBT >> +bool "Supervisor Indirect Branch Tracking" >> +depends on HAS_CC_CET_IBT >> +default y >> +help >> + Control-flow Enforcement Technology (CET) is a set of features in >> + hardware designed to combat Return-oriented Programming (ROP, also >> + call/jump COP/JOP) attacks. Indirect Branch Tracking is one CET >> + feature designed to provide function pointer protection. >> + >> + This option arranges for Xen to use CET-IBT for its own protection. > Shouldn't this depend on BROKEN until it's actually functional? It compiles fine right from now, and making it BROKEN would inhibit bisection through the series. Nothing actually matters until patch 65 turns on MSR_S_CET.ENDBR_EN. >> --- a/xen/arch/x86/x86_emulate/x86_emulate.h >> +++ b/xen/arch/x86/x86_emulate/x86_emulate.h >> @@ -35,6 +35,11 @@ >> # error Unknown compilation width >> #endif >> >> +#ifndef cf_check >> +/* Cope with userspace build not knowing about CET-IBT */ >> +#define cf_check >> +#endif > Imo this shouldn't go here, but in tools/tests/x86_emulator/x86-emulate.h, > and then presumably without #ifdef. I considered that, but the test harness isn't the only userspace harness. There is the fuzzing harness too, and I'm not sure we want to force every userspace harness to provide the same workaround. ~Andrew
Re: [PATCH 01/65] x86: Introduce support for CET-IBT
On 26.11.2021 13:33, Andrew Cooper wrote: > @@ -124,6 +129,18 @@ config XEN_SHSTK > When CET-SS is active, 32bit PV guests cannot be used. Backwards > compatiblity can be provided via the PV Shim mechanism. > > +config XEN_IBT > + bool "Supervisor Indirect Branch Tracking" > + depends on HAS_CC_CET_IBT > + default y > + help > + Control-flow Enforcement Technology (CET) is a set of features in > + hardware designed to combat Return-oriented Programming (ROP, also > + call/jump COP/JOP) attacks. Indirect Branch Tracking is one CET > + feature designed to provide function pointer protection. > + > + This option arranges for Xen to use CET-IBT for its own protection. Shouldn't this depend on BROKEN until it's actually functional? > --- a/xen/arch/x86/x86_emulate/x86_emulate.h > +++ b/xen/arch/x86/x86_emulate/x86_emulate.h > @@ -35,6 +35,11 @@ > # error Unknown compilation width > #endif > > +#ifndef cf_check > +/* Cope with userspace build not knowing about CET-IBT */ > +#define cf_check > +#endif Imo this shouldn't go here, but in tools/tests/x86_emulator/x86-emulate.h, and then presumably without #ifdef. Jan
[PATCH 01/65] x86: Introduce support for CET-IBT
CET Indirect Branch Tracking is a hardware feature designed to provide
forward-edge control flow integrity, protecting against jump/call oriented
programming.
IBT requires the placement of ENDBR{32,64} instructions at the target of every
indirect call/jmp, and every entrypoint.
However, the default -fcf-protection=branch places an ENDBR on every function
which far more than necessary, and reduces the quantity of protection
afforded. Therefore, we use manual placement using the cf_check attribute.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Roger Pau Monné
CC: Wei Liu
---
Config.mk | 1 -
tools/firmware/Makefile| 2 ++
xen/arch/x86/Kconfig | 17 +
xen/arch/x86/arch.mk | 6 ++
xen/arch/x86/x86_emulate/x86_emulate.h | 5 +
xen/include/asm-x86/asm-defns.h| 6 ++
xen/include/asm-x86/cpufeature.h | 1 +
xen/include/asm-x86/cpufeatures.h | 1 +
xen/include/xen/compiler.h | 6 ++
9 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/Config.mk b/Config.mk
index 6587c7d626c9..508261a7dcf4 100644
--- a/Config.mk
+++ b/Config.mk
@@ -199,7 +199,6 @@ APPEND_CFLAGS += $(foreach i, $(APPEND_INCLUDES), -I$(i))
EMBEDDED_EXTRA_CFLAGS := -nopie -fno-stack-protector -fno-stack-protector-all
EMBEDDED_EXTRA_CFLAGS += -fno-exceptions -fno-asynchronous-unwind-tables
-EMBEDDED_EXTRA_CFLAGS += -fcf-protection=none
XEN_EXTFILES_URL ?= http://xenbits.xen.org/xen-extfiles
# All the files at that location were downloaded from elsewhere on
diff --git a/tools/firmware/Makefile b/tools/firmware/Makefile
index 1f2711779400..b2fd73248604 100644
--- a/tools/firmware/Makefile
+++ b/tools/firmware/Makefile
@@ -6,6 +6,8 @@ TARGET := hvmloader/hvmloader
INST_DIR := $(DESTDIR)$(XENFIRMWAREDIR)
DEBG_DIR := $(DESTDIR)$(DEBUG_DIR)$(XENFIRMWAREDIR)
+EMBEDDED_EXTRA_CFLAGS += -fcf-protection=none
+
SUBDIRS-y :=
SUBDIRS-$(CONFIG_OVMF) += ovmf-dir
SUBDIRS-$(CONFIG_SEABIOS) += seabios-dir
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index b4abfca46f6a..8b7ad0145b29 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -39,6 +39,11 @@ config HAS_AS_CET_SS
# binutils >= 2.29 or LLVM >= 6
def_bool $(as-instr,wrssq %rax$(comma)0;setssbsy)
+config HAS_CC_CET_IBT
+ # GCC >= 9 and binutils >= 2.29
+ # Retpoline check to work around
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93654
+ def_bool $(cc-option,-fcf-protection=branch -mmanual-endbr
-mindirect-branch=thunk-extern) && $(as-instr,endbr64)
+
menu "Architecture Features"
source "arch/Kconfig"
@@ -124,6 +129,18 @@ config XEN_SHSTK
When CET-SS is active, 32bit PV guests cannot be used. Backwards
compatiblity can be provided via the PV Shim mechanism.
+config XEN_IBT
+ bool "Supervisor Indirect Branch Tracking"
+ depends on HAS_CC_CET_IBT
+ default y
+ help
+ Control-flow Enforcement Technology (CET) is a set of features in
+ hardware designed to combat Return-oriented Programming (ROP, also
+ call/jump COP/JOP) attacks. Indirect Branch Tracking is one CET
+ feature designed to provide function pointer protection.
+
+ This option arranges for Xen to use CET-IBT for its own protection.
+
config SHADOW_PAGING
bool "Shadow Paging"
default !PV_SHIM_EXCLUSIVE
diff --git a/xen/arch/x86/arch.mk b/xen/arch/x86/arch.mk
index ce0c1a0e7fb2..1c8381f7c9d8 100644
--- a/xen/arch/x86/arch.mk
+++ b/xen/arch/x86/arch.mk
@@ -46,6 +46,12 @@ CFLAGS-$(CONFIG_INDIRECT_THUNK) +=
-mindirect-branch=thunk-extern
CFLAGS-$(CONFIG_INDIRECT_THUNK) += -mindirect-branch-register
CFLAGS-$(CONFIG_INDIRECT_THUNK) += -fno-jump-tables
+ifdef CONFIG_HAS_CC_CET_IBT
+CFLAGS += -fcf-protection=branch -mmanual-endbr
+else
+$(call cc-option-add,CFLAGS,CC,-fcf-protection=none)
+endif
+
# If supported by the compiler, reduce stack alignment to 8 bytes. But allow
# this to be overridden elsewhere.
$(call cc-option-add,CFLAGS-stack-boundary,CC,-mpreferred-stack-boundary=3)
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h
b/xen/arch/x86/x86_emulate/x86_emulate.h
index d8fb3a990933..4a483a464804 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -35,6 +35,11 @@
# error Unknown compilation width
#endif
+#ifndef cf_check
+/* Cope with userspace build not knowing about CET-IBT */
+#define cf_check
+#endif
+
struct x86_emulate_ctxt;
/*
diff --git a/xen/include/asm-x86/asm-defns.h b/xen/include/asm-x86/asm-defns.h
index 505f39ad5f76..8bd9007731d5 100644
--- a/xen/include/asm-x86/asm-defns.h
+++ b/xen/include/asm-x86/asm-defns.h
@@ -57,6 +57,12 @@
INDIRECT_BRANCH jmp \arg
.endm
+#ifdef CONFIG_XEN_IBT
+# define ENDBR64 endbr64
+#else
+# define ENDBR64
+#endif
+
.macro guest_access_mask_ptr ptr:req, scratch1:req, scratch2:req
#if
