linux-kernel - Re: [PATCH] KVM: x86: Update Xen-specific CPUID leaves during mangling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87tt9q7orq.fsf@redhat.com>
Date: Wed, 22 Jan 2025 18:16:09 +0100
From: Vitaly Kuznetsov <vkuznets@...hat.com>
To: Fred Griffoul <fgriffo@...zon.co.uk>, kvm@...r.kernel.org
Cc: Fred Griffoul <fgriffo@...zon.co.uk>, Sean Christopherson
 <seanjc@...gle.com>, Paolo Bonzini <pbonzini@...hat.com>, Thomas Gleixner
 <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov
 <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
 "H. Peter Anvin" <hpa@...or.com>, David Woodhouse <dwmw2@...radead.org>,
 Paul Durrant <paul@....org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: x86: Update Xen-specific CPUID leaves during mangling

Fred Griffoul <fgriffo@...zon.co.uk> writes:

> Previous commit ee3a5f9e3d9b ("KVM: x86: Do runtime CPUID update before
> updating vcpu->arch.cpuid_entries") implemented CPUID data mangling in
> KVM_SET_CPUID2 support before verifying that no changes occur on running
> vCPUs. However, it overlooked the CPUID leaves that are modified by
> KVM's Xen emulation.
>
> Fix this by calling a Xen update function when mangling CPUID data.
>
> Fixes: ee3a5f9e3d9b ("KVM: x86: Do runtime CPUID update before
> updating vcpu->arch.cpuid_entries")

Well, kvm_xen_update_tsc_info() was added with

commit f422f853af0369be27d2a9f1b20079f2bc3d1ca2
Author: Paul Durrant <pdurrant@...zon.com>
Date:   Fri Jan 6 10:36:00 2023 +0000

    KVM: x86/xen: update Xen CPUID Leaf 4 (tsc info) sub-leaves, if present

and the commit you mention in 'Fixes' is older:

commit ee3a5f9e3d9bf94159f3cc80da542fbe83502dd8
Author: Vitaly Kuznetsov <vkuznets@...hat.com>
Date:   Mon Jan 17 16:05:39 2022 +0100

    KVM: x86: Do runtime CPUID update before updating vcpu->arch.cpuid_entries

so I guess we should be 'Fixing' f422f853af03 instead :-)

> Signed-off-by: Fred Griffoul <fgriffo@...zon.co.uk>
> ---
>  arch/x86/kvm/cpuid.c | 1 +
>  arch/x86/kvm/xen.c   | 5 +++++
>  arch/x86/kvm/xen.h   | 5 +++++
>  3 files changed, 11 insertions(+)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index edef30359c19..432d8e9e1bab 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -212,6 +212,7 @@ static int kvm_cpuid_check_equal(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2
>  	 */
>  	kvm_update_cpuid_runtime(vcpu);
>  	kvm_apply_cpuid_pv_features_quirk(vcpu);
> +	kvm_xen_update_cpuid_runtime(vcpu);

This one is weird as we update it in runtime (kvm_guest_time_update())
and values may change when we e.g. migrate the guest. First, I do not
understand how the guest is supposed to notice the change as CPUID data
is normally considered static. Second, I do not see how the VMM is
supposed to track it as if it tries to supply some different data for
these Xen leaves, kvm_cpuid_check_equal() will still fail.

Would it make more sense to just ignore these Xen CPUID leaves with TSC
information when we do the comparison?

>  
>  	if (nent != vcpu->arch.cpuid_nent)
>  		return -EINVAL;
> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index a909b817b9c0..219f9a9a92be 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -2270,6 +2270,11 @@ void kvm_xen_update_tsc_info(struct kvm_vcpu *vcpu)
>  		entry->eax = vcpu->arch.hw_tsc_khz;
>  }
>  
> +void kvm_xen_update_cpuid_runtime(struct kvm_vcpu *vcpu)
> +{
> +	kvm_xen_update_tsc_info(vcpu);
> +}
> +
>  void kvm_xen_init_vm(struct kvm *kvm)
>  {
>  	mutex_init(&kvm->arch.xen.xen_lock);
> diff --git a/arch/x86/kvm/xen.h b/arch/x86/kvm/xen.h
> index f5841d9000ae..d3182b0ab7e3 100644
> --- a/arch/x86/kvm/xen.h
> +++ b/arch/x86/kvm/xen.h
> @@ -36,6 +36,7 @@ int kvm_xen_setup_evtchn(struct kvm *kvm,
>  			 struct kvm_kernel_irq_routing_entry *e,
>  			 const struct kvm_irq_routing_entry *ue);
>  void kvm_xen_update_tsc_info(struct kvm_vcpu *vcpu);
> +void kvm_xen_update_cpuid_runtime(struct kvm_vcpu *vcpu);
>  
>  static inline void kvm_xen_sw_enable_lapic(struct kvm_vcpu *vcpu)
>  {
> @@ -160,6 +161,10 @@ static inline bool kvm_xen_timer_enabled(struct kvm_vcpu *vcpu)
>  static inline void kvm_xen_update_tsc_info(struct kvm_vcpu *vcpu)
>  {
>  }
> +
> +static inline void kvm_xen_update_cpuid_runtime(struct kvm_vcpu *vcpu)
> +{
> +}
>  #endif
>  
>  int kvm_xen_hypercall(struct kvm_vcpu *vcpu);

-- 
Vitaly