Re: [PATCH v2] KVM: nVMX: Don't expose eVMCS unsupported fields to L1

[Vitaly Kuznetsov]

Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> writes:

> Anirudh Rayabharam <anrayabh@xxxxxxxxxxxxxxxxxxx> writes:
>
>> When running cloud-hypervisor tests, VM entry into an L2 guest on KVM on
>> Hyper-V fails with this splat (stripped for brevity):
>>
>> [ 1481.600386] WARNING: CPU: 4 PID: 7641 at arch/x86/kvm/vmx/nested.c:4563 nested_vmx_vmexit+0x70d/0x790 [kvm_intel]
>> [ 1481.600427] CPU: 4 PID: 7641 Comm: vcpu2 Not tainted 5.15.0-1008-azure #9-Ubuntu
>> [ 1481.600429] Hardware name: Microsoft Corporation Virtual Machine/Virtual Machine, BIOS Hyper-V UEFI Release v4.1 07/22/2021
>> [ 1481.600430] RIP: 0010:nested_vmx_vmexit+0x70d/0x790 [kvm_intel]
>> [ 1481.600447] Call Trace:
>> [ 1481.600449]  <TASK>
>> [ 1481.600451]  nested_vmx_reflect_vmexit+0x10b/0x440 [kvm_intel]
>> [ 1481.600457]  __vmx_handle_exit+0xef/0x670 [kvm_intel]
>> [ 1481.600467]  vmx_handle_exit+0x12/0x50 [kvm_intel]
>> [ 1481.600472]  vcpu_enter_guest+0x83a/0xfd0 [kvm]
>> [ 1481.600524]  vcpu_run+0x5e/0x240 [kvm]
>> [ 1481.600560]  kvm_arch_vcpu_ioctl_run+0xd7/0x550 [kvm]
>> [ 1481.600597]  kvm_vcpu_ioctl+0x29a/0x6d0 [kvm]
>> [ 1481.600634]  __x64_sys_ioctl+0x91/0xc0
>> [ 1481.600637]  do_syscall_64+0x5c/0xc0
>> [ 1481.600667]  entry_SYSCALL_64_after_hwframe+0x44/0xae
>> [ 1481.600670] RIP: 0033:0x7f688becdaff
>> [ 1481.600686]  </TASK>
>>
>> TSC multiplier field is currently not supported in EVMCS in KVM. It was
>> previously not supported from Hyper-V but has been added since. Because
>> it is not supported in KVM the use "TSC scaling control" is filtered out
>> of vmcs_config by evmcs_sanitize_exec_ctrls().
>>
>> However, in nested_vmx_setup_ctls_msrs(), TSC scaling is exposed to L1.
>> eVMCS unsupported fields are not sanitized. When L1 tries to launch an L2
>> guest, vmcs12 has TSC scaling enabled. This propagates to vmcs02. But KVM
>> doesn't set the TSC multiplier value because kvm_has_tsc_control is false.
>> Due to this VM entry for L2 guest fails. (VM entry fails if
>> "use TSC scaling" is 1 but TSC multiplier is 0.)
>>
>> To fix, in nested_vmx_setup_ctls_msrs(), sanitize the values read from MSRs
>> by filtering out fields that are not supported by eVMCS.
>>
>> This is a stable-friendly intermediate fix. A more comprehensive fix is
>> in progress [1] but is probably too complicated to safely apply to
>> stable.
>>
>> [1]: https://lore.kernel.org/kvm/20220627160440.31857-1-vkuznets@xxxxxxxxxx/
>>
>> Fixes: d041b5ea93352 ("KVM: nVMX: Enable nested TSC scaling")
>> Signed-off-by: Anirudh Rayabharam <anrayabh@xxxxxxxxxxxxxxxxxxx>
>> ---
>>
>> Changes since v1:
>> - Sanitize all eVMCS unsupported fields instead of just TSC scaling.
>>
>> v1: https://lore.kernel.org/lkml/20220613161611.3567556-1-anrayabh@xxxxxxxxxxxxxxxxxxx/
>>
>> ---
>>  arch/x86/kvm/vmx/nested.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>> index f5cb18e00e78..f88d748c7cc6 100644
>> --- a/arch/x86/kvm/vmx/nested.c
>> +++ b/arch/x86/kvm/vmx/nested.c
>> @@ -6564,6 +6564,10 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps)
>>  		msrs->pinbased_ctls_high);
>>  	msrs->pinbased_ctls_low |=
>>  		PIN_BASED_ALWAYSON_WITHOUT_TRUE_MSR;
>> +#if IS_ENABLED(CONFIG_HYPERV)
>> +	if (static_branch_unlikely(&enable_evmcs))
>> +		msrs->pinbased_ctls_high &= ~EVMCS1_UNSUPPORTED_PINCTRL;
>> +#endif
>>  	msrs->pinbased_ctls_high &=
>>  		PIN_BASED_EXT_INTR_MASK |
>>  		PIN_BASED_NMI_EXITING |
>> @@ -6580,6 +6584,10 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps)
>>  	msrs->exit_ctls_low =
>>  		VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR;
>>  
>> +#if IS_ENABLED(CONFIG_HYPERV)
>> +	if (static_branch_unlikely(&enable_evmcs))
>> +		msrs->exit_ctls_high &= ~EVMCS1_UNSUPPORTED_VMEXIT_CTRL;
>> +#endif
>>  	msrs->exit_ctls_high &=
>>  #ifdef CONFIG_X86_64
>>  		VM_EXIT_HOST_ADDR_SPACE_SIZE |
>> @@ -6600,6 +6608,10 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps)
>>  		msrs->entry_ctls_high);
>>  	msrs->entry_ctls_low =
>>  		VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR;
>> +#if IS_ENABLED(CONFIG_HYPERV)
>> +	if (static_branch_unlikely(&enable_evmcs))
>> +		msrs->entry_ctls_high &= ~EVMCS1_UNSUPPORTED_VMENTRY_CTRL;
>> +#endif
>>  	msrs->entry_ctls_high &=
>>  #ifdef CONFIG_X86_64
>>  		VM_ENTRY_IA32E_MODE |
>> @@ -6657,6 +6669,10 @@ void nested_vmx_setup_ctls_msrs(struct nested_vmx_msrs *msrs, u32 ept_caps)
>>  		      msrs->secondary_ctls_high);
>>  
>>  	msrs->secondary_ctls_low = 0;
>> +#if IS_ENABLED(CONFIG_HYPERV)
>> +	if (static_branch_unlikely(&enable_evmcs))
>> +		msrs->secondary_ctls_high &= ~EVMCS1_UNSUPPORTED_2NDEXEC;
>> +#endif
>>  	msrs->secondary_ctls_high &=
>>  		SECONDARY_EXEC_DESC |
>>  		SECONDARY_EXEC_ENABLE_RDTSCP |
>
> (In theory, threre's also EVMCS1_UNSUPPORTED_VMFUNC filtering out
> VMX_VMFUNC_EPTP_SWITCHING (as eVMCS EPTP_LIST_ADDRESS) but it is not
> used by KVM)
>
> As I said in another thread, I think this is fine as a
> stable@/intermediate fix. Assuming the way to go for mainline is my
> "KVM: nVMX: Use vmcs_config for setting up nested VMX MSRs", this patch
> won't be needed and can be reverted.

(I've already said that in another thread, putting it here for
visibility)

I have to take this back. As-is, the patch is likely to break live
migration because of the reasons expressed by Jim:
https://lore.kernel.org/kvm/CALMp9eSBLcvuNDquvSfUnaF3S3f4ZkzqDRSsz-v93ZeX=xnssg@xxxxxxxxxxxxxx/

In case we filter out SECONDARY_EXEC_TSC_SCALING here in
nested_vmx_setup_ctls_msrs(), KVM_SET_MSRS on the destination will fail
because the data will have an unsupported bit.

It seems the easiest way to go for stable@ is to actually enable TSC
scaling, like the 4 patches I've suggested here:
https://lore.kernel.org/kvm/87bkujy4z9.fsf@xxxxxxxxxx/

The full solution including vmcs_config usage for
nested_vmx_setup_ctls_msrs() is 28 patches now, it's likely a no-go for
stable :-(

-- 
Vitaly