[PATCH v5 0/7] KVM: nSVM: avoid TOC/TOU race when checking vmcb12

[PATCH v5 0/7] 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 series introduces 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.

Similarly, svm->nested.ctl contains fields that are not used, so having
a full vmcb_control_area means passing uninitialized fields.

Patches 1,3 and 8 take care of renaming and refactoring code.
Patches 2 and 6 introduce respectively vmcb_ctrl_area_cached and
vmcb_save_area_cached.
Patches 4 and 5 use vmcb_save_area_cached to avoid TOC/TOU, and patch
7 uses vmcb_ctrl_area_cached.

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

---
v5:
* rebased on kvm/queue branch

v4:
* introduce _* helpers (_nested_vmcb_check_save,
  _nested_copy_vmcb_control_to_cache, _nested_copy_vmcb_save_to_cache)
  that take care of additional parameters.
* svm_set_nested_state: introduce {save, ctl}_cached variables
  to not pollute svm->nested.{save,ctl} state, especially if the
  check fails. remove also unnecessary memset added in previous versions.
* svm_get_nested_state: change stack variable ctl introduced in this series
 into a pointer that will be zeroed and freed after it has been copied to user

v3:
* merge this series with "KVM: nSVM: use vmcb_ctrl_area_cached instead
  of vmcb_control_area in nested state"
* rename "nested_load_save_from_vmcb12" in
  "nested_copy_vmcb_save_to_cache"
* rename "nested_load_control_from_vmcb12" in
  "nested_copy_vmcb_control_to_cache"
* change check functions (nested_vmcb_valid_sregs and nested_vmcb_valid_sregs)
  to accept only the vcpu parameter, since we only check
  nested state now
* rename "vmcb_is_intercept_cached" in "vmcb12_is_intercept"
  and duplicate the implementation instead of calling vmcb_is_intercept

v2:
* svm->nested.save is a separate struct vmcb_save_area_cached,
  and not vmcb_save_area.
* update also vmcb02->cr3 with svm->nested.save.cr3

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 (7):
  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: rename nested_load_control_from_vmcb12 in
    nested_copy_vmcb_control_to_cache
  nSVM: use vmcb_save_area_cached in nested_vmcb_valid_sregs()
  nSVM: use svm->nested.save to load vmcb12 registers and avoid TOC/TOU
    races
  nSVM: introduce struct vmcb_ctrl_area_cached
  nSVM: use vmcb_ctrl_area_cached instead of vmcb_control_area in struct
    svm_nested_state

 arch/x86/kvm/svm/nested.c | 250 ++++++++++++++++++++++++--------------
 arch/x86/kvm/svm/svm.c    |   7 +-
 arch/x86/kvm/svm/svm.h    |  59 ++++++++-
 3 files changed, 213 insertions(+), 103 deletions(-)

-- 
2.27.0

[PATCH v5 1/7] 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>
Reviewed-by: Maxim Levitsky <mlevitsk@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 f8b7bc04b3e7..946c06a25d37 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -275,27 +275,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)
@@ -317,7 +296,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

[PATCH v5 2/7] 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 struct vmcb_save_area_cached and
in nested_copy_vmcb_save_to_cache().

_nested_copy_vmcb_save_to_cache() takes a vmcb_save_area_cached
parameter, useful when we want to save the state to a local
variable instead of svm internals.

Note that in svm_set_nested_state() we want to cache the L2
save state only if we are in normal non guest mode, because
otherwise it is not touched.

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

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 946c06a25d37..ea64fea371c8 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -328,6 +328,28 @@ void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
 	svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
 }
 
+static void _nested_copy_vmcb_save_to_cache(struct vmcb_save_area_cached *to,
+					    struct vmcb_save_area *from)
+{
+	/*
+	 * Copy only fields that are validated, as we need them
+	 * to avoid TOC/TOU races.
+	 */
+	to->efer = from->efer;
+	to->cr0 = from->cr0;
+	to->cr3 = from->cr3;
+	to->cr4 = from->cr4;
+
+	to->dr6 = from->dr6;
+	to->dr7 = from->dr7;
+}
+
+void nested_copy_vmcb_save_to_cache(struct vcpu_svm *svm,
+				    struct vmcb_save_area *save)
+{
+	_nested_copy_vmcb_save_to_cache(&svm->nested.save, save);
+}
+
 /*
  * Synchronize fields that are written by the processor, so that
  * they can be copied back into the vmcb12.
@@ -670,6 +692,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
 		return -EINVAL;
 
 	nested_load_control_from_vmcb12(svm, &vmcb12->control);
+	nested_copy_vmcb_save_to_cache(svm, &vmcb12->save);
 
 	if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) ||
 	    !nested_vmcb_check_controls(vcpu, &svm->nested.ctl)) {
@@ -1402,8 +1425,10 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 
 	if (is_guest_mode(vcpu))
 		svm_leave_nested(svm);
-	else
+	else {
 		svm->nested.vmcb02.ptr->save = svm->vmcb01.ptr->save;
+		nested_copy_vmcb_save_to_cache(svm, &svm->nested.vmcb02.ptr->save);
+	}
 
 	svm_set_gif(svm, !!(kvm_state->flags & KVM_STATE_NESTED_GIF_SET));
 
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 21bb81710e0f..6e5c2671e823 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4438,6 +4438,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_copy_vmcb_save_to_cache(svm, &vmcb12->save);
 	ret = enter_svm_guest_mode(vcpu, vmcb12_gpa, vmcb12, false);
 
 unmap_save:
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 0d7bbe548ac3..0897551d8868 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -103,6 +103,19 @@ struct kvm_vmcb_info {
 	uint64_t asid_generation;
 };
 
+/*
+ * This struct is not kept up-to-date, and it is only valid within
+ * svm_set_nested_state and nested_svm_vmrun.
+ */
+struct vmcb_save_area_cached {
+	u64 efer;
+	u64 cr4;
+	u64 cr3;
+	u64 cr0;
+	u64 dr7;
+	u64 dr6;
+};
+
 struct svm_nested_state {
 	struct kvm_vmcb_info vmcb02;
 	u64 hsave_msr;
@@ -119,6 +132,7 @@ struct svm_nested_state {
 
 	/* cache for control fields of the guest */
 	struct vmcb_control_area ctl;
+	struct vmcb_save_area_cached save;
 
 	bool initialized;
 };
@@ -490,6 +504,8 @@ void nested_svm_update_tsc_ratio_msr(struct kvm_vcpu *vcpu);
 void svm_write_tsc_multiplier(struct kvm_vcpu *vcpu, u64 multiplier);
 void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
 				     struct vmcb_control_area *control);
