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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.01.2022 15:41, Andrew Cooper wrote:
> On 14/01/2022 14:14, Andrew Cooper wrote:
>> On 13/01/2022 16:38, 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.
>> Actually, it's just occurred to me that an ASSERT is actually quite easy
>> here.  I'm proposing this additional delta (totally untested).
>>
>> diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
>> index 297ed8685126..f569c3259b32 100644
>> --- a/xen/arch/x86/hvm/vmx/entry.S
>> +++ b/xen/arch/x86/hvm/vmx/entry.S
>> @@ -41,6 +41,13 @@ ENTRY(vmx_asm_vmexit_handler)
>>              movzbl CPUINFO_xen_spec_ctrl(%rsp), %eax
>>              xor    %edx, %edx
>>              wrmsr
>> +
>> +#ifdef CONFIG_DEBUG
>> +            testb $SCF_use_shadow, CPUINFO_spec_ctrl_flags(%rsp)
>> +            jz 1f
>> +            ASSERT_FAILED("MSR_SPEC_CTRL shadowing active")
>> +1:
>> +#endif
>>          .endm
>>          ALTERNATIVE "", restore_spec_ctrl, X86_FEATURE_SC_MSR_HVM
>>          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
> 
> Irritatingly this doesn't work, because the metadata associated with the
> ud2 instruction is tied to the compiled position in
> .altinstr_replacement, not the runtime position after alternatives have run.

Could we have the macro "return" ZF, leaving it to the invocation
site of the macro to act on it? ALTERNATIVE's first argument could
easily be "xor %reg, %reg" to set ZF without much other overhead.

Jan