[PATCH 0/2] KVM: VMX: PT (RTIT) bug fix and cleanup

[PATCH 0/2] KVM: VMX: PT (RTIT) bug fix and cleanup

[Sean Christopherson]

Add a missing non-canonical check on writes to the RTIT address MSRs
and tack on a cleanup patch.

** ALL PATCHES ARE COMPLETELY UNTESTED **

Untested due to lack of hardware.

Sean Christopherson (2):
  KVM: VMX: Add non-canonical check on writes to RTIT address MSRs
  KVM: VMX: Add helper to consolidate up PT/RTIT WRMSR fault logic

 arch/x86/kvm/vmx/vmx.c | 57 ++++++++++++++++++++++++------------------
 1 file changed, 33 insertions(+), 24 deletions(-)

-- 
2.24.0

[PATCH 1/2] KVM: VMX: Add non-canonical check on writes to RTIT address MSRs

[Sean Christopherson]

Reject writes to RTIT address MSRs if the data being written is a
non-canonical address as the MSRs are subject to canonical checks, e.g.
KVM will trigger an unchecked #GP when loading the values to hardware
during pt_guest_enter().

Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 51e3b27f90ed..9aa2006dbe04 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2152,6 +2152,8 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 			(index >= 2 * intel_pt_validate_cap(vmx->pt_desc.caps,
 					PT_CAP_num_address_ranges)))
 			return 1;
+		if (is_noncanonical_address(data, vcpu))
+			return 1;
 		if (index % 2)
 			vmx->pt_desc.guest.addr_b[index / 2] = data;
 		else
-- 
2.24.0

RE: [PATCH 1/2] KVM: VMX: Add non-canonical check on writes to RTIT address MSRs

[Kang, Luwei]

> -----Original Message-----
> From: Christopherson, Sean J <sean.j.christopherson@intel.com>
> Sent: Wednesday, December 11, 2019 7:25 AM
> To: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Christopherson, Sean J <sean.j.christopherson@intel.com>; Vitaly Kuznetsov <vkuznets@redhat.com>; Wanpeng Li
> <wanpengli@tencent.com>; Jim Mattson <jmattson@google.com>; Joerg Roedel <joro@8bytes.org>; kvm@vger.kernel.org; linux-
> kernel@vger.kernel.org; Chao Peng <chao.p.peng@linux.intel.com>; Kang, Luwei <luwei.kang@intel.com>
> Subject: [PATCH 1/2] KVM: VMX: Add non-canonical check on writes to RTIT address MSRs
> 
> Reject writes to RTIT address MSRs if the data being written is a non-canonical address as the MSRs are subject to canonical checks,
> e.g.
> KVM will trigger an unchecked #GP when loading the values to hardware during pt_guest_enter().
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 51e3b27f90ed..9aa2006dbe04 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2152,6 +2152,8 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>  			(index >= 2 * intel_pt_validate_cap(vmx->pt_desc.caps,
>  					PT_CAP_num_address_ranges)))
>  			return 1;
> +		if (is_noncanonical_address(data, vcpu))
> +			return 1;

Is this for live migrate a VM with 5 level page table to the VM with 4 level page table?

Thanks,
Luwei Kang

>  		if (index % 2)
>  			vmx->pt_desc.guest.addr_b[index / 2] = data;
>  		else
> --
> 2.24.0

Re: [PATCH 1/2] KVM: VMX: Add non-canonical check on writes to RTIT address MSRs

[Sean Christopherson]

On Tue, Dec 10, 2019 at 06:16:35PM -0800, Kang, Luwei wrote:
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index
> > 51e3b27f90ed..9aa2006dbe04 100644 --- a/arch/x86/kvm/vmx/vmx.c +++
> > b/arch/x86/kvm/vmx/vmx.c @@ -2152,6 +2152,8 @@ static int
> > vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) (index >= 2 *
> > intel_pt_validate_cap(vmx->pt_desc.caps, PT_CAP_num_address_ranges)))
> > return 1; +		if (is_noncanonical_address(data, vcpu)) +
> > return 1;
> 
> Is this for live migrate a VM with 5 level page table to the VM with 4 level
> page table?

This is orthogonal to live migration or 5-level paging.  Unless I'm missing
something, KVM simply fails to validate the incoming address.

[PATCH 2/2] KVM: VMX: Add helper to consolidate up PT/RTIT WRMSR fault logic

[Sean Christopherson]

Add a helper to consolidate the common checks for writing PT MSRs,
and opportunistically clean up the formatting of the affected code.

No functional change intended.

Cc: Chao Peng <chao.p.peng@linux.intel.com>
Cc: Luwei Kang <luwei.kang@intel.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx/vmx.c | 55 ++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 9aa2006dbe04..22e05e3fb0a3 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1057,6 +1057,12 @@ static unsigned long segment_base(u16 selector)
 }
 #endif
 