+void nested_copy_vmcb_save_to_cache(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: [PATCH v5 2/7] nSVM: introduce smv->nested.save to cache save area fields

[Maxim Levitsky]

On Wed, 2021-11-03 at 10:05 -0400, Emanuele Giuseppe Esposito wrote:
> 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 struct vmcb_save_area_cached and
> in nested_copy_vmcb_save_to_cache().
> 
> _nested_copy_vmcb_save_to_cache() takes a vmcb_save_area_cached
> parameter, useful when we want to save the state to a local
> variable instead of svm internals.
> 
> Note that in svm_set_nested_state() we want to cache the L2
> save state only if we are in normal non guest mode, because
> otherwise it is not touched.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  arch/x86/kvm/svm/nested.c | 27 ++++++++++++++++++++++++++-
>  arch/x86/kvm/svm/svm.c    |  1 +
>  arch/x86/kvm/svm/svm.h    | 16 ++++++++++++++++
>  3 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 946c06a25d37..ea64fea371c8 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -328,6 +328,28 @@ void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
>  	svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
>  }
>  
> +static void _nested_copy_vmcb_save_to_cache(struct vmcb_save_area_cached *to,
> +					    struct vmcb_save_area *from)
> +{
> +	/*
> +	 * Copy only fields that are validated, as we need them
> +	 * to avoid TOC/TOU races.
> +	 */
> +	to->efer = from->efer;
> +	to->cr0 = from->cr0;
> +	to->cr3 = from->cr3;
> +	to->cr4 = from->cr4;
> +
> +	to->dr6 = from->dr6;
> +	to->dr7 = from->dr7;
> +}
> +
> +void nested_copy_vmcb_save_to_cache(struct vcpu_svm *svm,
> +				    struct vmcb_save_area *save)
> +{
> +	_nested_copy_vmcb_save_to_cache(&svm->nested.save, save);
> +}
> +
>  /*
>   * Synchronize fields that are written by the processor, so that
>   * they can be copied back into the vmcb12.
> @@ -670,6 +692,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
>  		return -EINVAL;
>  
>  	nested_load_control_from_vmcb12(svm, &vmcb12->control);
> +	nested_copy_vmcb_save_to_cache(svm, &vmcb12->save);
>  
>  	if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) ||
>  	    !nested_vmcb_check_controls(vcpu, &svm->nested.ctl)) {
> @@ -1402,8 +1425,10 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  
>  	if (is_guest_mode(vcpu))
>  		svm_leave_nested(svm);
> -	else
> +	else {
>  		svm->nested.vmcb02.ptr->save = svm->vmcb01.ptr->save;
> +		nested_copy_vmcb_save_to_cache(svm, &svm->nested.vmcb02.ptr->save);
> +	}
>  
>  	svm_set_gif(svm, !!(kvm_state->flags & KVM_STATE_NESTED_GIF_SET));
>  
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 21bb81710e0f..6e5c2671e823 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4438,6 +4438,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_copy_vmcb_save_to_cache(svm, &vmcb12->save);
>  	ret = enter_svm_guest_mode(vcpu, vmcb12_gpa, vmcb12, false);
>  
>  unmap_save:
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 0d7bbe548ac3..0897551d8868 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -103,6 +103,19 @@ struct kvm_vmcb_info {
>  	uint64_t asid_generation;
>  };
>  

Nitpick: There is a bit of ambiguity in this comment. I think you mean that
the contents of the svm->nested.save are not always up to date,
but then the comment should be there, where the field is
declared.

I first read this comment as if the fields of this struct are not
up to date vs SVM spec or something.

Also the same can be said about the svm->nested.ctl and such when not
running a nested guest. Maybe we need to drop that coment at all.

> +/*
> + * This struct is not kept up-to-date, and it is only valid within
> + * svm_set_nested_state and nested_svm_vmrun.
> + */
> +struct vmcb_save_area_cached {
> +	u64 efer;
> +	u64 cr4;
> +	u64 cr3;
> +	u64 cr0;
> +	u64 dr7;
> +	u64 dr6;
> +};
> +
>  struct svm_nested_state {
>  	struct kvm_vmcb_info vmcb02;
>  	u64 hsave_msr;
> @@ -119,6 +132,7 @@ struct svm_nested_state {
>  
>  	/* cache for control fields of the guest */
>  	struct vmcb_control_area ctl;
> +	struct vmcb_save_area_cached save;
>  
>  	bool initialized;
>  };
> @@ -490,6 +504,8 @@ void nested_svm_update_tsc_ratio_msr(struct kvm_vcpu *vcpu);
>  void svm_write_tsc_multiplier(struct kvm_vcpu *vcpu, u64 multiplier);
>  void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
>  				     struct vmcb_control_area *control);
> +void nested_copy_vmcb_save_to_cache(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);

Other than the nitpick looks good.


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

Best regards,
	Maxim Levitsky

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

[Paolo Bonzini]

On 11/3/21 15:05, Emanuele Giuseppe Esposito wrote:
> Note that in svm_set_nested_state() we want to cache the L2
> save state only if we are in normal non guest mode, because
> otherwise it is not touched.

I think that call to nested_copy_vmcb_save_to_cache is not necessary at 
all, because svm->nested.save is not used afterwards and is not valid 
after VMRUN.

The relevant checks have already been done before:

         if (!(vcpu->arch.efer & EFER_SVME)) {
                 /* GIF=1 and no guest mode are required if SVME=0.  */
                 if (kvm_state->flags != KVM_STATE_NESTED_GIF_SET)
                         return -EINVAL;
         }

	...

         /*
          * Processor state contains L2 state.  Check that it is
          * valid for guest mode (see nested_vmcb_check_save).
          */
         cr0 = kvm_read_cr0(vcpu);
         if (((cr0 & X86_CR0_CD) == 0) && (cr0 & X86_CR0_NW))
                 goto out_free;

(and all other checks are done by KVM_SET_SREGS, KVM_SET_DEBUGREGS etc.)

Paolo

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

[Maxim Levitsky]

On Thu, 2021-11-11 at 14:52 +0100, Paolo Bonzini wrote:
> On 11/3/21 15:05, Emanuele Giuseppe Esposito wrote:
> > Note that in svm_set_nested_state() we want to cache the L2
> > save state only if we are in normal non guest mode, because
> > otherwise it is not touched.
> 
> I think that call to nested_copy_vmcb_save_to_cache is not necessary at 
> all, because svm->nested.save is not used afterwards and is not valid 
> after VMRUN.

Yes, but since setting nested state is absolutely not performance critical,
having it do the same thing as normal VMRUN is always better.

Best regards,
	Maxim Levitsky

> 
> The relevant checks have already been done before:
> 
>          if (!(vcpu->arch.efer & EFER_SVME)) {
>                  /* GIF=1 and no guest mode are required if SVME=0.  */
>                  if (kvm_state->flags != KVM_STATE_NESTED_GIF_SET)
>                          return -EINVAL;
>          }
> 
> 	...
> 
>          /*
>           * Processor state contains L2 state.  Check that it is
>           * valid for guest mode (see nested_vmcb_check_save).
>           */
>          cr0 = kvm_read_cr0(vcpu);
>          if (((cr0 & X86_CR0_CD) == 0) && (cr0 & X86_CR0_NW))
>                  goto out_free;
> 
> (and all other checks are done by KVM_SET_SREGS, KVM_SET_DEBUGREGS etc.)
> 
> Paolo
> 

[PATCH v5 3/7] nSVM: rename nested_load_control_from_vmcb12 in nested_copy_vmcb_control_to_cache

[Emanuele Giuseppe Esposito]

Following the same naming convention of the previous patch,
rename nested_load_control_from_vmcb12.
In addition, inline copy_vmcb_control_area as it is only called
by this function.

_nested_copy_vmcb_control_to_cache() works with vmcb_control_area
parameters and it will be useful in next patches, when we use
local variables instead of svm cached state.

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

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index ea64fea371c8..162b338a6c74 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -163,37 +163,6 @@ void recalc_intercepts(struct vcpu_svm *svm)
 	vmcb_set_intercept(c, INTERCEPT_VMSAVE);
 }
 
