[1/3,nVMX] : Check "load IA32_PAT" VM-exit control on vmentry of L2 guests
diff mbox series

Message ID 20190319014624.31399-2-krish.sadhukhan@oracle.com
State New
Headers show
Series
  • [1/3,nVMX] : Check "load IA32_PAT" VM-exit control on vmentry of L2 guests
Related show

Commit Message

Krish Sadhukhan March 19, 2019, 1:46 a.m. UTC
According to section "Checks on Host Control Registers and MSRs" in Intel
SDM vol 3C, the following check is performed on vmentry of L2 guests:

    If the "load IA32_PAT" VM-exit control is 1, the value of the field
    for the IA32_PAT MSR must be one that could be written by WRMSR
    without fault at CPL 0. Specifically, each of the 8 bytes in the
    field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP),
    6 (WB), or 7 (UC-).

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 arch/x86/kvm/vmx/nested.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Sean Christopherson March 19, 2019, 2:49 p.m. UTC | #1
On Mon, Mar 18, 2019 at 09:46:22PM -0400, Krish Sadhukhan wrote:
> According to section "Checks on Host Control Registers and MSRs" in Intel
> SDM vol 3C, the following check is performed on vmentry of L2 guests:

I think you can leave off "of L2 guests".  That implies VM-Entry to other
levels are not subject to this check.

> 
>     If the "load IA32_PAT" VM-exit control is 1, the value of the field
>     for the IA32_PAT MSR must be one that could be written by WRMSR
>     without fault at CPL 0. Specifically, each of the 8 bytes in the
>     field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP),
>     6 (WB), or 7 (UC-).
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 3170e291215d..a1b44d930d26 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2583,6 +2583,24 @@ static int nested_check_vm_entry_controls(struct kvm_vcpu *vcpu,
>  	return 0;
>  }
>  
> +/*
> + * Fields in IA32_PAT MSR are allowed to  contain one of the
> + * following values:
> + * 	0 (UC), 1 (WC), 4 (WT), 5 (WP), 6 (WB), or 7 (UC-)
> + */
> +static int nested_check_pat_fields(u64 msr)

Maybe just nested_check_pat()?  The SDM uses "field" to refer to the
VMCS field, not the individual bytes in the MSR.

