linux-kernel - Re: [PATCH v4 27/38] KVM: x86/pmu: Handle PMU MSRs interception and event filtering

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: <b29fc40d-4c05-44db-975f-b40d3c188c88@linux.intel.com>
Date: Mon, 19 May 2025 12:16:17 +0800
From: "Mi, Dapeng" <dapeng1.mi@...ux.intel.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Mingwei Zhang <mizhang@...gle.com>, Peter Zijlstra
 <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
 Arnaldo Carvalho de Melo <acme@...nel.org>,
 Namhyung Kim <namhyung@...nel.org>, Paolo Bonzini <pbonzini@...hat.com>,
 Mark Rutland <mark.rutland@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
 Adrian Hunter <adrian.hunter@...el.com>, Liang@...gle.com,
 Kan <kan.liang@...ux.intel.com>, "H. Peter Anvin" <hpa@...or.com>,
 linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
 kvm@...r.kernel.org, linux-kselftest@...r.kernel.org,
 Yongwei Ma <yongwei.ma@...el.com>,
 Xiong Zhang <xiong.y.zhang@...ux.intel.com>,
 Jim Mattson <jmattson@...gle.com>, Sandipan Das <sandipan.das@....com>,
 Zide Chen <zide.chen@...el.com>, Eranian Stephane <eranian@...gle.com>,
 Shukla Manali <Manali.Shukla@....com>,
 Nikunj Dadhania <nikunj.dadhania@....com>
Subject: Re: [PATCH v4 27/38] KVM: x86/pmu: Handle PMU MSRs interception and
 event filtering

On 5/17/2025 4:54 AM, Sean Christopherson wrote:
> On Fri, May 16, 2025, Dapeng Mi wrote:
>> On 3/25/2025 1:31 AM, Mingwei Zhang wrote:
>>> +	if (kvm_mediated_pmu_enabled(pmu_to_vcpu(pmu))) {
>>> +		bool allowed = check_pmu_event_filter(pmc);
>>> +
>>> +		if (pmc_is_gp(pmc)) {
>>> +			if (allowed)
>>> +				pmc->eventsel_hw |= pmc->eventsel &
>>> +						    ARCH_PERFMON_EVENTSEL_ENABLE;
>>> +			else
>>> +				pmc->eventsel_hw &= ~ARCH_PERFMON_EVENTSEL_ENABLE;
>>> +		} else {
>>> +			int idx = pmc->idx - KVM_FIXED_PMC_BASE_IDX;
>>> +
>>> +			if (allowed)
>>> +				pmu->fixed_ctr_ctrl_hw = pmu->fixed_ctr_ctrl;
>> Sean, just found there is a potential bug here.  The
>> "pmu->fixed_ctr_ctrl_hw" should not be assigned to "pmu->fixed_ctr_ctrl"
>> here, otherwise the other filtered fixed counter (not this allowed fixed
>> counter) could be enabled accidentally.
>>
>> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
>> index ba9d336f1d1d..f32e5f66f73b 100644
>> --- a/arch/x86/kvm/pmu.c
>> +++ b/arch/x86/kvm/pmu.c
>> @@ -473,7 +473,8 @@ static int reprogram_counter(struct kvm_pmc *pmc)
>>                         int idx = pmc->idx - KVM_FIXED_PMC_BASE_IDX;
>>
>>                         if (allowed)
>> -                               pmu->fixed_ctr_ctrl_hw = pmu->fixed_ctr_ctrl;
>> +                               pmu->fixed_ctr_ctrl_hw |= pmu->fixed_ctr_ctrl &
>> +                                              
>> intel_fixed_bits_by_idx(idx, 0xf);
> Hmm, I think that's fine, since pmu->fixed_ctr_ctrl should have changed?  But I'd

Not exactly. Assume guest enables the fixed counter 0 and 1, then
pmu->fixed_ctr_ctrl would be 0xbb. At the first time, user disables the
fixed counter 0 by KVM event filter, then pmu->fixed_ctr_ctrl_hw would be
0xb0, secondly user disables the fixed counter 1 as well, so
pmu->fixed_ctr_ctrl_hw is 0x0. At the third time, user re-enables fixed
counter 1, so pmu->fixed_ctr_ctrl_hw is expected to be 0xb0, but it's not
actually. Although the 1st calling (for fixed counter 0) to
reprogram_counter() would disable it, but the 2nd calling (for fixed
counter 1) to reprogram_counter() accidentally enables it
(pmu->fixed_ctr_ctrl_hw becomes 0xbb eventually). This is incorrect.

> rather play it safe and do (completely untested):
>
> 	if (pmc_is_gp(pmc)) {
> 		pmc->eventsel_hw &= ~ARCH_PERFMON_EVENTSEL_ENABLE;
> 		if (allowed)
> 			pmc->eventsel_hw |= pmc->eventsel &
> 					    ARCH_PERFMON_EVENTSEL_ENABLE;
> 	} else {
> 		u64 mask = intel_fixed_bits_by_idx(pmc->idx - KVM_FIXED_PMC_BASE_IDX, 0xf);
>
> 		pmu->fixed_ctr_ctrl_hw &= ~mask;
> 		if (allowed)
> 			pmu->fixed_ctr_ctrl_hw |= pmu->fixed_ctr_ctrl & mask;
> 	}

The code looks good.