[RFC PATCH 0/3] KVM: nSVM: avoid TOC/TOU race when checking vmcb12

[RFC PATCH 0/3] KVM: nSVM: avoid TOC/TOU race when checking vmcb12

[Emanuele Giuseppe Esposito]

Currently there is a TOC/TOU race between the check of vmcb12's
efer, cr0 and cr4 registers and the later save of their values in
svm_set_*, because the guest could modify the values in the meanwhile.

To solve this issue, this serie introuces and uses svm->nested.save
structure in enter_svm_guest_mode to save the current value of efer,
cr0 and cr4 and later use these to set the vcpu->arch.* state.

Patch 1 just refactor the code to simplify the next two patches,
patch 2 introduces svm->nested.save to cache the efer, cr0 and cr4 fields
and in patch 3 we use it to avoid TOC/TOU races.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

---
v1 -> RFC:
- use svm->nested.save instead of local variables.
- not dependent anymore from "KVM: nSVM: remove useless kvm_clear_*_queue"
- simplified patches, we just use the struct and not move the check
  nearer to the TOU.

Emanuele Giuseppe Esposito (3):
  KVM: nSVM: move nested_vmcb_check_cr3_cr4 logic in
    nested_vmcb_valid_sregs
  nSVM: introduce smv->nested.save to cache save area fields
  nSVM: use svm->nested.save to load vmcb12 registers and avoid TOC/TOU
    races

 arch/x86/kvm/svm/nested.c | 82 ++++++++++++++++++++-------------------
 arch/x86/kvm/svm/svm.c    |  1 +
 arch/x86/kvm/svm/svm.h    |  3 ++
 3 files changed, 47 insertions(+), 39 deletions(-)

-- 
2.27.0

[RFC PATCH 1/3] KVM: nSVM: move nested_vmcb_check_cr3_cr4 logic in nested_vmcb_valid_sregs

[Emanuele Giuseppe Esposito]

Inline nested_vmcb_check_cr3_cr4 as it is not called by anyone else.
Doing so simplifies next patches.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 arch/x86/kvm/svm/nested.c | 35 +++++++++++++----------------------
 1 file changed, 13 insertions(+), 22 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index e5515477c30a..d2fe65e2a7a4 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -260,27 +260,6 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
 	return true;
 }
 
-static bool nested_vmcb_check_cr3_cr4(struct kvm_vcpu *vcpu,
-				      struct vmcb_save_area *save)
-{
-	/*
-	 * These checks are also performed by KVM_SET_SREGS,
-	 * except that EFER.LMA is not checked by SVM against
-	 * CR0.PG && EFER.LME.
-	 */
-	if ((save->efer & EFER_LME) && (save->cr0 & X86_CR0_PG)) {
-		if (CC(!(save->cr4 & X86_CR4_PAE)) ||
-		    CC(!(save->cr0 & X86_CR0_PE)) ||
-		    CC(kvm_vcpu_is_illegal_gpa(vcpu, save->cr3)))
-			return false;
-	}
-
-	if (CC(!kvm_is_valid_cr4(vcpu, save->cr4)))
-		return false;
-
-	return true;
-}
-
 /* Common checks that apply to both L1 and L2 state.  */
 static bool nested_vmcb_valid_sregs(struct kvm_vcpu *vcpu,
 				    struct vmcb_save_area *save)
