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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

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

Thanks, Roger.