[PATCH] KVM: nVMX: Restore L2's DEBUGCTL/BNDCFGS/CET from vmcs12 on RSM and nested state restore

[PATCH] KVM: nVMX: Restore L2's DEBUGCTL/BNDCFGS/CET from vmcs12 on RSM and nested state restore

[Amirmohammad Eftekhar]

nested_vmx_enter_non_root_mode() snapshots the guest's DEBUGCTL, BNDCFGS
and CET state (S_CET/SSP/SSP_TBL) into vmx->nested.pre_vmenter_* so that
prepare_vmcs02() can preserve into vmcs02 any of those fields that VM
entry leaves unchanged, i.e. whose VM_ENTRY_LOAD_* control is clear.  The
snapshot reads the currently loaded VMCS.

That is correct for VMLAUNCH/VMRESUME: the active VMCS is vmcs01, and
when VM entry does not load a field L2 does inherit L1's value for it.
It is wrong for the two from_vmentry == false callers -- RSM back into L2
(vmx_leave_smm()) and KVM_SET_NESTED_STATE -- because there the active
VMCS is still vmcs01, holding L1's live state, while the state L2 must
resume with lives in vmcs12.  On RSM the pre-SMI values were put in
vmcs12 when the SMI forced the L2 -> L1 exit (CET via
sync_vmcs02_to_vmcs12(), DEBUGCTL/BNDCFGS tracked in vmcs12 as L2 updates
them); on nested state restore vmcs12 is the state userspace supplied.

So after an SMI taken while L2 is running, followed by RSM -- a sequence
entirely under the guest's control -- or after a restore, L2 resumes with
L1's DEBUGCTL, BNDCFGS and CET instead of its own whenever L1 left the
corresponding VM_ENTRY_LOAD_* control clear.  Nothing rejects it: the
physical VM entry sees a legal vmcs02 (the stale values are valid, just
not L2's), so the state is silently wrong rather than faulted.

Source the snapshot from vmcs12 on the !from_vmentry paths.

Signed-off-by: Amirmohammad Eftekhar <amirmohammad.eftekhar@cispa.de>
---
Notes (not for the commit message):

 - Fidelity/correctness bug: the victim is L2 and the entity that both
   owns L1 and drove the SMI is L1, so this is L1 corrupting its own L2,
   not a privilege-boundary crossing -- not memory-safety and not
   guest -> host.  It is a concrete instance of the existing in-tree
   "L2's values may be corrupted on SMI+RSM" TODO near vmx_enter_smm().
 - The same snapshot bug applies to the KVM_SET_NESTED_STATE restore
   path, which this fixes too.
 - DEBUGCTL/BNDCFGS have read the active VMCS here for a long time; the
   CET arm is newer.  Happy to add whatever Fixes:/Cc: stable scope you
   prefer rather than guess the split.
 - Verified against mainline (v7.3-rc6) and kvm-x86/next; please confirm
   it still applies on your tree, I will rebase if needed.

 arch/x86/kvm/vmx/nested.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 151873407ab..c51dcb1cb8d 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -3650,19 +3650,38 @@ enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,

 	kvm_service_local_tlb_flush_requests(vcpu);

+	/*
+	 * Snapshot the state that VM-Enter leaves unchanged when the matching
+	 * VM_ENTRY_LOAD_* control is clear, so it can be restored into vmcs02.
+	 * On VM-Enter the active VMCS is vmcs01 and its guest state is what L2
+	 * inherits.  On the !from_vmentry paths (RSM back into L2, and
+	 * KVM_SET_NESTED_STATE) the active VMCS is still vmcs01, i.e. holds
+	 * L1's state, whereas L2's state lives in vmcs12 -- read it from there
+	 * so that resuming L2 doesn't clobber its DEBUGCTL/BNDCFGS/CET with
+	 * L1's values.
+	 */
 	if (!vcpu->arch.nested_run_pending ||
 	    !(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_DEBUG_CONTROLS))
-		vmx->nested.pre_vmenter_debugctl = vmx_guest_debugctl_read();
+		vmx->nested.pre_vmenter_debugctl = from_vmentry ?
+			vmx_guest_debugctl_read() : vmcs12->guest_ia32_debugctl;
 	if (kvm_mpx_supported() &&
 	    (!vcpu->arch.nested_run_pending ||
 	     !(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS)))
-		vmx->nested.pre_vmenter_bndcfgs = vmcs_read64(GUEST_BNDCFGS);
+		vmx->nested.pre_vmenter_bndcfgs = from_vmentry ?
+			vmcs_read64(GUEST_BNDCFGS) : vmcs12->guest_bndcfgs;

 	if (!vcpu->arch.nested_run_pending ||
-	    !(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_CET_STATE))
-		vmcs_read_cet_state(vcpu, &vmx->nested.pre_vmenter_s_cet,
-				    &vmx->nested.pre_vmenter_ssp,
-				    &vmx->nested.pre_vmenter_ssp_tbl);
+	    !(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_CET_STATE)) {
+		if (from_vmentry) {
+			vmcs_read_cet_state(vcpu, &vmx->nested.pre_vmenter_s_cet,
+					    &vmx->nested.pre_vmenter_ssp,
+					    &vmx->nested.pre_vmenter_ssp_tbl);
+		} else {
+			vmx->nested.pre_vmenter_s_cet = vmcs12->guest_s_cet;
+			vmx->nested.pre_vmenter_ssp = vmcs12->guest_ssp;
+			vmx->nested.pre_vmenter_ssp_tbl = vmcs12->guest_ssp_tbl;
+		}
+	}

 	/*
 	 * Stash L1's CR3, so that in the event of a "late" VM-Fail, i.e. a
--
2.54.0

Re: [PATCH] KVM: nVMX: Restore L2's DEBUGCTL/BNDCFGS/CET from vmcs12 on RSM and nested state restore

[Amirmohammad Eftekhar]

The automated Sashiko review of this patch is right, please drop it.

I missed that the pre_vmenter_* snapshot is taken from the loaded VMCS on
purpose. The comment on struct nested_vmx says as much: on
KVM_SET_NESTED_STATE the snapshot ends up holding L2's value because
userspace restores the MSRs into vmcs01 before it restores nested state.
Reading from vmcs12 on that path discards those restored values, so the
patch breaks restore rather than helping it. My changelog had this
backwards.

The other points hold up too:

  - On RSM and on restore nested_run_pending is 0, so prepare_vmcs02()
    hands pre_vmenter_debugctl to vmx_guest_debugctl_write() unmasked.
    vmcs12->guest_ia32_debugctl is not validated when
    VM_ENTRY_LOAD_DEBUG_CONTROLS is clear, so with VMWRITE + SMI + RSM a
    guest can trip WARN_ON_ONCE(val & VMX_HOST_OWNED_DEBUGCTL_BITS) and
    get host-owned bits written into vmcs02.

  - vmcs12->guest_ia32_debugctl and guest_bndcfgs are not reliably L2's
    live values anyway. sync_vmcs02_to_vmcs12() deliberately does not
    save them, and vmx_set_msr() only updates them under the matching
    save/load controls, so sourcing them from vmcs12 can regress the
    plain inherit case.

The only gap I still see is narrower: on RSM into L2 with
VM_ENTRY_LOAD_CET_STATE clear, the CET snapshot comes from vmcs01 (L1's
state) while L2's pre-SMI CET was saved into vmcs12. That looks like the
SMI+RSM corruption the TODO near vmx_enter_smm() already flags, and if it
is worth fixing it needs a validated, RSM-only change, not this blanket
one.

Thanks,
Amirmohammad