@@ -302,7 +281,19 @@ static bool nested_vmcb_valid_sregs(struct kvm_vcpu *vcpu,
 	if (CC(!kvm_dr6_valid(save->dr6)) || CC(!kvm_dr7_valid(save->dr7)))
 		return false;
 
-	if (!nested_vmcb_check_cr3_cr4(vcpu, save))
+	/*
+	 * These checks are also performed by KVM_SET_SREGS,
+	 * except that EFER.LMA is not checked by SVM against
+	 * CR0.PG && EFER.LME.
+	 */
+	if ((save->efer & EFER_LME) && (save->cr0 & X86_CR0_PG)) {
+		if (CC(!(save->cr4 & X86_CR4_PAE)) ||
+		    CC(!(save->cr0 & X86_CR0_PE)) ||
+		    CC(kvm_vcpu_is_illegal_gpa(vcpu, save->cr3)))
+			return false;
+	}
+
+	if (CC(!kvm_is_valid_cr4(vcpu, save->cr4)))
 		return false;
 
 	if (CC(!kvm_valid_efer(vcpu, save->efer)))
-- 
2.27.0

Re: [RFC PATCH 1/3] KVM: nSVM: move nested_vmcb_check_cr3_cr4 logic in nested_vmcb_valid_sregs

[Maxim Levitsky]

On Fri, 2021-09-03 at 12:20 +0200, Emanuele Giuseppe Esposito wrote:
> Inline nested_vmcb_check_cr3_cr4 as it is not called by anyone else.
> Doing so simplifies next patches.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  arch/x86/kvm/svm/nested.c | 35 +++++++++++++----------------------
>  1 file changed, 13 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index e5515477c30a..d2fe65e2a7a4 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -260,27 +260,6 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> -static bool nested_vmcb_check_cr3_cr4(struct kvm_vcpu *vcpu,
> -				      struct vmcb_save_area *save)
> -{
> -	/*
> -	 * These checks are also performed by KVM_SET_SREGS,
> -	 * except that EFER.LMA is not checked by SVM against
> -	 * CR0.PG && EFER.LME.
> -	 */
> -	if ((save->efer & EFER_LME) && (save->cr0 & X86_CR0_PG)) {
> -		if (CC(!(save->cr4 & X86_CR4_PAE)) ||
> -		    CC(!(save->cr0 & X86_CR0_PE)) ||
> -		    CC(kvm_vcpu_is_illegal_gpa(vcpu, save->cr3)))
> -			return false;
> -	}
> -
> -	if (CC(!kvm_is_valid_cr4(vcpu, save->cr4)))
> -		return false;
> -
> -	return true;
> -}
> -
>  /* Common checks that apply to both L1 and L2 state.  */
>  static bool nested_vmcb_valid_sregs(struct kvm_vcpu *vcpu,
>  				    struct vmcb_save_area *save)
> @@ -302,7 +281,19 @@ static bool nested_vmcb_valid_sregs(struct kvm_vcpu *vcpu,
>  	if (CC(!kvm_dr6_valid(save->dr6)) || CC(!kvm_dr7_valid(save->dr7)))
>  		return false;
>  
> -	if (!nested_vmcb_check_cr3_cr4(vcpu, save))
> +	/*
> +	 * These checks are also performed by KVM_SET_SREGS,
> +	 * except that EFER.LMA is not checked by SVM against
> +	 * CR0.PG && EFER.LME.
> +	 */
> +	if ((save->efer & EFER_LME) && (save->cr0 & X86_CR0_PG)) {
> +		if (CC(!(save->cr4 & X86_CR4_PAE)) ||
> +		    CC(!(save->cr0 & X86_CR0_PE)) ||
> +		    CC(kvm_vcpu_is_illegal_gpa(vcpu, save->cr3)))
> +			return false;
> +	}
> +
> +	if (CC(!kvm_is_valid_cr4(vcpu, save->cr4)))
>  		return false;
>  
>  	if (CC(!kvm_valid_efer(vcpu, save->efer)))

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

[RFC PATCH 2/3] nSVM: introduce smv->nested.save to cache save area fields

[Emanuele Giuseppe Esposito]

This is useful in next patch, to avoid having temporary
copies of vmcb12 registers and passing them manually.

Right now, instead of blindly copying everything,
we just copy EFER, CR0, CR3, CR4, DR6 and DR7. If more fields
will need to be added, it will be more obvious to see
that they must be added in copy_vmcb_save_area,
otherwise the checks will fail.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 arch/x86/kvm/svm/nested.c | 24 ++++++++++++++++++++++++
 arch/x86/kvm/svm/svm.c    |  1 +
 arch/x86/kvm/svm/svm.h    |  3 +++
 3 files changed, 28 insertions(+)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index d2fe65e2a7a4..2491c77203c7 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -194,6 +194,22 @@ static void copy_vmcb_control_area(struct vmcb_control_area *dst,
 	dst->pause_filter_thresh  = from->pause_filter_thresh;
 }
 
+static void copy_vmcb_save_area(struct vmcb_save_area *dst,
+				struct vmcb_save_area *from)
+{
+	/*
+	 * Copy only necessary fields, as we need them
+	 * to avoid TOC/TOU races.
+	 */
+	dst->efer = from->efer;
+	dst->cr0 = from->cr0;
+	dst->cr3 = from->cr3;
+	dst->cr4 = from->cr4;
+
+	dst->dr6 = from->dr6;
+	dst->dr7 = from->dr7;
+}
+
 static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm)
 {
 	/*
@@ -313,6 +329,12 @@ void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
 	svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
 }
 
+void nested_load_save_from_vmcb12(struct vcpu_svm *svm,
+				  struct vmcb_save_area *save)
+{
+	copy_vmcb_save_area(&svm->nested.save, save);
+}
+
 /*
  * Synchronize fields that are written by the processor, so that
  * they can be copied back into the vmcb12.
@@ -647,6 +669,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
 		return -EINVAL;
 
 	nested_load_control_from_vmcb12(svm, &vmcb12->control);
+	nested_load_save_from_vmcb12(svm, &vmcb12->save);
 
 	if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) ||
 	    !nested_vmcb_check_controls(vcpu, &svm->nested.ctl)) {
@@ -1385,6 +1408,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 
 	svm_copy_vmrun_state(&svm->vmcb01.ptr->save, save);
 	nested_load_control_from_vmcb12(svm, ctl);
+	nested_load_save_from_vmcb12(svm, save);
 
 	svm_switch_vmcb(svm, &svm->nested.vmcb02);
 	nested_vmcb02_prepare_control(svm);
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 69639f9624f5..169b930322ef 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4386,6 +4386,7 @@ static int svm_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
 			vmcb12 = map.hva;
 
 			nested_load_control_from_vmcb12(svm, &vmcb12->control);
+			nested_load_save_from_vmcb12(svm, &vmcb12->save);
 
 			ret = enter_svm_guest_mode(vcpu, vmcb12_gpa, vmcb12);
 			kvm_vcpu_unmap(vcpu, &map, true);
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index bd0fe94c2920..6d12814cf64c 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -119,6 +119,7 @@ struct svm_nested_state {
 
 	/* cache for control fields of the guest */
 	struct vmcb_control_area ctl;
+	struct vmcb_save_area save;
 
 	bool initialized;
 };
@@ -484,6 +485,8 @@ int nested_svm_check_exception(struct vcpu_svm *svm, unsigned nr,
 int nested_svm_exit_special(struct vcpu_svm *svm);
 void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
 				     struct vmcb_control_area *control);
+void nested_load_save_from_vmcb12(struct vcpu_svm *svm,
+				  struct vmcb_save_area *save);
 void nested_sync_control_from_vmcb02(struct vcpu_svm *svm);
 void nested_vmcb02_compute_g_pat(struct vcpu_svm *svm);
 void svm_switch_vmcb(struct vcpu_svm *svm, struct kvm_vmcb_info *target_vmcb);
-- 
2.27.0

Re: [RFC PATCH 2/3] nSVM: introduce smv->nested.save to cache save area fields

[Maxim Levitsky]

On Fri, 2021-09-03 at 12:20 +0200, Emanuele Giuseppe Esposito wrote:
> This is useful in next patch, to avoid having temporary
> copies of vmcb12 registers and passing them manually.

This is NOT what I had in mind, but I do like that idea very much,
IMHO this is much better than what I had in mind!

The only thing that I would change is that I woudn't reuse 'struct vmcb_save_area'
for the copy, as this both wastes space (minor issue), 
and introduces a chance of someone later using non copied
fields from it (can cause a bug later on).

I would just define a new struct for that (but keep same names
for readability)

Maybe something like 'struct vmcb_save_area_cached'?

> 
> Right now, instead of blindly copying everything,
> we just copy EFER, CR0, CR3, CR4, DR6 and DR7. If more fields
> will need to be added, it will be more obvious to see
> that they must be added in copy_vmcb_save_area,
> otherwise the checks will fail.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  arch/x86/kvm/svm/nested.c | 24 ++++++++++++++++++++++++
>  arch/x86/kvm/svm/svm.c    |  1 +
>  arch/x86/kvm/svm/svm.h    |  3 +++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index d2fe65e2a7a4..2491c77203c7 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -194,6 +194,22 @@ static void copy_vmcb_control_area(struct vmcb_control_area *dst,
>  	dst->pause_filter_thresh  = from->pause_filter_thresh;
>  }
>  
> +static void copy_vmcb_save_area(struct vmcb_save_area *dst,
> +				struct vmcb_save_area *from)
> +{
> +	/*
> +	 * Copy only necessary fields, as we need them
> +	 * to avoid TOC/TOU races.
> +	 */
> +	dst->efer = from->efer;
> +	dst->cr0 = from->cr0;
> +	dst->cr3 = from->cr3;
> +	dst->cr4 = from->cr4;
> +
> +	dst->dr6 = from->dr6;
> +	dst->dr7 = from->dr7;
> +}
> +
>  static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm)
>  {
>  	/*
> @@ -313,6 +329,12 @@ void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
>  	svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
>  }
>  
> +void nested_load_save_from_vmcb12(struct vcpu_svm *svm,
> +				  struct vmcb_save_area *save)
> +{
> +	copy_vmcb_save_area(&svm->nested.save, save);
> +}
> +
>  /*
>   * Synchronize fields that are written by the processor, so that
>   * they can be copied back into the vmcb12.
> @@ -647,6 +669,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
>  		return -EINVAL;
>  
>  	nested_load_control_from_vmcb12(svm, &vmcb12->control);
> +	nested_load_save_from_vmcb12(svm, &vmcb12->save);
>  
>  	if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) ||
>  	    !nested_vmcb_check_controls(vcpu, &svm->nested.ctl)) {
> @@ -1385,6 +1408,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  
>  	svm_copy_vmrun_state(&svm->vmcb01.ptr->save, save);
>  	nested_load_control_from_vmcb12(svm, ctl);
> +	nested_load_save_from_vmcb12(svm, save);
>  
>  	svm_switch_vmcb(svm, &svm->nested.vmcb02);
>  	nested_vmcb02_prepare_control(svm);
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 69639f9624f5..169b930322ef 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4386,6 +4386,7 @@ static int svm_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
>  			vmcb12 = map.hva;
>  
>  			nested_load_control_from_vmcb12(svm, &vmcb12->control);
> +			nested_load_save_from_vmcb12(svm, &vmcb12->save);
>  
>  			ret = enter_svm_guest_mode(vcpu, vmcb12_gpa, vmcb12);
>  			kvm_vcpu_unmap(vcpu, &map, true);
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index bd0fe94c2920..6d12814cf64c 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -119,6 +119,7 @@ struct svm_nested_state {
>  
>  	/* cache for control fields of the guest */
>  	struct vmcb_control_area ctl;
> +	struct vmcb_save_area save;
>  
>  	bool initialized;
>  };
> @@ -484,6 +485,8 @@ int nested_svm_check_exception(struct vcpu_svm *svm, unsigned nr,
>  int nested_svm_exit_special(struct vcpu_svm *svm);
>  void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
>  				     struct vmcb_control_area *control);
> +void nested_load_save_from_vmcb12(struct vcpu_svm *svm,
> +				  struct vmcb_save_area *save);
>  void nested_sync_control_from_vmcb02(struct vcpu_svm *svm);
>  void nested_vmcb02_compute_g_pat(struct vcpu_svm *svm);
>  void svm_switch_vmcb(struct vcpu_svm *svm, struct kvm_vmcb_info *target_vmcb);

