linux-kernel - Re: [PATCH v2 1/2] KVM: nVMX: don't use vcpu->arch.efer when checking host state on nested state load

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <YZKB3Q1ZMsPD6hHl@google.com>
Date:   Mon, 15 Nov 2021 15:50:53 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Maxim Levitsky <mlevitsk@...hat.com>
Cc:     kvm@...r.kernel.org, Vitaly Kuznetsov <vkuznets@...hat.com>,
        Joerg Roedel <joro@...tes.org>,
        "maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)" <x86@...nel.org>,
        "open list:X86 ARCHITECTURE (32-BIT AND 64-BIT)" 
        <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Jim Mattson <jmattson@...gle.com>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH v2 1/2] KVM: nVMX: don't use vcpu->arch.efer when
 checking host state on nested state load

On Mon, Nov 15, 2021, Maxim Levitsky wrote:
> When loading the nested state, due to the way qemu loads
> the nested state, vcpu->arch.efer contains L2' IA32_EFER which
> can be completely different from L1's IA32_EFER, thus it is
> wrong to do consistency check of it vs the vmcs12 exit fields.

This is not sufficient justification.  It makes it sound like KVM is hacking
around a bug in its ABI, which it is not, but that fact is _very_ subtle.  The
"trust" blurb in bullet (3) in particular is misleading.

Instead, I would like something like:

  When loading nested state, don't use check vcpu->arch.efer to get the
  L1 host's 64-bit vs. 32-bit state and don't check it for consistency
  with respect to VM_EXIT_HOST_ADDR_SPACE_SIZE, as register state in vCPU
  may be stale when KVM_SET_NESTED_STATE is called and conceptually does
  not exist.  When the CPU is in non-root mode, i.e. when restoring L2
  state in KVM, there is no snapshot of L1 host state, it is (conditionally)
  loaded on VM-Exit.  E.g. EFER is either preserved on exit, loaded from the
  VMCS (vmcs12 in this case), or loaded from the MSR load list.

  Use vmcs12.VM_EXIT_HOST_ADDR_SPACE_SIZE to determine the target mode of
  the L1 host, as it is the source of truth in this case.  Perform the EFER
  vs. vmcs12.VM_EXIT_HOST_ADDR_SPACE_SIZE consistency check only on VM-Enter,
  as conceptually there's no "current" L1 EFER to check.

  Note, KVM still checks vmcs12.HOST_EFER for consistency if
  if vmcs12.VM_EXIT_LOAD_IA32_EFER is set, i.e. this skips only the check
  against current vCPU state, which does not exist, when loading nested state.

> To fix this
> 
> 1. Split the host state consistency check
> between current IA32_EFER.LMA and 'host address space' bit in VMCS12 into
> nested_vmx_check_address_state_size.
> 
> 2. Call this check only on a normal VM entry, while skipping this call
> on loading the nested state.
> 
> 3. Trust the 'host address space' bit to contain correct ia32e
> value on loading the nested state as it is the best value of
> it at that point.
> Still do a consistency check of it vs host_ia32_efer in vmcs12.
> 
> Suggested-by: Paolo Bonzini <pbonzini@...hat.com>
> Signed-off-by: Maxim Levitsky <mlevitsk@...hat.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index b4ee5e9f9e201..7b1d5510a7cdc 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -2866,6 +2866,17 @@ static int nested_vmx_check_controls(struct kvm_vcpu *vcpu,
>  	return 0;
>  }
>  
> +static int nested_vmx_check_address_state_size(struct kvm_vcpu *vcpu,
> +				       struct vmcs12 *vmcs12)

Bad indentation.

> +{
> +#ifdef CONFIG_X86_64
> +	if (CC(!!(vmcs12->vm_exit_controls & VM_EXIT_HOST_ADDR_SPACE_SIZE) !=
> +		!!(vcpu->arch.efer & EFER_LMA)))

Bad indentation.  The number of !'s is also unnecessary.  This also needs a comment
explaining why it's not included in the KVM_SET_NESTED_STATE path.

> +		return -EINVAL;
> +#endif
> +	return 0;
> +}
> +
>  static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
>  				       struct vmcs12 *vmcs12)
>  {
> @@ -2890,18 +2901,16 @@ static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
>  		return -EINVAL;
>  
>  #ifdef CONFIG_X86_64
> -	ia32e = !!(vcpu->arch.efer & EFER_LMA);
> +	ia32e = !!(vmcs12->vm_exit_controls & VM_EXIT_HOST_ADDR_SPACE_SIZE);


>  #else
>  	ia32e = false;
>  #endif
>  
>  	if (ia32e) {
> -		if (CC(!(vmcs12->vm_exit_controls & VM_EXIT_HOST_ADDR_SPACE_SIZE)) ||
> -		    CC(!(vmcs12->host_cr4 & X86_CR4_PAE)))
> +		if (CC(!(vmcs12->host_cr4 & X86_CR4_PAE)))
>  			return -EINVAL;
>  	} else {
> -		if (CC(vmcs12->vm_exit_controls & VM_EXIT_HOST_ADDR_SPACE_SIZE) ||
> -		    CC(vmcs12->vm_entry_controls & VM_ENTRY_IA32E_MODE) ||
> +		if (CC(vmcs12->vm_entry_controls & VM_ENTRY_IA32E_MODE) ||
>  		    CC(vmcs12->host_cr4 & X86_CR4_PCIDE) ||
>  		    CC((vmcs12->host_rip) >> 32))
>  			return -EINVAL;
> @@ -3571,6 +3580,9 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
>  	if (nested_vmx_check_controls(vcpu, vmcs12))
>  		return nested_vmx_fail(vcpu, VMXERR_ENTRY_INVALID_CONTROL_FIELD);
>  
> +	if (nested_vmx_check_address_state_size(vcpu, vmcs12))
> +		return nested_vmx_fail(vcpu, VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> +
>  	if (nested_vmx_check_host_state(vcpu, vmcs12))
>  		return nested_vmx_fail(vcpu, VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
>  
> -- 
> 2.26.3
>