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:   Thu, 27 Oct 2022 22:47:27 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Like Xu <like.xu.linux@...il.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Jim Mattson <jmattson@...gle.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, Sandipan Das <sandipan.das@....com>
Subject: Re: [PATCH v2 2/3] KVM: x86/svm/pmu: Add AMD PerfMonV2 support

On Mon, Sep 19, 2022, Like Xu wrote:
> @@ -162,20 +179,43 @@ static int amd_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  static void amd_pmu_refresh(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_pmu *pmu = vcpu_to_pmu(vcpu);
> +	struct kvm_cpuid_entry2 *entry;
> +	union cpuid_0x80000022_ebx ebx;
>  
> -	if (guest_cpuid_has(vcpu, X86_FEATURE_PERFCTR_CORE))
> -		pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS_CORE;
> -	else
> -		pmu->nr_arch_gp_counters = AMD64_NUM_COUNTERS;
> +	pmu->version = 1;
> +	if (kvm_pmu_cap.version > 1) {
> +		pmu->version = min_t(unsigned int, 2, kvm_pmu_cap.version);

This is wrong, it forces the guest PMU verson to match the max version supported
by KVM.  E.g. if userspace wants to expose v1 for whatever reason, pmu->version
will still end up 2+.

> +		entry = kvm_find_cpuid_entry_index(vcpu, 0x80000022, 0);
> +		if (entry) {
> +			ebx.full = entry->ebx;
> +			pmu->nr_arch_gp_counters = min3((unsigned int)ebx.split.num_core_pmc,
> +							(unsigned int)kvm_pmu_cap.num_counters_gp,
> +							(unsigned int)KVM_AMD_PMC_MAX_GENERIC);

This is technically wrong, the number of counters is supposed to be valid if and
only if v2 is supported.  On a related topic, does KVM explode if userspace
specifies a bogus PMU version on Intel?  I don't see any sanity checks there...

With a proper feature flag

	pmu->version = 1;
	if (kvm_cpu_has(X86_FEATURE_AMD_PMU_V2) &&
	    guest_cpuid_has(X86_FEATURE_AMD_PMU_V2)) {
		pmu->version = 2;

		entry = kvm_find_cpuid_entry_index(vcpu, 0x80000022, 0);
		if (entry) {
			...

Though technically the "if (entry)" check is unnecesary.

> +		}
> +	}
> +
> +	/* Commitment to minimal PMCs, regardless of CPUID.80000022 */
> +	if (guest_cpuid_has(vcpu, X86_FEATURE_PERFCTR_CORE)) {

Unnecessary braces.

> +		pmu->nr_arch_gp_counters = max_t(unsigned int,
> +						 pmu->nr_arch_gp_counters,
> +						 AMD64_NUM_COUNTERS_CORE);

What happens if userspace sets X86_FEATURE_PERFCTR_CORE when its not supported?
E.g. will KVM be coerced into taking a #GP on a non-existent counter?


> +	} else {
> +		pmu->nr_arch_gp_counters = max_t(unsigned int,
> +						 pmu->nr_arch_gp_counters,
> +						 AMD64_NUM_COUNTERS);
> +	}

Powered by blists - more mailing lists