linux-kernel - Re: [PATCH v2 16/49] KVM: x86: Don't update PV features caches when enabling enforcement capability

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: <2c0cb572deeec525da8e265d64ba20c7a6367fb4.camel@redhat.com>
Date: Thu, 04 Jul 2024 21:17:44 -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 16/49] KVM: x86: Don't update PV features caches when
 enabling enforcement capability

On Fri, 2024-05-17 at 10:38 -0700, Sean Christopherson wrote:
> Revert the chunk of commit 01b4f510b9f4 ("kvm: x86: ensure pv_cpuid.features
> is initialized when enabling cap") that forced a PV features cache refresh
> during KVM_CAP_ENFORCE_PV_FEATURE_CPUID, as whatever ioctl() ordering
> issue it alleged to have fixed never existed upstream, and likely never
> existed in any kernel.
> 
> At the time of the commit, there was a tangentially related ioctl()
> ordering issue, as toggling KVM_X86_DISABLE_EXITS_HLT after KVM_SET_CPUID2
> would have resulted in KVM potentially leaving KVM_FEATURE_PV_UNHALT set.
> But (a) that bug affected the entire guest CPUID, not just the cache, (b)
> commit 01b4f510b9f4 didn't address that bug, it only refreshed the cache
> (with the bad CPUID), and (c) setting KVM_X86_DISABLE_EXITS_HLT after vCPU
> creation is completely broken as KVM configures HLT-exiting only during
> vCPU creation, which is why KVM_CAP_X86_DISABLE_EXITS is now disallowed if
> vCPUs have been created.
> 
> Another tangentially related bug was KVM's failure to clear the cache when
> handling KVM_SET_CPUID2, but again commit 01b4f510b9f4 did nothing to fix
> that bug.
> 
> The most plausible explanation for the what commit 01b4f510b9f4 was trying
> to fix is a bug that existed in Google's internal kernel that was the
> source of commit 01b4f510b9f4.  At the time, Google's internal kernel had
> not yet picked up commit 0d3b2ba16ba68 ("KVM: X86: Go on updating other
> CPUID leaves when leaf 1 is absent"), i.e. KVM would not initialize the
> PV features cache if KVM_SET_CPUID2 was called without a CPUID.0x1 entry.
> 
> Of course, no sane real world VMM would omit CPUID.0x1, including the KVM
> selftest added by commit ac4a4d6de22e ("selftests: kvm: test enforcement
> of paravirtual cpuid features").  And the test didn't actually try to
> verify multiple orderings, nor did the selftest enter the guest without
> doing KVM_SET_CPUID2, so who knows what motivated the change.
> 
> Regardless of why commit 01b4f510b9f4 ("kvm: x86: ensure pv_cpuid.features
> is initialized when enabling cap") was added, refreshing the cache during
> KVM_CAP_ENFORCE_PV_FEATURE_CPUID isn't necessary.
> 
> Cc: Oliver Upton <oliver.upton@...ux.dev>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> ---
>  arch/x86/kvm/cpuid.c | 2 +-
>  arch/x86/kvm/cpuid.h | 1 -
>  arch/x86/kvm/x86.c   | 3 ---
>  3 files changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index be1c8f43e090..a51e48663f53 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -242,7 +242,7 @@ static struct kvm_cpuid_entry2 *kvm_find_kvm_cpuid_features(struct kvm_vcpu *vcp
>  					     vcpu->arch.cpuid_nent, base);
>  }
>  
> -void kvm_update_pv_runtime(struct kvm_vcpu *vcpu)
> +static void kvm_update_pv_runtime(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_cpuid_entry2 *best = kvm_find_kvm_cpuid_features(vcpu);
>  
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index 0a8b561b5434..7eb3d7318fc4 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -13,7 +13,6 @@ void kvm_set_cpu_caps(void);
>  
>  void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu);
>  void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu);
> -void kvm_update_pv_runtime(struct kvm_vcpu *vcpu);
>  struct kvm_cpuid_entry2 *kvm_find_cpuid_entry_index(struct kvm_vcpu *vcpu,
>  						    u32 function, u32 index);
>  struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c729227c6501..7160c5ab8e3e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5849,9 +5849,6 @@ static int kvm_vcpu_ioctl_enable_cap(struct kvm_vcpu *vcpu,
>  
>  	case KVM_CAP_ENFORCE_PV_FEATURE_CPUID:
>  		vcpu->arch.pv_cpuid.enforce = cap->args[0];
> -		if (vcpu->arch.pv_cpuid.enforce)
> -			kvm_update_pv_runtime(vcpu);
> -
>  		return 0;
>  	default:
>  		return -EINVAL;

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

Best regards,
	Maxim Levitsky