| Message ID | 20200320212833.3507-3-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: TLB flushing fixes and enhancements | expand |
Sean Christopherson <sean.j.christopherson@intel.com> writes: > Signal VM-Fail for the single-context variant of INVEPT if the specified > EPTP is invalid. Per the INEVPT pseudocode in Intel's SDM, it's subject > to the standard EPT checks: > > If VM entry with the "enable EPT" VM execution control set to 1 would > fail due to the EPTP value then VMfail(Invalid operand to INVEPT/INVVPID); > > Fixes: bfd0a56b90005 ("nEPT: Nested INVEPT") > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > arch/x86/kvm/vmx/nested.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index 8578513907d7..f3774cef4fd4 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -5156,8 +5156,12 @@ static int handle_invept(struct kvm_vcpu *vcpu) > } > > switch (type) { > - case VMX_EPT_EXTENT_GLOBAL: > case VMX_EPT_EXTENT_CONTEXT: > + if (!nested_vmx_check_eptp(vcpu, operand.eptp)) > + return nested_vmx_failValid(vcpu, > + VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID); I was going to ask "and we don't seem to check that current nested VMPTR is valid, how can we know that nested_vmx_failValid() is the right VMfail() to use" but then I checked our nested_vmx_failValid() and there is a fallback there: if (vmx->nested.current_vmptr == -1ull && !vmx->nested.hv_evmcs) return nested_vmx_failInvalid(vcpu); so this is a non-issue. My question, however, transforms into "would it make sense to introduce nested_vmx_fail() implementing the logic from SDM: VMfail(ErrorNumber): IF VMCS pointer is valid THEN VMfailValid(ErrorNumber); ELSE VMfailInvalid; FI; to assist an innocent reader of the code?" > + fallthrough; > + case VMX_EPT_EXTENT_GLOBAL: > /* > * TODO: Sync the necessary shadow EPT roots here, rather than > * at the next emulated VM-entry. Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
On Mon, Mar 23, 2020 at 03:51:17PM +0100, Vitaly Kuznetsov wrote: > Sean Christopherson <sean.j.christopherson@intel.com> writes: > > > Signal VM-Fail for the single-context variant of INVEPT if the specified > > EPTP is invalid. Per the INEVPT pseudocode in Intel's SDM, it's subject > > to the standard EPT checks: > > > > If VM entry with the "enable EPT" VM execution control set to 1 would > > fail due to the EPTP value then VMfail(Invalid operand to INVEPT/INVVPID); > > > > Fixes: bfd0a56b90005 ("nEPT: Nested INVEPT") > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > > --- > > arch/x86/kvm/vmx/nested.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > > index 8578513907d7..f3774cef4fd4 100644 > > --- a/arch/x86/kvm/vmx/nested.c > > +++ b/arch/x86/kvm/vmx/nested.c > > @@ -5156,8 +5156,12 @@ static int handle_invept(struct kvm_vcpu *vcpu) > > } > > > > switch (type) { > > - case VMX_EPT_EXTENT_GLOBAL: > > case VMX_EPT_EXTENT_CONTEXT: > > + if (!nested_vmx_check_eptp(vcpu, operand.eptp)) > > + return nested_vmx_failValid(vcpu, > > + VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID); > > I was going to ask "and we don't seem to check that current nested VMPTR > is valid, how can we know that nested_vmx_failValid() is the right > VMfail() to use" but then I checked our nested_vmx_failValid() and there > is a fallback there: > > if (vmx->nested.current_vmptr == -1ull && !vmx->nested.hv_evmcs) > return nested_vmx_failInvalid(vcpu); > > so this is a non-issue. My question, however, transforms into "would it > make sense to introduce nested_vmx_fail() implementing the logic from > SDM: > > VMfail(ErrorNumber): > IF VMCS pointer is valid > THEN VMfailValid(ErrorNumber); > ELSE VMfailInvalid; > FI; > Hmm, I wouldn't be opposed to such a wrapper. It would pair with nested_vmx_succeed(). > > > + fallthrough; > > + case VMX_EPT_EXTENT_GLOBAL: > > /* > > * TODO: Sync the necessary shadow EPT roots here, rather than > > * at the next emulated VM-entry. > > Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > -- > Vitaly >
On 23/03/20 16:45, Sean Christopherson wrote: >> My question, however, transforms into "would it >> make sense to introduce nested_vmx_fail() implementing the logic from >> SDM: >> >> VMfail(ErrorNumber): >> IF VMCS pointer is valid >> THEN VMfailValid(ErrorNumber); >> ELSE VMfailInvalid; >> FI; >> > Hmm, I wouldn't be opposed to such a wrapper. It would pair with > nested_vmx_succeed(). > Neither would I. Paolo
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 8578513907d7..f3774cef4fd4 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -5156,8 +5156,12 @@ static int handle_invept(struct kvm_vcpu *vcpu) } switch (type) { - case VMX_EPT_EXTENT_GLOBAL: case VMX_EPT_EXTENT_CONTEXT: + if (!nested_vmx_check_eptp(vcpu, operand.eptp)) + return nested_vmx_failValid(vcpu, + VMXERR_INVALID_OPERAND_TO_INVEPT_INVVPID); + fallthrough; + case VMX_EPT_EXTENT_GLOBAL: /* * TODO: Sync the necessary shadow EPT roots here, rather than * at the next emulated VM-entry.
Signal VM-Fail for the single-context variant of INVEPT if the specified EPTP is invalid. Per the INEVPT pseudocode in Intel's SDM, it's subject to the standard EPT checks: If VM entry with the "enable EPT" VM execution control set to 1 would fail due to the EPTP value then VMfail(Invalid operand to INVEPT/INVVPID); Fixes: bfd0a56b90005 ("nEPT: Nested INVEPT") Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- arch/x86/kvm/vmx/nested.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)