+static inline bool pt_can_write_msr(struct vcpu_vmx *vmx)
+{
+	return (pt_mode == PT_MODE_HOST_GUEST) &&
+	       !(vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN);
+}
+
 static inline void pt_load_msr(struct pt_ctx *ctx, u32 addr_range)
 {
 	u32 i;
@@ -2110,47 +2116,48 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 		pt_update_intercept_for_msr(vmx);
 		break;
 	case MSR_IA32_RTIT_STATUS:
-		if ((pt_mode != PT_MODE_HOST_GUEST) ||
-			(vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN) ||
-			(data & MSR_IA32_RTIT_STATUS_MASK))
+		if (!pt_can_write_msr(vmx))
+			return 1;
+		if (data & MSR_IA32_RTIT_STATUS_MASK)
 			return 1;
 		vmx->pt_desc.guest.status = data;
 		break;
 	case MSR_IA32_RTIT_CR3_MATCH:
-		if ((pt_mode != PT_MODE_HOST_GUEST) ||
-			(vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN) ||
-			!intel_pt_validate_cap(vmx->pt_desc.caps,
-						PT_CAP_cr3_filtering))
+		if (!pt_can_write_msr(vmx))
+			return 1;
+		if (!intel_pt_validate_cap(vmx->pt_desc.caps,
+					   PT_CAP_cr3_filtering))
 			return 1;
 		vmx->pt_desc.guest.cr3_match = data;
 		break;
 	case MSR_IA32_RTIT_OUTPUT_BASE:
-		if ((pt_mode != PT_MODE_HOST_GUEST) ||
-			(vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN) ||
-			(!intel_pt_validate_cap(vmx->pt_desc.caps,
-					PT_CAP_topa_output) &&
-			 !intel_pt_validate_cap(vmx->pt_desc.caps,
-					PT_CAP_single_range_output)) ||
-			(data & MSR_IA32_RTIT_OUTPUT_BASE_MASK))
+		if (!pt_can_write_msr(vmx))
+			return 1;
+		if (!intel_pt_validate_cap(vmx->pt_desc.caps,
+					   PT_CAP_topa_output) &&
+		    !intel_pt_validate_cap(vmx->pt_desc.caps,
+					   PT_CAP_single_range_output))
+			return 1;
+		if (data & MSR_IA32_RTIT_OUTPUT_BASE_MASK)
 			return 1;
 		vmx->pt_desc.guest.output_base = data;
 		break;
 	case MSR_IA32_RTIT_OUTPUT_MASK:
-		if ((pt_mode != PT_MODE_HOST_GUEST) ||
-			(vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN) ||
-			(!intel_pt_validate_cap(vmx->pt_desc.caps,
-					PT_CAP_topa_output) &&
-			 !intel_pt_validate_cap(vmx->pt_desc.caps,
-					PT_CAP_single_range_output)))
+		if (!pt_can_write_msr(vmx))
+			return 1;
+		if (!intel_pt_validate_cap(vmx->pt_desc.caps,
+					   PT_CAP_topa_output) &&
+		    !intel_pt_validate_cap(vmx->pt_desc.caps,
+					   PT_CAP_single_range_output))
 			return 1;
 		vmx->pt_desc.guest.output_mask = data;
 		break;
 	case MSR_IA32_RTIT_ADDR0_A ... MSR_IA32_RTIT_ADDR3_B:
+		if (!pt_can_write_msr(vmx))
+			return 1;
 		index = msr_info->index - MSR_IA32_RTIT_ADDR0_A;
-		if ((pt_mode != PT_MODE_HOST_GUEST) ||
-			(vmx->pt_desc.guest.ctl & RTIT_CTL_TRACEEN) ||
-			(index >= 2 * intel_pt_validate_cap(vmx->pt_desc.caps,
-					PT_CAP_num_address_ranges)))
+		if (index >= 2 * intel_pt_validate_cap(vmx->pt_desc.caps,
+						       PT_CAP_num_address_ranges))
 			return 1;
 		if (is_noncanonical_address(data, vcpu))
 			return 1;
-- 
2.24.0

Re: [PATCH 0/2] KVM: VMX: PT (RTIT) bug fix and cleanup

[Paolo Bonzini]

On 11/12/19 00:24, Sean Christopherson wrote:
> Add a missing non-canonical check on writes to the RTIT address MSRs
> and tack on a cleanup patch.
> 
> ** ALL PATCHES ARE COMPLETELY UNTESTED **
> 
> Untested due to lack of hardware.
> 
> Sean Christopherson (2):
>   KVM: VMX: Add non-canonical check on writes to RTIT address MSRs
>   KVM: VMX: Add helper to consolidate up PT/RTIT WRMSR fault logic
> 
>  arch/x86/kvm/vmx/vmx.c | 57 ++++++++++++++++++++++++------------------
>  1 file changed, 33 insertions(+), 24 deletions(-)
> 

Queued, thanks.

Paolo