linux-kernel - Re: [PATCH v2 28/49] KVM: x86: Clear PV_UNHALT for !HLT-exiting only when userspace sets CPUID

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: <8dfa84f39a718f82c23c11fbe02e4710351cbf73.camel@redhat.com>
Date: Thu, 04 Jul 2024 21:32:45 -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 28/49] KVM: x86: Clear PV_UNHALT for !HLT-exiting
 only when userspace sets CPUID

On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote:
> Now that KVM disallows disabling HLT-exiting after vCPUs have been created,
> i.e. now that it's impossible for kvm_hlt_in_guest() to change while vCPUs
> are running, apply KVM's PV_UNHALT quirk only when userspace is setting
> guest CPUID.
> 
> Opportunistically rename the helper to make it clear that KVM's behavior
> is a quirk that should never have been added.  KVM's documentation
> explicitly states that userspace should not advertise PV_UNHALT if
> HLT-exiting is disabled, but for unknown reasons, commit caa057a2cad6
> ("KVM: X86: Provide a capability to disable HLT intercepts") didn't stop
> at documenting the requirement and also massaged the incoming guest CPUID.
> 
> Unfortunately, it's quite likely that userspace has come to rely on KVM's
> behavior, i.e. the code can't simply be deleted.  The only reason KVM
> doesn't have an "official" quirk is that there is no known use case where
> disabling the quirk would make sense, i.e. letting userspace disable the
> quirk would further increase KVM's burden without any benefit.

Makes sense overall.


> 
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/kvm/cpuid.c | 26 +++++++++-----------------
>  1 file changed, 9 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 4ad01867cb8d..93a7399dc0db 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -287,18 +287,17 @@ static struct kvm_cpuid_entry2 *kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcp
>  					     vcpu->arch.cpuid_nent, base);
>  }
>  
> -static void kvm_update_pv_runtime(struct kvm_vcpu *vcpu)
> +static u32 kvm_apply_cpuid_pv_features_quirk(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_cpuid_entry2 *best = kvm_find_kvm_cpuid_features(vcpu);
>  
> -	vcpu->arch.pv_cpuid.features = 0;
> +	if (!best)
> +		return 0;
>  
> -	/*
> -	 * save the feature bitmap to avoid cpuid lookup for every PV
> -	 * operation
> -	 */
> -	if (best)
> -		vcpu->arch.pv_cpuid.features = best->eax;
> +	if (kvm_hlt_in_guest(vcpu->kvm))
> +		best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT);
> +
> +	return best->eax;
>  }
>  
>  /*
> @@ -320,7 +319,6 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e
>  				       int nent)
>  {
>  	struct kvm_cpuid_entry2 *best;
> -	struct kvm_hypervisor_cpuid kvm_cpuid;
>  
>  	best = cpuid_entry2_find(entries, nent, 1, KVM_CPUID_INDEX_NOT_SIGNIFICANT);
>  	if (best) {
> @@ -347,13 +345,6 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e
>  		     cpuid_entry_has(best, X86_FEATURE_XSAVEC)))
>  		best->ebx = xstate_required_size(vcpu->arch.xcr0, true);
>  
> -	kvm_cpuid = __kvm_get_hypervisor_cpuid(entries, nent, KVM_SIGNATURE);
> -	if (kvm_cpuid.base) {
> -		best = __kvm_find_kvm_cpuid_features(entries, nent, kvm_cpuid.base);
> -		if (kvm_hlt_in_guest(vcpu->kvm) && best)
> -			best->eax &= ~(1 << KVM_FEATURE_PV_UNHALT);
> -	}
> -
>  	if (!kvm_check_has_quirk(vcpu->kvm, KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT)) {
>  		best = cpuid_entry2_find(entries, nent, 0x1, KVM_CPUID_INDEX_NOT_SIGNIFICANT);
>  		if (best)
> @@ -425,7 +416,7 @@ void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  	vcpu->arch.guest_supported_xcr0 =
>  		cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent);
>  
> -	kvm_update_pv_runtime(vcpu);
> +	vcpu->arch.pv_cpuid.features = kvm_apply_cpuid_pv_features_quirk(vcpu);
>  
>  	vcpu->arch.is_amd_compatible = guest_cpuid_is_amd_or_hygon(vcpu);
>  	vcpu->arch.maxphyaddr = cpuid_query_maxphyaddr(vcpu);
> @@ -508,6 +499,7 @@ static int kvm_set_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *e2,
>  		 * stale data is ok because KVM will reject the change.
>  		 */
>  		kvm_update_cpuid_runtime(vcpu);
> +		kvm_apply_cpuid_pv_features_quirk(vcpu);
>  
>  		r = kvm_cpuid_check_equal(vcpu, e2, nent);
>  		if (r)

Reviewed-by: Maxim Levitsky <mlevitsk@...hat.com>

Best regards,
	Maxim Levitsky