linux-kernel - Re: [PATCH 1/6] KVM: x86: Fix tracing of CPUID.function when function is out-of-range

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]

Date:   Tue, 3 Mar 2020 10:50:03 +0800
From:   Xiaoyao Li <xiaoyao.li@...el.com>
To:     Sean Christopherson <sean.j.christopherson@...el.com>,
        Paolo Bonzini <pbonzini@...hat.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Jan Kiszka <jan.kiszka@...mens.com>
Subject: Re: [PATCH 1/6] KVM: x86: Fix tracing of CPUID.function when function
 is out-of-range

On 3/3/2020 3:57 AM, Sean Christopherson wrote:
> Rework kvm_cpuid() to query entry->function when adjusting the output
> values so that the original function (in the aptly named "function") is
> preserved for tracing.  This fixes a bug where trace_kvm_cpuid() will
> trace the max function for a range instead of the requested function if
> the requested function is out-of-range and an entry for the max function
> exists.
> 
> Fixes: 43561123ab37 ("kvm: x86: Improve emulation of CPUID leaves 0BH and 1FH")
> Reported-by: Jan Kiszka <jan.kiszka@...mens.com>
> Cc: Jim Mattson <jmattson@...gle.com>
> Cc: Xiaoyao Li <xiaoyao.li@...el.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@...el.com>
> ---
>   arch/x86/kvm/cpuid.c | 15 +++++++--------
>   1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index b1c469446b07..6be012937eba 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -997,12 +997,12 @@ static bool cpuid_function_in_range(struct kvm_vcpu *vcpu, u32 function)
>   	return max && function <= max->eax;
>   }
>   
> +/* Returns true if the requested leaf/function exists in guest CPUID. */
>   bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
>   	       u32 *ecx, u32 *edx, bool check_limit)
>   {
> -	u32 function = *eax, index = *ecx;
> +	const u32 function = *eax, index = *ecx;
>   	struct kvm_cpuid_entry2 *entry;
> -	struct kvm_cpuid_entry2 *max;
>   	bool found;
>   
>   	entry = kvm_find_cpuid_entry(vcpu, function, index);
> @@ -1015,18 +1015,17 @@ bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
>   	 */
>   	if (!entry && check_limit && !guest_cpuid_is_amd(vcpu) &&
>   	    !cpuid_function_in_range(vcpu, function)) {
> -		max = kvm_find_cpuid_entry(vcpu, 0, 0);
> -		if (max) {
> -			function = max->eax;
> -			entry = kvm_find_cpuid_entry(vcpu, function, index);
> -		}
> +		entry = kvm_find_cpuid_entry(vcpu, 0, 0);
> +		if (entry)
> +			entry = kvm_find_cpuid_entry(vcpu, entry->eax, index);

There is a problem.

when queried leaf is out of range on Intel CPU, it returns the maximum 
basic leaf, and any dependence on input ECX (i.e., subleaf) value in the 
basic leaf is honored. As disclaimed in SDM of CPUID instruction.

The ECX should be honored if and only the leaf has a significant index.
If the leaf doesn't has a significant index, it just ignores the EDX 
input in bare metal.

So it should be something like:

if (!entry && check_limit && !guest_cpuid_is_amd(vcpu) &&
	!cpuid_function_in_range(vcpu, function)) {
	entry = kvm_find_cpuid_entry(vcpu, 0, 0);
	if (entry) {
		entry = kvm_find_cpuid_entry(vcpu, entry->eax, 0);
		if (entry &&
		    entry->flags & KVM_CPUID_FLAG_SIGNIFCANT_INDEX ) {
			entry = kvm_find_cpuid_entry(vcpu, entry->eax,
						     index);
		}
	}
}

>   	}
>   	if (entry) {
>   		*eax = entry->eax;
>   		*ebx = entry->ebx;
>   		*ecx = entry->ecx;
>   		*edx = entry->edx;
> -		if (function == 7 && index == 0) {
> +
> +		if (entry->function == 7 && index == 0) {
>   			u64 data;
>   		        if (!__kvm_get_msr(vcpu, MSR_IA32_TSX_CTRL, &data, true) &&
>   			    (data & TSX_CTRL_CPUID_CLEAR))
>