| Message ID | 20180924180543.227780-1-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] KVM: nVMX: Unrestricted guest mode requires EPT | expand |
On Mon, 2018-09-24 at 11: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. > > Reported-by: Andrew Thornton <andrewth@google.com> > Signed-off-by: Jim Mattson <jmattson@google.com> > Reviewed-by: Peter Shier <pshier@google.com> Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
On Tue, 25 Sep 2018 at 02:08, Jim Mattson <jmattson@google.com> 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> Reviewed-by: Wanpeng Li <wanpengli@tencent.com> > --- > arch/x86/include/asm/vmx.h | 1 + > arch/x86/kvm/vmx.c | 24 ++++++++++++++++++++++++ > 2 files changed, 25 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 06412ba46aa3..b78607dd113c 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -11775,6 +11775,24 @@ static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu, > return 0; > } > > +static int nested_vmx_check_unrestricted_guest_controls(struct kvm_vcpu *vcpu, > + struct vmcs12 *vmcs12) > +{ > + if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) && > + !nested_cpu_has_ept(vmcs12)) > + return -EINVAL; > + return 0; > +} > + > +static int nested_vmx_check_mode_based_ept_exec_controls(struct kvm_vcpu *vcpu, > + struct vmcs12 *vmcs12) > +{ > + if (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) > { > @@ -12397,6 +12415,12 @@ 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_unrestricted_guest_controls(vcpu, vmcs12)) > + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > + > + if (nested_vmx_check_mode_based_ept_exec_controls(vcpu, vmcs12)) > + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > + > if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12)) > return VMXERR_ENTRY_INVALID_CONTROL_FIELD; > > -- > 2.19.0.444.g18242da7ef-goog >
On Mon, Sep 24, 2018 at 7:39 PM, Wanpeng Li <kernellwp@gmail.com> wrote: > On Tue, 25 Sep 2018 at 02:08, Jim Mattson <jmattson@google.com> 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> > > Reviewed-by: Wanpeng Li <wanpengli@tencent.com> > >> --- >> arch/x86/include/asm/vmx.h | 1 + >> arch/x86/kvm/vmx.c | 24 ++++++++++++++++++++++++ >> 2 files changed, 25 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 06412ba46aa3..b78607dd113c 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -11775,6 +11775,24 @@ static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu, >> return 0; >> } >> >> +static int nested_vmx_check_unrestricted_guest_controls(struct kvm_vcpu *vcpu, >> + struct vmcs12 *vmcs12) >> +{ >> + if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) && >> + !nested_cpu_has_ept(vmcs12)) >> + return -EINVAL; >> + return 0; >> +} >> + >> +static int nested_vmx_check_mode_based_ept_exec_controls(struct kvm_vcpu *vcpu, >> + struct vmcs12 *vmcs12) >> +{ >> + if (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) >> { >> @@ -12397,6 +12415,12 @@ 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_unrestricted_guest_controls(vcpu, vmcs12)) >> + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; >> + >> + if (nested_vmx_check_mode_based_ept_exec_controls(vcpu, vmcs12)) >> + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; >> + >> if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12)) >> return VMXERR_ENTRY_INVALID_CONTROL_FIELD; >> >> -- >> 2.19.0.444.g18242da7ef-goog >> Ping?
On Fri, Oct 12, 2018 at 9:32 AM, Jim Mattson <jmattson@google.com> wrote: > On Mon, Sep 24, 2018 at 7:39 PM, Wanpeng Li <kernellwp@gmail.com> wrote: >> On Tue, 25 Sep 2018 at 02:08, Jim Mattson <jmattson@google.com> 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> >> >> Reviewed-by: Wanpeng Li <wanpengli@tencent.com> >> >>> --- >>> arch/x86/include/asm/vmx.h | 1 + >>> arch/x86/kvm/vmx.c | 24 ++++++++++++++++++++++++ >>> 2 files changed, 25 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 06412ba46aa3..b78607dd113c 100644 >>> --- a/arch/x86/kvm/vmx.c >>> +++ b/arch/x86/kvm/vmx.c >>> @@ -11775,6 +11775,24 @@ static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu, >>> return 0; >>> } >>> >>> +static int nested_vmx_check_unrestricted_guest_controls(struct kvm_vcpu *vcpu, >>> + struct vmcs12 *vmcs12) >>> +{ >>> + if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) && >>> + !nested_cpu_has_ept(vmcs12)) >>> + return -EINVAL; >>> + return 0; >>> +} >>> + >>> +static int nested_vmx_check_mode_based_ept_exec_controls(struct kvm_vcpu *vcpu, >>> + struct vmcs12 *vmcs12) >>> +{ >>> + if (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) >>> { >>> @@ -12397,6 +12415,12 @@ 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_unrestricted_guest_controls(vcpu, vmcs12)) >>> + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; >>> + >>> + if (nested_vmx_check_mode_based_ept_exec_controls(vcpu, vmcs12)) >>> + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; >>> + >>> if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12)) >>> return VMXERR_ENTRY_INVALID_CONTROL_FIELD; >>> >>> -- >>> 2.19.0.444.g18242da7ef-goog >>> > > Ping? Should I construe the continued silence as rejection?
> On 12 Nov 2018, at 22:39, Jim Mattson <jmattson@google.com> wrote: > > On Fri, Oct 12, 2018 at 9:32 AM, Jim Mattson <jmattson@google.com> wrote: >> On Mon, Sep 24, 2018 at 7:39 PM, Wanpeng Li <kernellwp@gmail.com> wrote: >>> On Tue, 25 Sep 2018 at 02:08, Jim Mattson <jmattson@google.com> 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> >>> >>> Reviewed-by: Wanpeng Li <wanpengli@tencent.com> >>> >>>> --- >>>> arch/x86/include/asm/vmx.h | 1 + >>>> arch/x86/kvm/vmx.c | 24 ++++++++++++++++++++++++ >>>> 2 files changed, 25 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 06412ba46aa3..b78607dd113c 100644 >>>> --- a/arch/x86/kvm/vmx.c >>>> +++ b/arch/x86/kvm/vmx.c >>>> @@ -11775,6 +11775,24 @@ static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu, >>>> return 0; >>>> } >>>> >>>> +static int nested_vmx_check_unrestricted_guest_controls(struct kvm_vcpu *vcpu, >>>> + struct vmcs12 *vmcs12) >>>> +{ >>>> + if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) && >>>> + !nested_cpu_has_ept(vmcs12)) >>>> + return -EINVAL; >>>> + return 0; >>>> +} >>>> + >>>> +static int nested_vmx_check_mode_based_ept_exec_controls(struct kvm_vcpu *vcpu, >>>> + struct vmcs12 *vmcs12) >>>> +{ >>>> + if (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) >>>> { >>>> @@ -12397,6 +12415,12 @@ 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_unrestricted_guest_controls(vcpu, vmcs12)) >>>> + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; >>>> + >>>> + if (nested_vmx_check_mode_based_ept_exec_controls(vcpu, vmcs12)) >>>> + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; >>>> + >>>> if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12)) >>>> return VMXERR_ENTRY_INVALID_CONTROL_FIELD; >>>> >>>> -- >>>> 2.19.0.444.g18242da7ef-goog >>>> >> >> Ping? > > Should I construe the continued silence as rejection? LOL. I believe this was just missed. Looking at the code, you can at least have my approval: Reviewed-by: Liran Alon <liran.alon@oracle.com> P.S: I would have maybe also gather together all the EPT related controls pre checks (besides the one related to eptp-switching) on a single nested_vmx_check_ept_related_controls(). But that’s a matter of taste.
On 12/11/18 21:39, Jim Mattson wrote: >> Ping? > Should I construe the continued silence as rejection? No, just maintainer sloppiness. But I'm alive. Paolo
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 06412ba46aa3..b78607dd113c 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -11775,6 +11775,24 @@ static int nested_vmx_check_pml_controls(struct kvm_vcpu *vcpu, return 0; } +static int nested_vmx_check_unrestricted_guest_controls(struct kvm_vcpu *vcpu, + struct vmcs12 *vmcs12) +{ + if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_UNRESTRICTED_GUEST) && + !nested_cpu_has_ept(vmcs12)) + return -EINVAL; + return 0; +} + +static int nested_vmx_check_mode_based_ept_exec_controls(struct kvm_vcpu *vcpu, + struct vmcs12 *vmcs12) +{ + if (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) { @@ -12397,6 +12415,12 @@ 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_unrestricted_guest_controls(vcpu, vmcs12)) + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; + + if (nested_vmx_check_mode_based_ept_exec_controls(vcpu, vmcs12)) + return VMXERR_ENTRY_INVALID_CONTROL_FIELD; + if (nested_vmx_check_shadow_vmcs_controls(vcpu, vmcs12)) return VMXERR_ENTRY_INVALID_CONTROL_FIELD;