linux-kernel - Re: [PATCH v2 44/49] KVM: x86: Update guest cpu_caps at runtime for dynamic CPUID-based features

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <2d554577722d30605ecd0f920f4777129fff3951.camel@redhat.com>
Date: Thu, 04 Jul 2024 22:26:03 -0400
From: Maxim Levitsky <mlevitsk@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>, Paolo Bonzini
 <pbonzini@...hat.com>,  Vitaly Kuznetsov <vkuznets@...hat.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org, Hou Wenlong
 <houwenlong.hwl@...group.com>, Kechen Lu <kechenl@...dia.com>, Oliver Upton
 <oliver.upton@...ux.dev>, Binbin Wu <binbin.wu@...ux.intel.com>, Yang
 Weijiang <weijiang.yang@...el.com>, Robert Hoo <robert.hoo.linux@...il.com>
Subject: Re: [PATCH v2 44/49] KVM: x86: Update guest cpu_caps at runtime for
 dynamic CPUID-based features

On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote:
> When updating guest CPUID entries to emulate runtime behavior, e.g. when
> the guest enables a CR4-based feature that is tied to a CPUID flag, also
> update the vCPU's cpu_caps accordingly.  This will allow replacing all
> usage of guest_cpuid_has() with guest_cpu_cap_has().
> 
> Note, this relies on kvm_set_cpuid() taking a snapshot of cpu_caps before
> invoking kvm_update_cpuid_runtime(), i.e. when KVM is updating CPUID
> entries that *may* become the vCPU's CPUID, so that unwinding to the old
> cpu_caps is possible if userspace tries to set bogus CPUID information.
> 
> Note #2, none of the features in question use guest_cpu_cap_has() at this
> time, i.e. aside from settings bits in cpu_caps, this is a glorified nop.
> 
> Cc: Yang Weijiang <weijiang.yang@...el.com>
> Cc: Robert Hoo <robert.hoo.linux@...il.com>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/kvm/cpuid.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 552e65ba5efa..1424a9d4eb17 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -330,28 +330,38 @@ static u64 cpuid_get_supported_xcr0(struct kvm_vcpu *vcpu)
>  	return (best->eax | ((u64)best->edx << 32)) & kvm_caps.supported_xcr0;
>  }
>  
> +static __always_inline void kvm_update_feature_runtime(struct kvm_vcpu *vcpu,
> +						       struct kvm_cpuid_entry2 *entry,
> +						       unsigned int x86_feature,
> +						       bool has_feature)
> +{
> +	cpuid_entry_change(entry, x86_feature, has_feature);
> +	guest_cpu_cap_change(vcpu, x86_feature, has_feature);
> +}
> +
>  void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_cpuid_entry2 *best;
>  
>  	best = kvm_find_cpuid_entry(vcpu, 1);
>  	if (best) {
> -		cpuid_entry_change(best, X86_FEATURE_OSXSAVE,
> -				   kvm_is_cr4_bit_set(vcpu, X86_CR4_OSXSAVE));
> +		kvm_update_feature_runtime(vcpu, best, X86_FEATURE_OSXSAVE,
> +					   kvm_is_cr4_bit_set(vcpu, X86_CR4_OSXSAVE));
>  
> -		cpuid_entry_change(best, X86_FEATURE_APIC,
> -			   vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
> +		kvm_update_feature_runtime(vcpu, best, X86_FEATURE_APIC,
> +					   vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE);
>  
>  		if (!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT))
> -			cpuid_entry_change(best, X86_FEATURE_MWAIT,
> -					   vcpu->arch.ia32_misc_enable_msr &
> -					   MSR_IA32_MISC_ENABLE_MWAIT);
> +			kvm_update_feature_runtime(vcpu, best, X86_FEATURE_MWAIT,
> +						   vcpu->arch.ia32_misc_enable_msr &
> +						   MSR_IA32_MISC_ENABLE_MWAIT);
>  	}
>  
>  	best = kvm_find_cpuid_entry_index(vcpu, 7, 0);
>  	if (best)
> -		cpuid_entry_change(best, X86_FEATURE_OSPKE,
> -				   kvm_is_cr4_bit_set(vcpu, X86_CR4_PKE));
> +		kvm_update_feature_runtime(vcpu, best, X86_FEATURE_OSPKE,
> +					   kvm_is_cr4_bit_set(vcpu, X86_CR4_PKE));
> +
>  
>  	best = kvm_find_cpuid_entry_index(vcpu, 0xD, 0);
>  	if (best)


I am not 100% sure that we need to do this.

Runtime cpuid changes are a hack that Intel did back then,
due to various reasons, These changes don't really change the feature set
that CPU supports, but merly as you like to say 'massage' the output of
the CPUID instruction to make the unmodified OS happy usually.

Thus it feels to me that CPU caps should not include the dynamic features, and neither
KVM should use the value of these as a source for truth, but rather the underlying
source of the truth (e.g CR4).

But if you insist, I don't really have a very strong reason to object this.

Best regards,
	Maxim Levitsky