[PATCH] KVM: x86/pmu: Fix type length error when reading pmu->fixed_ctr_ctrl

[PATCH] KVM: x86/pmu: Fix type length error when reading pmu->fixed_ctr_ctrl

[Mingwei Zhang]

Fix type length error since pmu->fixed_ctr_ctrl is u64 but the local
variable old_fixed_ctr_ctrl is u8. Truncating the value leads to
information loss at runtime. This leads to incorrect value in old_ctrl
retrieved from each field of old_fixed_ctr_ctrl and causes incorrect code
execution within the for loop of reprogram_fixed_counters(). So fix this
type to u64.

Fixes: 76d287b2342e ("KVM: x86/pmu: Drop "u8 ctrl, int idx" for reprogram_fixed_counter()")
Signed-off-by: Mingwei Zhang <mizhang@google.com>
---
 arch/x86/kvm/vmx/pmu_intel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index a6216c874729..315c7c2ba89b 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -71,7 +71,7 @@ static int fixed_pmc_events[] = {
 static void reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)
 {
 	struct kvm_pmc *pmc;
-	u8 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
+	u64 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
 	int i;
 
 	pmu->fixed_ctr_ctrl = data;

base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
-- 
2.43.0.429.g432eaa2c6b-goog

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

[Sean Christopherson]

On Tue, Jan 23, 2024, Mingwei Zhang wrote:
> Fix type length error since pmu->fixed_ctr_ctrl is u64 but the local
> variable old_fixed_ctr_ctrl is u8. Truncating the value leads to
> information loss at runtime. This leads to incorrect value in old_ctrl
> retrieved from each field of old_fixed_ctr_ctrl and causes incorrect code
> execution within the for loop of reprogram_fixed_counters(). So fix this
> type to u64.

But what is the actual fallout from this?  Stating that the bug causes incorrect
code execution isn't helpful, that's akin to saying water is wet.

If I'm following the code correctly, the only fallout is that KVM may unnecessarily
mark a fixed PMC as in use and reprogram it.  I.e. the bug can result in (minor?)
performance issues, but it won't cause functional problems.

Understanding what actually goes wrong matters, because I'm trying to determine
whether or not this needs to be fixed in 6.8 and backported to stable trees.  If
the bug is relatively benign, then this is fodder for 6.9.

> Fixes: 76d287b2342e ("KVM: x86/pmu: Drop "u8 ctrl, int idx" for reprogram_fixed_counter()")
> Signed-off-by: Mingwei Zhang <mizhang@google.com>
> ---
>  arch/x86/kvm/vmx/pmu_intel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> index a6216c874729..315c7c2ba89b 100644
> --- a/arch/x86/kvm/vmx/pmu_intel.c
> +++ b/arch/x86/kvm/vmx/pmu_intel.c
> @@ -71,7 +71,7 @@ static int fixed_pmc_events[] = {
>  static void reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)
>  {
>  	struct kvm_pmc *pmc;
> -	u8 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
> +	u64 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
>  	int i;
>  
>  	pmu->fixed_ctr_ctrl = data;
> 
> base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
> -- 
> 2.43.0.429.g432eaa2c6b-goog
> 

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

[Dongli Zhang]

On 1/31/24 07:43, Sean Christopherson wrote:
> On Tue, Jan 23, 2024, Mingwei Zhang wrote:
>> Fix type length error since pmu->fixed_ctr_ctrl is u64 but the local
>> variable old_fixed_ctr_ctrl is u8. Truncating the value leads to
>> information loss at runtime. This leads to incorrect value in old_ctrl
>> retrieved from each field of old_fixed_ctr_ctrl and causes incorrect code
>> execution within the for loop of reprogram_fixed_counters(). So fix this
>> type to u64.
> 
> But what is the actual fallout from this?  Stating that the bug causes incorrect
> code execution isn't helpful, that's akin to saying water is wet.
> 
> If I'm following the code correctly, the only fallout is that KVM may unnecessarily
> mark a fixed PMC as in use and reprogram it.  I.e. the bug can result in (minor?)
> performance issues, but it won't cause functional problems.

My this issue cause "Uhhuh. NMI received for unknown reason XX on CPU XX." at VM side?

The PMC is still active while the VM side handle_pmi_common() is not going to handle it?

Thank you very much!

Dongli Zhang

> 
> Understanding what actually goes wrong matters, because I'm trying to determine
> whether or not this needs to be fixed in 6.8 and backported to stable trees.  If
> the bug is relatively benign, then this is fodder for 6.9.
> 
>> Fixes: 76d287b2342e ("KVM: x86/pmu: Drop "u8 ctrl, int idx" for reprogram_fixed_counter()")
>> Signed-off-by: Mingwei Zhang <mizhang@google.com>
>> ---
>>  arch/x86/kvm/vmx/pmu_intel.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
>> index a6216c874729..315c7c2ba89b 100644
>> --- a/arch/x86/kvm/vmx/pmu_intel.c
>> +++ b/arch/x86/kvm/vmx/pmu_intel.c
>> @@ -71,7 +71,7 @@ static int fixed_pmc_events[] = {
>>  static void reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)
>>  {
>>  	struct kvm_pmc *pmc;
>> -	u8 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
>> +	u64 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
>>  	int i;
>>  
>>  	pmu->fixed_ctr_ctrl = data;
>>
>> base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
>> -- 
>> 2.43.0.429.g432eaa2c6b-goog
>>
> 

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

[Mingwei Zhang]

On Wed, Jan 31, 2024 at 9:02 AM Dongli Zhang <dongli.zhang@oracle.com> wrote:
>
>
>
> On 1/31/24 07:43, Sean Christopherson wrote:
> > On Tue, Jan 23, 2024, Mingwei Zhang wrote:
> >> Fix type length error since pmu->fixed_ctr_ctrl is u64 but the local
> >> variable old_fixed_ctr_ctrl is u8. Truncating the value leads to
> >> information loss at runtime. This leads to incorrect value in old_ctrl
> >> retrieved from each field of old_fixed_ctr_ctrl and causes incorrect code
> >> execution within the for loop of reprogram_fixed_counters(). So fix this
> >> type to u64.
> >
> > But what is the actual fallout from this?  Stating that the bug causes incorrect
> > code execution isn't helpful, that's akin to saying water is wet.
> >
> > If I'm following the code correctly, the only fallout is that KVM may unnecessarily
> > mark a fixed PMC as in use and reprogram it.  I.e. the bug can result in (minor?)
> > performance issues, but it won't cause functional problems.
>
> My this issue cause "Uhhuh. NMI received for unknown reason XX on CPU XX." at VM side?
>
> 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?

I did not see this warning on my side, but it seems possible.

Thanks.
-Mingwei
>
> Thank you very much!
>
> Dongli Zhang
>
> >
> > Understanding what actually goes wrong matters, because I'm trying to determine
> > whether or not this needs to be fixed in 6.8 and backported to stable trees.  If
> > the bug is relatively benign, then this is fodder for 6.9.
> >
> >> Fixes: 76d287b2342e ("KVM: x86/pmu: Drop "u8 ctrl, int idx" for reprogram_fixed_counter()")
> >> Signed-off-by: Mingwei Zhang <mizhang@google.com>
> >> ---
> >>  arch/x86/kvm/vmx/pmu_intel.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
> >> index a6216c874729..315c7c2ba89b 100644
> >> --- a/arch/x86/kvm/vmx/pmu_intel.c
> >> +++ b/arch/x86/kvm/vmx/pmu_intel.c
> >> @@ -71,7 +71,7 @@ static int fixed_pmc_events[] = {
> >>  static void reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)
> >>  {
> >>      struct kvm_pmc *pmc;
> >> -    u8 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
> >> +    u64 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
> >>      int i;
> >>
> >>      pmu->fixed_ctr_ctrl = data;
> >>
> >> base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
> >> --
> >> 2.43.0.429.g432eaa2c6b-goog
> >>
> >

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

[Sean Christopherson]

On Wed, Jan 31, 2024, Mingwei Zhang wrote:
> On Wed, Jan 31, 2024 at 9:02 AM Dongli Zhang <dongli.zhang@oracle.com> wrote:
> > On 1/31/24 07:43, Sean Christopherson wrote:
> > > On Tue, Jan 23, 2024, Mingwei Zhang wrote:
> > >> Fix type length error since pmu->fixed_ctr_ctrl is u64 but the local
> > >> variable old_fixed_ctr_ctrl is u8. Truncating the value leads to
> > >> information loss at runtime. This leads to incorrect value in old_ctrl
> > >> retrieved from each field of old_fixed_ctr_ctrl and causes incorrect code
> > >> execution within the for loop of reprogram_fixed_counters(). So fix this
> > >> type to u64.
> > >
> > > But what is the actual fallout from this?  Stating that the bug causes incorrect
> > > code execution isn't helpful, that's akin to saying water is wet.
> > >
> > > If I'm following the code correctly, the only fallout is that KVM may unnecessarily
> > > mark a fixed PMC as in use and reprogram it.  I.e. the bug can result in (minor?)
> > > performance issues, but it won't cause functional problems.
> >
> > My this issue cause "Uhhuh. NMI received for unknown reason XX on CPU XX." at VM side?
> >
> > 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.

And FWIW, KVM does do the right thing (well, "right" might be too strong) when a
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.

Side topic, looking at this code made me realize just how terrible the names
pmc_in_use and pmc_speculative_in_use() are.  "pmc_in_use" sounds like it tracks
which PMCs have perf_events, and at first glance at kvm_pmu_cleanup(), it even
_looks_ like that's the case.  But kvm_pmu_cleanup() is _skipping_ PMCs that are
not "in use".  And conversely, there is nothing speculative about checking the
local enable bit for a PMC.

I'll send patches to rename pmc_in_use to pmc_accessed, and pmc_speculative_in_use()
to pmc_is_locally_enabled().

As for this one, unless someone spends the time to prove me wrong, it's destined
for 6.9 with a changelog that says the bug is likely benign.

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

[Mingwei Zhang]

On Thu, Feb 01, 2024, Sean Christopherson wrote:
> On Wed, Jan 31, 2024, Mingwei Zhang wrote:
> > On Wed, Jan 31, 2024 at 9:02 AM Dongli Zhang <dongli.zhang@oracle.com> wrote:
> > > On 1/31/24 07:43, Sean Christopherson wrote:
> > > > On Tue, Jan 23, 2024, Mingwei Zhang wrote:
> > > >> Fix type length error since pmu->fixed_ctr_ctrl is u64 but the local
> > > >> variable old_fixed_ctr_ctrl is u8. Truncating the value leads to
> > > >> information loss at runtime. This leads to incorrect value in old_ctrl
> > > >> retrieved from each field of old_fixed_ctr_ctrl and causes incorrect code
> > > >> execution within the for loop of reprogram_fixed_counters(). So fix this
> > > >> type to u64.
> > > >
> > > > But what is the actual fallout from this?  Stating that the bug causes incorrect
> > > > code execution isn't helpful, that's akin to saying water is wet.
> > > >
> > > > If I'm following the code correctly, the only fallout is that KVM may unnecessarily
> > > > mark a fixed PMC as in use and reprogram it.  I.e. the bug can result in (minor?)
> > > > performance issues, but it won't cause functional problems.
> > >
> > > My this issue cause "Uhhuh. NMI received for unknown reason XX on CPU XX." at VM side?
> > >
> > > 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.

So this will be an ugly bug in an ancient PMU.
> 
> And FWIW, KVM does do the right thing (well, "right" might be too strong) when a

right thing? yes. too strong.

> 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!".

> 
> Side topic, looking at this code made me realize just how terrible the names
> pmc_in_use and pmc_speculative_in_use() are.  "pmc_in_use" sounds like it tracks
> which PMCs have perf_events, and at first glance at kvm_pmu_cleanup(), it even
> _looks_ like that's the case.  But kvm_pmu_cleanup() is _skipping_ PMCs that are
> not "in use".  And conversely, there is nothing speculative about checking the
> local enable bit for a PMC.

pmc_in_use is a terrible name. It seems the only usage point is for
LBR...
> 
> I'll send patches to rename pmc_in_use to pmc_accessed, and pmc_speculative_in_use()
> to pmc_is_locally_enabled().

yes, I like pmc_is_locally_enabled(). But I don't know what is better
for pmc_in_use.

> 
> As for this one, unless someone spends the time to prove me wrong, it's destined
> for 6.9 with a changelog that says the bug is likely benign.

It is not benign, but does not matter for modern CPUs with Intel PerfMon
v2 and later. So, for virtualized environment, it might be still
critical for those VMs with PerfMon v1.

Thanks.
-Mingwei

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

[Sean Christopherson]

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, are already disabled.  As
  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.

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

[Mingwei Zhang]

On Thu, Feb 1, 2024 at 11:36 AM Sean Christopherson <seanjc@google.com> 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, are already disabled.  As
>   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.

That works for me. Maybe scoping that to the guest VMs with PerfMon v1 enabled?

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

[Sean Christopherson]

On Thu, Feb 01, 2024, Mingwei Zhang wrote:
> On Thu, Feb 1, 2024 at 11:36 AM Sean Christopherson <seanjc@google.com> 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, are already disabled.  As
> >   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.
> 
> That works for me. Maybe scoping that to the guest VMs with PerfMon v1 enabled?

No, because from a purely architectural perspective, the bug isn't limited to
VMs without PERF_GLOBAL_CTRL.  Linux may always clear the associated enable bit
in PERF_GLOBAL_CTRL, but that's not a hard requirement, a guest could choose to
always leave bits set in PERF_GLOBAL_CTRL and instead use IA32_FIXED_CTR_CTRL to
toggle PMCs on and off.

  Each enable bit in MSR_PERF_GLOBAL_CTRL is AND’ed with the enable bits for all
  privilege levels in the respective IA32_PERFEVTSELx or IA32_FIXED_CTR_CTRL MSRs
  to start/stop the counting of respective counters. Counting is enabled if the
  AND’ed results is true; counting is disabled when the result is false.

I'm not saying that such guests are likely to show up in the wild, but I don't
want to make any assumptions about what the guest does or does not do when it
comes to making statements about the impact of bugs.

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

[Mingwei Zhang]

On Thu, Feb 1, 2024 at 2:53 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Feb 01, 2024, Mingwei Zhang wrote:
> > On Thu, Feb 1, 2024 at 11:36 AM Sean Christopherson <seanjc@google.com> 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, are already disabled.  As
> > >   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.
> >
> > That works for me. Maybe scoping that to the guest VMs with PerfMon v1 enabled?
>
> No, because from a purely architectural perspective, the bug isn't limited to
> VMs without PERF_GLOBAL_CTRL.  Linux may always clear the associated enable bit
> in PERF_GLOBAL_CTRL, but that's not a hard requirement, a guest could choose to
> always leave bits set in PERF_GLOBAL_CTRL and instead use IA32_FIXED_CTR_CTRL to
> toggle PMCs on and off.
>
>   Each enable bit in MSR_PERF_GLOBAL_CTRL is AND’ed with the enable bits for all
>   privilege levels in the respective IA32_PERFEVTSELx or IA32_FIXED_CTR_CTRL MSRs
>   to start/stop the counting of respective counters. Counting is enabled if the
>   AND’ed results is true; counting is disabled when the result is false.
>
> I'm not saying that such guests are likely to show up in the wild, but I don't
> want to make any assumptions about what the guest does or does not do when it
> comes to making statements about the impact of bugs.

I agree with you. We don't know what kind of guest we are serving. It
may not be Linux, it may not even be the perf subsystem in Linux. They
could choose to do whatever style that is allowed architecturally to
start/stop/reprogram the fixed counter.

Thanks.

-Mingwei

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

[Zhang, Xiong Y]

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.
> 

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

[Sean Christopherson]

On Fri, Feb 02, 2024, Xiong Y Zhang wrote:
> 
> 
> 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" 

Ugh, math.  I'll adjust it to:

  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 fixed counter 2 is already disabled (the bug also affects fixed
  counters 3+, but KVM doesn't yet support those).  As a result, if the
  guest disables fixed counter 2, KVM will get a false negative and fail to
  reprogram/disable emulation of the counter, which can leads to incorrect
  counts and spurious PMIs in the guest.

Thanks!

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

[Sean Christopherson]

On Tue, 23 Jan 2024 22:12:20 +0000, Mingwei Zhang wrote:
> Fix type length error since pmu->fixed_ctr_ctrl is u64 but the local
> variable old_fixed_ctr_ctrl is u8. Truncating the value leads to
> information loss at runtime. This leads to incorrect value in old_ctrl
> retrieved from each field of old_fixed_ctr_ctrl and causes incorrect code
> execution within the for loop of reprogram_fixed_counters(). So fix this
> type to u64.
> 
> [...]

Applied to kvm-x86 fixes.  I'll let it stew in -next for a few days before
sending a pull request to Paolo.  Thanks!

[1/1] KVM: x86/pmu: Fix type length error when reading pmu->fixed_ctr_ctrl
      https://github.com/kvm-x86/linux/commit/05519c86d699

--
https://github.com/kvm-x86/linux/tree/next