| Message ID | 20200501225838.9866-6-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Support for CET Supervisor Shadow Stacks | expand |
On 02.05.2020 00:58, Andrew Cooper wrote: > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -34,6 +34,9 @@ config ARCH_DEFCONFIG > config INDIRECT_THUNK > def_bool $(cc-option,-mindirect-branch-register) > > +config HAS_AS_CET > + def_bool $(as-instr,wrssq %rax$(comma)0;setssbsy;endbr64) I see you add as-instr here as a side effect. Until the other similar checks get converted, I think for the time being we should use the old model, to have all such checks in one place. I realize this means you can't have a Kconfig dependency then. Also why do you check multiple insns, when just one (like we do elsewhere) would suffice? The crucial insns to check are those which got changed pretty close before the release of 2.29 (in the cover letter you mention 2.32): One of incssp{d,q}, setssbsy, or saveprevssp. There weren't official binutils releases with the original insns, but distros may still have picked up intermediate snapshots. > @@ -97,6 +100,20 @@ config HVM > > If unsure, say Y. > > +config XEN_SHSTK > + bool "Supervisor Shadow Stacks" > + depends on HAS_AS_CET && EXPERT = "y" > + 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. Shadow Stacks are one CET feature > + designed to provide return address protection. > + > + This option arranges for Xen to use CET-SS for its own protection. > + When CET-SS is active, 32bit PV guests cannot be used. Backwards > + compatiblity can be provided vai the PV Shim mechanism. Indentation looks odd here - the whole help section should start with hard tabs, I think. > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -95,6 +95,36 @@ unsigned long __initdata highmem_start; > size_param("highmem-start", highmem_start); > #endif > > +static bool __initdata opt_xen_shstk = true; > + > +static int parse_xen(const char *s) > +{ > + const char *ss; > + int val, rc = 0; > + > + do { > + ss = strchr(s, ','); > + if ( !ss ) > + ss = strchr(s, '\0'); > + > + if ( (val = parse_boolean("shstk", s, ss)) >= 0 ) > + { > +#ifdef CONFIG_XEN_SHSTK > + opt_xen_shstk = val; > +#else > + no_config_param("XEN_SHSTK", "xen", s, ss); > +#endif > + } > + else > + rc = -EINVAL; > + > + s = ss + 1; > + } while ( *ss ); > + > + return rc; > +} > +custom_param("xen", parse_xen); What's the idea here going forward, i.e. why the new top level "xen" option? Almost all options are for Xen itself, after all. Did you perhaps mean this to be "cet"? Also you surely meant to document this new option? > --- a/xen/scripts/Kconfig.include > +++ b/xen/scripts/Kconfig.include > @@ -31,6 +31,10 @@ cc-option = $(success,$(CC) -Werror $(CLANG_FLAGS) $(1) -E -x c /dev/null -o /de > # Return y if the linker supports <flag>, n otherwise > ld-option = $(success,$(LD) -v $(1)) > > +# $(as-instr,<instr>) > +# Return y if the assembler supports <instr>, n otherwise > +as-instr = $(success,printf "%b\n" "$(1)" | $(CC) $(CLANG_FLAGS) -c -x assembler -o /dev/null -) CLANG_FLAGS caught my eye here, then noticing that cc-option also uses it. Anthony - what's the deal with this? It doesn't look to get defined anywhere, and I also don't see what clang- specific about these constructs. Jan
On 04/05/2020 14:52, Jan Beulich wrote: > [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe. > > On 02.05.2020 00:58, Andrew Cooper wrote: >> --- a/xen/arch/x86/Kconfig >> +++ b/xen/arch/x86/Kconfig >> @@ -34,6 +34,9 @@ config ARCH_DEFCONFIG >> config INDIRECT_THUNK >> def_bool $(cc-option,-mindirect-branch-register) >> >> +config HAS_AS_CET >> + def_bool $(as-instr,wrssq %rax$(comma)0;setssbsy;endbr64) > I see you add as-instr here as a side effect. Until the other > similar checks get converted, I think for the time being we should > use the old model, to have all such checks in one place. I realize > this means you can't have a Kconfig dependency then. No. That's like asking me to keep on using bool_t, and you are the first person to point out and object to that in newly submitted patches. > Also why do you check multiple insns, when just one (like we do > elsewhere) would suffice? Because CET-SS and CET-IBT are orthogonal ABIs, but you wanted a single CET symbol, rather than a CET_SS symbol. I picked a sample of various instructions to get broad coverage of CET without including every instruction. > The crucial insns to check are those which got changed pretty > close before the release of 2.29 (in the cover letter you > mention 2.32): One of incssp{d,q}, setssbsy, or saveprevssp. > There weren't official binutils releases with the original > insns, but distros may still have picked up intermediate > snapshots. I've got zero interest in catering to distros which are still using obsolete pre-release toolchains. Bleeding edge toolchains are one thing, but this is like asking us to support the middle changeset of the series introducing CET, which is absolutely a non-starter. If the instructions were missing from real binutils releases, then obviously we should exclude those releases, but that doesn't appear to be the case. >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -95,6 +95,36 @@ unsigned long __initdata highmem_start; >> size_param("highmem-start", highmem_start); >> #endif >> >> +static bool __initdata opt_xen_shstk = true; >> + >> +static int parse_xen(const char *s) >> +{ >> + const char *ss; >> + int val, rc = 0; >> + >> + do { >> + ss = strchr(s, ','); >> + if ( !ss ) >> + ss = strchr(s, '\0'); >> + >> + if ( (val = parse_boolean("shstk", s, ss)) >= 0 ) >> + { >> +#ifdef CONFIG_XEN_SHSTK >> + opt_xen_shstk = val; >> +#else >> + no_config_param("XEN_SHSTK", "xen", s, ss); >> +#endif >> + } >> + else >> + rc = -EINVAL; >> + >> + s = ss + 1; >> + } while ( *ss ); >> + >> + return rc; >> +} >> +custom_param("xen", parse_xen); > What's the idea here going forward, i.e. why the new top level > "xen" option? Almost all options are for Xen itself, after all. > Did you perhaps mean this to be "cet"? I forgot an RFC for this, as I couldn't think of anything better. "cet" as a top level option isn't going to gain more than {no-}shstk and {no-}ibt as suboptions. >> --- a/xen/scripts/Kconfig.include >> +++ b/xen/scripts/Kconfig.include >> @@ -31,6 +31,10 @@ cc-option = $(success,$(CC) -Werror $(CLANG_FLAGS) $(1) -E -x c /dev/null -o /de >> # Return y if the linker supports <flag>, n otherwise >> ld-option = $(success,$(LD) -v $(1)) >> >> +# $(as-instr,<instr>) >> +# Return y if the assembler supports <instr>, n otherwise >> +as-instr = $(success,printf "%b\n" "$(1)" | $(CC) $(CLANG_FLAGS) -c -x assembler -o /dev/null -) > CLANG_FLAGS caught my eye here, then noticing that cc-option > also uses it. Anthony - what's the deal with this? It doesn't > look to get defined anywhere, and I also don't see what clang- > specific about these constructs. This is as it inherits from Linux. There is obviously a reason. However, I'm not interested in diving into that rabbit hole in an unrelated series. ~Andrew
On 11.05.2020 17:46, Andrew Cooper wrote: > On 04/05/2020 14:52, Jan Beulich wrote: >> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments unless you have verified the sender and know the content is safe. >> >> On 02.05.2020 00:58, Andrew Cooper wrote: >>> --- a/xen/arch/x86/Kconfig >>> +++ b/xen/arch/x86/Kconfig >>> @@ -34,6 +34,9 @@ config ARCH_DEFCONFIG >>> config INDIRECT_THUNK >>> def_bool $(cc-option,-mindirect-branch-register) >>> >>> +config HAS_AS_CET >>> + def_bool $(as-instr,wrssq %rax$(comma)0;setssbsy;endbr64) >> I see you add as-instr here as a side effect. Until the other >> similar checks get converted, I think for the time being we should >> use the old model, to have all such checks in one place. I realize >> this means you can't have a Kconfig dependency then. > > No. That's like asking me to keep on using bool_t, and you are the > first person to point out and object to that in newly submitted patches. These are entirely different things. The bool_t => bool conversion is agreed upon. The conversion to record tool chain capabilities in xen/.config isn't. I've raised my reservations against this elsewhere. I can be convinced, but not by trying to introduce such functionality as a side effect of an unrelated change. >> Also why do you check multiple insns, when just one (like we do >> elsewhere) would suffice? > > Because CET-SS and CET-IBT are orthogonal ABIs, but you wanted a single > CET symbol, rather than a CET_SS symbol. > > I picked a sample of various instructions to get broad coverage of CET > without including every instruction. I wanted HAS_AS_CET rather than HAS_AS_CET_SS and HAS_AS_CET_IBT because both got added to gas at the same time, and hence there's little point in having separate symbols. If there was a reason to assume assemblers might be out there which support one but not the other, then we indeed ought to have two symbols. >> The crucial insns to check are those which got changed pretty >> close before the release of 2.29 (in the cover letter you >> mention 2.32): One of incssp{d,q}, setssbsy, or saveprevssp. >> There weren't official binutils releases with the original >> insns, but distros may still have picked up intermediate >> snapshots. > > I've got zero interest in catering to distros which are still using > obsolete pre-release toolchains. Bleeding edge toolchains are one > thing, but this is like asking us to support the middle changeset of the > series introducing CET, which is absolutely a non-starter. > > If the instructions were missing from real binutils releases, then > obviously we should exclude those releases, but that doesn't appear to > be the case. But you realize that there's no special effort needed? We merely need to check for the right insns. Their operands not matching for binutils from the intermediate time window is enough for our purposes. With my remark I merely meant to guide which of the three insns you've picked needs to remain, and which would imo better be dropped. >>> --- a/xen/arch/x86/setup.c >>> +++ b/xen/arch/x86/setup.c >>> @@ -95,6 +95,36 @@ unsigned long __initdata highmem_start; >>> size_param("highmem-start", highmem_start); >>> #endif >>> >>> +static bool __initdata opt_xen_shstk = true; >>> + >>> +static int parse_xen(const char *s) >>> +{ >>> + const char *ss; >>> + int val, rc = 0; >>> + >>> + do { >>> + ss = strchr(s, ','); >>> + if ( !ss ) >>> + ss = strchr(s, '\0'); >>> + >>> + if ( (val = parse_boolean("shstk", s, ss)) >= 0 ) >>> + { >>> +#ifdef CONFIG_XEN_SHSTK >>> + opt_xen_shstk = val; >>> +#else >>> + no_config_param("XEN_SHSTK", "xen", s, ss); >>> +#endif >>> + } >>> + else >>> + rc = -EINVAL; >>> + >>> + s = ss + 1; >>> + } while ( *ss ); >>> + >>> + return rc; >>> +} >>> +custom_param("xen", parse_xen); >> What's the idea here going forward, i.e. why the new top level >> "xen" option? Almost all options are for Xen itself, after all. >> Did you perhaps mean this to be "cet"? > > I forgot an RFC for this, as I couldn't think of anything better. "cet" > as a top level option isn't going to gain more than {no-}shstk and > {no-}ibt as suboptions. Imo that's still better than "xen=". >>> --- a/xen/scripts/Kconfig.include >>> +++ b/xen/scripts/Kconfig.include >>> @@ -31,6 +31,10 @@ cc-option = $(success,$(CC) -Werror $(CLANG_FLAGS) $(1) -E -x c /dev/null -o /de >>> # Return y if the linker supports <flag>, n otherwise >>> ld-option = $(success,$(LD) -v $(1)) >>> >>> +# $(as-instr,<instr>) >>> +# Return y if the assembler supports <instr>, n otherwise >>> +as-instr = $(success,printf "%b\n" "$(1)" | $(CC) $(CLANG_FLAGS) -c -x assembler -o /dev/null -) >> CLANG_FLAGS caught my eye here, then noticing that cc-option >> also uses it. Anthony - what's the deal with this? It doesn't >> look to get defined anywhere, and I also don't see what clang- >> specific about these constructs. > > This is as it inherits from Linux. There is obviously a reason. > > However, I'm not interested in diving into that rabbit hole in an > unrelated series. That's fine - my question was directed at Anthony after all. Jan
On Mon, May 04, 2020 at 03:52:58PM +0200, Jan Beulich wrote: > On 02.05.2020 00:58, Andrew Cooper wrote: > > --- a/xen/scripts/Kconfig.include > > +++ b/xen/scripts/Kconfig.include > > @@ -31,6 +31,10 @@ cc-option = $(success,$(CC) -Werror $(CLANG_FLAGS) $(1) -E -x c /dev/null -o /de > > # Return y if the linker supports <flag>, n otherwise > > ld-option = $(success,$(LD) -v $(1)) > > > > +# $(as-instr,<instr>) > > +# Return y if the assembler supports <instr>, n otherwise > > +as-instr = $(success,printf "%b\n" "$(1)" | $(CC) $(CLANG_FLAGS) -c -x assembler -o /dev/null -) > > CLANG_FLAGS caught my eye here, then noticing that cc-option > also uses it. Anthony - what's the deal with this? It doesn't > look to get defined anywhere, and I also don't see what clang- > specific about these constructs. It's because these constructs are gcc-specific :-). Indeed CLANG_FLAGS probably needs to be defined as I don't think providing the full AFLAGS/CFLAGS is going to be a good idea and may change the result of the commands. Linux has a few clang specific flags in CLANG_FLAGS, I have found those: The ones for cross compilation: --prefix, --target, --gcc-toolchain -no-integrated-as -Werror=unknown-warning-option And that's it. So, I think we could keep using CLANG_FLAGS in Kconfig.include and define it in Makefile with a comment saying that it's only used by Kconfig. It would always have -Werror=unknown-warning-option and have -no-integrated-as when needed, the -Wunknown-warning-option is present in clang 3.0.0, according Linux's commit 589834b3a009 ("kbuild: Add -Werror=unknown-warning-option to CLANG_FLAGS"). The options -Werror=unknown-warning-option is to make sure that the warning is enabled, even though it is by default but could be disabled in a particular build of clang, see e8de12fb7cde ("kbuild: Check for unknown options with cc-option usage in Kconfig and clang") I'll write a patch with this new CLANG_FLAGS.
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 96432f1f69..ebd01e6893 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -34,6 +34,9 @@ config ARCH_DEFCONFIG config INDIRECT_THUNK def_bool $(cc-option,-mindirect-branch-register) +config HAS_AS_CET + def_bool $(as-instr,wrssq %rax$(comma)0;setssbsy;endbr64) + menu "Architecture Features" source "arch/Kconfig" @@ -97,6 +100,20 @@ config HVM If unsure, say Y. +config XEN_SHSTK + bool "Supervisor Shadow Stacks" + depends on HAS_AS_CET && EXPERT = "y" + 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. Shadow Stacks are one CET feature + designed to provide return address protection. + + This option arranges for Xen to use CET-SS for its own protection. + When CET-SS is active, 32bit PV guests cannot be used. Backwards + compatiblity can be provided vai the PV Shim mechanism. + config SHADOW_PAGING bool "Shadow Paging" default y diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 9e9576344c..aa21201507 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -95,6 +95,36 @@ unsigned long __initdata highmem_start; size_param("highmem-start", highmem_start); #endif +static bool __initdata opt_xen_shstk = true; + +static int parse_xen(const char *s) +{ + const char *ss; + int val, rc = 0; + + do { + ss = strchr(s, ','); + if ( !ss ) + ss = strchr(s, '\0'); + + if ( (val = parse_boolean("shstk", s, ss)) >= 0 ) + { +#ifdef CONFIG_XEN_SHSTK + opt_xen_shstk = val; +#else + no_config_param("XEN_SHSTK", "xen", s, ss); +#endif + } + else + rc = -EINVAL; + + s = ss + 1; + } while ( *ss ); + + return rc; +} +custom_param("xen", parse_xen); + cpumask_t __read_mostly cpu_present_map; unsigned long __read_mostly xen_phys_start; diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h index 859970570b..6b25f61832 100644 --- a/xen/include/asm-x86/cpufeature.h +++ b/xen/include/asm-x86/cpufeature.h @@ -136,6 +136,7 @@ #define cpu_has_aperfmperf boot_cpu_has(X86_FEATURE_APERFMPERF) #define cpu_has_lfence_dispatch boot_cpu_has(X86_FEATURE_LFENCE_DISPATCH) #define cpu_has_xen_lbr boot_cpu_has(X86_FEATURE_XEN_LBR) +#define cpu_has_xen_shstk boot_cpu_has(X86_FEATURE_XEN_SHSTK) #define cpu_has_msr_tsc_aux (cpu_has_rdtscp || cpu_has_rdpid) diff --git a/xen/include/asm-x86/cpufeatures.h b/xen/include/asm-x86/cpufeatures.h index b9d3cac975..d7e42d9bb6 100644 --- a/xen/include/asm-x86/cpufeatures.h +++ b/xen/include/asm-x86/cpufeatures.h @@ -38,6 +38,7 @@ XEN_CPUFEATURE(XEN_LBR, X86_SYNTH(22)) /* Xen uses MSR_DEBUGCTL.LBR */ XEN_CPUFEATURE(SC_VERW_PV, X86_SYNTH(23)) /* VERW used by Xen for PV */ XEN_CPUFEATURE(SC_VERW_HVM, X86_SYNTH(24)) /* VERW used by Xen for HVM */ XEN_CPUFEATURE(SC_VERW_IDLE, X86_SYNTH(25)) /* VERW used by Xen for idle */ +XEN_CPUFEATURE(XEN_SHSTK, X86_SYNTH(26)) /* Xen uses CET Shadow Stacks */ /* Bug words follow the synthetic words. */ #define X86_NR_BUG 1 diff --git a/xen/scripts/Kconfig.include b/xen/scripts/Kconfig.include index 8221095ca3..e1f13e1720 100644 --- a/xen/scripts/Kconfig.include +++ b/xen/scripts/Kconfig.include @@ -31,6 +31,10 @@ cc-option = $(success,$(CC) -Werror $(CLANG_FLAGS) $(1) -E -x c /dev/null -o /de # Return y if the linker supports <flag>, n otherwise ld-option = $(success,$(LD) -v $(1)) +# $(as-instr,<instr>) +# Return y if the assembler supports <instr>, n otherwise +as-instr = $(success,printf "%b\n" "$(1)" | $(CC) $(CLANG_FLAGS) -c -x assembler -o /dev/null -) + # check if $(CC) and $(LD) exist $(error-if,$(failure,command -v $(CC)),compiler '$(CC)' not found) $(error-if,$(failure,command -v $(LD)),linker '$(LD)' not found)
Introduce CONFIG_HAS_AS_CET to determine whether CET instructions are supported in the assembler, and CONFIG_XEN_SHSTK as the main build option. Introduce xen={no-,}shstk to for a user to select whether or not to use shadow stacks at runtime, and X86_FEATURE_XEN_SHSTK to determine Xen's overall enablement of shadow stacks. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/Kconfig | 17 +++++++++++++++++ xen/arch/x86/setup.c | 30 ++++++++++++++++++++++++++++++ xen/include/asm-x86/cpufeature.h | 1 + xen/include/asm-x86/cpufeatures.h | 1 + xen/scripts/Kconfig.include | 4 ++++ 5 files changed, 53 insertions(+)