* [PATCH] KVM: nVMX: Inject #GP, not #UD, if "generic" VMXON CR0/CR4 check fails
@ 2022-10-06 0:19 Sean Christopherson
2022-10-06 0:21 ` Sean Christopherson
[not found] ` <783d89e3c98ee933ec789f1bffc6873ac3ac2e7a.camel@ucdavis.edu>
0 siblings, 2 replies; 3+ messages in thread
From: Sean Christopherson @ 2022-10-06 0:19 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Eric Li
Inject #GP for if VMXON is attempting with a CR0/CR4 that fails the
generic "is CRx valid" check, but passes the CR4.VMXE check, and do the
generic checks _after_ handling the post-VMXON VM-Fail.
The CR4.VMXE check, and all other #UD cases, are special pre-conditions
that are enforced prior to pivoting on the current VMX mode, i.e. occur
before interception if VMXON is attempted in VMX non-root mode.
All other CR0/CR4 checks generate #GP and effectively have lower priority
than the post-VMXON check.
Per the SDM:
IF (register operand) or (CR0.PE = 0) or (CR4.VMXE = 0) or ...
THEN #UD;
ELSIF not in VMX operation
THEN
IF (CPL > 0) or (in A20M mode) or
(the values of CR0 and CR4 are not supported in VMX operation)
THEN #GP(0);
ELSIF in VMX non-root operation
THEN VMexit;
ELSIF CPL > 0
THEN #GP(0);
ELSE VMfail("VMXON executed in VMX root operation");
FI;
which, if re-written without ELSIF, yields:
IF (register operand) or (CR0.PE = 0) or (CR4.VMXE = 0) or ...
THEN #UD
IF in VMX non-root operation
THEN VMexit;
IF CPL > 0
THEN #GP(0)
IF in VMX operation
THEN VMfail("VMXON executed in VMX root operation");
IF (in A20M mode) or
(the values of CR0 and CR4 are not supported in VMX operation)
THEN #GP(0);
Note, KVM unconditionally forwards VMXON VM-Exits that occur in L2 to L1,
i.e. there is no need to check the vCPU is not in VMX non-root mode. Add
a comment to explain why unconditionally forwarding such exits is
functionally correct.
Reported-by: Eric Li <ercli@ucdavis.edu>
Fixes: c7d855c2aff2 ("KVM: nVMX: Inject #UD if VMXON is attempted with incompatible CR0/CR4")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
Eric, any testing you can provide would be very welcome. It took me an
embarassingly long time to wrap my head around the SDM's pseucode. I
_think_ I got it right this time...
arch/x86/kvm/vmx/nested.c | 60 ++++++++++++++++++++++++++-------------
1 file changed, 41 insertions(+), 19 deletions(-)
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 8f67a9c4a287..19b2f55e7666 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -5099,33 +5099,55 @@ static int handle_vmxon(struct kvm_vcpu *vcpu)
| FEAT_CTL_VMX_ENABLED_OUTSIDE_SMX;
/*
- * Note, KVM cannot rely on hardware to perform the CR0/CR4 #UD checks
- * that have higher priority than VM-Exit (see Intel SDM's pseudocode
- * for VMXON), as KVM must load valid CR0/CR4 values into hardware while
- * running the guest, i.e. KVM needs to check the _guest_ values.
+ * Manually check CR4.VMXE checks, KVM must force CR4.VMXE=1 to enter
+ * the guest and so cannot rely on hardware to perform the check,
+ * which has higher priority than VM-Exit (see Intel SDM's pseudocode
+ * for VMXON).
*
- * Rely on hardware for the other two pre-VM-Exit checks, !VM86 and
- * !COMPATIBILITY modes. KVM may run the guest in VM86 to emulate Real
- * Mode, but KVM will never take the guest out of those modes.
+ * Rely on hardware for the other pre-VM-Exit checks, CR0.PE=1, !VM86
+ * and !COMPATIBILITY modes. For an unrestricted guest, KVM doesn't
+ * force any of the relevant guest state. For a restricted guest, KVM
+ * does force CR0.PE=1, but only to also force VM86 in order to emulate
+ * Real Mode, and so there's no need to check CR0.PE manually.
+ */
+ if (!kvm_read_cr4_bits(vcpu, X86_CR4_VMXE)) {
+ kvm_queue_exception(vcpu, UD_VECTOR);
+ return 1;
+ }
+
+ /*
+ * The CPL is checked for "not in VMX operation" and for "in VMX root",
+ * and has higher priority than the VM-Fail due to being post-VMXON,
+ * i.e. VMXON #GPs outside of VMX non-root if CPL!=0. In VMX non-root,
+ * VMXON causes VM-Exit and KVM unconditionally forwards VMXON VM-Exits
+ * from L2 to L1, i.e. there's no need to check for the vCPU being in
+ * VMX non-root.
+ *
+ * Forwarding the VM-Exit unconditionally, i.e. without performing the
+ * #UD checks (see above), is functionally ok because KVM doesn't allow
+ * L1 to run L2 without CR4.VMXE=0, and because KVM never modifies L2's
+ * CR0 or CR4, i.e. it's L2's responsibility to emulate #UDs that are
+ * missed by hardware due to shadowing CR0 and/or CR4.
+ */
+ if (vmx_get_cpl(vcpu)) {
+ kvm_inject_gp(vcpu, 0);
+ return 1;
+ }
+
+ if (vmx->nested.vmxon)
+ return nested_vmx_fail(vcpu, VMXERR_VMXON_IN_VMX_ROOT_OPERATION);
+
+ /*
+ * Invalid CR0/CR4 generates #GP. These checks are performed if and
+ * only if the vCPU isn't already in VMX operation, i.e. effectively
+ * have lower priority than the VM-Fail above.
*/
if (!nested_host_cr0_valid(vcpu, kvm_read_cr0(vcpu)) ||
!nested_host_cr4_valid(vcpu, kvm_read_cr4(vcpu))) {
- kvm_queue_exception(vcpu, UD_VECTOR);
- return 1;
- }
-
- /*
- * CPL=0 and all other checks that are lower priority than VM-Exit must
- * be checked manually.
- */
- if (vmx_get_cpl(vcpu)) {
kvm_inject_gp(vcpu, 0);
return 1;
}
- if (vmx->nested.vmxon)
- return nested_vmx_fail(vcpu, VMXERR_VMXON_IN_VMX_ROOT_OPERATION);
-
if ((vmx->msr_ia32_feature_control & VMXON_NEEDED_FEATURES)
!= VMXON_NEEDED_FEATURES) {
kvm_inject_gp(vcpu, 0);
base-commit: e779ce9d17c44a338b4fa3be8715e3b7eb9706f0
--
2.38.0.rc1.362.ged0d419d3c-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: nVMX: Inject #GP, not #UD, if "generic" VMXON CR0/CR4 check fails
2022-10-06 0:19 [PATCH] KVM: nVMX: Inject #GP, not #UD, if "generic" VMXON CR0/CR4 check fails Sean Christopherson
@ 2022-10-06 0:21 ` Sean Christopherson
[not found] ` <783d89e3c98ee933ec789f1bffc6873ac3ac2e7a.camel@ucdavis.edu>
1 sibling, 0 replies; 3+ messages in thread
From: Sean Christopherson @ 2022-10-06 0:21 UTC (permalink / raw)
To: Paolo Bonzini, kvm, linux-kernel, Eric Li
On Thu, Oct 06, 2022, Sean Christopherson wrote:
> + * Forwarding the VM-Exit unconditionally, i.e. without performing the
> + * #UD checks (see above), is functionally ok because KVM doesn't allow
> + * L1 to run L2 without CR4.VMXE=0, and because KVM never modifies L2's
> + * CR0 or CR4, i.e. it's L2's responsibility to emulate #UDs that are
Grr, s/L2's/L1's. Fixed the comment locally but didn't commit it before hitting "send".
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] KVM: nVMX: Inject #GP, not #UD, if "generic" VMXON CR0/CR4 check fails
[not found] ` <783d89e3c98ee933ec789f1bffc6873ac3ac2e7a.camel@ucdavis.edu>
@ 2022-10-06 15:23 ` Sean Christopherson
0 siblings, 0 replies; 3+ messages in thread
From: Sean Christopherson @ 2022-10-06 15:23 UTC (permalink / raw)
To: Eric Li; +Cc: Paolo Bonzini, kvm, linux-kernel
On Thu, Oct 06, 2022, Eric Li wrote:
> 在 2022-10-06星期四的 00:19 +0000,Sean Christopherson写道:
> > Eric, any testing you can provide would be very welcome. It took me
> > an
> > embarassingly long time to wrap my head around the SDM's pseucode. I
> > _think_ I got it right this time...
>
> I tested by applying the patch to Linux 5.19.14 tarball I downloaded
> from kernel.org. The fix looks good on my end.
Thanks! Was just doing the same. :-)
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-10-06 15:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-06 0:19 [PATCH] KVM: nVMX: Inject #GP, not #UD, if "generic" VMXON CR0/CR4 check fails Sean Christopherson
2022-10-06 0:21 ` Sean Christopherson
[not found] ` <783d89e3c98ee933ec789f1bffc6873ac3ac2e7a.camel@ucdavis.edu>
2022-10-06 15:23 ` Sean Christopherson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).