Thread (18 messages) 18 messages, 4 authors, 2021-09-29

Re: [RFC PATCH 3/3] nSVM: use svm->nested.save to load vmcb12 registers and avoid TOC/TOU races

From: Maxim Levitsky <hidden>
Date: 2021-09-12 10:42:33
Also in: lkml

On Fri, 2021-09-03 at 12:20 +0200, Emanuele Giuseppe Esposito wrote:
quoted hunk ↗ jump to hunk
Move the checks done by nested_vmcb_valid_sregs and
nested_vmcb_check_controls directly in enter_svm_guest_mode,
and use svm->nested.save cached fields (EFER, CR0, CR4)
instead of vmcb12's.
This prevents from creating TOC/TOU races.

This also avoids the need of force-setting EFER_SVME in
nested_vmcb02_prepare_save.

Signed-off-by: Emanuele Giuseppe Esposito <redacted>
---
 arch/x86/kvm/svm/nested.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 2491c77203c7..487810cfefde 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -280,13 +280,6 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
 static bool nested_vmcb_valid_sregs(struct kvm_vcpu *vcpu,
 				    struct vmcb_save_area *save)
 {
-	/*
-	 * FIXME: these should be done after copying the fields,
-	 * to avoid TOC/TOU races.  For these save area checks
-	 * the possible damage is limited since kvm_set_cr0 and
-	 * kvm_set_cr4 handle failure; EFER_SVME is an exception
-	 * so it is force-set later in nested_prepare_vmcb_save.
-	 */
 	if (CC(!(save->efer & EFER_SVME)))
 		return false;
 
@@ -459,7 +452,8 @@ void nested_vmcb02_compute_g_pat(struct vcpu_svm *svm)
 	svm->nested.vmcb02.ptr->save.g_pat = svm->vmcb01.ptr->save.g_pat;
 }
 
-static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12)
+static void nested_vmcb02_prepare_save(struct vcpu_svm *svm,
+				       struct vmcb *vmcb12)
Tiny nitpick: the kernel these days allow up to 100 characters in a line,
thus this change is not needed IMHO.
quoted hunk ↗ jump to hunk
 {
 	bool new_vmcb12 = false;
 
@@ -488,15 +482,10 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12
 
 	kvm_set_rflags(&svm->vcpu, vmcb12->save.rflags | X86_EFLAGS_FIXED);
 
-	/*
-	 * Force-set EFER_SVME even though it is checked earlier on the
-	 * VMCB12, because the guest can flip the bit between the check
-	 * and now.  Clearing EFER_SVME would call svm_free_nested.
-	 */
-	svm_set_efer(&svm->vcpu, vmcb12->save.efer | EFER_SVME);
+	svm_set_efer(&svm->vcpu, svm->nested.save.efer);
 
-	svm_set_cr0(&svm->vcpu, vmcb12->save.cr0);
-	svm_set_cr4(&svm->vcpu, vmcb12->save.cr4);
+	svm_set_cr0(&svm->vcpu, svm->nested.save.cr0);
+	svm_set_cr4(&svm->vcpu, svm->nested.save.cr4);
 
 	svm->vcpu.arch.cr2 = vmcb12->save.cr2;
 
@@ -671,7 +660,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
 	nested_load_control_from_vmcb12(svm, &vmcb12->control);
 	nested_load_save_from_vmcb12(svm, &vmcb12->save);
 
-	if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) ||
+	if (!nested_vmcb_valid_sregs(vcpu, &svm->nested.save) ||
 	    !nested_vmcb_check_controls(vcpu, &svm->nested.ctl)) {
If you use a different struct for the copied fields, then it makes
sense IMHO to drop the 'control' parameter from nested_vmcb_check_controls,
and just use the svm->nested.save there directly.
 		vmcb12->control.exit_code    = SVM_EXIT_ERR;
 		vmcb12->control.exit_code_hi = 0;

I think you forgot to use svm->nested.save.cr3.
It is used in enter_svm_guest_mode to setup the mmu.

While there are likely no TOC/TOU races in regard to it, it is still
better to be consistent about it.

Looks very good otherwise.

Best regards,
	Maxim Levitsky


Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help