So besides the struct comment:

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

Re: [RFC PATCH 2/3] nSVM: introduce smv->nested.save to cache save area fields

[Paolo Bonzini]

On 12/09/21 12:39, Maxim Levitsky wrote:
> On Fri, 2021-09-03 at 12:20 +0200, Emanuele Giuseppe Esposito wrote:
>> This is useful in next patch, to avoid having temporary
>> copies of vmcb12 registers and passing them manually.
> 
> This is NOT what I had in mind, but I do like that idea very much,
> IMHO this is much better than what I had in mind!
> 
> The only thing that I would change is that I woudn't reuse 'struct vmcb_save_area'
> for the copy, as this both wastes space (minor issue),
> and introduces a chance of someone later using non copied
> fields from it (can cause a bug later on).
> 
> I would just define a new struct for that (but keep same names
> for readability)
> 
> Maybe something like 'struct vmcb_save_area_cached'?

I agree, I like this too.  However, it needs a comment that this new 
struct is not kept up-to-date, and is only valid until enter_svm_guest_mode.

I might even propose a

#ifdef CONFIG_DEBUG_KERNEL
	memset(&svm->nested.save, 0xaf, sizeof(svm->nested.save));
#endif

but there are no uses of CONFIG_DEBUG_KERNEL in all of Linux so it's 
probably not the way one should use that symbol.  Can anybody think of a 
similar alternative?  Or should the memset simply be unconditional?

