| Message ID | 20210830125539.1768833-6-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: few more SMM fixes | expand |
On Mon, Aug 30, 2021, Maxim Levitsky wrote: > It is possible that when non root mode is entered via special entry > (!from_vmentry), that is from SMM or from loading the nested state, > the L2 state could be invalid in regard to non unrestricted guest mode, > but later it can become valid. > > (for example when RSM emulation restores segment registers from SMRAM) > > Thus delay the check to VM entry, where we will check this and fail. And then do what? Won't invalidate state send KVM into handle_invalid_guest_state(), which we very much don't want to do for L2? E.g. this is meant to fire, but won't because nested_run_pending is false for the !from_vmentry paths. /* * We should never reach this point with a pending nested VM-Enter, and * more specifically emulation of L2 due to invalid guest state (see * below) should never happen as that means we incorrectly allowed a * nested VM-Enter with an invalid vmcs12. */ if (KVM_BUG_ON(vmx->nested.nested_run_pending, vcpu->kvm)) return -EIO; > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > arch/x86/kvm/vmx/nested.c | 7 ++++++- > arch/x86/kvm/vmx/vmx.c | 5 ++++- > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > index bc6327950657..1a05ae83dae5 100644 > --- a/arch/x86/kvm/vmx/nested.c > +++ b/arch/x86/kvm/vmx/nested.c > @@ -2546,8 +2546,13 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, > * Guest state is invalid and unrestricted guest is disabled, > * which means L1 attempted VMEntry to L2 with invalid state. > * Fail the VMEntry. > + * > + * However when force loading the guest state (SMM exit or > + * loading nested state after migration, it is possible to > + * have invalid guest state now, which will be later fixed by > + * restoring L2 register state > */ > - if (CC(!vmx_guest_state_valid(vcpu))) { > + if (CC(from_vmentry && !vmx_guest_state_valid(vcpu))) { > *entry_failure_code = ENTRY_FAIL_DEFAULT; > return -EINVAL; > } > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 1c113195c846..02d061f5956a 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -6624,7 +6624,10 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu) > * consistency check VM-Exit due to invalid guest state and bail. > */ > if (unlikely(vmx->emulation_required)) { > - vmx->fail = 0; > + > + /* We don't emulate invalid state of a nested guest */ > + vmx->fail = is_guest_mode(vcpu); This is contradictory and wrong. (a) it's impossible to have both a VM-Fail and VM-Exit, (b) vmcs.EXIT_REASON is not modified on VM-Fail, and (c) emulation_required refers to guest state and guest state checks are always VM-Exits, not VM-Fails. I don't understand this change, AFAICT vmx->fail won't actually be consumed as either the above KVM_BUG_ON() will be hit or KVM will incorrectly emulate L2 state. > + > vmx->exit_reason.full = EXIT_REASON_INVALID_STATE; > vmx->exit_reason.failed_vmentry = 1; > kvm_register_mark_available(vcpu, VCPU_EXREG_EXIT_INFO_1); > -- > 2.26.3 >
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index bc6327950657..1a05ae83dae5 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -2546,8 +2546,13 @@ static int prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12, * Guest state is invalid and unrestricted guest is disabled, * which means L1 attempted VMEntry to L2 with invalid state. * Fail the VMEntry. + * + * However when force loading the guest state (SMM exit or + * loading nested state after migration, it is possible to + * have invalid guest state now, which will be later fixed by + * restoring L2 register state */ - if (CC(!vmx_guest_state_valid(vcpu))) { + if (CC(from_vmentry && !vmx_guest_state_valid(vcpu))) { *entry_failure_code = ENTRY_FAIL_DEFAULT; return -EINVAL; } diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 1c113195c846..02d061f5956a 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -6624,7 +6624,10 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu) * consistency check VM-Exit due to invalid guest state and bail. */ if (unlikely(vmx->emulation_required)) { - vmx->fail = 0; + + /* We don't emulate invalid state of a nested guest */ + vmx->fail = is_guest_mode(vcpu); + vmx->exit_reason.full = EXIT_REASON_INVALID_STATE; vmx->exit_reason.failed_vmentry = 1; kvm_register_mark_available(vcpu, VCPU_EXREG_EXIT_INFO_1);
It is possible that when non root mode is entered via special entry (!from_vmentry), that is from SMM or from loading the nested state, the L2 state could be invalid in regard to non unrestricted guest mode, but later it can become valid. (for example when RSM emulation restores segment registers from SMRAM) Thus delay the check to VM entry, where we will check this and fail. Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- arch/x86/kvm/vmx/nested.c | 7 ++++++- arch/x86/kvm/vmx/vmx.c | 5 ++++- 2 files changed, 10 insertions(+), 2 deletions(-)