-static void copy_vmcb_control_area(struct vmcb_control_area *dst,
-				   struct vmcb_control_area *from)
-{
-	unsigned int i;
-
-	for (i = 0; i < MAX_INTERCEPT; i++)
-		dst->intercepts[i] = from->intercepts[i];
-
-	dst->iopm_base_pa         = from->iopm_base_pa;
-	dst->msrpm_base_pa        = from->msrpm_base_pa;
-	dst->tsc_offset           = from->tsc_offset;
-	/* asid not copied, it is handled manually for svm->vmcb.  */
-	dst->tlb_ctl              = from->tlb_ctl;
-	dst->int_ctl              = from->int_ctl;
-	dst->int_vector           = from->int_vector;
-	dst->int_state            = from->int_state;
-	dst->exit_code            = from->exit_code;
-	dst->exit_code_hi         = from->exit_code_hi;
-	dst->exit_info_1          = from->exit_info_1;
-	dst->exit_info_2          = from->exit_info_2;
-	dst->exit_int_info        = from->exit_int_info;
-	dst->exit_int_info_err    = from->exit_int_info_err;
-	dst->nested_ctl           = from->nested_ctl;
-	dst->event_inj            = from->event_inj;
-	dst->event_inj_err        = from->event_inj_err;
-	dst->nested_cr3           = from->nested_cr3;
-	dst->virt_ext              = from->virt_ext;
-	dst->pause_filter_count   = from->pause_filter_count;
-	dst->pause_filter_thresh  = from->pause_filter_thresh;
-}
-
 static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm)
 {
 	/*
@@ -317,15 +286,46 @@ static bool nested_vmcb_valid_sregs(struct kvm_vcpu *vcpu,
 	return true;
 }
 
-void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
-				     struct vmcb_control_area *control)
+static
+void _nested_copy_vmcb_control_to_cache(struct vmcb_control_area *to,
+					struct vmcb_control_area *from)
 {
-	copy_vmcb_control_area(&svm->nested.ctl, control);
+	unsigned int i;
+
+	for (i = 0; i < MAX_INTERCEPT; i++)
+		to->intercepts[i] = from->intercepts[i];
+
+	to->iopm_base_pa        = from->iopm_base_pa;
+	to->msrpm_base_pa       = from->msrpm_base_pa;
+	to->tsc_offset          = from->tsc_offset;
+	to->tlb_ctl             = from->tlb_ctl;
+	to->int_ctl             = from->int_ctl;
+	to->int_vector          = from->int_vector;
+	to->int_state           = from->int_state;
+	to->exit_code           = from->exit_code;
+	to->exit_code_hi        = from->exit_code_hi;
+	to->exit_info_1         = from->exit_info_1;
+	to->exit_info_2         = from->exit_info_2;
+	to->exit_int_info       = from->exit_int_info;
+	to->exit_int_info_err   = from->exit_int_info_err;
+	to->nested_ctl          = from->nested_ctl;
+	to->event_inj           = from->event_inj;
+	to->event_inj_err       = from->event_inj_err;
+	to->nested_cr3          = from->nested_cr3;
+	to->virt_ext            = from->virt_ext;
+	to->pause_filter_count  = from->pause_filter_count;
+	to->pause_filter_thresh = from->pause_filter_thresh;
+
+	/* Copy asid here because nested_vmcb_check_controls will check it.  */
+	to->asid           = from->asid;
+	to->msrpm_base_pa &= ~0x0fffULL;
+	to->iopm_base_pa  &= ~0x0fffULL;
+}
 
-	/* Copy it here because nested_svm_check_controls will check it.  */
-	svm->nested.ctl.asid           = control->asid;
-	svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
-	svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
+void nested_copy_vmcb_control_to_cache(struct vcpu_svm *svm,
+				       struct vmcb_control_area *control)
+{
+	_nested_copy_vmcb_control_to_cache(&svm->nested.ctl, control);
 }
 
 static void _nested_copy_vmcb_save_to_cache(struct vmcb_save_area_cached *to,
@@ -691,7 +691,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
 	if (WARN_ON_ONCE(!svm->nested.initialized))
 		return -EINVAL;
 
-	nested_load_control_from_vmcb12(svm, &vmcb12->control);
+	nested_copy_vmcb_control_to_cache(svm, &vmcb12->control);
 	nested_copy_vmcb_save_to_cache(svm, &vmcb12->save);
 
 	if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) ||
@@ -1438,7 +1438,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	svm->nested.vmcb12_gpa = kvm_state->hdr.svm.vmcb_pa;
 
 	svm_copy_vmrun_state(&svm->vmcb01.ptr->save, save);
-	nested_load_control_from_vmcb12(svm, ctl);
+	nested_copy_vmcb_control_to_cache(svm, ctl);
 
 	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 6e5c2671e823..74d6db9017ea 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4437,7 +4437,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_copy_vmcb_control_to_cache(svm, &vmcb12->control);
 	nested_copy_vmcb_save_to_cache(svm, &vmcb12->save);
 	ret = enter_svm_guest_mode(vcpu, vmcb12_gpa, vmcb12, false);
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 0897551d8868..a0609fe2e68c 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -502,8 +502,8 @@ int nested_svm_check_exception(struct vcpu_svm *svm, unsigned nr,
 int nested_svm_exit_special(struct vcpu_svm *svm);
 void nested_svm_update_tsc_ratio_msr(struct kvm_vcpu *vcpu);
 void svm_write_tsc_multiplier(struct kvm_vcpu *vcpu, u64 multiplier);
-void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
-				     struct vmcb_control_area *control);
+void nested_copy_vmcb_control_to_cache(struct vcpu_svm *svm,
+				       struct vmcb_control_area *control);
 void nested_copy_vmcb_save_to_cache(struct vcpu_svm *svm,
 				    struct vmcb_save_area *save);
 void nested_sync_control_from_vmcb02(struct vcpu_svm *svm);
-- 
2.27.0

Re: [PATCH v5 3/7] nSVM: rename nested_load_control_from_vmcb12 in nested_copy_vmcb_control_to_cache

[Maxim Levitsky]

On Wed, 2021-11-03 at 10:05 -0400, Emanuele Giuseppe Esposito wrote:
> Following the same naming convention of the previous patch,
> rename nested_load_control_from_vmcb12.
> In addition, inline copy_vmcb_control_area as it is only called
> by this function.
> 
> _nested_copy_vmcb_control_to_cache() works with vmcb_control_area
> parameters and it will be useful in next patches, when we use
> local variables instead of svm cached state.

Tiny nitpick: usually when we have a patch which intends to just move/rename
things around, and should generate exact same machine code other
that some inlining/optimizations/etc, we usually mark this with
'No functional change intended', so that it is easier to spot
a mistake which only slighlty changes something.


The patch itself looks good.

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

> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  arch/x86/kvm/svm/nested.c | 80 +++++++++++++++++++--------------------
>  arch/x86/kvm/svm/svm.c    |  2 +-
>  arch/x86/kvm/svm/svm.h    |  4 +-
>  3 files changed, 43 insertions(+), 43 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index ea64fea371c8..162b338a6c74 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -163,37 +163,6 @@ void recalc_intercepts(struct vcpu_svm *svm)
>  	vmcb_set_intercept(c, INTERCEPT_VMSAVE);
>  }
>  
> -static void copy_vmcb_control_area(struct vmcb_control_area *dst,
> -				   struct vmcb_control_area *from)
> -{
> -	unsigned int i;
> -
> -	for (i = 0; i < MAX_INTERCEPT; i++)
> -		dst->intercepts[i] = from->intercepts[i];
> -
> -	dst->iopm_base_pa         = from->iopm_base_pa;
> -	dst->msrpm_base_pa        = from->msrpm_base_pa;
> -	dst->tsc_offset           = from->tsc_offset;
> -	/* asid not copied, it is handled manually for svm->vmcb.  */
> -	dst->tlb_ctl              = from->tlb_ctl;
> -	dst->int_ctl              = from->int_ctl;
> -	dst->int_vector           = from->int_vector;
> -	dst->int_state            = from->int_state;
> -	dst->exit_code            = from->exit_code;
> -	dst->exit_code_hi         = from->exit_code_hi;
> -	dst->exit_info_1          = from->exit_info_1;
> -	dst->exit_info_2          = from->exit_info_2;
> -	dst->exit_int_info        = from->exit_int_info;
> -	dst->exit_int_info_err    = from->exit_int_info_err;
> -	dst->nested_ctl           = from->nested_ctl;
> -	dst->event_inj            = from->event_inj;
> -	dst->event_inj_err        = from->event_inj_err;
> -	dst->nested_cr3           = from->nested_cr3;
> -	dst->virt_ext              = from->virt_ext;
> -	dst->pause_filter_count   = from->pause_filter_count;
> -	dst->pause_filter_thresh  = from->pause_filter_thresh;
> -}
> -
>  static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm)
>  {
>  	/*
> @@ -317,15 +286,46 @@ static bool nested_vmcb_valid_sregs(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> -void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
> -				     struct vmcb_control_area *control)
> +static
> +void _nested_copy_vmcb_control_to_cache(struct vmcb_control_area *to,
> +					struct vmcb_control_area *from)
>  {
> -	copy_vmcb_control_area(&svm->nested.ctl, control);
> +	unsigned int i;
> +
> +	for (i = 0; i < MAX_INTERCEPT; i++)
> +		to->intercepts[i] = from->intercepts[i];
> +
> +	to->iopm_base_pa        = from->iopm_base_pa;
> +	to->msrpm_base_pa       = from->msrpm_base_pa;
> +	to->tsc_offset          = from->tsc_offset;
> +	to->tlb_ctl             = from->tlb_ctl;
> +	to->int_ctl             = from->int_ctl;
> +	to->int_vector          = from->int_vector;
> +	to->int_state           = from->int_state;
> +	to->exit_code           = from->exit_code;
> +	to->exit_code_hi        = from->exit_code_hi;
> +	to->exit_info_1         = from->exit_info_1;
> +	to->exit_info_2         = from->exit_info_2;
> +	to->exit_int_info       = from->exit_int_info;
> +	to->exit_int_info_err   = from->exit_int_info_err;
> +	to->nested_ctl          = from->nested_ctl;
> +	to->event_inj           = from->event_inj;
> +	to->event_inj_err       = from->event_inj_err;
> +	to->nested_cr3          = from->nested_cr3;
> +	to->virt_ext            = from->virt_ext;
> +	to->pause_filter_count  = from->pause_filter_count;
> +	to->pause_filter_thresh = from->pause_filter_thresh;
> +
> +	/* Copy asid here because nested_vmcb_check_controls will check it.  */
> +	to->asid           = from->asid;
> +	to->msrpm_base_pa &= ~0x0fffULL;
> +	to->iopm_base_pa  &= ~0x0fffULL;
> +}
>  
> -	/* Copy it here because nested_svm_check_controls will check it.  */
> -	svm->nested.ctl.asid           = control->asid;
> -	svm->nested.ctl.msrpm_base_pa &= ~0x0fffULL;
> -	svm->nested.ctl.iopm_base_pa  &= ~0x0fffULL;
> +void nested_copy_vmcb_control_to_cache(struct vcpu_svm *svm,
> +				       struct vmcb_control_area *control)
> +{
> +	_nested_copy_vmcb_control_to_cache(&svm->nested.ctl, control);
>  }
>  
>  static void _nested_copy_vmcb_save_to_cache(struct vmcb_save_area_cached *to,
> @@ -691,7 +691,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
>  	if (WARN_ON_ONCE(!svm->nested.initialized))
>  		return -EINVAL;
>  
> -	nested_load_control_from_vmcb12(svm, &vmcb12->control);
> +	nested_copy_vmcb_control_to_cache(svm, &vmcb12->control);
>  	nested_copy_vmcb_save_to_cache(svm, &vmcb12->save);
>  
>  	if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) ||
> @@ -1438,7 +1438,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  	svm->nested.vmcb12_gpa = kvm_state->hdr.svm.vmcb_pa;
>  
>  	svm_copy_vmrun_state(&svm->vmcb01.ptr->save, save);
> -	nested_load_control_from_vmcb12(svm, ctl);
> +	nested_copy_vmcb_control_to_cache(svm, ctl);
>  
>  	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 6e5c2671e823..74d6db9017ea 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4437,7 +4437,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_copy_vmcb_control_to_cache(svm, &vmcb12->control);
>  	nested_copy_vmcb_save_to_cache(svm, &vmcb12->save);
>  	ret = enter_svm_guest_mode(vcpu, vmcb12_gpa, vmcb12, false);
>  
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 0897551d8868..a0609fe2e68c 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -502,8 +502,8 @@ int nested_svm_check_exception(struct vcpu_svm *svm, unsigned nr,
>  int nested_svm_exit_special(struct vcpu_svm *svm);
>  void nested_svm_update_tsc_ratio_msr(struct kvm_vcpu *vcpu);
>  void svm_write_tsc_multiplier(struct kvm_vcpu *vcpu, u64 multiplier);
> -void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
> -				     struct vmcb_control_area *control);
> +void nested_copy_vmcb_control_to_cache(struct vcpu_svm *svm,
> +				       struct vmcb_control_area *control);
>  void nested_copy_vmcb_save_to_cache(struct vcpu_svm *svm,
>  				    struct vmcb_save_area *save);
>  void nested_sync_control_from_vmcb02(struct vcpu_svm *svm);

[PATCH v5 4/7] nSVM: use vmcb_save_area_cached in nested_vmcb_valid_sregs()

[Emanuele Giuseppe Esposito]

Now that struct vmcb_save_area_cached contains the required
vmcb fields values (done in nested_load_save_from_vmcb12()),
check them to see if they are correct in nested_vmcb_valid_sregs().

While at it, rename nested_vmcb_valid_sregs in nested_vmcb_check_save.
_nested_vmcb_check_save takes the additional @save parameter, so it
is helpful when we want to check a non-svm save state, like in
svm_set_nested_state. The reason for that is that save is the L1
state, not L2, so we just check it.

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

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 162b338a6c74..64fb43234e06 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -245,8 +245,8 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
 }
 
 /* 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)
+static bool _nested_vmcb_check_save(struct kvm_vcpu *vcpu,
+				     struct vmcb_save_area_cached *save)
 {
 	/*
 	 * FIXME: these should be done after copying the fields,
@@ -286,6 +286,14 @@ static bool nested_vmcb_valid_sregs(struct kvm_vcpu *vcpu,
 	return true;
 }
 
+static bool nested_vmcb_check_save(struct kvm_vcpu *vcpu)
+{
+	struct vcpu_svm *svm = to_svm(vcpu);
+	struct vmcb_save_area_cached *save = &svm->nested.save;
+
+	return _nested_vmcb_check_save(vcpu, save);
+}
+
 static
 void _nested_copy_vmcb_control_to_cache(struct vmcb_control_area *to,
 					struct vmcb_control_area *from)
@@ -694,7 +702,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
 	nested_copy_vmcb_control_to_cache(svm, &vmcb12->control);
 	nested_copy_vmcb_save_to_cache(svm, &vmcb12->save);
 
-	if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) ||
+	if (!nested_vmcb_check_save(vcpu) ||
 	    !nested_vmcb_check_controls(vcpu, &svm->nested.ctl)) {
 		vmcb12->control.exit_code    = SVM_EXIT_ERR;
 		vmcb12->control.exit_code_hi = 0;
@@ -1330,6 +1338,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 		&user_kvm_nested_state->data.svm[0];
 	struct vmcb_control_area *ctl;
 	struct vmcb_save_area *save;
+	struct vmcb_save_area_cached save_cached;
 	unsigned long cr0;
 	int ret;
 
@@ -1397,10 +1406,11 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	 * Validate host state saved from before VMRUN (see
 	 * nested_svm_check_permissions).
 	 */
