On Tue, 2021-05-04 at 10:17 -0700, Sean Christopherson wrote:
> Disable preemption when probing a user return MSR via RDSMR/WRMSR. If
> the MSR holds a different value per logical CPU, the WRMSR could corrupt
> the host's value if KVM is preempted between the RDMSR and WRMSR, and
> then rescheduled on a different CPU.
>
> Opportunistically land the helper in common x86, SVM will use the helper
> in a future commit.
>
> Fixes: 4be534102624 ("KVM: VMX: Initialize vmx->guest_msrs[] right after allocation")
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: Xiaoyao Li <xiaoyao.li@xxxxxxxxx>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
> arch/x86/include/asm/kvm_host.h | 1 +
> arch/x86/kvm/vmx/vmx.c | 5 +----
> arch/x86/kvm/x86.c | 16 ++++++++++++++++
> 3 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 3e5fc80a35c8..a02c9bf3f7f1 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1778,6 +1778,7 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> unsigned long icr, int op_64_bit);
>
> void kvm_define_user_return_msr(unsigned index, u32 msr);
> +int kvm_probe_user_return_msr(u32 msr);
> int kvm_set_user_return_msr(unsigned index, u64 val, u64 mask);
>
> u64 kvm_scale_tsc(struct kvm_vcpu *vcpu, u64 tsc);
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 99591e523b47..990ee339a05f 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6914,12 +6914,9 @@ static int vmx_create_vcpu(struct kvm_vcpu *vcpu)
>
> for (i = 0; i < ARRAY_SIZE(vmx_uret_msrs_list); ++i) {
> u32 index = vmx_uret_msrs_list[i];
> - u32 data_low, data_high;
> int j = vmx->nr_uret_msrs;
>
> - if (rdmsr_safe(index, &data_low, &data_high) < 0)
> - continue;
> - if (wrmsr_safe(index, data_low, data_high) < 0)
> + if (kvm_probe_user_return_msr(index))
> continue;
>
> vmx->guest_uret_msrs[j].slot = i;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 3bf52ba5f2bb..e304447be42d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -339,6 +339,22 @@ static void kvm_on_user_return(struct user_return_notifier *urn)
> }
> }
>
> +int kvm_probe_user_return_msr(u32 msr)
> +{
> + u64 val;
> + int ret;
> +
> + preempt_disable();
> + ret = rdmsrl_safe(msr, &val);
> + if (ret)
> + goto out;
> + ret = wrmsrl_safe(msr, val);
> +out:
> + preempt_enable();
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(kvm_probe_user_return_msr);
> +
> void kvm_define_user_return_msr(unsigned slot, u32 msr)
> {
> BUG_ON(slot >= KVM_MAX_NR_USER_RETURN_MSRS);
Reviewed-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
One note though: every time we probe for a MSR existance via
checking for a #GP on write, we risk getting nonsense results
if the L1 has ignore_msrs=1.
Thus if possible to use the CPUID instead,
that would be preferred.
Best regards,
Maxim Levitsky
