linux-kernel - Re: [PATCH] KVM: nVMX: add missing consistency checks for CR0 and CR4

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: <ZAvGjCqfKgsSEQhZ@google.com>
Date:   Sat, 11 Mar 2023 00:08:44 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Reima ISHII <ishiir@...cc.u-tokyo.ac.jp>,
        stable@...r.kernel.org
Subject: Re: [PATCH] KVM: nVMX: add missing consistency checks for CR0 and CR4

On Fri, Mar 10, 2023, Paolo Bonzini wrote:
> The effective values of the guest CR0 and CR4 registers may differ from
> those included in the VMCS12.  In particular, disabling EPT forces
> CR4.PAE=1 and disabling unrestricted guest mode forces CR0.PG=CR0.PE=1.
> 
> Therefore, checks on these bits cannot be delegated to the processor
> and must be performed by KVM.
> 
> Reported-by: Reima ISHII <ishiir@...cc.u-tokyo.ac.jp>
> Cc: stable@...r.kernel.org
> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
> ---
>  arch/x86/kvm/vmx/nested.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index d93c715cda6a..43693e4772ff 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -3047,6 +3047,19 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
>  					   vmcs12->guest_ia32_perf_global_ctrl)))
>  		return -EINVAL;
>  
> +	if (CC((vmcs12->guest_cr0 & (X86_CR0_PG | X86_CR0_PE)) == X86_CR0_PG))
> +		return -EINVAL;
> +
> +#ifdef CONFIG_X86_64

The #ifdef isn't necessary, attempting to set for a 32-bit host should be rejected
by nested_vmx_check_controls() since nested_vmx_setup_ctls_msrs() clears the bit.
Ditto for the host logic related to VM_EXIT_HOST_ADDR_SPACE_SIZE, which looks
suspiciously similar ;-)

I'd rather delete the #ifdef in nested_vmx_check_host_state() and do something
similar here, e.g.

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 7c4f5ca405c7..3e367ec5086a 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2903,7 +2903,7 @@ static int nested_vmx_check_address_space_size(struct kvm_vcpu *vcpu,
 static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
                                       struct vmcs12 *vmcs12)
 {
-       bool ia32e;
+       bool ia32e = !!(vmcs12->vm_exit_controls & VM_EXIT_HOST_ADDR_SPACE_SIZE);
 
        if (CC(!nested_host_cr0_valid(vcpu, vmcs12->host_cr0)) ||
            CC(!nested_host_cr4_valid(vcpu, vmcs12->host_cr4)) ||
@@ -2923,12 +2923,6 @@ static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
                                           vmcs12->host_ia32_perf_global_ctrl)))
                return -EINVAL;
 
-#ifdef CONFIG_X86_64
-       ia32e = !!(vmcs12->vm_exit_controls & VM_EXIT_HOST_ADDR_SPACE_SIZE);
-#else
-       ia32e = false;
-#endif
-
        if (ia32e) {
                if (CC(!(vmcs12->host_cr4 & X86_CR4_PAE)))
                        return -EINVAL;


> +	ia32e = !!(vmcs12->vm_entry_controls & VM_ENTRY_IA32E_MODE);
> +#else
> +	ia32e = false;
> +#endif
> +	if (CC(ia32e &&
> +	       (!(vmcs12->guest_cr4 & X86_CR4_PAE) ||
> +		!(vmcs12->guest_cr0 & X86_CR0_PG))))
> +		return -EINVAL;

This is a lot easier to read IMO, and has the advantage of more precisely
identifying the failure in the tracepoint.

	if (CC(ia32e && !(vmcs12->guest_cr4 & X86_CR4_PAE)) ||
	    CC(ia32e && !(vmcs12->guest_cr4 & X86_CR0_PG)))
		return -EINVAL;

> +
>  	/*
>  	 * If the load IA32_EFER VM-entry control is 1, the following checks
>  	 * are performed on the field for the IA32_EFER MSR:
> @@ -3058,7 +3071,6 @@ static int nested_vmx_check_guest_state(struct kvm_vcpu *vcpu,
>  	 */
>  	if (to_vmx(vcpu)->nested.nested_run_pending &&
>  	    (vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_EFER)) {
> -		ia32e = (vmcs12->vm_entry_controls & VM_ENTRY_IA32E_MODE) != 0;
>  		if (CC(!kvm_valid_efer(vcpu, vmcs12->guest_ia32_efer)) ||
>  		    CC(ia32e != !!(vmcs12->guest_ia32_efer & EFER_LMA)) ||
>  		    CC(((vmcs12->guest_cr0 & X86_CR0_PG) &&
> -- 
> 2.39.1
>