Re: [PATCH 3/3] x86/spec-ctrl: Fix NMI race condition with VT-x MSR_SPEC_CTRL handling

[Andrew Cooper <amc96@xxxxxxxx>]

On 14/01/2022 13:43, Roger Pau Monné wrote:
> On Fri, Jan 14, 2022 at 01:08:43PM +0000, Andrew Cooper wrote:
>> On 14/01/2022 12:50, Roger Pau Monné wrote:
>>> On Thu, Jan 13, 2022 at 04:38:33PM +0000, Andrew Cooper wrote:
>>>> The logic was based on a mistaken understanding of how NMI blocking on 
>>>> vmexit
>>>> works.  NMIs are only blocked for EXIT_REASON_NMI, and not for general 
>>>> exits.
>>>> Therefore, an NMI can in general hit early in the vmx_asm_vmexit_handler 
>>>> path,
>>>> and the guest's value will be clobbered before it is saved.
>>>>
>>>> Switch to using MSR load/save lists.  This causes the guest value to be 
>>>> saved
>>>> atomically with respect to NMIs/MCEs/etc.
>>>>
>>>> First, update vmx_cpuid_policy_changed() to configure the load/save lists 
>>>> at
>>>> the same time as configuring the intercepts.  This function is always used 
>>>> in
>>>> remote context, so extend the vmx_vmcs_{enter,exit}() block to cover the 
>>>> whole
>>>> function, rather than having multiple remote acquisitions of the same VMCS.
>>>>
>>>> vmx_add_guest_msr() can fail, but only in ways which are entirely fatal to 
>>>> the
>>>> guest, so handle failures using domain_crash().  vmx_del_msr() can fail 
>>>> with
>>>> -ESRCH but we don't matter, and this path will be taken during domain 
>>>> create
>>>> on a system lacking IBRSB.
>>>>
>>>> Second, update vmx_msr_{read,write}_intercept() to use the load/save lists
>>>> rather than vcpu_msrs, and update the comment to describe the new state
>>>> location.
>>>>
>>>> Finally, adjust the entry/exit asm.  Drop DO_SPEC_CTRL_ENTRY_FROM_HVM
>>>> entirely, and use a shorter code sequence to simply reload Xen's setting 
>>>> from
>>>> the top-of-stack block.
>>>>
>>>> Because the guest values are loaded/saved atomically, we do not need to use
>>>> the shadowing logic to cope with late NMIs/etc, so we can omit
>>>> DO_SPEC_CTRL_EXIT_TO_GUEST entirely and VMRESUME/VMLAUNCH with Xen's value 
>>>> in
>>>> context.  Furthermore, we can drop the SPEC_CTRL_ENTRY_FROM_PV too, as 
>>>> there's
>>>> no need to switch back to Xen's context on an early failure.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>>> CC: Wei Liu <wl@xxxxxxx>
>>>> CC: Jun Nakajima <jun.nakajima@xxxxxxxxx>
>>>> CC: Kevin Tian <kevin.tian@xxxxxxxxx>
>>>>
>>>> Needs backporting as far as people can tolerate.
>>>>
>>>> If the entry/exit logic were in C, I'd ASSERT() that shadow tracking is 
>>>> off,
>>>> but this is awkard to arrange in asm.
>>>> ---
>>>>  xen/arch/x86/hvm/vmx/entry.S             | 19 ++++++++++-------
>>>>  xen/arch/x86/hvm/vmx/vmx.c               | 36 
>>>> +++++++++++++++++++++++++++-----
>>>>  xen/arch/x86/include/asm/msr.h           | 10 ++++++++-
>>>>  xen/arch/x86/include/asm/spec_ctrl_asm.h | 32 ++++------------------------
>>>>  4 files changed, 56 insertions(+), 41 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
>>>> index 30139ae58e9d..297ed8685126 100644
>>>> --- a/xen/arch/x86/hvm/vmx/entry.S
>>>> +++ b/xen/arch/x86/hvm/vmx/entry.S
>>>> @@ -35,7 +35,14 @@ ENTRY(vmx_asm_vmexit_handler)
>>>>  
>>>>          /* SPEC_CTRL_ENTRY_FROM_VMX    Req: b=curr %rsp=regs/cpuinfo, 
>>>> Clob: acd */
>>>>          ALTERNATIVE "", DO_OVERWRITE_RSB, X86_FEATURE_SC_RSB_HVM
>>>> -        ALTERNATIVE "", DO_SPEC_CTRL_ENTRY_FROM_HVM, 
>>>> X86_FEATURE_SC_MSR_HVM
>>>> +
>>>> +        .macro restore_spec_ctrl
>>>> +            mov    $MSR_SPEC_CTRL, %ecx
>>>> +            movzbl CPUINFO_xen_spec_ctrl(%rsp), %eax
>>>> +            xor    %edx, %edx
>>>> +            wrmsr
>>>> +        .endm
>>>> +        ALTERNATIVE "", restore_spec_ctrl, X86_FEATURE_SC_MSR_HVM
>>> I assume loading the host value into SPEC_CTRL strictly needs to
>>> happen after the RSB overwrite, or else you could use a VM exit host
>>> load MSR entry to handle MSR_SPEC_CTRL.
>> We could use the host load list, but Intel warned that the lists aren't
>> as efficient as writing it like this, hence why I didn't use them
>> originally when we thought the NMI behaviour was different.  Obviously,
>> we're using them now for correctness reasons, which is more important
>> than avoiding them for (unquantified) perf reasons.
>>
>> I've got some plans for changes to how Xen handles MSR_SPEC_CTRL for its
>> own protection, which I hope will bring a substantial efficiency
>> improvements, and those would require us not to use a host load list here.
> Might be worth mentioning that further work will prevent us from using
> host load list for SPEC_CTRL has a comment here or in the commit
> message.
>
> Using host load lists would also allow us to get rid of
> X86_FEATURE_SC_MSR_HVM.

No it wouldn't.  I need to reuse this for the AMD side, and it also
encodes "admin turned it off via spec-ctrl=".

> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

~Andrew