| Message ID | 20180830230537.80356-1-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nVMX: Unrestricted guest mode requires EPT | expand |
On 08/30/2018 04:05 PM, Jim Mattson wrote: > As specified in Intel's SDM, do not allow the L1 hypervisor to launch > an L2 guest with the VM-execution controls for "unrestricted guest" or > "mode-based execute control for EPT" set and the VM-execution control > for "enable EPT" clear. > > Note that the VM-execution control for "mode-based execute control for > EPT" is not yet virtualized by kvm. > > Reported-by: Andrew Thornton <andrewth@google.com> > Signed-off-by: Jim Mattson <jmattson@google.com> > Reviewed-by: Peter Shier <pshier@google.com> > --- > arch/x86/include/asm/vmx.h | 1 + > arch/x86/kvm/vmx.c | 13 +++++++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h > index 9527ba5d62da..665632a4b54b 100644 > --- a/arch/x86/include/asm/vmx.h > +++ b/arch/x86/include/asm/vmx.h > @@ -78,6 +78,7 @@ > #define SECONDARY_EXEC_RDSEED_EXITING 0x00010000 > #define SECONDARY_EXEC_ENABLE_PML 0x00020000 > #define SECONDARY_EXEC_XSAVES 0x00100000 > +#define SECONDARY_EXEC_MODE_BASED_EPT_EXEC 0x00400000 > #define SECONDARY_EXEC_TSC_SCALING 0x02000000 > > #define PIN_BASED_EXT_INTR_MASK 0x00000001 > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 1d26f3c4985b..2bf990b5848f 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -11719,6 +11719,16 @@ static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu, > return 0; > } > > +static int nested_vmx_check_ept_enable(struct kvm_vcpu *vcpu, > + struct vmcs12 *vmcs12) > +{ > + if ((nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) || > + nested_cpu_has2(vmcs12, SECONDARY_EXEC_MODE_BASED_EPT_EXEC)) && > + !nested_cpu_has_ept(vmcs12)) > + return -EINVAL; > + return 0; > +} > + > static int nested_vmx_check_shadow_vmcs_controls(struct kvm_vcpu *vcpu, > struct vmcs12 *vmcs12) > { > @@ -12339,6 +12349,9 @@ static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) > if (nested_vmx_check_pml_controls(vcpu, vmcs12)) > return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > > + if (nested_vmx_check_ept_enable(vcpu, vmcs12)) > + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > + > if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12)) > return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
On Fri, Aug 31, 2018 at 3:27 PM, Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote: > > > On 08/30/2018 04:05 PM, Jim Mattson wrote: >> >> As specified in Intel's SDM, do not allow the L1 hypervisor to launch >> an L2 guest with the VM-execution controls for "unrestricted guest" or >> "mode-based execute control for EPT" set and the VM-execution control >> for "enable EPT" clear. >> >> Note that the VM-execution control for "mode-based execute control for >> EPT" is not yet virtualized by kvm. >> >> Reported-by: Andrew Thornton <andrewth@google.com> >> Signed-off-by: Jim Mattson <jmattson@google.com> >> Reviewed-by: Peter Shier <pshier@google.com> >> --- >> arch/x86/include/asm/vmx.h | 1 + >> arch/x86/kvm/vmx.c | 13 +++++++++++++ >> 2 files changed, 14 insertions(+) >> >> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h >> index 9527ba5d62da..665632a4b54b 100644 >> --- a/arch/x86/include/asm/vmx.h >> +++ b/arch/x86/include/asm/vmx.h >> @@ -78,6 +78,7 @@ >> #define SECONDARY_EXEC_RDSEED_EXITING 0x00010000 >> #define SECONDARY_EXEC_ENABLE_PML 0x00020000 >> #define SECONDARY_EXEC_XSAVES 0x00100000 >> +#define SECONDARY_EXEC_MODE_BASED_EPT_EXEC 0x00400000 >> #define SECONDARY_EXEC_TSC_SCALING 0x02000000 >> #define PIN_BASED_EXT_INTR_MASK 0x00000001 >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index 1d26f3c4985b..2bf990b5848f 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -11719,6 +11719,16 @@ static int nested_vmx_check_pml_controls(struct >> kvm_vcpu *vcpu, >> return 0; >> } >> +static int nested_vmx_check_ept_enable(struct kvm_vcpu *vcpu, >> + struct vmcs12 *vmcs12) >> +{ >> + if ((nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) || >> + nested_cpu_has2(vmcs12, SECONDARY_EXEC_MODE_BASED_EPT_EXEC)) >> && >> + !nested_cpu_has_ept(vmcs12)) >> + return -EINVAL; >> + return 0; >> +} >> + >> static int nested_vmx_check_shadow_vmcs_controls(struct kvm_vcpu *vcpu, >> struct vmcs12 *vmcs12) >> { >> @@ -12339,6 +12349,9 @@ static int check_vmentry_prereqs(struct kvm_vcpu >> *vcpu, struct vmcs12 *vmcs12) >> if (nested_vmx_check_pml_controls(vcpu, vmcs12)) >> return VMXERR_ENTRY_INVALID_CONTROL_FIELD; >> + if (nested_vmx_check_ept_enable(vcpu, vmcs12)) >> + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; >> + >> if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12)) >> return VMXERR_ENTRY_INVALID_CONTROL_FIELD; >> > > Reviewed-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> Ping?
On Thu, 2018-08-30 at 16:05 -0700, Jim Mattson wrote: > As specified in Intel's SDM, do not allow the L1 hypervisor to launch > an L2 guest with the VM-execution controls for "unrestricted guest" or > "mode-based execute control for EPT" set and the VM-execution control > for "enable EPT" clear. > > Note that the VM-execution control for "mode-based execute control for > EPT" is not yet virtualized by kvm. The EPT violation #VE control also falls into this category. > Reported-by: Andrew Thornton <andrewth@google.com> > Signed-off-by: Jim Mattson <jmattson@google.com> > Reviewed-by: Peter Shier <pshier@google.com> > --- > arch/x86/include/asm/vmx.h | 1 + > arch/x86/kvm/vmx.c | 13 +++++++++++++ > 2 files changed, 14 insertions(+) > > diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h > index 9527ba5d62da..665632a4b54b 100644 > --- a/arch/x86/include/asm/vmx.h > +++ b/arch/x86/include/asm/vmx.h > @@ -78,6 +78,7 @@ > #define SECONDARY_EXEC_RDSEED_EXITING 0x00010000 > #define SECONDARY_EXEC_ENABLE_PML 0x00020000 > #define SECONDARY_EXEC_XSAVES 0x00100000 > +#define SECONDARY_EXEC_MODE_BASED_EPT_EXEC 0x00400000 > #define SECONDARY_EXEC_TSC_SCALING 0x02000000 > > #define PIN_BASED_EXT_INTR_MASK 0x00000001 > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 1d26f3c4985b..2bf990b5848f 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -11719,6 +11719,16 @@ static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu, > return 0; > } > > +static int nested_vmx_check_ept_enable(struct kvm_vcpu *vcpu, > + struct vmcs12 *vmcs12) nested_vmx_check_ept_enable() is a bit weird, it makes it sound like we're always requiring EPT to be enabled. And checking for EPT being enabled isn't unique to this function, e.g. EPT is also required for PML and is checked by nested_vmx_check_pml_controls(). That being said, I actually like the approach of checking all controls that depend EPT in a single function, i.e. what about moving the EPT part of the PML check to this function? > +{ > + if ((nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) || > + nested_cpu_has2(vmcs12, SECONDARY_EXEC_MODE_BASED_EPT_EXEC)) && > + !nested_cpu_has_ept(vmcs12)) > + return -EINVAL; > + return 0; > +} What about borrowing the style of nested_vmx_check_shadow_vmcs_controls() and returning 0 immediately if nested_cpu_has_ept() is true?! E.g. combining everything together... diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h index 9527ba5d62da..242c88ee48e9 100644 --- a/arch/x86/include/asm/vmx.h +++ b/arch/x86/include/asm/vmx.h @@ -77,7 +77,9 @@ #define SECONDARY_EXEC_ENCLS_EXITING 0x00008000 #define SECONDARY_EXEC_RDSEED_EXITING 0x00010000 #define SECONDARY_EXEC_ENABLE_PML 0x00020000 +#define SECONDARY_EXEC_EPT_VIOLATION_VE 0x00040000 #define SECONDARY_EXEC_XSAVES 0x00100000 +#define SECONDARY_EXEC_MODE_BASED_EPT_EXEC 0x00400000 #define SECONDARY_EXEC_TSC_SCALING 0x02000000 #define PIN_BASED_EXT_INTR_MASK 0x00000001 diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 1d26f3c4985b..fd0321d813bc 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -11710,15 +11710,28 @@ static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu, int maxphyaddr = cpuid_maxphyaddr(vcpu); if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_PML)) { - if (!nested_cpu_has_ept(vmcs12) || - !IS_ALIGNED(address, 4096) || - address >> maxphyaddr) + if (!IS_ALIGNED(address, 4096) || address >> maxphyaddr) return -EINVAL; } return 0; } +static int nested_vmx_check_ept_dependants(struct kvm_vcpu *vcpu, + struct vmcs12 *vmcs12) +{ + if (nested_cpu_has_ept(vmcs12)) + return 0; + + if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) || + nested_cpu_has2(vmcs12, SECONDARY_EXEC_MODE_BASED_EPT_EXEC) || + nested_cpu_has2(vmcs12, SECONDARY_EXEC_EPT_VIOLATION_VE) || + nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENABLE_PML)) + return -EINVAL; + + return 0; +} + static int nested_vmx_check_shadow_vmcs_controls(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) { @@ -12336,6 +12349,9 @@ static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) if (nested_vmx_check_msr_switch_controls(vcpu, vmcs12)) return VMXERR_ENTRY_INVALID_CONTROL_FIELD; + if (nested_vmx_check_ept_dependants(vcpu, vmcs12)) + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; + if (nested_vmx_check_pml_controls(vcpu, vmcs12)) return VMXERR_ENTRY_INVALID_CONTROL_FIELD; -- > static int nested_vmx_check_shadow_vmcs_controls(struct kvm_vcpu *vcpu, > struct vmcs12 *vmcs12) > { > @@ -12339,6 +12349,9 @@ static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) > if (nested_vmx_check_pml_controls(vcpu, vmcs12)) > return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > > + if (nested_vmx_check_ept_enable(vcpu, vmcs12)) > + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > + > if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12)) > return VMXERR_ENTRY_INVALID_CONTROL_FIELD; >
On Mon, Sep 24, 2018 at 6:31 AM, Sean Christopherson <sean.j.christopherson@intel.com> wrote: > On Thu, 2018-08-30 at 16:05 -0700, Jim Mattson wrote: >> As specified in Intel's SDM, do not allow the L1 hypervisor to launch >> an L2 guest with the VM-execution controls for "unrestricted guest" or >> "mode-based execute control for EPT" set and the VM-execution control >> for "enable EPT" clear. >> >> Note that the VM-execution control for "mode-based execute control for >> EPT" is not yet virtualized by kvm. > > The EPT violation #VE control also falls into this category. Though that makes sense to me, the "enable EPT" requirement isn't documented. > nested_vmx_check_ept_enable() is a bit weird, it makes it sound like > we're always requiring EPT to be enabled. And checking for EPT being > enabled isn't unique to this function, e.g. EPT is also required for > PML and is checked by nested_vmx_check_pml_controls(). And EPTP switching... I was just trying to implement one bullet point from section 26.2.1.1 of the SDM: o If either the “unrestricted guest” VM-execution control or the “mode-based execute control for EPT” VM-execution control is 1, the “enable EPT” VM-execution control must also be 1. > That being said, I actually like the approach of checking all controls > that depend EPT in a single function, i.e. what about moving the EPT > part of the PML check to this function? I think that mixing the checks for multiple bullet points from the SDM makes it harder to verify that all of the checks have been properly implemented. However, the obvious "nested_vmx_check_unrestricted_guest_or_mode_based_exec_controls" seems a bit long. I'd rather split one bullet point into multiple checks than merge portions of multiple bullet points into a single check. Maybe nested_vmx_check_unrestricted_guest_controls and nested_vmx_check_mode_based_exec_controls? > What about borrowing the style of nested_vmx_check_shadow_vmcs_controls() > and returning 0 immediately if nested_cpu_has_ept() is true?! Sure.
On Mon, 2018-09-24 at 09:26 -0700, Jim Mattson wrote: > On Mon, Sep 24, 2018 at 6:31 AM, Sean Christopherson > <sean.j.christopherson@intel.com> wrote: > > > > On Thu, 2018-08-30 at 16:05 -0700, Jim Mattson wrote: > > > > > > As specified in Intel's SDM, do not allow the L1 hypervisor to launch > > > an L2 guest with the VM-execution controls for "unrestricted guest" or > > > "mode-based execute control for EPT" set and the VM-execution control > > > for "enable EPT" clear. > > > > > > Note that the VM-execution control for "mode-based execute control for > > > EPT" is not yet virtualized by kvm. > > The EPT violation #VE control also falls into this category. > Though that makes sense to me, the "enable EPT" requirement isn't documented. Huh. I didn't actually check anything, I just assumed this was the case. The SDM appears to be correct, I can't find anything that suggests that setting EPT_VIOLATION_VE requires EPT to be enabled, which is hilarious. > > nested_vmx_check_ept_enable() is a bit weird, it makes it sound like > > we're always requiring EPT to be enabled. And checking for EPT being > > enabled isn't unique to this function, e.g. EPT is also required for > > PML and is checked by nested_vmx_check_pml_controls(). > And EPTP switching... > > I was just trying to implement one bullet point from section 26.2.1.1 > of the SDM: > > o If either the “unrestricted guest” VM-execution control or the > “mode-based execute control for EPT” VM-execution control is 1, the > “enable EPT” VM-execution control must also be 1. > > > > > That being said, I actually like the approach of checking all controls > > that depend EPT in a single function, i.e. what about moving the EPT > > part of the PML check to this function? > I think that mixing the checks for multiple bullet points from the SDM > makes it harder to verify that all of the checks have been properly > implemented. However, the obvious > "nested_vmx_check_unrestricted_guest_or_mode_based_exec_controls" > seems a bit long. I'd rather split one bullet point into multiple > checks than merge portions of multiple bullet points into a single > check. Maybe nested_vmx_check_unrestricted_guest_controls and > nested_vmx_check_mode_based_exec_controls? Makes sense, I didn't realize the SDM put those under a single bullet. I'd vote to separate the two checks, they're completely unrelated other than the EPT dependency. I'm guessing they're lumped together in the SDM only because they don't have any other consistency checks. > > What about borrowing the style of nested_vmx_check_shadow_vmcs_controls() > > and returning 0 immediately if nested_cpu_has_ept() is true?! > Sure. Feel free to ignore this, it doesn't make as much sense if each check is in a separate function. I might argue the other way in that case :)
diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h index 9527ba5d62da..665632a4b54b 100644 --- a/arch/x86/include/asm/vmx.h +++ b/arch/x86/include/asm/vmx.h @@ -78,6 +78,7 @@ #define SECONDARY_EXEC_RDSEED_EXITING 0x00010000 #define SECONDARY_EXEC_ENABLE_PML 0x00020000 #define SECONDARY_EXEC_XSAVES 0x00100000 +#define SECONDARY_EXEC_MODE_BASED_EPT_EXEC 0x00400000 #define SECONDARY_EXEC_TSC_SCALING 0x02000000 #define PIN_BASED_EXT_INTR_MASK 0x00000001 diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 1d26f3c4985b..2bf990b5848f 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -11719,6 +11719,16 @@ static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu, return 0; } +static int nested_vmx_check_ept_enable(struct kvm_vcpu *vcpu, + struct vmcs12 *vmcs12) +{ + if ((nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) || + nested_cpu_has2(vmcs12, SECONDARY_EXEC_MODE_BASED_EPT_EXEC)) && + !nested_cpu_has_ept(vmcs12)) + return -EINVAL; + return 0; +} + static int nested_vmx_check_shadow_vmcs_controls(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) { @@ -12339,6 +12349,9 @@ static int check_vmentry_prereqs(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12) if (nested_vmx_check_pml_controls(vcpu, vmcs12)) return VMXERR_ENTRY_INVALID_CONTROL_FIELD; + if (nested_vmx_check_ept_enable(vcpu, vmcs12)) + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; + if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12)) return VMXERR_ENTRY_INVALID_CONTROL_FIELD;