linux-kernel - RE: [PATCH] KVM: x86/pmu: Return correct value of IA32_PERF_CAPABILITIES for userspace after vCPU has run

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: <DS0PR11MB63731F54EA26D14CF7D6A3FDDC2A2@DS0PR11MB6373.namprd11.prod.outlook.com>
Date: Wed, 13 Mar 2024 10:22:15 +0000
From: "Wang, Wei W" <wei.w.wang@...el.com>
To: Mingwei Zhang <mizhang@...gle.com>, Sean Christopherson
	<seanjc@...gle.com>, Paolo Bonzini <pbonzini@...hat.com>
CC: "H. Peter Anvin" <hpa@...or.com>, "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, Aaron Lewis <aaronlewis@...gle.com>, "Jim
 Mattson" <jmattson@...gle.com>
Subject: RE: [PATCH] KVM: x86/pmu: Return correct value of
 IA32_PERF_CAPABILITIES for userspace after vCPU has run

On Wednesday, March 13, 2024 8:38 AM, Mingwei Zhang wrote:
> Return correct value of IA32_PERF_CAPABILITIES when userspace tries to read
> it after vCPU has already run. Previously, KVM will always return the guest
> cached value on get_msr() even if guest CPUID lacks X86_FEATURE_PDCM. The
> guest cached value on default is kvm_caps.supported_perf_cap. However,
> when userspace sets the value during live migration, the call fails because of
> the check on X86_FEATURE_PDCM.

Could you point where in the set_msr path that could fail?
(I don’t find there is a check of X86_FEATURE_PDCM in vmx_set_msr and
kvm_set_msr_common)

> 
> Initially, it sounds like a pure userspace issue. It is not. After vCPU has run,
> KVM should faithfully return correct value to satisify legitimate requests from
> userspace such as VM suspend/resume and live migrartion. In this case, KVM
> should return 0 when guest cpuid lacks X86_FEATURE_PDCM. 
Some typos above (satisfy, migration, CPUID)

Seems the description here isn’t aligned to your code below?
The code below prevents userspace from reading the MSR value (not return 0 as the
read value) in that case.

>So fix the
> problem by adding an additional check in vmx_set_msr().
> 
> Note that IA32_PERF_CAPABILITIES is emulated on AMD side, which is fine
> because it set_msr() is guarded by kvm_caps.supported_perf_cap which is
> always 0.
> 
> Cc: Aaron Lewis <aaronlewis@...gle.com>
> Cc: Jim Mattson <jmattson@...gle.com>
> Signed-off-by: Mingwei Zhang <mizhang@...gle.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index
> 40e3780d73ae..6d8667b56091 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2049,6 +2049,17 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu,
> struct msr_data *msr_info)
>  		msr_info->data = to_vmx(vcpu)->msr_ia32_sgxlepubkeyhash
>  			[msr_info->index - MSR_IA32_SGXLEPUBKEYHASH0];
>  		break;
> +	case MSR_IA32_PERF_CAPABILITIES:
> +		/*
> +		 * Host VMM should not get potentially invalid MSR value if
> vCPU
> +		 * has already run but guest cpuid lacks the support for the
> +		 * MSR.
> +		 */
> +		if (msr_info->host_initiated &&
> +		    kvm_vcpu_has_run(vcpu) &&
> +		    !guest_cpuid_has(vcpu, X86_FEATURE_PDCM))
> +			return 1;
> +		break;
>  	case KVM_FIRST_EMULATED_VMX_MSR ...
> KVM_LAST_EMULATED_VMX_MSR:
>  		if (!guest_can_use(vcpu, X86_FEATURE_VMX))
>  			return 1;
> 
> base-commit: fd89499a5151d197ba30f7b801f6d8f4646cf446
> --
> 2.44.0.291.gc1ea87d7ee-goog
>