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