Re: [Xen-devel] [PATCH v10 07/11] x86/entry: Avoid using alternatives in NMI/#MC paths

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 24.01.18 at 14:12, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/xen/include/asm-x86/spec_ctrl_asm.h
> +++ b/xen/include/asm-x86/spec_ctrl_asm.h
> @@ -20,6 +20,11 @@
>  #ifndef __X86_SPEC_CTRL_ASM_H__
>  #define __X86_SPEC_CTRL_ASM_H__
>  
> +/* Encoding of the bottom bits in cpuinfo.bti_ist_info */
> +#define BTI_IST_IBRS  (1 << 0)

This should be annotated to make clear it can't change its value. Or
maybe have something BUILD_BUG_ON()-like elsewhere.

> @@ -256,6 +261,69 @@
>          DO_SPEC_CTRL_EXIT_TO_GUEST, X86_FEATURE_XEN_IBRS_SET,           \
>          DO_SPEC_CTRL_EXIT_TO_GUEST, X86_FEATURE_XEN_IBRS_CLEAR
>  
> +/* TODO: Drop these when the alternatives infrastructure is NMI/#MC safe. */
> +.macro SPEC_CTRL_ENTRY_FROM_INTR_IST
> +/*
> + * Requires %rsp=regs, %r14=stack_end
> + * Clobbers %rax, %rcx, %rdx
> + *
> + * This is logical merge of DO_OVERWRITE_RSB and DO_SPEC_CTRL_ENTRY
> + * maybexen=1, but with conditionals rather than alternatives.
> + */
> +    movzbl STACK_CPUINFO_FIELD(bti_ist_info)(%r14), %eax
> +
> +    testb $BTI_IST_RSB, %al
> +    jz .L\@_skip_rsb
> +
> +    DO_OVERWRITE_RSB
> +
> +.L\@_skip_rsb:
> +
> +    testb $BTI_IST_WRMSR, %al
> +    jz .L\@_skip_wrmsr
> +
> +    testb $3, UREGS_cs(%rsp)
> +    jz .L\@_entry_from_xen
> +
> +    movb $0, STACK_CPUINFO_FIELD(use_shadow_spec_ctrl)(%r14)
> +
> +.L\@_entry_from_xen:
> +    /*
> +     * Load Xen's intended value.  SPEC_CTRL_IBRS vs 0 is encoded in the
> +     * bottom bit of bti_ist_info, via a deliberate alias with BTI_IST_IBRS.
> +     */
> +    mov $MSR_SPEC_CTRL, %ecx
> +    and $BTI_IST_IBRS, %eax
> +    xor %edx, %edx
> +    wrmsr
> +
> +    /* Opencoded UNLIKELY_START() with no condition. */
> +.Ldispatch.\@_serialise:
> +    .subsection 1

How about adding .ifnb to UNLIKELY_START(), instead of open coding?
Or wait - why can't you use it as-is above (instead of the
"jz .L\@_skip_wrmsr")?

> +    /*
> +     * In the case that we might need to write to MSR_SPEC_CTRL for safety, 
> we
> +     * need to ensure that an attacker can't poison the `jz 
> .L\@_skip_wrmsr`
> +     * to speculate around the WRMSR.  As a result, we need a dispatch
> +     * serialising instruction in the else clause.
> +     */
> +.L\@_skip_wrmsr:
> +    lfence
> +    UNLIKELY_END(\@_serialise)
> +.endm

Is LFENCE alone sufficient here for AMD in case "x86/amd: Try to
set lfence as being Dispatch Serialising" failed to achieve the intended
effect? In fact an LFENCE -> MFENCE conversion (through
alternatives patching) would even be safe on the IST paths, as the
two encodings differ by a single byte only.

> +.macro SPEC_CTRL_EXIT_TO_XEN_IST
> +/*
> + * Requires %rbx=stack_end
> + * Clobbers %rax, %rcx, %rdx
> + */
> +    testb $BTI_IST_WRMSR, STACK_CPUINFO_FIELD(bti_ist_info)(%rbx)
> +    jz .L\@_skip
> +
> +    DO_SPEC_CTRL_EXIT_TO_XEN
> +
> +.L\@_skip:
> +.endm

Why a new macro, instead of modifying the existing
SPEC_CTRL_EXIT_TO_XEN, the sole use site of which is being
changed? Just for the ease of eventually reverting?

And then, having reached the end of the patch - where is the
new struct cpu_info field written? Or is this again happening only in
later patches, with the one here just setting the stage? If so,
shouldn't you at least zero the field in init_shadow_spec_ctrl_state()?

Jan


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