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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/01/18 13:43, Jan Beulich wrote:
>>>> 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.

Ok, although I don't intend for this code to stay around for very long.

>
>> @@ -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")?

Because then the end label is wrong.  One way of another, we either need
to opencode something, or implement UNLIKELY_ELSE() which is actually
the likely case, and this is a substantially larger can of worms.

>
>> +    /*
>> +     * 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?

Technically, no.  In practice, yes.

The lfence is only needed when we require the WRMSR to set
SPEC_CTRL.IBRS to 1, and AMD don't implement IBRS yet.

>  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?

Correct.  Having the IST name also makes it more obvious when reading
entry.S

> 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()?

Hmm - I should set 0 in init_shadow_spec_ctrl_state().

All the other calculation logic needs to be deferred to the subsequent
patch for bisectiblaty.

~Andrew

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