| Message ID | 1520877373-5986-1-git-send-email-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2018-03-12 10:56-0700, Sean Christopherson: > Fail a nested VMEntry with EXIT_REASON_INVALID_STATE if L2 guest state > is invalid, i.e. vmcs12 contained invalid guest state, and unrestricted > guest is disabled in L0 (and by extension disabled in L1). > > WARN_ON_ONCE in handle_invalid_guest_state() if we're attempting to > emulate L2, i.e. nested_run_pending is true, to aid debug in the > (hopefully unlikely) scenario that we somehow skip the nested VMEntry > consistency check, e.g. due to a L0 bug. > > Note: KVM relies on hardware to detect the scenario where unrestricted > guest is enabled in L0 but disabled in L1 and vmcs12 contains invalid > guest state, i.e. checking emulation_required in prepare_vmcs02 is > required only to handle the case were unrestricted guest is disabled > in L0 since L0 never actually attempts VMLAUNCH/VMRESUME with vmcs02. Oh, right, it is an L0 bug if we ever get to emulate the state. > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > v2: Rework the patch to fix the underlying bug of not catching nested > VMEntry with invalid guest state and unrestricted guest disabled. > > v1: https://patchwork.kernel.org/patch/10259397 > > arch/x86/kvm/vmx.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 5912148..e4858f6 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -6823,6 +6823,13 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu) > bool intr_window_requested; > unsigned count = 130; > > + /* > + * We should never reach the point where we are emulating L2 > + * due to invalid guest state as that means we incorrectly > + * allowed a nested VMEntry with an invalid vmcs12. > + */ > + WARN_ON_ONCE(vmx->emulation_required && vmx->nested.nested_run_pending); > + > cpu_exec_ctrl = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL); > intr_window_requested = cpu_exec_ctrl & CPU_BASED_VIRTUAL_INTR_PENDING; > > @@ -10945,6 +10952,14 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, > vmx->nested.dirty_vmcs12 = false; > } > > + /* > + * Guest state is invalid and unrestricted guest is disabled, > + * which means L1 attempted VMEntry to L2 with invalid state. > + * Fail the VMEntry. > + */ > + if (vmx->emulation_required) We should do "*entry_failure_code = ENTRY_FAIL_DEFAULT" to avoid leaking the L0 stack to L1. With that, Reviewed-by: Radim Krčmář <rkrcmar@redhat.com> thanks. > + return 1; > + > /* Shadow page tables on either EPT or shadow page tables. */ > if (nested_vmx_load_cr3(vcpu, vmcs12->guest_cr3, nested_cpu_has_ept(vmcs12), > entry_failure_code)) > -- > 2.7.4 >
On 12/03/2018 18:56, Sean Christopherson wrote: > Fail a nested VMEntry with EXIT_REASON_INVALID_STATE if L2 guest state > is invalid, i.e. vmcs12 contained invalid guest state, and unrestricted > guest is disabled in L0 (and by extension disabled in L1). > > WARN_ON_ONCE in handle_invalid_guest_state() if we're attempting to > emulate L2, i.e. nested_run_pending is true, to aid debug in the > (hopefully unlikely) scenario that we somehow skip the nested VMEntry > consistency check, e.g. due to a L0 bug. > > Note: KVM relies on hardware to detect the scenario where unrestricted > guest is enabled in L0 but disabled in L1 and vmcs12 contains invalid > guest state, i.e. checking emulation_required in prepare_vmcs02 is > required only to handle the case were unrestricted guest is disabled > in L0 since L0 never actually attempts VMLAUNCH/VMRESUME with vmcs02. > > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > v2: Rework the patch to fix the underlying bug of not catching nested > VMEntry with invalid guest state and unrestricted guest disabled. > > v1: https://patchwork.kernel.org/patch/10259397 > > arch/x86/kvm/vmx.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index 5912148..e4858f6 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -6823,6 +6823,13 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu) > bool intr_window_requested; > unsigned count = 130; > > + /* > + * We should never reach the point where we are emulating L2 > + * due to invalid guest state as that means we incorrectly > + * allowed a nested VMEntry with an invalid vmcs12. > + */ > + WARN_ON_ONCE(vmx->emulation_required && vmx->nested.nested_run_pending); > + > cpu_exec_ctrl = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL); > intr_window_requested = cpu_exec_ctrl & CPU_BASED_VIRTUAL_INTR_PENDING; > > @@ -10945,6 +10952,14 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, > vmx->nested.dirty_vmcs12 = false; > } > > + /* > + * Guest state is invalid and unrestricted guest is disabled, > + * which means L1 attempted VMEntry to L2 with invalid state. > + * Fail the VMEntry. > + */ > + if (vmx->emulation_required) > + return 1; > + > /* Shadow page tables on either EPT or shadow page tables. */ > if (nested_vmx_load_cr3(vcpu, vmcs12->guest_cr3, nested_cpu_has_ept(vmcs12), > entry_failure_code)) > Queued, thanks. Paolo
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index 5912148..e4858f6 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -6823,6 +6823,13 @@ static int handle_invalid_guest_state(struct kvm_vcpu *vcpu) bool intr_window_requested; unsigned count = 130; + /* + * We should never reach the point where we are emulating L2 + * due to invalid guest state as that means we incorrectly + * allowed a nested VMEntry with an invalid vmcs12. + */ + WARN_ON_ONCE(vmx->emulation_required && vmx->nested.nested_run_pending); + cpu_exec_ctrl = vmcs_read32(CPU_BASED_VM_EXEC_CONTROL); intr_window_requested = cpu_exec_ctrl & CPU_BASED_VIRTUAL_INTR_PENDING; @@ -10945,6 +10952,14 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, vmx->nested.dirty_vmcs12 = false; } + /* + * Guest state is invalid and unrestricted guest is disabled, + * which means L1 attempted VMEntry to L2 with invalid state. + * Fail the VMEntry. + */ + if (vmx->emulation_required) + return 1; + /* Shadow page tables on either EPT or shadow page tables. */ if (nested_vmx_load_cr3(vcpu, vmcs12->guest_cr3, nested_cpu_has_ept(vmcs12), entry_failure_code))
Fail a nested VMEntry with EXIT_REASON_INVALID_STATE if L2 guest state is invalid, i.e. vmcs12 contained invalid guest state, and unrestricted guest is disabled in L0 (and by extension disabled in L1). WARN_ON_ONCE in handle_invalid_guest_state() if we're attempting to emulate L2, i.e. nested_run_pending is true, to aid debug in the (hopefully unlikely) scenario that we somehow skip the nested VMEntry consistency check, e.g. due to a L0 bug. Note: KVM relies on hardware to detect the scenario where unrestricted guest is enabled in L0 but disabled in L1 and vmcs12 contains invalid guest state, i.e. checking emulation_required in prepare_vmcs02 is required only to handle the case were unrestricted guest is disabled in L0 since L0 never actually attempts VMLAUNCH/VMRESUME with vmcs02. Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- v2: Rework the patch to fix the underlying bug of not catching nested VMEntry with invalid guest state and unrestricted guest disabled. v1: https://patchwork.kernel.org/patch/10259397 arch/x86/kvm/vmx.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)