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