+	_nested_copy_vmcb_save_to_cache(&save_cached, save);
 	if (!(save->cr0 & X86_CR0_PG) ||
 	    !(save->cr0 & X86_CR0_PE) ||
 	    (save->rflags & X86_EFLAGS_VM) ||
-	    !nested_vmcb_valid_sregs(vcpu, save))
+	    !_nested_vmcb_check_save(vcpu, &save_cached))
 		goto out_free;
 
 	/*
-- 
2.27.0

Re: [PATCH v5 4/7] nSVM: use vmcb_save_area_cached in nested_vmcb_valid_sregs()

[Maxim Levitsky]

On Wed, 2021-11-03 at 10:05 -0400, Emanuele Giuseppe Esposito wrote:
> Now that struct vmcb_save_area_cached contains the required
> vmcb fields values (done in nested_load_save_from_vmcb12()),
> check them to see if they are correct in nested_vmcb_valid_sregs().
> 
> While at it, rename nested_vmcb_valid_sregs in nested_vmcb_check_save.
> _nested_vmcb_check_save takes the additional @save parameter, so it
> is helpful when we want to check a non-svm save state, like in
> svm_set_nested_state. The reason for that is that save is the L1
> state, not L2, so we just check it.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  arch/x86/kvm/svm/nested.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 162b338a6c74..64fb43234e06 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -245,8 +245,8 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
>  }
>  
>  /* 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)
> +static bool _nested_vmcb_check_save(struct kvm_vcpu *vcpu,
> +				     struct vmcb_save_area_cached *save)
>  {
>  	/*
>  	 * FIXME: these should be done after copying the fields,
> @@ -286,6 +286,14 @@ static bool nested_vmcb_valid_sregs(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> +static bool nested_vmcb_check_save(struct kvm_vcpu *vcpu)
> +{
> +	struct vcpu_svm *svm = to_svm(vcpu);
> +	struct vmcb_save_area_cached *save = &svm->nested.save;
> +
> +	return _nested_vmcb_check_save(vcpu, save);
> +}
> +
>  static
>  void _nested_copy_vmcb_control_to_cache(struct vmcb_control_area *to,
>  					struct vmcb_control_area *from)
> @@ -694,7 +702,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
>  	nested_copy_vmcb_control_to_cache(svm, &vmcb12->control);
>  	nested_copy_vmcb_save_to_cache(svm, &vmcb12->save);
>  
> -	if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) ||
> +	if (!nested_vmcb_check_save(vcpu) ||
>  	    !nested_vmcb_check_controls(vcpu, &svm->nested.ctl)) {
>  		vmcb12->control.exit_code    = SVM_EXIT_ERR;
>  		vmcb12->control.exit_code_hi = 0;
> @@ -1330,6 +1338,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  		&user_kvm_nested_state->data.svm[0];
>  	struct vmcb_control_area *ctl;
>  	struct vmcb_save_area *save;
> +	struct vmcb_save_area_cached save_cached;
>  	unsigned long cr0;
>  	int ret;
>  
> @@ -1397,10 +1406,11 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  	 * Validate host state saved from before VMRUN (see
>  	 * nested_svm_check_permissions).
>  	 */


> +	_nested_copy_vmcb_save_to_cache(&save_cached, save);
Nitpick: here I would probalby add a comment that we are dealing here with L1
save area and we just want to check it and not cache it.

Also suddenly the name _nested_copy_vmcb_save_to_cache sounds a bit off,
but I don't have any better idea so I don't mind leaving it as is.

Anyway,

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

Best regards,
	Maxim Levitsky


>  	if (!(save->cr0 & X86_CR0_PG) ||
>  	    !(save->cr0 & X86_CR0_PE) ||
>  	    (save->rflags & X86_EFLAGS_VM) ||
> -	    !nested_vmcb_valid_sregs(vcpu, save))
> +	    !_nested_vmcb_check_save(vcpu, &save_cached))
>  		goto out_free;
>  
>  	/*

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

[Emanuele Giuseppe Esposito]

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@redhat.com>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/svm/nested.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 64fb43234e06..cdddd3258ddf 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -248,13 +248,6 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
 static bool _nested_vmcb_check_save(struct kvm_vcpu *vcpu,
 				     struct vmcb_save_area_cached *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;
 
@@ -511,15 +504,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;
 
@@ -534,8 +522,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);
 	}
 }
@@ -649,7 +637,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), from_vmrun);
 	if (ret)
 		return ret;
-- 
2.27.0

[PATCH v5 6/7] nSVM: introduce struct vmcb_ctrl_area_cached

[Emanuele Giuseppe Esposito]

This structure will replace vmcb_control_area in
svm_nested_state, providing only the fields that are actually
used by the nested state. This avoids having and copying around
uninitialized fields. The cost of this, however, is that all
functions (in this case vmcb_is_intercept) expect the old
structure, so they need to be duplicated.

Introduce also nested_copy_vmcb_cache_to_control(), useful to copy
vmcb_ctrl_area_cached fields in vmcb_control_area. This will
be used in the next patch.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 arch/x86/kvm/svm/nested.c | 34 ++++++++++++++++++++++++++++++++++
 arch/x86/kvm/svm/svm.h    | 31 +++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index cdddd3258ddf..cd15d5373c05 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -1261,6 +1261,40 @@ void nested_svm_update_tsc_ratio_msr(struct kvm_vcpu *vcpu)
 	svm_write_tsc_multiplier(vcpu, vcpu->arch.tsc_scaling_ratio);
 }
 
+/* Inverse operation of nested_copy_vmcb_control_to_cache(). asid is copied too. */
+static void nested_copy_vmcb_cache_to_control(struct vmcb_control_area *dst,
+					      struct vmcb_ctrl_area_cached *from)
+{
+	unsigned int i;
+
+	memset(dst, 0, sizeof(struct vmcb_control_area));
+
+	for (i = 0; i < MAX_INTERCEPT; i++)
+		dst->intercepts[i] = from->intercepts[i];
+
+	dst->iopm_base_pa         = from->iopm_base_pa;
+	dst->msrpm_base_pa        = from->msrpm_base_pa;
+	dst->tsc_offset           = from->tsc_offset;
+	dst->asid                 = from->asid;
+	dst->tlb_ctl              = from->tlb_ctl;
+	dst->int_ctl              = from->int_ctl;
+	dst->int_vector           = from->int_vector;
+	dst->int_state            = from->int_state;
+	dst->exit_code            = from->exit_code;
+	dst->exit_code_hi         = from->exit_code_hi;
+	dst->exit_info_1          = from->exit_info_1;
+	dst->exit_info_2          = from->exit_info_2;
+	dst->exit_int_info        = from->exit_int_info;
+	dst->exit_int_info_err    = from->exit_int_info_err;
+	dst->nested_ctl           = from->nested_ctl;
+	dst->event_inj            = from->event_inj;
+	dst->event_inj_err        = from->event_inj_err;
+	dst->nested_cr3           = from->nested_cr3;
+	dst->virt_ext              = from->virt_ext;
+	dst->pause_filter_count   = from->pause_filter_count;
+	dst->pause_filter_thresh  = from->pause_filter_thresh;
+}
+
 static int svm_get_nested_state(struct kvm_vcpu *vcpu,
 				struct kvm_nested_state __user *user_kvm_nested_state,
 				u32 user_data_size)
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index a0609fe2e68c..e29423d4337c 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -116,6 +116,31 @@ struct vmcb_save_area_cached {
 	u64 dr6;
 };
 
+struct vmcb_ctrl_area_cached {
+	u32 intercepts[MAX_INTERCEPT];
+	u16 pause_filter_thresh;
+	u16 pause_filter_count;
+	u64 iopm_base_pa;
+	u64 msrpm_base_pa;
+	u64 tsc_offset;
+	u32 asid;
+	u8 tlb_ctl;
+	u32 int_ctl;
+	u32 int_vector;
+	u32 int_state;
+	u32 exit_code;
+	u32 exit_code_hi;
+	u64 exit_info_1;
+	u64 exit_info_2;
+	u32 exit_int_info;
+	u32 exit_int_info_err;
+	u64 nested_ctl;
+	u32 event_inj;
+	u32 event_inj_err;
+	u64 nested_cr3;
+	u64 virt_ext;
+};
+
 struct svm_nested_state {
 	struct kvm_vmcb_info vmcb02;
 	u64 hsave_msr;
@@ -311,6 +336,12 @@ static inline bool vmcb_is_intercept(struct vmcb_control_area *control, u32 bit)
 	return test_bit(bit, (unsigned long *)&control->intercepts);
 }
 
