| Message ID | 20190319014624.31399-2-krish.sadhukhan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3,nVMX] : Check "load IA32_PAT" VM-exit control on vmentry of L2 guests | expand |
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 >
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 > >
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.
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,
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(+)