Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.09.2019 20:24, Andrew Cooper wrote:
> The code generation for barrier_nospec_true() is not correct.  We are taking a
> perf it from the added fences, but not gaining any speculative safety.

You want to be more specific here, I think: ISTR you saying that the LFENCEs
get inserted at the wrong place. IIRC we want them on either side of a
conditional branch, i.e. immediately following a branch insn as well as right
at the branch target. I've taken, as a simple example,
p2m_mem_access_sanity_check(), and this looks to be the way gcc9 has generated
code (in a non-debug build). Hence either I'm mis-remembering what we want
things to look like, or there's more to it than code generation simply being
"not correct".

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -96,6 +96,23 @@ config SPECULATIVE_ARRAY_HARDEN
>  
>         If unsure, say Y.
>  
> +config SPECULATIVE_BRANCH_HARDEN
> +     bool "Speculative Branch Hardening"
> +     depends on BROKEN
> +        ---help---
> +       Contemporary processors may use speculative execution as a
> +       performance optimisation, but this can potentially be abused by an
> +       attacker to leak data via speculative sidechannels.
> +
> +       One source of misbehaviour is by executing the wrong basic block
> +       following a conditional jump.
> +
> +       When enabled, specific conditions which have been deemed liable to
> +       be speculatively abused will be hardened to avoid entering the wrong
> +       basic block.
> +
> +       !!! WARNING - This is broken and doesn't generate safe code !!!

Not being a native speaker, this read to me as "is broken in general",
whereas the brokenness is that according to your analysis safe code
does not result. Therefore how about "This is broken in that it doesn't
generate safe code"?

> --- a/xen/include/asm-x86/nospec.h
> +++ b/xen/include/asm-x86/nospec.h
> @@ -9,8 +9,8 @@
>  /* Allow to insert a read memory barrier into conditionals */
>  static always_inline bool barrier_nospec_true(void)
>  {
> -#ifdef CONFIG_HVM
> -    alternative("", "lfence", X86_FEATURE_SC_L1TF_VULN);
> +#ifdef CONFIG_SPECULATIVE_BRANCH_HARDEN
> +    alternative("", "lfence", X86_FEATURE_ALWAYS);

Why alternative() then and not just asm()?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel