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 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.

~Andrew