+static inline bool vmcb12_is_intercept(struct vmcb_ctrl_area_cached *control, u32 bit)
+{
+	WARN_ON_ONCE(bit >= 32 * MAX_INTERCEPT);
+	return test_bit(bit, (unsigned long *)&control->intercepts);
+}
+
 static inline void set_dr_intercepts(struct vcpu_svm *svm)
 {
 	struct vmcb *vmcb = svm->vmcb01.ptr;
-- 
2.27.0

[PATCH v5 7/7] nSVM: use vmcb_ctrl_area_cached instead of vmcb_control_area in struct svm_nested_state

[Emanuele Giuseppe Esposito]

This requires changing all vmcb_is_intercept(&svm->nested.ctl, ...)
calls with vmcb12_is_intercept().

In addition, in svm_get_nested_state() user space expects a
vmcb_control_area struct, so we need to copy back all fields
in a temporary structure to provide to the user space.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 arch/x86/kvm/svm/nested.c | 48 +++++++++++++++++++++++++--------------
 arch/x86/kvm/svm/svm.c    |  4 ++--
 arch/x86/kvm/svm/svm.h    |  8 +++----
 3 files changed, 37 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index cd15d5373c05..6281d1877211 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -58,8 +58,9 @@ static void svm_inject_page_fault_nested(struct kvm_vcpu *vcpu, struct x86_excep
        struct vcpu_svm *svm = to_svm(vcpu);
        WARN_ON(!is_guest_mode(vcpu));
 
-       if (vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_EXCEPTION_OFFSET + PF_VECTOR) &&
-	   !svm->nested.nested_run_pending) {
+	if (vmcb12_is_intercept(&svm->nested.ctl,
+				INTERCEPT_EXCEPTION_OFFSET + PF_VECTOR) &&
+	    !svm->nested.nested_run_pending) {
                svm->vmcb->control.exit_code = SVM_EXIT_EXCP_BASE + PF_VECTOR;
                svm->vmcb->control.exit_code_hi = 0;
                svm->vmcb->control.exit_info_1 = fault->error_code;
@@ -121,7 +122,8 @@ static void nested_svm_uninit_mmu_context(struct kvm_vcpu *vcpu)
 
 void recalc_intercepts(struct vcpu_svm *svm)
 {
-	struct vmcb_control_area *c, *h, *g;
+	struct vmcb_control_area *c, *h;
+	struct vmcb_ctrl_area_cached *g;
 	unsigned int i;
 
 	vmcb_mark_dirty(svm->vmcb, VMCB_INTERCEPTS);
@@ -172,7 +174,7 @@ static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm)
 	 */
 	int i;
 
-	if (!(vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_MSR_PROT)))
+	if (!(vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_MSR_PROT)))
 		return true;
 
 	for (i = 0; i < MSRPM_OFFSETS; i++) {
@@ -220,9 +222,9 @@ static bool nested_svm_check_tlb_ctl(struct kvm_vcpu *vcpu, u8 tlb_ctl)
 }
 
 static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
-				       struct vmcb_control_area *control)
+				       struct vmcb_ctrl_area_cached *control)
 {
-	if (CC(!vmcb_is_intercept(control, INTERCEPT_VMRUN)))
+	if (CC(!vmcb12_is_intercept(control, INTERCEPT_VMRUN)))
 		return false;
 
 	if (CC(control->asid == 0))
@@ -288,7 +290,7 @@ static bool nested_vmcb_check_save(struct kvm_vcpu *vcpu)
 }
 
 static
-void _nested_copy_vmcb_control_to_cache(struct vmcb_control_area *to,
+void _nested_copy_vmcb_control_to_cache(struct vmcb_ctrl_area_cached *to,
 					struct vmcb_control_area *from)
 {
 	unsigned int i;
@@ -998,7 +1000,7 @@ static int nested_svm_exit_handled_msr(struct vcpu_svm *svm)
 	u32 offset, msr, value;
 	int write, mask;
 
-	if (!(vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_MSR_PROT)))
+	if (!(vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_MSR_PROT)))
 		return NESTED_EXIT_HOST;
 
 	msr    = svm->vcpu.arch.regs[VCPU_REGS_RCX];
@@ -1025,7 +1027,7 @@ static int nested_svm_intercept_ioio(struct vcpu_svm *svm)
 	u8 start_bit;
 	u64 gpa;
 
-	if (!(vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_IOIO_PROT)))
+	if (!(vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_IOIO_PROT)))
 		return NESTED_EXIT_HOST;
 
 	port = svm->vmcb->control.exit_info_1 >> 16;
@@ -1056,12 +1058,12 @@ static int nested_svm_intercept(struct vcpu_svm *svm)
 		vmexit = nested_svm_intercept_ioio(svm);
 		break;
 	case SVM_EXIT_READ_CR0 ... SVM_EXIT_WRITE_CR8: {
-		if (vmcb_is_intercept(&svm->nested.ctl, exit_code))
+		if (vmcb12_is_intercept(&svm->nested.ctl, exit_code))
 			vmexit = NESTED_EXIT_DONE;
 		break;
 	}
 	case SVM_EXIT_READ_DR0 ... SVM_EXIT_WRITE_DR7: {
-		if (vmcb_is_intercept(&svm->nested.ctl, exit_code))
+		if (vmcb12_is_intercept(&svm->nested.ctl, exit_code))
 			vmexit = NESTED_EXIT_DONE;
 		break;
 	}
@@ -1079,7 +1081,7 @@ static int nested_svm_intercept(struct vcpu_svm *svm)
 		break;
 	}
 	default: {
-		if (vmcb_is_intercept(&svm->nested.ctl, exit_code))
+		if (vmcb12_is_intercept(&svm->nested.ctl, exit_code))
 			vmexit = NESTED_EXIT_DONE;
 	}
 	}
@@ -1157,7 +1159,7 @@ static void nested_svm_inject_exception_vmexit(struct vcpu_svm *svm)
 
 static inline bool nested_exit_on_init(struct vcpu_svm *svm)
 {
-	return vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_INIT);
+	return vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_INIT);
 }
 
 static int svm_check_nested_events(struct kvm_vcpu *vcpu)