Paolo

Re: [RFC PATCH 2/3] nSVM: introduce smv->nested.save to cache save area fields

[Sean Christopherson]

On Tue, Sep 28, 2021, Paolo Bonzini wrote:
> On 12/09/21 12:39, Maxim Levitsky wrote:
> > On Fri, 2021-09-03 at 12:20 +0200, Emanuele Giuseppe Esposito wrote:
> > > This is useful in next patch, to avoid having temporary
> > > copies of vmcb12 registers and passing them manually.
> > 
> > This is NOT what I had in mind, but I do like that idea very much,
> > IMHO this is much better than what I had in mind!
> > 
> > The only thing that I would change is that I woudn't reuse 'struct vmcb_save_area'
> > for the copy, as this both wastes space (minor issue),
> > and introduces a chance of someone later using non copied
> > fields from it (can cause a bug later on).
> > 
> > I would just define a new struct for that (but keep same names
> > for readability)
> > 
> > Maybe something like 'struct vmcb_save_area_cached'?
> 
> I agree, I like this too.  However, it needs a comment that this new struct
> is not kept up-to-date, and is only valid until enter_svm_guest_mode.
> 
> I might even propose a
> 
> #ifdef CONFIG_DEBUG_KERNEL
> 	memset(&svm->nested.save, 0xaf, sizeof(svm->nested.save));
> #endif
> 
> but there are no uses of CONFIG_DEBUG_KERNEL in all of Linux so it's
> probably not the way one should use that symbol.  Can anybody think of a
> similar alternative?  Or should the memset simply be unconditional?

