Re: [PATCH 05/16] x86/shstk: Introduce Supervisor Shadow Stack support

[Jan Beulich <jbeulich@xxxxxxxx>]

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