linux-kernel - Re: [PATCH v2 5/6] KVM: nVMX: don't fail nested VM entry on invalid guest state if !from

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <YWDWPbgJik5spT1D@google.com>
Date:   Fri, 8 Oct 2021 23:37:33 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Maxim Levitsky <mlevitsk@...hat.com>
Cc:     kvm@...r.kernel.org, Jim Mattson <jmattson@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        Ingo Molnar <mingo@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" 
        <linux-kernel@...r.kernel.org>, Joerg Roedel <joro@...tes.org>,
        Wanpeng Li <wanpengli@...cent.com>,
        "H. Peter Anvin" <hpa@...or.com>, Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH v2 5/6] KVM: nVMX: don't fail nested VM entry on invalid
 guest state if !from_vmentry

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@...hat.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
>