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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 26/01/18 12:23, Jan Beulich wrote:
>>>> On 25.01.18 at 18:21, <andrew.cooper3@xxxxxxxxxx> wrote:
>> This patch is deliberately arranged to be easy to revert if/when alternatives
>> patching becomes NMI/#MC safe.
>>
>> For safety, there must be a dispatch serialising instruction in (what is
>> logically) DO_SPEC_CTRL_ENTRY so that, in the case that Xen needs IBRS set in
>> context, an attacker can't speculate around the WRMSR and reach an indirect
>> branch within the speculation window.
>>
>> Using conditionals opens this attack vector up, so the else clause gets an
>> LFENCE to force the pipeline to catch up before continuing.  This also covers
>> the safety of RSB conditional, as execution it is guaranteed to either hit 
>> the
>> WRMSR or LFENCE.
>>
>> One downside of not using alternatives is that there unconditionally an 
>> LFENCE
>> in the IST path in cases where we are not using the features from 
>> IBRS-capable
>> microcode.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> Despite this yet another remark:
>
>> @@ -255,6 +260,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)
> I take it you deliberately didn't want to use the same setz/and
> pair here to avoid at least the second branch here too, as we
> very much hope to only rarely hit this code path?

No - straight oversight.  I will fix up equivalently.

~Andrew

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