[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 3/3] x86/spec-ctrl: Fix NMI race condition with VT-x MSR_SPEC_CTRL handling
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.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |