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

[Maxim Levitsky <mlevitsk@redhat.com>]

On Fri, 2021-09-03 at 12:20 +0200, Emanuele Giuseppe Esposito wrote:
> 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 <eesposit@redhat.com>
> ---
>  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.

>  {
>  	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