linux-kernel - Re: [PATCH] KVM: x86/pmu: Fix type length error when reading pmu->fixed_ctr

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: Fri, 2 Feb 2024 11:25:14 +0800
From: "Zhang, Xiong Y" <xiong.y.zhang@...ux.intel.com>
To: Sean Christopherson <seanjc@...gle.com>,
 Mingwei Zhang <mizhang@...gle.com>
Cc: Dongli Zhang <dongli.zhang@...cle.com>,
 Paolo Bonzini <pbonzini@...hat.com>, "H. Peter Anvin" <hpa@...or.com>,
 kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: x86/pmu: Fix type length error when reading
 pmu->fixed_ctr_ctrl



On 2/2/2024 3:36 AM, Sean Christopherson wrote:
> On Thu, Feb 01, 2024, Mingwei Zhang wrote:
>> On Thu, Feb 01, 2024, Sean Christopherson wrote:
>>> On Wed, Jan 31, 2024, Mingwei Zhang wrote:
>>>>> The PMC is still active while the VM side handle_pmi_common() is not going to handle it?
>>>>
>>>> hmm, so the new value is '0', but the old value is non-zero, KVM is
>>>> supposed to zero out (stop) the fix counter), but it skips it. This
>>>> leads to the counter continuously increasing until it overflows, but
>>>> guest PMU thought it had disabled it. That's why you got this warning?
>>>
>>> No, that can't happen, and KVM would have a massive bug if that were the case.
>>> The truncation can _only_ cause bits to disappear, it can't magically make bits
>>> appear, i.e. the _only_ way this can cause a problem is for KVM to incorrectly
>>> think a PMC is being disabled.
>>
>> The reason why the bug does not happen is because there is global
>> control. So disabling a counter will be effectively done in the global
>> disable part, ie., when guest PMU writes to MSR 0x38f.
> 
> 
>>> fixed PMC is disabled. KVM will pause the counter in reprogram_counter(), and
>>> then leave the perf event paused counter as pmc_event_is_allowed() will return
>>> %false due to the PMC being locally disabled.
>>>
>>> But in this case, _if_ the counter is actually enabled, KVM will simply reprogram
>>> the PMC.  Reprogramming is unnecessary and wasteful, but it's not broken.
>>
>> no, if the counter is actually enabled, but then it is assigned to
>> old_fixed_ctr_ctrl, the value is truncated. When control goes to the
>> check at the time of disabling the counter, KVM thinks it is disabled,
>> since the value is already truncated to 0. So KVM will skip by saying
>> "oh, the counter is already disabled, why reprogram? No need!".
> 
> Ooh, I had them backwards.  KVM can miss 1=>0, but not 0=>1.  I'll apply this
> for 6.8; does this changelog work for you?
> 
>   Use a u64 instead of a u8 when taking a snapshot of pmu->fixed_ctr_ctrl
>   when reprogramming fixed counters, as truncating the value results in KVM
>   thinking all fixed counters, except counter 0, 
each counter has four bits in fixed_ctr_ctrl, here u8 could cover counter 0 and counter 1, so "except counter 0" can be modified to "except counter 0 and 1" 
> are already disabled.  
>   a result, if the guest disables a fixed counter, KVM will get a false
>   negative and fail to reprogram/disable emulation of the counter, which can
>   leads to spurious PMIs in the guest.
>