[v2,03/14] x86/shstk: Introduce Supervisor Shadow Stack support
diff mbox series

Message ID 20200527191847.17207-4-andrew.cooper3@citrix.com
State New
Headers show
Series
  • x86: Support for CET Supervisor Shadow Stacks
Related show

Commit Message

Andrew Cooper May 27, 2020, 7:18 p.m. UTC
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(+)

Comments

Jan Beulich May 28, 2020, 10:25 a.m. UTC | #1
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
Andrew Cooper May 28, 2020, 6:10 p.m. UTC | #2
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
Jan Beulich May 29, 2020, 11:59 a.m. UTC | #3
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
Anthony PERARD May 29, 2020, 3:51 p.m. UTC | #4
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
Andrew Cooper May 29, 2020, 6:36 p.m. UTC | #5
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.
Andrew Cooper May 29, 2020, 6:39 p.m. UTC | #6
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
Jan Beulich June 2, 2020, 12:06 p.m. UTC | #7
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
Jan Beulich June 2, 2020, 12:09 p.m. UTC | #8
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
Anthony PERARD June 2, 2020, 12:26 p.m. UTC | #9
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,
Jan Beulich June 2, 2020, 12:41 p.m. UTC | #10
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
Anthony PERARD June 2, 2020, 1:50 p.m. UTC | #11
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.
Jan Beulich June 2, 2020, 2:13 p.m. UTC | #12
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.
>

Patch
diff mbox series

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)