[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


  • To: Andrew Cooper <amc96@xxxxxxxx>
  • From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Date: Fri, 14 Jan 2022 14:43:37 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=w5zm/qg9AgnMmqKsmLuBRBFsWgZfbtV31x/hP1k2gfk=; b=k7Hb4pQ7vZf0gCEx68mjjISLKXhhQUPQT1FAZzVwjxeKVZgDOYPpKvXAGmHaA99suP8dZqoBLQMVXJfnCNRieoh3qP/JNSeNq/PL8FnBrbUXVVrYcEJt69emsLbPRnqS927Hn0GJ+9oKaW69Ik/p91IHY0ug0QPMUfyiYtWok/7cOVH+SjGH1citZW4pwyMToGwNCW2PPKgVYmbs1/LDOtVQbMq3Zl67G7Je90HjvLN2COigBW4NmAGxwbpHhkxc3tV+KisNgbRZYnYUqo7BAsZtfq4CFTSVOce8uyQ+1ejJUZUDTCruLTeZbzM23NlZCOS+XUIE7jGs4RmAmXNwTQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=gjGdtLy2LLq2/wjJXwHc0lwZccWAfzmGpactN2dZBHxxsUrVVpUlyAg1QzQaPCMea8D0z3+tkplHq/2DTUnB1LXv8LcJUEWVcTfCEvQ8eD2vwYbuDUIA+YKakhoD6IvgZHDm/0JE/+mLgGGyAWdcK6hFxIgS7Pv32B28B3m6BKjTgOtcoS3Z+7Up/K1kGfYMpkGlXNw66yskxTzJCVz/WQ4DMCsLjdP9iPNMpbV1AFOtHd+lmXDcc7ZS9qtkfJVChGdLcMaXd2DMJcuVcJIW8GEs6pjQwNIvRfI+0lwjQZHBpTqmeU6uBuAZvarW0NxxncftIkuR1lpzuw3kDp0G6g==
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Jan Beulich <JBeulich@xxxxxxxx>, Wei Liu <wl@xxxxxxx>, Jun Nakajima <jun.nakajima@xxxxxxxxx>, Kevin Tian <kevin.tian@xxxxxxxxx>
  • Delivery-date: Fri, 14 Jan 2022 13:44:10 +0000
  • Ironport-data: A9a23:4KsX4K/QIDBmQq13Q6v6DrUDJHmTJUtcMsCJ2f8bNWPcYEJGY0x3m 2EeCziEbP2LZmfzLd9+Pt60pEMDv5HWzt43TgFlqys8E34SpcT7XtnIdU2Y0wF+jyHgoOCLy +1EN7Es+ehtFie0Si9AttENlFEkvU2ybuOU5NXsZ2YhFWeIdA970Ug5w7dg3tYy6TSEK1jlV e3a8pW31GCNg1aYAkpMg05UgEoy1BhakGpwUm0WPZinjneH/5UmJMt3yZWKB2n5WuFp8tuSH I4v+l0bElTxpH/BAvv9+lryn9ZjrrT6ZWBigVIOM0Sub4QrSoXfHc/XOdJFAXq7hQllkPh2+ ohXrqHzUzsQAY/Tqt8ETiN0NA5xaPguFL/veRBTsOSWxkzCNXDt3+9vHAc9OohwFuRfWD8Us 6ZCcXZUM07F17neLLGTE4GAguw5K8bmJsUHs2xIxjDFF/c2B5vERs0m4PcGhWth35kVRJ4yY eIlMWVwaBf4YyZKK3BMUJVhnbmBmmfGJmgwRFW9+vNsvjm7IBZK+KfpGMrYfJqNX8o9tkSFo CTA9mfwABAfPfSezyaI9jSngeqntSD2RoMUUqG5//hCgVuPy2hVAxoTPXOypPLo1GalQdlRb UoZ5kIGvaU0sUCmUNT5dxm5u2Kf+A4RXcJKFO834x3LzbDbiy67LGUZSj9KaPQ9qdQ7Azct0 ze0c8jBXGI19ufPEDTEq+nS/Wja1TUpwXEqYh1ZVxA4oMPfmb4RkSD0V8tZPImagYigcd3v+ AyioC87jrQVqMcE0aSn4FzK6w6RSoj1oh0dvVuOAD/8hu9tTMv8PtHztwCHhRpVBNvBFjG8U G44d99yBQzkJbWEj2SzTeoEB9lFDN7VYWSH0TaD83TMnglBGkJPn6gMsVmSx28za67onAMFh meJ6Wu9A7cJbROXgVdfOd7ZNijT5fGI+S7Zfv7VdMFSRZN6aRWK+ipjDWbJgTy3yBl9zPllY 83BGSpJMZr8If43pNZRb71MuYLHOwhknT+DLXwF50nPPUWiiI69Fu5ebQrmghER56KYugTFm +uzxOPRoyizpNbWO3GNmaZKdAhiBSFiWfje9pILHsbefFsOMDxxWpf5nOJ6E6Q4zvs9qws91 iznMqOu4ACh1SSvxMTjQi0LVY4Dqr4k/C1rZnJ9bA/4s5XhCK72hJoim1IMVeBP3MRozOJuT ulDfMOFA/9VTS/A9ShbZp74xLGOvjzy7e5XFyb6MjU5YbB6QAnFpo3tcgf1rXFcBSurr8ou5 ban01qDE5YEQg1jCufQae6ukAzt7SRMxroqUhuaOMRXdWXt7JNud377gMgoLpxeMh7E3Dabi VqbWE9KuenXroYp29DVnqTY/ZyxGu5zExMCTWnW5Lq7LwfA+W+nzdMSWeqEZ2mFBmj15L+jd aNeyPSlaK8Lm1NDsoxdFbd3zP1hu4uz9uEClgk9RSfFdVWmDL9kM0Kq58gXu/0f3KJdtCu3R lmLpotQN4KWNZ63C1UWPgckMLiOjKlGhjnI4P0pC0zm/ysrrqGfWEBfMhTQ2ixQKLx5bNEsz es74ZNE7gW+jlwhM8qcjzAS/GOJdyRSX6Iiv5AcIYnqlgt0lQ0SPc2CUnf7sMOVdtFBEkg2O TvF1qPNioNVylfGb3duR2PG2vBQhMhWtR1HpLPYy49lRjYRaicL4SBs
  • Ironport-hdrordr: A9a23:fLHEyKw4/5eWmubFWWg2KrPxs+skLtp133Aq2lEZdPULSKOlfp GV8MjziyWYtN9wYhAdcdDpAtjmfZr5z+8O3WB3B8beYOCGghrSEGgG1+XfKlLbak/DH4JmpM Jdmu1FeaHN5DtB/LfHCWuDYq8dKbC8mcjC74eurEuFDzsaE52Ihz0JdDpzeXcGIjWua6BJcK Z1saF81kWdkDksH46G7j5vZZm3m/T70LbdJTIWDR8u7weDyRuu9b7BChCdmjMTSSlGz7sO+X XM11WR3NTuj9iLjjvnk0PD5ZVfn9XsjvNFGcy3k8AQbhHhkByhaohNU6CL+Bo1vOaswlA3l8 SkmWZsA+1Dr1fqOk2lqxrk3AftlB4o9n/Z0FedxUDupMToLQhKQvZptMZ8SF/0+kAgtNZz3O ZgxGSCradaChvGgWDU+8XIfwsCrDv0nVMS1cooy1BPW4oXb7Fc6aYF+llOLZsGFCXmrKg6De hVCt3G7vo+SyLVU5nghBgt/DWQZAVwIv/fKXJy//B9kgIm00yR9nFohPD2xRw7hdYAo5ot3Z WzDk0nrsAIciYsV9MPOA42e7rBNoX8e2O9DIusGyWUKEgmAQOEl3el2sR/2AmVEKZ4uKfa3q 6xFm9liQ==
  • Ironport-sdr: OyBNRnItrgu8TrjFlKAfuFC4fP+gLHRobV7Ge1j8XnnUWKhyM22dh59wXkRjGjhMr9f82NIbhx KufQsu+14IfTGni7tknVATsNMkv4kUFY3XyWTYwRsDzSzZEHN+x73ajfrd7SY/NKEg9I8GM1c/ Tlmyctc5Nv+C1N8EMXY2VKqQWAx/2id1yoaranUBJRDTdnJZqYQSOJ7pdKvMb92dzmfa1q2E69 fjOAjws3SP39TJHS6xb6HU6dqEwe1iZL9Eand06wVVz9SUPEMzBfzDBeG6tsdkXZUNAiJymG8p 150LGmHkWgCxWEgk5KxihMNs
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.