I still think this doesn't go far enough to prevent TOCTOU bugs, and in general
KVM lacks a coherent design/approach in this area.  Case in point, the next patch
fails to handle at least one, probably more, TOCTOU bugs.  CR3 is checked using
KVM's copy (svm->nested.save)

	/*
	 * These checks are also performed by KVM_SET_SREGS,
	 * except that EFER.LMA is not checked by SVM against
	 * CR0.PG && EFER.LME.
	 */
	if ((save->efer & EFER_LME) && (save->cr0 & X86_CR0_PG)) {
		if (CC(!(save->cr4 & X86_CR4_PAE)) ||
		    CC(!(save->cr0 & X86_CR0_PE)) ||
		    CC(kvm_vcpu_is_illegal_gpa(vcpu, save->cr3)))
			return false;
	}

but KVM prepares vmcb02 and the MMU using the CR3 value directly from vmcb12.

	nested_vmcb02_prepare_control(svm);
	nested_vmcb02_prepare_save(svm, vmcb12);

	ret = nested_svm_load_cr3(&svm->vcpu, vmcb12->save.cr3,
				  nested_npt_enabled(svm), true);

I assume there is similar badness in nested_vmcb02_prepare_save()'s usage of
vmcb12, but even if there isn't, IMO that it's even a question/possibility means
KVM is broken.  I.e. KVM should fully unmap L1's vmcb12 before doing _anything_.
(1) map, (2) copy, (3) unmap, (4) check, (5) consume.  Yes, there are performance
gains to be had (or lost), but we need a fully correct/functional baseline before
we start worrying about performance.  E.g. the copy at step (2) can be optimized
to copy only data that is not marked cleaned, but first we should have a version
of KVM that has no optimizations and just copies the entire vmcb (or at least the
chunks KVM consumes).

On a related topic, this would be a good opportunity to resolve the naming
discrepancies between VMX and SVM.  VMX generally refers to vmcs12 as KVM's copy
of L1's VMCS, whereas SVM generally refers to vmcb12 as the "direct" mapping of
L1's VMCB.  I'd prefer to go with VMX's terminology, i.e. rework nSVM to refer to
the copy as vmcb12, but I'm more than a bit biased since I've spent so much time
in nVMX,

Re: [RFC PATCH 2/3] nSVM: introduce smv->nested.save to cache save area fields

[Paolo Bonzini]

