diff mbox series

[v4,05/12] KVM: x86/pmu: Error when user sets the GLOBAL_STATUS reserved bits

Message ID 20230214050757.9623-6-likexu@tencent.com
State New
Headers
Series KVM: x86: Add AMD Guest PerfMonV2 PMU support |

Commit Message

Like Xu Feb. 14, 2023, 5:07 a.m. UTC 
  From: Like Xu <likexu@tencent.com>

If the user space sets reserved bits when restoring the MSR_CORE_
PERF_GLOBAL_STATUS register, these bits will be accidentally returned
when the guest runs a read access to this register, and cannot be cleared
up inside the guest, which makes the guest's PMI handler very confused.

Signed-off-by: Like Xu <likexu@tencent.com>
---
 arch/x86/kvm/vmx/pmu_intel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Sean Christopherson April 6, 2023, 11:45 p.m. UTC | #1 
On Tue, Feb 14, 2023, Like Xu wrote:
> From: Like Xu <likexu@tencent.com>
> 
> If the user space sets reserved bits when restoring the MSR_CORE_
> PERF_GLOBAL_STATUS register, these bits will be accidentally returned
> when the guest runs a read access to this register, and cannot be cleared
> up inside the guest, which makes the guest's PMI handler very confused.

The changelog needs to state what the patch actually does.

> Signed-off-by: Like Xu <likexu@tencent.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 904f832fc55d..aaea25d2cae8 100644
> --- a/arch/x86/kvm/vmx/pmu_intel.c
> +++ b/arch/x86/kvm/vmx/pmu_intel.c
> @@ -397,7 +397,7 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  			reprogram_fixed_counters(pmu, data);
>  		break;
>  	case MSR_CORE_PERF_GLOBAL_STATUS:
> -		if (!msr_info->host_initiated)
> +		if (!msr_info->host_initiated || (data & pmu->global_ovf_ctrl_mask))

This is wrong.  Bits 60:58 are reserved in IA32_PERF_GLOBAL_OVF_CTRL, but are
ASCI, CTR_FREEZE, and LBR_FREEZE respectively in MSR_CORE_PERF_GLOBAL_STATUS.

>  			return 1; /* RO MSR */
>  
>  		pmu->global_status = data;
> -- 
> 2.39.1
>
Like Xu April 7, 2023, 5:08 a.m. UTC | #2 
On 7/4/2023 7:45 am, Sean Christopherson wrote:
> On Tue, Feb 14, 2023, Like Xu wrote:
>> From: Like Xu <likexu@tencent.com>
>>
>> If the user space sets reserved bits when restoring the MSR_CORE_
>> PERF_GLOBAL_STATUS register, these bits will be accidentally returned
>> when the guest runs a read access to this register, and cannot be cleared
>> up inside the guest, which makes the guest's PMI handler very confused.
> 
> The changelog needs to state what the patch actually does.
> 
>> Signed-off-by: Like Xu <likexu@tencent.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 904f832fc55d..aaea25d2cae8 100644
>> --- a/arch/x86/kvm/vmx/pmu_intel.c
>> +++ b/arch/x86/kvm/vmx/pmu_intel.c
>> @@ -397,7 +397,7 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>   			reprogram_fixed_counters(pmu, data);
>>   		break;
>>   	case MSR_CORE_PERF_GLOBAL_STATUS:
>> -		if (!msr_info->host_initiated)
>> +		if (!msr_info->host_initiated || (data & pmu->global_ovf_ctrl_mask))
> 
> This is wrong.  Bits 60:58 are reserved in IA32_PERF_GLOBAL_OVF_CTRL, but are
> ASCI, CTR_FREEZE, and LBR_FREEZE respectively in MSR_CORE_PERF_GLOBAL_STATUS.

CTR_FREEZE and LBR_FREEZE are only required for the guest CPUID.0AH: EAX[7:0]>3.
PMU support (ASCI bit) for guest SGX isn't supported either.

So for now, reusing pmu->global_ovf_ctrl_mask here is effective enough.

> 
>>   			return 1; /* RO MSR */
>>   
>>   		pmu->global_status = data;
>> -- 
>> 2.39.1
>>
Sean Christopherson April 7, 2023, 3:43 p.m. UTC | #3 
On Fri, Apr 07, 2023, Like Xu wrote:
> On 7/4/2023 7:45 am, Sean Christopherson wrote:
> > On Tue, Feb 14, 2023, Like Xu wrote:
> > > From: Like Xu <likexu@tencent.com>
> > > 
> > > If the user space sets reserved bits when restoring the MSR_CORE_
> > > PERF_GLOBAL_STATUS register, these bits will be accidentally returned
> > > when the guest runs a read access to this register, and cannot be cleared
> > > up inside the guest, which makes the guest's PMI handler very confused.
> > 
> > The changelog needs to state what the patch actually does.
> > 
> > > Signed-off-by: Like Xu <likexu@tencent.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 904f832fc55d..aaea25d2cae8 100644
> > > --- a/arch/x86/kvm/vmx/pmu_intel.c
> > > +++ b/arch/x86/kvm/vmx/pmu_intel.c
> > > @@ -397,7 +397,7 @@ static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > >   			reprogram_fixed_counters(pmu, data);
> > >   		break;
> > >   	case MSR_CORE_PERF_GLOBAL_STATUS:
> > > -		if (!msr_info->host_initiated)
> > > +		if (!msr_info->host_initiated || (data & pmu->global_ovf_ctrl_mask))
> > 
> > This is wrong.  Bits 60:58 are reserved in IA32_PERF_GLOBAL_OVF_CTRL, but are
> > ASCI, CTR_FREEZE, and LBR_FREEZE respectively in MSR_CORE_PERF_GLOBAL_STATUS.
> 
> CTR_FREEZE and LBR_FREEZE are only required for the guest CPUID.0AH: EAX[7:0]>3.
> PMU support (ASCI bit) for guest SGX isn't supported either.
> 
> So for now, reusing pmu->global_ovf_ctrl_mask here is effective enough.

And "good enough for now" is exactly how we end up with bugs, especially when
"good enough" relies on assumptions that aren't well documented.
diff mbox series

Patch

diff --git a/arch/x86/kvm/vmx/pmu_intel.c b/arch/x86/kvm/vmx/pmu_intel.c
index 904f832fc55d..aaea25d2cae8 100644
--- a/arch/x86/kvm/vmx/pmu_intel.c
+++ b/arch/x86/kvm/vmx/pmu_intel.c
@@ -397,7 +397,7 @@  static int intel_pmu_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			reprogram_fixed_counters(pmu, data);
 		break;
 	case MSR_CORE_PERF_GLOBAL_STATUS:
-		if (!msr_info->host_initiated)
+		if (!msr_info->host_initiated || (data & pmu->global_ovf_ctrl_mask))
 			return 1; /* RO MSR */
 
 		pmu->global_status = data;