| Message ID | 20200527191847.17207-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Support for CET Supervisor Shadow Stacks | expand |
On 27.05.2020 21:18, Andrew Cooper wrote: > 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 cet={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> > > LLVM 6 supports CET-SS instructions while only LLVM 7 supports CET-IBT > instructions. We'd need to split HAS_AS_CET into two if we want to support > supervisor shadow stacks with LLVM 6. (This demonstrates exactly why picking > a handful of instructions to test is the right approach.) In this case I agree with splitting; I wasn't aware clang implemented the respective insns piecemeal. > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -270,6 +270,23 @@ 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. > > +### cet > + = List of [ shstk=<bool> ] > + > + Applicability: x86 > + > +Controls for the use of Control-flow Enforcement Technology. CET is group of Nit: "... is a group of ..." > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -34,6 +34,10 @@ config ARCH_DEFCONFIG > config INDIRECT_THUNK > def_bool $(cc-option,-mindirect-branch-register) > > +config HAS_AS_CET > + # binutils >= 2.29 and LLVM >= 7 > + def_bool $(as-instr,wrssq %rax$(comma)0;setssbsy;endbr64) So you put me in a really awkward position: I'd really like to see this series go in for 4.14, yet I've previously indicated I want the underlying concept to first be agreed upon, before any uses get introduced. A fundamental problem with this, at least as long as (a) more of Anthony's series hasn't been committed and (b) we re-build Xen upon installing (as root), even if it was fully built (as non-root) and is ready without any re-building. Each of these aspects means that what you've configured and built may not be what gets installed, by virtue of the tool chains differing. (a) in addition may lead to the install-time rebuild to actually go wrong, due to there being dependency tracking issues on at least {xen,efi}.lds (which I've noticed in a different context yesterday). > --- 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 -) Is this actually checking the right thing in the clang case? I.e. doesn't "-x assembler" make clang invoke the system assembler rather than use its integrated one, whereas you need the insns to be recognized by the integrated assembler unless we find a need to pass -no-integrated-as? Jan
On 28/05/2020 11:25, Jan Beulich wrote: > On 27.05.2020 21:18, Andrew Cooper wrote: >> 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 cet={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> >> >> LLVM 6 supports CET-SS instructions while only LLVM 7 supports CET-IBT >> instructions. We'd need to split HAS_AS_CET into two if we want to support >> supervisor shadow stacks with LLVM 6. (This demonstrates exactly why picking >> a handful of instructions to test is the right approach.) > In this case I agree with splitting; I wasn't aware clang implemented > the respective insns piecemeal. I only became aware when trying Clang for v2. I'll turn it into HAS_AS_CET_SS, because the more I think about IBT, the more I think that will need to be tied to the compiler, rather than the assembler. >> --- a/docs/misc/xen-command-line.pandoc >> +++ b/docs/misc/xen-command-line.pandoc >> @@ -270,6 +270,23 @@ 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. >> >> +### cet >> + = List of [ shstk=<bool> ] >> + >> + Applicability: x86 >> + >> +Controls for the use of Control-flow Enforcement Technology. CET is group of > Nit: "... is a group of ..." Oops. > >> --- a/xen/arch/x86/Kconfig >> +++ b/xen/arch/x86/Kconfig >> @@ -34,6 +34,10 @@ config ARCH_DEFCONFIG >> config INDIRECT_THUNK >> def_bool $(cc-option,-mindirect-branch-register) >> >> +config HAS_AS_CET >> + # binutils >= 2.29 and LLVM >= 7 >> + def_bool $(as-instr,wrssq %rax$(comma)0;setssbsy;endbr64) > So you put me in a really awkward position: I'd really like to see > this series go in for 4.14, yet I've previously indicated I want the > underlying concept to first be agreed upon, before any uses get > introduced. There are already users. One of them is even in context. I don't see that there is anything open for dispute in the first place. Being able to do exactly this was a one key driving factor to a newer Kconfig, because it is superior mechanism to the ad-hoc mess we had previously (not to mention, a vast detriment to build time). > A fundamental problem with this, at least as long as (a) more of > Anthony's series hasn't been committed and (b) we re-build Xen upon > installing (as root), even if it was fully built (as non-root) and > is ready without any re-building. How is any any of this relevant to Anthony's recent changes? You've always been asking for trouble if your `make` before `make install` has as different toolchain, even in regular autotools/userspace software. The newer Kconfig logic might make this trouble far more obvious, but doesn't introduce the problem. > Each of these aspects means that > what you've configured and built may not be what gets installed, by > virtue of the tool chains differing. (a) in addition may lead to > the install-time rebuild to actually go wrong, due to there being > dependency tracking issues on at least {xen,efi}.lds (which I've > noticed in a different context yesterday). Again - how is any of this related to the recent changes? Fundamentally, it is either no regression from 4.13, or already needs bugfixing due to the existing users in 4.14. Neither of these options affects this patch. >> --- 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 -) > Is this actually checking the right thing in the clang case? Appears to work correctly for me. (Again, its either fine, or need bugfixing anyway for 4.14, and raising with Linux as this is unmodified upstream code like the rest of Kconfig.include). ~Andrew
On 28.05.2020 20:10, Andrew Cooper wrote: > On 28/05/2020 11:25, Jan Beulich wrote: >> On 27.05.2020 21:18, Andrew Cooper wrote: >>> --- a/xen/arch/x86/Kconfig >>> +++ b/xen/arch/x86/Kconfig >>> @@ -34,6 +34,10 @@ config ARCH_DEFCONFIG >>> config INDIRECT_THUNK >>> def_bool $(cc-option,-mindirect-branch-register) >>> >>> +config HAS_AS_CET >>> + # binutils >= 2.29 and LLVM >= 7 >>> + def_bool $(as-instr,wrssq %rax$(comma)0;setssbsy;endbr64) >> So you put me in a really awkward position: I'd really like to see >> this series go in for 4.14, yet I've previously indicated I want the >> underlying concept to first be agreed upon, before any uses get >> introduced. > > There are already users. One of them is even in context. Hmm, indeed. I clearly didn't notice this aspect when reviewing Anthony's series. > I don't see that there is anything open for dispute in the first place. > Being able to do exactly this was a one key driving factor to a newer > Kconfig, because it is superior mechanism to the ad-hoc mess we had > previously (not to mention, a vast detriment to build time). This "key driving factor" was presumably from your perspective. Could you point me to a discussion (and resulting decision) that this is an explicit goal of that work? I don't recall any, and hence I also don't recall having been given a chance in influence the direction, decision, and overall outcome. In the interest of getting this series in for 4.14, and on the assumption that you're willing to have a discussion on the direction wrt storing tool chain capabilities in .config before any further uses get added (and with the potential need to undo the ones we have / gain here) Reviewed-by: Jan Beulich <jbeulich@suse.com> The comment at the very end may point at an issue that wants sorting, the others below are merely replies to some of the points you've made. >> A fundamental problem with this, at least as long as (a) more of >> Anthony's series hasn't been committed and (b) we re-build Xen upon >> installing (as root), even if it was fully built (as non-root) and >> is ready without any re-building. > > How is any any of this relevant to Anthony's recent changes? Command line options not getting written to .*.cmd and read back and compare upon rebuild is the issue here. Albeit thinking about it again for anything that's stored in .config that's for now compensated for by a change to .config triggering a global rebuild. The specific issue I had run into was with XEN_BUILD_EFI changing without xen.lds getting re-generated. > You've always been asking for trouble if your `make` before `make > install` has as different toolchain, even in regular autotools/userspace > software. The newer Kconfig logic might make this trouble far more > obvious, but doesn't introduce the problem. I disagree. There should be no dependency on the tool chain at all at install time, with a fully built tree. It ought to be fine to not even have a tool chain accessible to root, even less so the precise one that was used for building the tree. >>> --- 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 -) >> Is this actually checking the right thing in the clang case? > > Appears to work correctly for me. (Again, its either fine, or need > bugfixing anyway for 4.14, and raising with Linux as this is unmodified > upstream code like the rest of Kconfig.include). This answer made me try it out: On a system with clang 5 and binutils 2.32 "incsspd %eax" translates fine with -no-integrated-as but doesn't without. The previously mentioned odd use of CLANG_FLAGS, perhaps together with the disconnect from where we establish whether to use -no-integrated-as in the first place (arch.mk; I have no idea whether the CFLAGS determined would be usable by the kconfig invocation, nor how to solve the chicken-and-egg problem there if this is meant to work that way), may be the reason for this. Cc-ing Anthony once again ... As an aside - this being taken from Linux doesn't mean it's suitable for our use. For example, Linux'es way to use (or not) -no-integrated-as is entirely different from ours (via LLVM_IAS setting, used in the top level Makefile). I don't think I can see whether the check above, for the case at hand, would work correctly there, both with and without that variable set. Jan
On Fri, May 29, 2020 at 01:59:55PM +0200, Jan Beulich wrote: > On 28.05.2020 20:10, Andrew Cooper wrote: > > On 28/05/2020 11:25, Jan Beulich wrote: > >> On 27.05.2020 21:18, 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 -) > >> Is this actually checking the right thing in the clang case? > > > > Appears to work correctly for me. (Again, its either fine, or need > > bugfixing anyway for 4.14, and raising with Linux as this is unmodified > > upstream code like the rest of Kconfig.include). > > This answer made me try it out: On a system with clang 5 and > binutils 2.32 "incsspd %eax" translates fine with > -no-integrated-as but doesn't without. The previously mentioned > odd use of CLANG_FLAGS, perhaps together with the disconnect > from where we establish whether to use -no-integrated-as in the > first place (arch.mk; I have no idea whether the CFLAGS > determined would be usable by the kconfig invocation, nor how > to solve the chicken-and-egg problem there if this is meant to > work that way), may be the reason for this. Cc-ing Anthony once > again ... I've just prepare/sent a patch which should fix this CLANG_FLAGS issue and allows to tests the assembler in Kconfig. See: [XEN PATCH] xen/build: introduce CLANG_FLAGS for testing other CFLAGS
On 29/05/2020 12:59, Jan Beulich wrote: > On 28.05.2020 20:10, Andrew Cooper wrote: >> On 28/05/2020 11:25, Jan Beulich wrote: >>> On 27.05.2020 21:18, Andrew Cooper wrote: >>>> --- a/xen/arch/x86/Kconfig >>>> +++ b/xen/arch/x86/Kconfig >>>> @@ -34,6 +34,10 @@ config ARCH_DEFCONFIG >>>> config INDIRECT_THUNK >>>> def_bool $(cc-option,-mindirect-branch-register) >>>> >>>> +config HAS_AS_CET >>>> + # binutils >= 2.29 and LLVM >= 7 >>>> + def_bool $(as-instr,wrssq %rax$(comma)0;setssbsy;endbr64) >>> So you put me in a really awkward position: I'd really like to see >>> this series go in for 4.14, yet I've previously indicated I want the >>> underlying concept to first be agreed upon, before any uses get >>> introduced. >> There are already users. One of them is even in context. > Hmm, indeed. I clearly didn't notice this aspect when reviewing > Anthony's series. > >> I don't see that there is anything open for dispute in the first place. >> Being able to do exactly this was a one key driving factor to a newer >> Kconfig, because it is superior mechanism to the ad-hoc mess we had >> previously (not to mention, a vast detriment to build time). > This "key driving factor" was presumably from your perspective. > Could you point me to a discussion (and resulting decision) that > this is an explicit goal of that work? I don't recall any, and > hence I also don't recall having been given a chance in influence > the direction, decision, and overall outcome. It took up a large chunk of the build system design session in Chicago. > > In the interest of getting this series in for 4.14, and on the > assumption that you're willing to have a discussion on the > direction wrt storing tool chain capabilities in .config before > any further uses get added (and with the potential need to undo > the ones we have / gain here) > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks.
On 29/05/2020 16:51, Anthony PERARD wrote: > On Fri, May 29, 2020 at 01:59:55PM +0200, Jan Beulich wrote: >> On 28.05.2020 20:10, Andrew Cooper wrote: >>> On 28/05/2020 11:25, Jan Beulich wrote: >>>> On 27.05.2020 21:18, 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 -) >>>> Is this actually checking the right thing in the clang case? >>> Appears to work correctly for me. (Again, its either fine, or need >>> bugfixing anyway for 4.14, and raising with Linux as this is unmodified >>> upstream code like the rest of Kconfig.include). >> This answer made me try it out: On a system with clang 5 and >> binutils 2.32 "incsspd %eax" translates fine with >> -no-integrated-as but doesn't without. The previously mentioned >> odd use of CLANG_FLAGS, perhaps together with the disconnect >> from where we establish whether to use -no-integrated-as in the >> first place (arch.mk; I have no idea whether the CFLAGS >> determined would be usable by the kconfig invocation, nor how >> to solve the chicken-and-egg problem there if this is meant to >> work that way), may be the reason for this. Cc-ing Anthony once >> again ... > I've just prepare/sent a patch which should fix this CLANG_FLAGS issue > and allows to tests the assembler in Kconfig. > > See: [XEN PATCH] xen/build: introduce CLANG_FLAGS for testing other CFLAGS Thanks. I've checked carefully, and this is an improvement from before. Therefore I have acked and included the patch. However, I think there is a further problem. When -no-integrated-as does get passed down, I don't see Clang falling back to using the Gnu assember, so I suspect we have a further plumbing problem. (It only affects Clang 5.0 and earlier, so obsolete toolchains these days). ~Andrew
On 29.05.2020 20:36, Andrew Cooper wrote: > On 29/05/2020 12:59, Jan Beulich wrote: >> On 28.05.2020 20:10, Andrew Cooper wrote: >>> On 28/05/2020 11:25, Jan Beulich wrote: >>>> On 27.05.2020 21:18, Andrew Cooper wrote: >>>>> --- a/xen/arch/x86/Kconfig >>>>> +++ b/xen/arch/x86/Kconfig >>>>> @@ -34,6 +34,10 @@ config ARCH_DEFCONFIG >>>>> config INDIRECT_THUNK >>>>> def_bool $(cc-option,-mindirect-branch-register) >>>>> >>>>> +config HAS_AS_CET >>>>> + # binutils >= 2.29 and LLVM >= 7 >>>>> + def_bool $(as-instr,wrssq %rax$(comma)0;setssbsy;endbr64) >>>> So you put me in a really awkward position: I'd really like to see >>>> this series go in for 4.14, yet I've previously indicated I want the >>>> underlying concept to first be agreed upon, before any uses get >>>> introduced. >>> There are already users. One of them is even in context. >> Hmm, indeed. I clearly didn't notice this aspect when reviewing >> Anthony's series. >> >>> I don't see that there is anything open for dispute in the first place. >>> Being able to do exactly this was a one key driving factor to a newer >>> Kconfig, because it is superior mechanism to the ad-hoc mess we had >>> previously (not to mention, a vast detriment to build time). >> This "key driving factor" was presumably from your perspective. >> Could you point me to a discussion (and resulting decision) that >> this is an explicit goal of that work? I don't recall any, and >> hence I also don't recall having been given a chance in influence >> the direction, decision, and overall outcome. > > It took up a large chunk of the build system design session in Chicago. I don't recall; perhaps I was in another parallel session? If it's the one with notes at https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg00786.html then a remark close to the top suggests I was there, but there's no sign of this aspect having got discussed. There is, among the issues listed, "Xen build re-evaluates compiler support for every translation unit", but that's only remotely related. Jan
On 29.05.2020 20:39, Andrew Cooper wrote: > On 29/05/2020 16:51, Anthony PERARD wrote: >> On Fri, May 29, 2020 at 01:59:55PM +0200, Jan Beulich wrote: >>> On 28.05.2020 20:10, Andrew Cooper wrote: >>>> On 28/05/2020 11:25, Jan Beulich wrote: >>>>> On 27.05.2020 21:18, 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 -) >>>>> Is this actually checking the right thing in the clang case? >>>> Appears to work correctly for me. (Again, its either fine, or need >>>> bugfixing anyway for 4.14, and raising with Linux as this is unmodified >>>> upstream code like the rest of Kconfig.include). >>> This answer made me try it out: On a system with clang 5 and >>> binutils 2.32 "incsspd %eax" translates fine with >>> -no-integrated-as but doesn't without. The previously mentioned >>> odd use of CLANG_FLAGS, perhaps together with the disconnect >>> from where we establish whether to use -no-integrated-as in the >>> first place (arch.mk; I have no idea whether the CFLAGS >>> determined would be usable by the kconfig invocation, nor how >>> to solve the chicken-and-egg problem there if this is meant to >>> work that way), may be the reason for this. Cc-ing Anthony once >>> again ... >> I've just prepare/sent a patch which should fix this CLANG_FLAGS issue >> and allows to tests the assembler in Kconfig. >> >> See: [XEN PATCH] xen/build: introduce CLANG_FLAGS for testing other CFLAGS > > Thanks. I've checked carefully, and this is an improvement from before. > > Therefore I have acked and included the patch. > > However, I think there is a further problem. When -no-integrated-as > does get passed down, I don't see Clang falling back to using the Gnu > assember, so I suspect we have a further plumbing problem. (It only > affects Clang 5.0 and earlier, so obsolete toolchains these days). Hmm, what exactly do you mean saying "I don't see Clang falling back ..."? In the playing I did, I specifically passed -v to see what gets or does not get invoked, and it was /usr/bin/as that did get used (clang 5.0.1). Obviously it'll be whatever /usr/bin/as is then. Jan
On Tue, Jun 02, 2020 at 02:06:11PM +0200, Jan Beulich wrote: > I don't recall; perhaps I was in another parallel session? If it's > the one with notes at > https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg00786.html > then a remark close to the top suggests I was there, but there's no > sign of this aspect having got discussed. There is, among the issues > listed, "Xen build re-evaluates compiler support for every translation > unit", but that's only remotely related. What is a "translation unit"? What would that have to do with Kconfig? Thanks,
On 02.06.2020 14:26, Anthony PERARD wrote: > On Tue, Jun 02, 2020 at 02:06:11PM +0200, Jan Beulich wrote: >> I don't recall; perhaps I was in another parallel session? If it's >> the one with notes at >> https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg00786.html >> then a remark close to the top suggests I was there, but there's no >> sign of this aspect having got discussed. There is, among the issues >> listed, "Xen build re-evaluates compiler support for every translation >> unit", but that's only remotely related. > > What is a "translation unit"? What would that have to do with Kconfig? A translation unit is the collective input from all source and header files seen by the compiler during the processing of one top level input file's worth of compilation. The connection to Kconfig here is that by switching to it, the compiler flags don't get re-constructed once per CU. Of course doing it via Kconfig is not the only possible solution to the problem (as can be seen from the patch that I had intermediately submitted to get at least the HAVE_AS_* out of that set), but for us the change in behavior is one intended (side-)effect of the switch to newer Kconfig. Jan
On Tue, Jun 02, 2020 at 02:41:30PM +0200, Jan Beulich wrote: > On 02.06.2020 14:26, Anthony PERARD wrote: > > On Tue, Jun 02, 2020 at 02:06:11PM +0200, Jan Beulich wrote: > >> I don't recall; perhaps I was in another parallel session? If it's > >> the one with notes at > >> https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg00786.html > >> then a remark close to the top suggests I was there, but there's no > >> sign of this aspect having got discussed. There is, among the issues > >> listed, "Xen build re-evaluates compiler support for every translation > >> unit", but that's only remotely related. > > > > What is a "translation unit"? What would that have to do with Kconfig? > > A translation unit is the collective input from all source and > header files seen by the compiler during the processing of one > top level input file's worth of compilation. Thanks. Now it feels like the issue listed in the design session note isn't exactly correct, compiler didn't used to be evaluated once per CU, but only once per Makefile loaded/executed. > The connection to > Kconfig here is that by switching to it, the compiler flags > don't get re-constructed once per CU. Of course doing it via > Kconfig is not the only possible solution to the problem (as > can be seen from the patch that I had intermediately submitted > to get at least the HAVE_AS_* out of that set), but for us the > change in behavior is one intended (side-)effect of the switch > to newer Kconfig. Well, with a recent patch of mine, tool chain support should already be only evaluated only once in the root Makefile. So I do also think that we don't need to move all of them to Kconfig. So advantage of evaluating some compiler flags in Kconfig is that we can have other CONFIG_* depends on those flags; the inconvenient is to figure out when do we need to re-evaluate the .config.
On 02.06.2020 15:50, Anthony PERARD wrote: > On Tue, Jun 02, 2020 at 02:41:30PM +0200, Jan Beulich wrote: >> On 02.06.2020 14:26, Anthony PERARD wrote: >>> On Tue, Jun 02, 2020 at 02:06:11PM +0200, Jan Beulich wrote: >>>> I don't recall; perhaps I was in another parallel session? If it's >>>> the one with notes at >>>> https://lists.xenproject.org/archives/html/xen-devel/2019-07/msg00786.html >>>> then a remark close to the top suggests I was there, but there's no >>>> sign of this aspect having got discussed. There is, among the issues >>>> listed, "Xen build re-evaluates compiler support for every translation >>>> unit", but that's only remotely related. >>> >>> What is a "translation unit"? What would that have to do with Kconfig? >> >> A translation unit is the collective input from all source and >> header files seen by the compiler during the processing of one >> top level input file's worth of compilation. > > Thanks. Now it feels like the issue listed in the design session note > isn't exactly correct, compiler didn't used to be evaluated once per CU, > but only once per Makefile loaded/executed. > >> The connection to >> Kconfig here is that by switching to it, the compiler flags >> don't get re-constructed once per CU. Of course doing it via >> Kconfig is not the only possible solution to the problem (as >> can be seen from the patch that I had intermediately submitted >> to get at least the HAVE_AS_* out of that set), but for us the >> change in behavior is one intended (side-)effect of the switch >> to newer Kconfig. > > Well, with a recent patch of mine, tool chain support should already be > only evaluated only once in the root Makefile. So I do also think that > we don't need to move all of them to Kconfig. > > So advantage of evaluating some compiler flags in Kconfig is that we > can have other CONFIG_* depends on those flags; I'm not even certain this is an advantage: CET-SS is going a different direction than the majority of cases we have had in the tree so far (INDIRECT_THUNK is another such case). In the majority of cases, though, the tool chain capabilities don't affect functionality, but just code quality. And indeed I think that's how Kconfig should be used: When configuring Xen, it ought to be specified what functionality is wanted, irrespective of compilers used/available. In particular a syncconfig run would better not require a host compiler to be available at all, such that configurations for targets for which there's no cross compiler can also be produced. (Not sure whether that's still the case, but our internal kernel processing used to work that way, without requiring everyone to have compilers for all target architectures our kernels support.) It would be the responsibility of the person doing the config to ensure only options get enabled that can actually be built in the environment that the full build is actually to be run in. (That is, for the case it's inconvenient [CET-SS] or impossible [INDIRECT_THUNK] to build without suitable tool chain support, I'm not questioning the desire to have certain features outright unavailable. It's just that I don't see why this unavailability needs to be expressed at the Kconfig level.) Jan > the inconvenient is to > figure out when do we need to re-evaluate the .config. >
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index e16bb90184..d4934eabb7 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -270,6 +270,23 @@ 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. +### cet + = List of [ shstk=<bool> ] + + Applicability: x86 + +Controls for the use of Control-flow Enforcement Technology. CET is group of +hardware features designed to combat Return-oriented Programming (ROP, also +call/jmp COP/JOP) attacks. + +* The `shstk=` boolean controls whether Xen uses Shadow Stacks for its own + protection. + + The option is available when `CONFIG_XEN_SHSTK` is compiled in, and + defaults to `true` on hardware supporting CET-SS. Specifying + `cet=no-shstk` will cause Xen not to use Shadow Stacks even when support + is available in hardware. + ### clocksource (x86) > `= pit | hpet | acpi | tsc` diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index b565f6831d..304a42ffb2 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -34,6 +34,10 @@ config ARCH_DEFCONFIG config INDIRECT_THUNK def_bool $(cc-option,-mindirect-branch-register) +config HAS_AS_CET + # binutils >= 2.29 and LLVM >= 7 + def_bool $(as-instr,wrssq %rax$(comma)0;setssbsy;endbr64) + menu "Architecture Features" source "arch/Kconfig" @@ -97,6 +101,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 2dec7a3fc6..584589baff 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 __init parse_cet(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", "cet", s, ss); +#endif + } + else + rc = -EINVAL; + + s = ss + 1; + } while ( *ss ); + + return rc; +} +custom_param("cet", parse_cet); + 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 cadef4e824..b831448eba 100644 --- a/xen/include/asm-x86/cpufeature.h +++ b/xen/include/asm-x86/cpufeature.h @@ -137,6 +137,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 cet={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> LLVM 6 supports CET-SS instructions while only LLVM 7 supports CET-IBT instructions. We'd need to split HAS_AS_CET into two if we want to support supervisor shadow stacks with LLVM 6. (This demonstrates exactly why picking a handful of instructions to test is the right approach.) v2: * Leave a comment identifying minimum toolchain support, to make it easier to remove ifdefary in the future when bumping minima. * Reindent CONFIG_XEN_SHSTK help text. * Rename xen= to cet=. Add documentation, __init. --- docs/misc/xen-command-line.pandoc | 17 +++++++++++++++++ xen/arch/x86/Kconfig | 18 ++++++++++++++++++ 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 ++++ 6 files changed, 71 insertions(+)