linux-kernel - Re: [PATCH] KVM: selftests: Avoid KVM_SET_CPUID2 after KVM_RUN in vmx_pmu_msrs

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: <YbotG5neKyzhv22Z@google.com>
Date:   Wed, 15 Dec 2021 17:59:55 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>
Cc:     kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>, oliver.sang@...el.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: selftests: Avoid KVM_SET_CPUID2 after KVM_RUN in
 vmx_pmu_msrs_test

On Wed, Dec 15, 2021, Vitaly Kuznetsov wrote:
> Commit feb627e8d6f6 ("KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN")
> forbade chaning vCPU's CPUID data after the first KVM_RUN but
> vmx_pmu_msrs_test does exactly that. Test VM needs to be re-created after
> vcpu_run().
> 
> Reported-by: kernel test robot <oliver.sang@...el.com>
> Fixes: feb627e8d6f6 ("KVM: x86: Forbid KVM_SET_CPUID{,2} after KVM_RUN")
> Signed-off-by: Vitaly Kuznetsov <vkuznets@...hat.com>
> ---
>  tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c b/tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c
> index 23051d84b907..17882f79deed 100644
> --- a/tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/vmx_pmu_msrs_test.c
> @@ -99,6 +99,11 @@ int main(int argc, char *argv[])
>  	vcpu_run(vm, VCPU_ID);
>  	ASSERT_EQ(vcpu_get_msr(vm, VCPU_ID, MSR_IA32_PERF_CAPABILITIES), PMU_CAP_FW_WRITES);
>  
> +	/* Re-create guest VM after KVM_RUN so CPUID can be changed */
> +	kvm_vm_free(vm);
> +	vm = vm_create_default(VCPU_ID, 0, guest_code);
> +	vcpu_set_cpuid(vm, VCPU_ID, cpuid);

Why is this test even setting CPUID for the below cases?  Guest CPUID shouldn't
affect host_initiated writes.  This part in particular looks wrong:

	entry_1_0->ecx |= X86_FEATURE_PDCM;
	eax.split.version_id = 0;
	entry_1_0->ecx = eax.full;
	vcpu_set_cpuid(vm, VCPU_ID, cpuid);
	ret = _vcpu_set_msr(vm, 0, MSR_IA32_PERF_CAPABILITIES, PMU_CAP_FW_WRITES);
	TEST_ASSERT(ret == 0, "Bad PERF_CAPABILITIES didn't fail.");

As does the KVM code.  The WRMSR path for MSR_IA32_PERF_CAPABILITIES looks especially
wrong, as rejects a bad write iff userspace set PDCM in guest CPUID.

		struct kvm_msr_entry msr_ent = {.index = msr, .data = 0};

		if (!msr_info->host_initiated)
			return 1;
		if (guest_cpuid_has(vcpu, X86_FEATURE_PDCM) && kvm_get_msr_feature(&msr_ent))  <===== Huh?
			return 1;
		if (data & ~msr_ent.data)
			return 1;

		vcpu->arch.perf_capabilities = data;

		return 0;
		}

So I think we should fix KVM and then clean up the test accordingly.

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 85127b3e3690..65e297875405 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3424,7 +3424,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)

                if (!msr_info->host_initiated)
                        return 1;
-               if (guest_cpuid_has(vcpu, X86_FEATURE_PDCM) && kvm_get_msr_feature(&msr_ent))
+               if (kvm_get_msr_feature(&msr_ent))
                        return 1;
                if (data & ~msr_ent.data)
                        return 1;
@@ -3779,14 +3779,12 @@ int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
                msr_info->data = vcpu->arch.microcode_version;
                break;
        case MSR_IA32_ARCH_CAPABILITIES:
-               if (!msr_info->host_initiated &&
-                   !guest_cpuid_has(vcpu, X86_FEATURE_ARCH_CAPABILITIES))
+               if (!msr_info->host_initiated)
                        return 1;
                msr_info->data = vcpu->arch.arch_capabilities;
                break;
        case MSR_IA32_PERF_CAPABILITIES:
-               if (!msr_info->host_initiated &&
-                   !guest_cpuid_has(vcpu, X86_FEATURE_PDCM))
+               if (!msr_info->host_initiated)
                        return 1;
                msr_info->data = vcpu->arch.perf_capabilities;
                break;

> +
>  	/* testcase 2, check valid LBR formats are accepted */
>  	vcpu_set_msr(vm, 0, MSR_IA32_PERF_CAPABILITIES, 0);
>  	ASSERT_EQ(vcpu_get_msr(vm, VCPU_ID, MSR_IA32_PERF_CAPABILITIES), 0);
> -- 
> 2.33.1
>