| Message ID | 20230310234304.2908714-1-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nVMX: add missing consistency checks for CR0 and CR4 | expand |
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@g.ecc.u-tokyo.ac.jp> > Cc: stable@vger.kernel.org > Signed-off-by: Paolo Bonzini <pbonzini@redhat.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 >
On Sat, Mar 11, 2023 at 1:17 AM Sean Christopherson <seanjc@google.com> wrote: > > @@ -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 ;-) Yeah, I noticed that too and decided that the idea could have been to allow some dead code elimination on 32-bit kernels, so I copied what the host state checks were doing. But if you prefer the more compact way I am absolutely not going to complain. > > + 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; Looks good. I squashed everything in. Paolo
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 + 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; + /* * 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) &&
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@g.ecc.u-tokyo.ac.jp> Cc: stable@vger.kernel.org Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/kvm/vmx/nested.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)