On Mon, 2021-10-11 at 10:36 -0400, Emanuele Giuseppe Esposito wrote: > Use the already checked svm->nested.save cached fields > (EFER, CR0, CR4, ...) instead of vmcb12's in > nested_vmcb02_prepare_save(). > This prevents from creating TOC/TOU races, since the > guest could modify the vmcb12 fields. > > This also avoids the need of force-setting EFER_SVME in > nested_vmcb02_prepare_save. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@xxxxxxxxxx> > --- > arch/x86/kvm/svm/nested.c | 25 +++++++------------------ > 1 file changed, 7 insertions(+), 18 deletions(-) > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c > index d07cd4b88acd..e08f2c31beae 100644 > --- a/arch/x86/kvm/svm/nested.c > +++ b/arch/x86/kvm/svm/nested.c > @@ -234,13 +234,7 @@ static bool nested_vmcb_valid_sregs(struct kvm_vcpu *vcpu) > { > struct vcpu_svm *svm = to_svm(vcpu); > struct vmcb_save_area_cached *save = &svm->nested.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; > > @@ -476,15 +470,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; > > @@ -499,8 +488,8 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12 > > /* These bits will be set properly on the first execution when new_vmc12 is true */ > if (unlikely(new_vmcb12 || vmcb_is_dirty(vmcb12, VMCB_DR))) { > - svm->vmcb->save.dr7 = vmcb12->save.dr7 | DR7_FIXED_1; > - svm->vcpu.arch.dr6 = vmcb12->save.dr6 | DR6_ACTIVE_LOW; > + svm->vmcb->save.dr7 = svm->nested.save.dr7 | DR7_FIXED_1; > + svm->vcpu.arch.dr6 = svm->nested.save.dr6 | DR6_ACTIVE_LOW; > vmcb_mark_dirty(svm->vmcb, VMCB_DR); > } > } > @@ -609,7 +598,7 @@ int enter_svm_guest_mode(struct kvm_vcpu *vcpu, u64 vmcb12_gpa, > nested_vmcb02_prepare_control(svm); > nested_vmcb02_prepare_save(svm, vmcb12); > > - ret = nested_svm_load_cr3(&svm->vcpu, vmcb12->save.cr3, > + ret = nested_svm_load_cr3(&svm->vcpu, svm->nested.save.cr3, > nested_npt_enabled(svm), true); > if (ret) > return ret; Reviewed-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx> Best regards, Maxim Levitsky