On 29/09/21 00:23, Sean Christopherson wrote:
> On a related topic, this would be a good opportunity to resolve the naming
> discrepancies between VMX and SVM.  VMX generally refers to vmcs12 as KVM's copy
> of L1's VMCS, whereas SVM generally refers to vmcb12 as the "direct" mapping of
> L1's VMCB.  I'd prefer to go with VMX's terminology, i.e. rework nSVM to refer to
> the copy as vmcb12, but I'm more than a bit biased since I've spent so much time
> in nVMX,

I agree, and I think Emanuele's patches are a step in the right 
direction.  Once we ensure that all state in svm->nested is cached 
vmcb12 content, we can get rid of vmcb12 pointers in the functions.

Paolo

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

[Emanuele Giuseppe Esposito]

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)
 {
 	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)) {
 		vmcb12->control.exit_code    = SVM_EXIT_ERR;
 		vmcb12->control.exit_code_hi = 0;
-- 
2.27.0

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

[Maxim Levitsky]

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

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

[Emanuele Giuseppe Esposito]

On 12/09/2021 12:42, Maxim Levitsky wrote:
>>   
>> -	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.
> 

Ok, what you say in patch 2 makes sense to me. I can create a new struct 
vmcb_save_area_cached, but I need to keep nested.ctl because 1) it is 
used also elsewhere, and different fields from the one checked here are 
read/set and 2) using another structure (or the same 
vmcb_save_area_cached) in its place would just duplicate the same fields 
of nested.ctl, creating even more confusion and possible inconsistency.

Let me know if you disagree.

Thank you,
Emanuele

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

[Maxim Levitsky]

On Tue, 2021-09-14 at 10:20 +0200, Emanuele Giuseppe Esposito wrote:
> 
> On 12/09/2021 12:42, Maxim Levitsky wrote:
> > >   
> > > -	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.
> > 
> 
> Ok, what you say in patch 2 makes sense to me. I can create a new struct 
> vmcb_save_area_cached, but I need to keep nested.ctl because 1) it is 
> used also elsewhere, and different fields from the one checked here are 
> read/set and 2) using another structure (or the same 

Yes, keep nested.ctl, since vast majority of the fields are copied I think.

Best regards,
	Maxim Levitsky


> vmcb_save_area_cached) in its place would just duplicate the same fields 
> of nested.ctl, creating even more confusion and possible inconsistency.
> 
> Let me know if you disagree.
> 
> Thank you,
> Emanuele
> 

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

[Maxim Levitsky]