@@ -1300,6 +1302,8 @@ static int svm_get_nested_state(struct kvm_vcpu *vcpu,
 				u32 user_data_size)
 {
 	struct vcpu_svm *svm;
+	struct vmcb_control_area *ctl;
+	unsigned long r;
 	struct kvm_nested_state kvm_state = {
 		.flags = 0,
 		.format = KVM_STATE_NESTED_FORMAT_SVM,
@@ -1341,9 +1345,18 @@ static int svm_get_nested_state(struct kvm_vcpu *vcpu,
 	 */
 	if (clear_user(user_vmcb, KVM_STATE_NESTED_SVM_VMCB_SIZE))
 		return -EFAULT;
-	if (copy_to_user(&user_vmcb->control, &svm->nested.ctl,
-			 sizeof(user_vmcb->control)))
+
+	ctl = kzalloc(sizeof(*ctl), GFP_KERNEL);
+	if (!ctl)
+		return -ENOMEM;
+
+	nested_copy_vmcb_cache_to_control(ctl, &svm->nested.ctl);
+	r = copy_to_user(&user_vmcb->control, ctl,
+			 sizeof(user_vmcb->control));
+	kfree(ctl);
+	if (r)
 		return -EFAULT;
+
 	if (copy_to_user(&user_vmcb->save, &svm->vmcb01.ptr->save,
 			 sizeof(user_vmcb->save)))
 		return -EFAULT;
@@ -1361,6 +1374,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	struct vmcb_control_area *ctl;
 	struct vmcb_save_area *save;
 	struct vmcb_save_area_cached save_cached;
+	struct vmcb_ctrl_area_cached ctl_cached;
 	unsigned long cr0;
 	int ret;
 
@@ -1413,7 +1427,8 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 		goto out_free;
 
 	ret = -EINVAL;
-	if (!nested_vmcb_check_controls(vcpu, ctl))
+	_nested_copy_vmcb_control_to_cache(&ctl_cached, ctl);
+	if (!nested_vmcb_check_controls(vcpu, &ctl_cached))
 		goto out_free;
 
 	/*
@@ -1470,7 +1485,6 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 	svm->nested.vmcb12_gpa = kvm_state->hdr.svm.vmcb_pa;
 
 	svm_copy_vmrun_state(&svm->vmcb01.ptr->save, save);
-	nested_copy_vmcb_control_to_cache(svm, ctl);
 
 	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 74d6db9017ea..134205678462 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2506,7 +2506,7 @@ static bool check_selective_cr0_intercepted(struct kvm_vcpu *vcpu,
 	bool ret = false;
 
 	if (!is_guest_mode(vcpu) ||
-	    (!(vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_SELECTIVE_CR0))))
+	    (!(vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_SELECTIVE_CR0))))
 		return false;
 
 	cr0 &= ~SVM_CR0_SELECTIVE_MASK;
@@ -4218,7 +4218,7 @@ static int svm_check_intercept(struct kvm_vcpu *vcpu,
 		    info->intercept == x86_intercept_clts)
 			break;
 
-		if (!(vmcb_is_intercept(&svm->nested.ctl,
+		if (!(vmcb12_is_intercept(&svm->nested.ctl,
 					INTERCEPT_SELECTIVE_CR0)))
 			break;
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index e29423d4337c..a896a52417ee 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -156,7 +156,7 @@ struct svm_nested_state {
 	bool nested_run_pending;
 
 	/* cache for control fields of the guest */
-	struct vmcb_control_area ctl;
+	struct vmcb_ctrl_area_cached ctl;
 	struct vmcb_save_area_cached save;
 
 	bool initialized;
@@ -494,17 +494,17 @@ static inline bool nested_svm_virtualize_tpr(struct kvm_vcpu *vcpu)
 
 static inline bool nested_exit_on_smi(struct vcpu_svm *svm)
 {
-	return vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_SMI);
+	return vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_SMI);
 }
 
 static inline bool nested_exit_on_intr(struct vcpu_svm *svm)
 {
-	return vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_INTR);
+	return vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_INTR);
 }
 
 static inline bool nested_exit_on_nmi(struct vcpu_svm *svm)
 {
-	return vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_NMI);
+	return vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_NMI);
 }
 
 int enter_svm_guest_mode(struct kvm_vcpu *vcpu,
-- 
2.27.0

Re: [PATCH v5 7/7] nSVM: use vmcb_ctrl_area_cached instead of vmcb_control_area in struct svm_nested_state

[Maxim Levitsky]

On Wed, 2021-11-03 at 10:05 -0400, Emanuele Giuseppe Esposito wrote:
> This requires changing all vmcb_is_intercept(&svm->nested.ctl, ...)
> calls with vmcb12_is_intercept().
> 
> In addition, in svm_get_nested_state() user space expects a
> vmcb_control_area struct, so we need to copy back all fields
> in a temporary structure to provide to the user space.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  arch/x86/kvm/svm/nested.c | 48 +++++++++++++++++++++++++--------------
>  arch/x86/kvm/svm/svm.c    |  4 ++--
>  arch/x86/kvm/svm/svm.h    |  8 +++----
>  3 files changed, 37 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index cd15d5373c05..6281d1877211 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -58,8 +58,9 @@ static void svm_inject_page_fault_nested(struct kvm_vcpu *vcpu, struct x86_excep
>         struct vcpu_svm *svm = to_svm(vcpu);
>         WARN_ON(!is_guest_mode(vcpu));
>  
> -       if (vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_EXCEPTION_OFFSET + PF_VECTOR) &&
> -	   !svm->nested.nested_run_pending) {
> +	if (vmcb12_is_intercept(&svm->nested.ctl,
> +				INTERCEPT_EXCEPTION_OFFSET + PF_VECTOR) &&
> +	    !svm->nested.nested_run_pending) {
>                 svm->vmcb->control.exit_code = SVM_EXIT_EXCP_BASE + PF_VECTOR;
>                 svm->vmcb->control.exit_code_hi = 0;
>                 svm->vmcb->control.exit_info_1 = fault->error_code;
> @@ -121,7 +122,8 @@ static void nested_svm_uninit_mmu_context(struct kvm_vcpu *vcpu)
>  
>  void recalc_intercepts(struct vcpu_svm *svm)
>  {
> -	struct vmcb_control_area *c, *h, *g;
> +	struct vmcb_control_area *c, *h;
> +	struct vmcb_ctrl_area_cached *g;
>  	unsigned int i;
>  
>  	vmcb_mark_dirty(svm->vmcb, VMCB_INTERCEPTS);
> @@ -172,7 +174,7 @@ static bool nested_svm_vmrun_msrpm(struct vcpu_svm *svm)
>  	 */
>  	int i;
>  
> -	if (!(vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_MSR_PROT)))
> +	if (!(vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_MSR_PROT)))
>  		return true;
>  
>  	for (i = 0; i < MSRPM_OFFSETS; i++) {
> @@ -220,9 +222,9 @@ static bool nested_svm_check_tlb_ctl(struct kvm_vcpu *vcpu, u8 tlb_ctl)
>  }
>  
>  static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu,
> -				       struct vmcb_control_area *control)
> +				       struct vmcb_ctrl_area_cached *control)
>  {
> -	if (CC(!vmcb_is_intercept(control, INTERCEPT_VMRUN)))
> +	if (CC(!vmcb12_is_intercept(control, INTERCEPT_VMRUN)))
>  		return false;
>  
>  	if (CC(control->asid == 0))
> @@ -288,7 +290,7 @@ static bool nested_vmcb_check_save(struct kvm_vcpu *vcpu)
>  }
>  
>  static
> -void _nested_copy_vmcb_control_to_cache(struct vmcb_control_area *to,
> +void _nested_copy_vmcb_control_to_cache(struct vmcb_ctrl_area_cached *to,
>  					struct vmcb_control_area *from)
>  {
>  	unsigned int i;
> @@ -998,7 +1000,7 @@ static int nested_svm_exit_handled_msr(struct vcpu_svm *svm)
>  	u32 offset, msr, value;
>  	int write, mask;
>  
> -	if (!(vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_MSR_PROT)))
> +	if (!(vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_MSR_PROT)))
>  		return NESTED_EXIT_HOST;
>  
>  	msr    = svm->vcpu.arch.regs[VCPU_REGS_RCX];
> @@ -1025,7 +1027,7 @@ static int nested_svm_intercept_ioio(struct vcpu_svm *svm)
>  	u8 start_bit;
>  	u64 gpa;
>  
> -	if (!(vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_IOIO_PROT)))
> +	if (!(vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_IOIO_PROT)))
>  		return NESTED_EXIT_HOST;
>  
>  	port = svm->vmcb->control.exit_info_1 >> 16;
> @@ -1056,12 +1058,12 @@ static int nested_svm_intercept(struct vcpu_svm *svm)
>  		vmexit = nested_svm_intercept_ioio(svm);
>  		break;
>  	case SVM_EXIT_READ_CR0 ... SVM_EXIT_WRITE_CR8: {
> -		if (vmcb_is_intercept(&svm->nested.ctl, exit_code))
> +		if (vmcb12_is_intercept(&svm->nested.ctl, exit_code))
>  			vmexit = NESTED_EXIT_DONE;
>  		break;
>  	}
>  	case SVM_EXIT_READ_DR0 ... SVM_EXIT_WRITE_DR7: {
> -		if (vmcb_is_intercept(&svm->nested.ctl, exit_code))
> +		if (vmcb12_is_intercept(&svm->nested.ctl, exit_code))
>  			vmexit = NESTED_EXIT_DONE;
>  		break;
>  	}
> @@ -1079,7 +1081,7 @@ static int nested_svm_intercept(struct vcpu_svm *svm)
>  		break;
>  	}
>  	default: {
> -		if (vmcb_is_intercept(&svm->nested.ctl, exit_code))
> +		if (vmcb12_is_intercept(&svm->nested.ctl, exit_code))
>  			vmexit = NESTED_EXIT_DONE;
>  	}
>  	}
> @@ -1157,7 +1159,7 @@ static void nested_svm_inject_exception_vmexit(struct vcpu_svm *svm)
>  
>  static inline bool nested_exit_on_init(struct vcpu_svm *svm)
>  {
> -	return vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_INIT);
> +	return vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_INIT);
>  }
>  
>  static int svm_check_nested_events(struct kvm_vcpu *vcpu)
> @@ -1300,6 +1302,8 @@ static int svm_get_nested_state(struct kvm_vcpu *vcpu,
>  				u32 user_data_size)
>  {
>  	struct vcpu_svm *svm;
> +	struct vmcb_control_area *ctl;
> +	unsigned long r;
>  	struct kvm_nested_state kvm_state = {
>  		.flags = 0,
>  		.format = KVM_STATE_NESTED_FORMAT_SVM,
> @@ -1341,9 +1345,18 @@ static int svm_get_nested_state(struct kvm_vcpu *vcpu,
>  	 */
>  	if (clear_user(user_vmcb, KVM_STATE_NESTED_SVM_VMCB_SIZE))
>  		return -EFAULT;
> -	if (copy_to_user(&user_vmcb->control, &svm->nested.ctl,
> -			 sizeof(user_vmcb->control)))
> +
> +	ctl = kzalloc(sizeof(*ctl), GFP_KERNEL);
> +	if (!ctl)
> +		return -ENOMEM;
> +
> +	nested_copy_vmcb_cache_to_control(ctl, &svm->nested.ctl);
> +	r = copy_to_user(&user_vmcb->control, ctl,
> +			 sizeof(user_vmcb->control));
> +	kfree(ctl);
> +	if (r)
>  		return -EFAULT;
> +
>  	if (copy_to_user(&user_vmcb->save, &svm->vmcb01.ptr->save,
>  			 sizeof(user_vmcb->save)))
>  		return -EFAULT;
> @@ -1361,6 +1374,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  	struct vmcb_control_area *ctl;
>  	struct vmcb_save_area *save;
>  	struct vmcb_save_area_cached save_cached;
> +	struct vmcb_ctrl_area_cached ctl_cached;
>  	unsigned long cr0;
>  	int ret;
>  
> @@ -1413,7 +1427,8 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  		goto out_free;
>  
>  	ret = -EINVAL;
> -	if (!nested_vmcb_check_controls(vcpu, ctl))
> +	_nested_copy_vmcb_control_to_cache(&ctl_cached, ctl);
> +	if (!nested_vmcb_check_controls(vcpu, &ctl_cached))
>  		goto out_free;
>  
>  	/*
> @@ -1470,7 +1485,6 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>  	svm->nested.vmcb12_gpa = kvm_state->hdr.svm.vmcb_pa;
>  
>  	svm_copy_vmrun_state(&svm->vmcb01.ptr->save, save);
> -	nested_copy_vmcb_control_to_cache(svm, ctl);
You should keep this line of code, to actualy load the control area,

The whole copying to 'cache' is a bit ugly here, but let it be,
as there is no other way around and the uglyness is only limited to those two functions
(svm_get_nested_state/svm_set_nested_state).


I am currently testing your patches with the above change reverted. 
Seems to pass at least my basic test so far.

Thanks,
Best regards,
	Maxim Levitsky


>  
>  	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 74d6db9017ea..134205678462 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2506,7 +2506,7 @@ static bool check_selective_cr0_intercepted(struct kvm_vcpu *vcpu,
>  	bool ret = false;
>  
>  	if (!is_guest_mode(vcpu) ||
> -	    (!(vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_SELECTIVE_CR0))))
> +	    (!(vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_SELECTIVE_CR0))))
>  		return false;
>  
>  	cr0 &= ~SVM_CR0_SELECTIVE_MASK;
> @@ -4218,7 +4218,7 @@ static int svm_check_intercept(struct kvm_vcpu *vcpu,
>  		    info->intercept == x86_intercept_clts)
>  			break;
>  
> -		if (!(vmcb_is_intercept(&svm->nested.ctl,
> +		if (!(vmcb12_is_intercept(&svm->nested.ctl,
>  					INTERCEPT_SELECTIVE_CR0)))
>  			break;
>  
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index e29423d4337c..a896a52417ee 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -156,7 +156,7 @@ struct svm_nested_state {
>  	bool nested_run_pending;
>  
>  	/* cache for control fields of the guest */
> -	struct vmcb_control_area ctl;
> +	struct vmcb_ctrl_area_cached ctl;
>  	struct vmcb_save_area_cached save;
>  
>  	bool initialized;
> @@ -494,17 +494,17 @@ static inline bool nested_svm_virtualize_tpr(struct kvm_vcpu *vcpu)
>  
>  static inline bool nested_exit_on_smi(struct vcpu_svm *svm)
>  {
> -	return vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_SMI);
> +	return vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_SMI);
>  }
>  
>  static inline bool nested_exit_on_intr(struct vcpu_svm *svm)
>  {
> -	return vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_INTR);
> +	return vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_INTR);
>  }
>  
>  static inline bool nested_exit_on_nmi(struct vcpu_svm *svm)
>  {
> -	return vmcb_is_intercept(&svm->nested.ctl, INTERCEPT_NMI);
> +	return vmcb12_is_intercept(&svm->nested.ctl, INTERCEPT_NMI);
>  }
>  
>  int enter_svm_guest_mode(struct kvm_vcpu *vcpu,

Re: [PATCH v5 0/7] KVM: nSVM: avoid TOC/TOU race when checking vmcb12

[Paolo Bonzini]

On 11/3/21 15:05, Emanuele Giuseppe Esposito wrote:
> 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 series introduces 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.
> 
> Similarly, svm->nested.ctl contains fields that are not used, so having
> a full vmcb_control_area means passing uninitialized fields.
> 
> Patches 1,3 and 8 take care of renaming and refactoring code.
> Patches 2 and 6 introduce respectively vmcb_ctrl_area_cached and
> vmcb_save_area_cached.
> Patches 4 and 5 use vmcb_save_area_cached to avoid TOC/TOU, and patch
> 7 uses vmcb_ctrl_area_cached.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Queued (for 5.17).  I changed the helper functions to have a "__" prefix 
since that's more common in Linux.

Paolo

> 
> ---
> v5:
> * rebased on kvm/queue branch
> 
> v4:
> * introduce _* helpers (_nested_vmcb_check_save,
>    _nested_copy_vmcb_control_to_cache, _nested_copy_vmcb_save_to_cache)
>    that take care of additional parameters.
> * svm_set_nested_state: introduce {save, ctl}_cached variables
>    to not pollute svm->nested.{save,ctl} state, especially if the
>    check fails. remove also unnecessary memset added in previous versions.
> * svm_get_nested_state: change stack variable ctl introduced in this series
>   into a pointer that will be zeroed and freed after it has been copied to user
> 
> v3:
> * merge this series with "KVM: nSVM: use vmcb_ctrl_area_cached instead
>    of vmcb_control_area in nested state"
> * rename "nested_load_save_from_vmcb12" in
>    "nested_copy_vmcb_save_to_cache"
> * rename "nested_load_control_from_vmcb12" in
>    "nested_copy_vmcb_control_to_cache"
> * change check functions (nested_vmcb_valid_sregs and nested_vmcb_valid_sregs)
>    to accept only the vcpu parameter, since we only check
>    nested state now
> * rename "vmcb_is_intercept_cached" in "vmcb12_is_intercept"
>    and duplicate the implementation instead of calling vmcb_is_intercept
> 
> v2:
> * svm->nested.save is a separate struct vmcb_save_area_cached,
>    and not vmcb_save_area.
> * update also vmcb02->cr3 with svm->nested.save.cr3
> 
> 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 (7):
>    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: rename nested_load_control_from_vmcb12 in
>      nested_copy_vmcb_control_to_cache
>    nSVM: use vmcb_save_area_cached in nested_vmcb_valid_sregs()
>    nSVM: use svm->nested.save to load vmcb12 registers and avoid TOC/TOU
>      races
>    nSVM: introduce struct vmcb_ctrl_area_cached
>    nSVM: use vmcb_ctrl_area_cached instead of vmcb_control_area in struct
>      svm_nested_state
> 
>   arch/x86/kvm/svm/nested.c | 250 ++++++++++++++++++++++++--------------
>   arch/x86/kvm/svm/svm.c    |   7 +-
>   arch/x86/kvm/svm/svm.h    |  59 ++++++++-
>   3 files changed, 213 insertions(+), 103 deletions(-)
>