* [PATCH] KVM: nVMX: add missing consistency checks for CR0 and CR4
@ 2023-03-10 23:43 Paolo Bonzini
2023-03-11 0:08 ` Sean Christopherson
0 siblings, 1 reply; 3+ messages in thread
From: Paolo Bonzini @ 2023-03-10 23:43 UTC (permalink / raw)
To: linux-kernel, kvm; +Cc: Reima ISHII, stable
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
+ 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) &&
--
2.39.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: nVMX: add missing consistency checks for CR0 and CR4
2023-03-10 23:43 [PATCH] KVM: nVMX: add missing consistency checks for CR0 and CR4 Paolo Bonzini
@ 2023-03-11 0:08 ` Sean Christopherson
2023-03-14 11:40 ` Paolo Bonzini
0 siblings, 1 reply; 3+ messages in thread
From: Sean Christopherson @ 2023-03-11 0:08 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, kvm, Reima ISHII, stable
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
>
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: nVMX: add missing consistency checks for CR0 and CR4
2023-03-11 0:08 ` Sean Christopherson
@ 2023-03-14 11:40 ` Paolo Bonzini
0 siblings, 0 replies; 3+ messages in thread
From: Paolo Bonzini @ 2023-03-14 11:40 UTC (permalink / raw)
To: Sean Christopherson; +Cc: linux-kernel, kvm, Reima ISHII, stable
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-03-14 11:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-10 23:43 [PATCH] KVM: nVMX: add missing consistency checks for CR0 and CR4 Paolo Bonzini
2023-03-11 0:08 ` Sean Christopherson
2023-03-14 11:40 ` Paolo Bonzini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.