On Tue, 2021-09-14 at 12:02 +0300, Maxim Levitsky wrote:
> On Tue, 2021-09-14 at 10:20 +0200, Emanuele Giuseppe Esposito wrote:
> > On 12/09/2021 12:42, Maxim Levitsky wrote:
> > > >   
> > > > -	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.
> > > 
> > 
> > Ok, what you say in patch 2 makes sense to me. I can create a new struct 
> > vmcb_save_area_cached, but I need to keep nested.ctl because 1) it is 
> > used also elsewhere, and different fields from the one checked here are 
> > read/set and 2) using another structure (or the same 
> 
> Yes, keep nested.ctl, since vast majority of the fields are copied I think.

But actually that you mention it, I'll say why not to create vmcb_control_area_cached
as well indeed and change the type of svm->nested.save to it. (in a separate patch)

I see what you mean that we modify it a bit (but we shoudn't to be honest) and such, but
all of this can be fixed.

The advantage of having vmcb_control_area_cached is that it becomes impossible to use
by mistake a non copied field from the guest.

It would also emphasize that this stuff came from the guest and should be treated as
a toxic waste.

Note again that this should be done if we agree as a separate patch.

> 
> Best regards,
> 	Maxim Levitsky
> 
> 
> > vmcb_save_area_cached) in its place would just duplicate the same fields 
> > of nested.ctl, creating even more confusion and possible inconsistency.
> > 
> > Let me know if you disagree.
> > 
> > Thank you,
> > Emanuele
> > 

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

[Emanuele Giuseppe Esposito]

On 14/09/2021 11:12, Maxim Levitsky wrote:
> On Tue, 2021-09-14 at 12:02 +0300, Maxim Levitsky wrote:
>> On Tue, 2021-09-14 at 10:20 +0200, Emanuele Giuseppe Esposito wrote:
>>> On 12/09/2021 12:42, Maxim Levitsky wrote:
>>>>>    
>>>>> -	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.
>>>>
>>>
>>> Ok, what you say in patch 2 makes sense to me. I can create a new struct
>>> vmcb_save_area_cached, but I need to keep nested.ctl because 1) it is
>>> used also elsewhere, and different fields from the one checked here are
>>> read/set and 2) using another structure (or the same
>>
>> Yes, keep nested.ctl, since vast majority of the fields are copied I think.
> 
> But actually that you mention it, I'll say why not to create vmcb_control_area_cached
> as well indeed and change the type of svm->nested.save to it. (in a separate patch)
> 
> I see what you mean that we modify it a bit (but we shoudn't to be honest) and such, but
> all of this can be fixed.

So basically you are proposing:

struct svm_nested_state {
	...
	struct vmcb_control_area ctl; // we need this because it is used 
everywhere, I think
	struct vmcb_control_area_cached ctl_cached;
	struct vmcb_save_area_cached save_cached;
	...
}

and then

if (!nested_vmcb_valid_sregs(vcpu, &svm->nested.save_cached) ||
     !nested_vmcb_check_controls(vcpu, &svm->nested.ctl_cached)) {

like that?

Or do you want to delete nested.ctl completely and just keep the fields 
actually used in ctl_cached?

Also, note that as I am trying to use vmcb_save_area_cached, it is worth 
noticing that nested_vmcb_valid_sregs() is also used in 
svm_set_nested_state(), so it requires some additional little changes.

Thank you,
Emanuele

> 
> The advantage of having vmcb_control_area_cached is that it becomes impossible to use
> by mistake a non copied field from the guest.
> 
> It would also emphasize that this stuff came from the guest and should be treated as
> a toxic waste.
> 
> Note again that this should be done if we agree as a separate patch.
> 
>>
>> Best regards,
>> 	Maxim Levitsky
>>
>>
>>> vmcb_save_area_cached) in its place would just duplicate the same fields
>>> of nested.ctl, creating even more confusion and possible inconsistency.
>>>
>>> Let me know if you disagree.
>>>
>>> Thank you,
>>> Emanuele
>>>
> 
> 

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

[Maxim Levitsky]

On Tue, 2021-09-14 at 11:24 +0200, Emanuele Giuseppe Esposito wrote:
> 
> On 14/09/2021 11:12, Maxim Levitsky wrote:
> > On Tue, 2021-09-14 at 12:02 +0300, Maxim Levitsky wrote:
> > > On Tue, 2021-09-14 at 10:20 +0200, Emanuele Giuseppe Esposito wrote:
> > > > On 12/09/2021 12:42, Maxim Levitsky wrote:
> > > > > >    
> > > > > > -	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.
> > > > > 
> > > > 
> > > > Ok, what you say in patch 2 makes sense to me. I can create a new struct
> > > > vmcb_save_area_cached, but I need to keep nested.ctl because 1) it is
> > > > used also elsewhere, and different fields from the one checked here are
> > > > read/set and 2) using another structure (or the same
> > > 
> > > Yes, keep nested.ctl, since vast majority of the fields are copied I think.
> > 
> > But actually that you mention it, I'll say why not to create vmcb_control_area_cached
> > as well indeed and change the type of svm->nested.save to it. (in a separate patch)
> > 
> > I see what you mean that we modify it a bit (but we shoudn't to be honest) and such, but
> > all of this can be fixed.
> 
> So basically you are proposing:
> 
> struct svm_nested_state {
> 	...
> 	struct vmcb_control_area ctl; // we need this because it is used 
> everywhere, I think
> 	struct vmcb_control_area_cached ctl_cached;
> 	struct vmcb_save_area_cached save_cached;
> 	...
> }
> 
> and then
> 
> if (!nested_vmcb_valid_sregs(vcpu, &svm->nested.save_cached) ||
>      !nested_vmcb_check_controls(vcpu, &svm->nested.ctl_cached)) {
> 
> like that?
> 
> Or do you want to delete nested.ctl completely and just keep the fields 
> actually used in ctl_cached?


I would do it this way:

struct svm_nested_state {
        ...
	/* cached fields from the vmcb12 */
	struct  vmcb_control_area_cached ctl;
	struct  vmcb_save_area_cached save;
        ...
};


Best regards,
     Maxim Levitsky

> 
> 
> Also, note that as I am trying to use vmcb_save_area_cached, it is worth 
> noticing that nested_vmcb_valid_sregs() is also used in 
> svm_set_nested_state(), so it requires some additional little changes.
> 
> Thank you,
> Emanuele
> 
> > The advantage of having vmcb_control_area_cached is that it becomes impossible to use
> > by mistake a non copied field from the guest.
> > 
> > It would also emphasize that this stuff came from the guest and should be treated as
> > a toxic waste.
> > 
> > Note again that this should be done if we agree as a separate patch.
> > 
> > > Best regards,
> > > 	Maxim Levitsky
> > > 
> > > 
> > > > vmcb_save_area_cached) in its place would just duplicate the same fields
> > > > of nested.ctl, creating even more confusion and possible inconsistency.
> > > > 
> > > > Let me know if you disagree.
> > > > 
> > > > Thank you,
> > > > Emanuele
> > > > 

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

[Emanuele Giuseppe Esposito]

> 
> I would do it this way:
> 
> struct svm_nested_state {
>          ...
> 	/* cached fields from the vmcb12 */
> 	struct  vmcb_control_area_cached ctl;
> 	struct  vmcb_save_area_cached save;
>          ...
> };
> 
> 

The only thing that requires a little bit of additional work when 
applying this is svm_get_nested_state() (and theoretically 
svm_set_nested_state(), in option 2). In this function, nested.ctl is 
copied in user_vmcb->control. But now nested.ctl is not anymore a 
vmcb_control_area, so the sizes differ.

There are 2 options here:
1) copy nested.ctl into a full vmcb_control_area, and copy it to user 
space without modifying the API. The advantage is that the API is left 
intact, but an additional copy is required.

2) modify KVM_GET_NESTED_STATE and KVM_SET_NESTED_STATE to handle 
vmcb_control_area_cached. Advantage is that there is a lightweight copy 
+ the benefits explained by you in the previous email (no unset field).