> +{
> +	int i;
> +	u64 tmp = (msr >> i * 8) & 0xff;

This uses 'i' uninitialized and is obviously intended to be in the for
loop.  You can actually avoid 'i' and the multiplication entirely since
'0' is a legal PAT value (UC), e.g.:

	u64 tmp;

	for ( ; msr; msr >>= 8) {
		tmp = msr & 0xff;
		if (tmp == 0x2 || tmp == 0x3 || tmp > 0x7)
			return -EINVAL;
	}
	return 0;

> +
> +	for (i = 0; i < 8; i++) {
> +		if (tmp == 0x2 || tmp == 0x3 || tmp > 0x7)
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * Checks related to Host Control Registers and MSRs
>   */
> @@ -2595,6 +2613,12 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
>  	    !nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
>  	    !nested_cr3_valid(vcpu, vmcs12->host_cr3))
>  		return -EINVAL;
> +
> +	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PAT) {
> +		if (nested_check_pat_fields(vmcs12->host_ia32_pat))

This can be a single if statement connected with &&.

> +			return -EINVAL;
> +	}
> +
>  	/*
>  	 * If the load IA32_EFER VM-exit control is 1, bits reserved in the
>  	 * IA32_EFER MSR must be 0 in the field for that register. In addition,
> -- 
> 2.17.2
>
Sean Christopherson March 19, 2019, 3:14 p.m. UTC | #2
On Tue, Mar 19, 2019 at 07:49:42AM -0700, Sean Christopherson wrote:
> On Mon, Mar 18, 2019 at 09:46:22PM -0400, Krish Sadhukhan wrote:
> > According to section "Checks on Host Control Registers and MSRs" in Intel
> > SDM vol 3C, the following check is performed on vmentry of L2 guests:
> 
> I think you can leave off "of L2 guests".  That implies VM-Entry to other
> levels are not subject to this check.
> 
> > 
> >     If the "load IA32_PAT" VM-exit control is 1, the value of the field
> >     for the IA32_PAT MSR must be one that could be written by WRMSR
> >     without fault at CPL 0. Specifically, each of the 8 bytes in the
> >     field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP),
> >     6 (WB), or 7 (UC-).
> > 
> > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> > Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> > ---
> >  arch/x86/kvm/vmx/nested.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> > index 3170e291215d..a1b44d930d26 100644
> > --- a/arch/x86/kvm/vmx/nested.c
> > +++ b/arch/x86/kvm/vmx/nested.c
> > @@ -2583,6 +2583,24 @@ static int nested_check_vm_entry_controls(struct kvm_vcpu *vcpu,
> >  	return 0;
> >  }
> >  
> > +/*
> > + * Fields in IA32_PAT MSR are allowed to  contain one of the
> > + * following values:
> > + * 	0 (UC), 1 (WC), 4 (WT), 5 (WP), 6 (WB), or 7 (UC-)
> > + */
> > +static int nested_check_pat_fields(u64 msr)
> 
> Maybe just nested_check_pat()?  The SDM uses "field" to refer to the
> VMCS field, not the individual bytes in the MSR.
> 
> > +{
> > +	int i;
> > +	u64 tmp = (msr >> i * 8) & 0xff;
> 
> This uses 'i' uninitialized and is obviously intended to be in the for
> loop.  You can actually avoid 'i' and the multiplication entirely since
> '0' is a legal PAT value (UC), e.g.:
> 
> 	u64 tmp;
> 
> 	for ( ; msr; msr >>= 8) {
> 		tmp = msr & 0xff;
> 		if (tmp == 0x2 || tmp == 0x3 || tmp > 0x7)
> 			return -EINVAL;
> 	}
> 	return 0;

Even better, just use kvm_mtrr_valid().

> 
> > +
> > +	for (i = 0; i < 8; i++) {
> > +		if (tmp == 0x2 || tmp == 0x3 || tmp > 0x7)
> > +			return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Checks related to Host Control Registers and MSRs
> >   */
> > @@ -2595,6 +2613,12 @@ static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
> >  	    !nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
> >  	    !nested_cr3_valid(vcpu, vmcs12->host_cr3))
> >  		return -EINVAL;
> > +
> > +	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PAT) {
> > +		if (nested_check_pat_fields(vmcs12->host_ia32_pat))
> 
> This can be a single if statement connected with &&.
> 
> > +			return -EINVAL;
> > +	}
> > +
> >  	/*
> >  	 * If the load IA32_EFER VM-exit control is 1, bits reserved in the
> >  	 * IA32_EFER MSR must be 0 in the field for that register. In addition,
> > -- 
> > 2.17.2
> >
Jim Mattson March 19, 2019, 4:23 p.m. UTC | #3
On Tue, Mar 19, 2019 at 8:14 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:

> Even better, just use kvm_mtrr_valid().

I concur! As much as possible, the VM-entry checks should leverage the
existing checks for valid state.

Patch
diff mbox series

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 3170e291215d..a1b44d930d26 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2583,6 +2583,24 @@  static int nested_check_vm_entry_controls(struct kvm_vcpu *vcpu,
 	return 0;
 }
 
+/*
+ * Fields in IA32_PAT MSR are allowed to  contain one of the
+ * following values:
+ * 	0 (UC), 1 (WC), 4 (WT), 5 (WP), 6 (WB), or 7 (UC-)
+ */
+static int nested_check_pat_fields(u64 msr)
+{
+	int i;
+	u64 tmp = (msr >> i * 8) & 0xff;
+
+	for (i = 0; i < 8; i++) {
+		if (tmp == 0x2 || tmp == 0x3 || tmp > 0x7)
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
 /*
  * Checks related to Host Control Registers and MSRs
  */
@@ -2595,6 +2613,12 @@  static int nested_check_host_control_regs(struct kvm_vcpu *vcpu,
 	    !nested_host_cr4_valid(vcpu, vmcs12->host_cr4) ||
 	    !nested_cr3_valid(vcpu, vmcs12->host_cr3))
 		return -EINVAL;
+
+	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_PAT) {
+		if (nested_check_pat_fields(vmcs12->host_ia32_pat))
+			return -EINVAL;
+	}
+
 	/*
 	 * If the load IA32_EFER VM-exit control is 1, bits reserved in the
 	 * IA32_EFER MSR must be 0 in the field for that register. In addition,