I am not sure which one is the preferred way here.

Thank you,
Emanuele

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

[Maxim Levitsky]

On Tue, 2021-09-14 at 12:52 +0200, Emanuele Giuseppe Esposito wrote:
> > I would do it this way:
> > 
> > struct svm_nested_state {
> >          ...
> > 	/* cached fields from the vmcb12 */
> > 	struct  vmcb_control_area_cached ctl;
> > 	struct  vmcb_save_area_cached save;
> >          ...
> > };
> > 
> > 
> 
> The only thing that requires a little bit of additional work when 
> applying this is svm_get_nested_state() (and theoretically 
> svm_set_nested_state(), in option 2). In this function, nested.ctl is 
> copied in user_vmcb->control. But now nested.ctl is not anymore a 
> vmcb_control_area, so the sizes differ.
> 
> There are 2 options here:
> 1) copy nested.ctl into a full vmcb_control_area, and copy it to user 
> space without modifying the API. The advantage is that the API is left 
> intact, but an additional copy is required.

Thankfully there KVM_GET_NESTED_STATE is not performance critical at all,
so a copy isn't that big problem, other that it is a bit ugly.
Ugh..

> 
> 2) modify KVM_GET_NESTED_STATE and KVM_SET_NESTED_STATE to handle 
> vmcb_control_area_cached. Advantage is that there is a lightweight copy 
> + the benefits explained by you in the previous email (no unset field).

That would break the KVM_GET_NESTED_STATE ABI without a very good reason,
especially since some of the currently unused fields in the ctl (there
are I think very few of them), might became used later on, needing
to break the ABI again.

Best regards,
	Maxim Levitsky



> 
> I am not sure which one is the preferred way here.
> 
> Thank you,
> Emanuele
> 

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

[Paolo Bonzini]

On 14/09/21 11:12, Maxim Levitsky wrote:
> But actually that you mention it, I'll say why not to create vmcb_control_area_cached
> as well indeed and change the type of svm->nested.save to it. (in a separate patch)

I think you should have two structs, struct vmcb12_control_area_cache 
and struct vmcb12_save_area_cache.  Otherwise the two series that 
Emanuele posted are nice and can be combined into just one.

Paolo