The VMCB12 is stored in guest memory and can be mangled while in SMM; it
is then reloaded by svm_leave_smm(), but it is not checked again for
validity.

Move the check code out of vmx_set_nested_state()
(the other "not a VMLAUNCH/VMRESUME" path that emulates a nested vmentry)
and reuse it in svm_leave_smm().

Cc: stable@vger.kernel.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/svm/nested.c | 12 ++++++++++--
 arch/x86/kvm/svm/svm.c    |  4 ++++
 arch/x86/kvm/svm/svm.h    |  1 +
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 7b61124051a7..de9906adb73b 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -419,6 +419,15 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu)
 	return __nested_vmcb_check_controls(vcpu, ctl);
 }
 
+int nested_svm_check_cached_vmcb12(struct kvm_vcpu *vcpu)
+{
+	if (!nested_vmcb_check_save(vcpu) ||
+	    !nested_vmcb_check_controls(vcpu))
+		return -EINVAL;
+
+	return 0;
+}
+
 /*
  * If a feature is not advertised to L1, clear the corresponding vmcb12
  * intercept.
@@ -1034,8 +1043,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_check_save(vcpu) ||
-	    !nested_vmcb_check_controls(vcpu)) {
+	if (nested_svm_check_cached_vmcb12(vcpu) < 0) {
 		vmcb12->control.exit_code    = SVM_EXIT_ERR;
 		vmcb12->control.exit_info_1  = 0;
 		vmcb12->control.exit_info_2  = 0;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 477fda63653b..95495048902c 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4890,6 +4890,10 @@ static int svm_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram)
 	vmcb12 = map.hva;
 	nested_copy_vmcb_control_to_cache(svm, &vmcb12->control);
 	nested_copy_vmcb_save_to_cache(svm, &vmcb12->save);
+
+	if (nested_svm_check_cached_vmcb12(vcpu) < 0)
+		goto unmap_save;
+
 	ret = enter_svm_guest_mode(vcpu, smram64->svm_guest_vmcb_gpa, vmcb12, false);
 
 	if (ret)
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index ebd7b36b1ceb..6942e6b0eda6 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -797,6 +797,7 @@ static inline int nested_svm_simple_vmexit(struct vcpu_svm *svm, u32 exit_code)
 
 int nested_svm_exit_handled(struct vcpu_svm *svm);
 int nested_svm_check_permissions(struct kvm_vcpu *vcpu);
+int nested_svm_check_cached_vmcb12(struct kvm_vcpu *vcpu);
 int nested_svm_check_exception(struct vcpu_svm *svm, unsigned nr,
 			       bool has_error_code, u32 error_code);
 int nested_svm_exit_special(struct vcpu_svm *svm);
-- 
2.53.0

On Tue, Mar 10, 2026 at 1:24 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> The VMCB12 is stored in guest memory and can be mangled while in SMM; it
> is then reloaded by svm_leave_smm(), but it is not checked again for
> validity.
>
> Move the check code out of vmx_set_nested_state()
> (the other "not a VMLAUNCH/VMRESUME" path that emulates a nested vmentry)
> and reuse it in svm_leave_smm().

This chunk probably needs to be:

Move the cached vmcb12 control and save consistency checks into a
helper and reuse it in svm_leave_smm().

>
> Cc: stable@vger.kernel.org
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/svm/nested.c | 12 ++++++++++--
>  arch/x86/kvm/svm/svm.c    |  4 ++++
>  arch/x86/kvm/svm/svm.h    |  1 +
>  3 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 7b61124051a7..de9906adb73b 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -419,6 +419,15 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu)
>         return __nested_vmcb_check_controls(vcpu, ctl);
>  }
>
> +int nested_svm_check_cached_vmcb12(struct kvm_vcpu *vcpu)
> +{
> +       if (!nested_vmcb_check_save(vcpu) ||
> +           !nested_vmcb_check_controls(vcpu))
> +               return -EINVAL;
> +
> +       return 0;
> +}

Nit: if we make this a boolean we could just do:

bool nested_svm_check_cached_vmcb12(struct kvm_vcpu *vcpu)
{
       return nested_vmcb_check_save(vcpu) && nested_vmcb_check_controls(vcpu);
}

> +
>  /*
>   * If a feature is not advertised to L1, clear the corresponding vmcb12
>   * intercept.
> @@ -1034,8 +1043,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_check_save(vcpu) ||
> -           !nested_vmcb_check_controls(vcpu)) {
> +       if (nested_svm_check_cached_vmcb12(vcpu) < 0) {
>                 vmcb12->control.exit_code    = SVM_EXIT_ERR;
>                 vmcb12->control.exit_info_1  = 0;
>                 vmcb12->control.exit_info_2  = 0;
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 477fda63653b..95495048902c 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4890,6 +4890,10 @@ static int svm_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram)
>         vmcb12 = map.hva;
>         nested_copy_vmcb_control_to_cache(svm, &vmcb12->control);
>         nested_copy_vmcb_save_to_cache(svm, &vmcb12->save);
> +
> +       if (nested_svm_check_cached_vmcb12(vcpu) < 0)
> +               goto unmap_save;
> +
>         ret = enter_svm_guest_mode(vcpu, smram64->svm_guest_vmcb_gpa, vmcb12, false);
>
>         if (ret)
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index ebd7b36b1ceb..6942e6b0eda6 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -797,6 +797,7 @@ static inline int nested_svm_simple_vmexit(struct vcpu_svm *svm, u32 exit_code)
>
>  int nested_svm_exit_handled(struct vcpu_svm *svm);
>  int nested_svm_check_permissions(struct kvm_vcpu *vcpu);
> +int nested_svm_check_cached_vmcb12(struct kvm_vcpu *vcpu);
>  int nested_svm_check_exception(struct vcpu_svm *svm, unsigned nr,
>                                bool has_error_code, u32 error_code);
>  int nested_svm_exit_special(struct vcpu_svm *svm);
> --
> 2.53.0
>
>

On Tue, Mar 10, 2026, Yosry Ahmed wrote:
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  arch/x86/kvm/svm/nested.c | 12 ++++++++++--
> >  arch/x86/kvm/svm/svm.c    |  4 ++++
> >  arch/x86/kvm/svm/svm.h    |  1 +
> >  3 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > index 7b61124051a7..de9906adb73b 100644
> > --- a/arch/x86/kvm/svm/nested.c
> > +++ b/arch/x86/kvm/svm/nested.c
> > @@ -419,6 +419,15 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu)
> >         return __nested_vmcb_check_controls(vcpu, ctl);
> >  }
> >
> > +int nested_svm_check_cached_vmcb12(struct kvm_vcpu *vcpu)
> > +{
> > +       if (!nested_vmcb_check_save(vcpu) ||
> > +           !nested_vmcb_check_controls(vcpu))
> > +               return -EINVAL;
> > +
> > +       return 0;
> > +}
> 
> Nit: if we make this a boolean we could just do:
> 
> bool nested_svm_check_cached_vmcb12(struct kvm_vcpu *vcpu)
> {
>        return nested_vmcb_check_save(vcpu) && nested_vmcb_check_controls(vcpu);

I don't care one way or the other for this particular patch, but once the dust
settles on nSVM (assuming it ever does) I do think we should align the "nested
check" return types across nVMX and nSVM (which is likely why Paolo ended up with
the above; I requested using -EINVAL for the nVMXx) patch.

My fairly strong preference is to use 0/-errno as "return -EINVAL" is more
obviously an error than "return true".  But we can bikeshed later :-)

Il mar 10 mar 2026, 22:45 Sean Christopherson <seanjc@google.com> ha scritto:
>
> On Tue, Mar 10, 2026, Yosry Ahmed wrote:
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > ---
> > >  arch/x86/kvm/svm/nested.c | 12 ++++++++++--
> > >  arch/x86/kvm/svm/svm.c    |  4 ++++
> > >  arch/x86/kvm/svm/svm.h    |  1 +
> > >  3 files changed, 15 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > > index 7b61124051a7..de9906adb73b 100644
> > > --- a/arch/x86/kvm/svm/nested.c
> > > +++ b/arch/x86/kvm/svm/nested.c
> > > @@ -419,6 +419,15 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu)
> > >         return __nested_vmcb_check_controls(vcpu, ctl);
> > >  }
> > >
> > > +int nested_svm_check_cached_vmcb12(struct kvm_vcpu *vcpu)
> > > +{
> > > +       if (!nested_vmcb_check_save(vcpu) ||
> > > +           !nested_vmcb_check_controls(vcpu))
> > > +               return -EINVAL;
> > > +
> > > +       return 0;
> > > +}
> >
> > Nit: if we make this a boolean we could just do:
> >
> > bool nested_svm_check_cached_vmcb12(struct kvm_vcpu *vcpu)
> > {
> >        return nested_vmcb_check_save(vcpu) && nested_vmcb_check_controls(vcpu);
>
> I don't care one way or the other for this particular patch, but once the dust
> settles on nSVM (assuming it ever does) I do think we should align the "nested
> check" return types across nVMX and nSVM (which is likely why Paolo ended up with
> the above; I requested using -EINVAL for the nVMXx) patch.

I was indeed aiming for more similar code between the two. The last
few nSVM shakedowns prior to Yosry's (nested_run_pending/live
migration, vmcb01/02 split) already took some inspiration from nVMX
code and naming, so most of the low hanging fruit is gone and I didn't
want to actually make things worse...

Paolo


> My fairly strong preference is to use 0/-errno as "return -EINVAL" is more
> obviously an error than "return true".  But we can bikeshed later :-)
>

On Tue, Mar 10, 2026 at 2:45 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Tue, Mar 10, 2026, Yosry Ahmed wrote:
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > ---
> > >  arch/x86/kvm/svm/nested.c | 12 ++++++++++--
> > >  arch/x86/kvm/svm/svm.c    |  4 ++++
> > >  arch/x86/kvm/svm/svm.h    |  1 +
> > >  3 files changed, 15 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > > index 7b61124051a7..de9906adb73b 100644
> > > --- a/arch/x86/kvm/svm/nested.c
> > > +++ b/arch/x86/kvm/svm/nested.c
> > > @@ -419,6 +419,15 @@ static bool nested_vmcb_check_controls(struct kvm_vcpu *vcpu)
> > >         return __nested_vmcb_check_controls(vcpu, ctl);
> > >  }
> > >
> > > +int nested_svm_check_cached_vmcb12(struct kvm_vcpu *vcpu)
> > > +{
> > > +       if (!nested_vmcb_check_save(vcpu) ||
> > > +           !nested_vmcb_check_controls(vcpu))
> > > +               return -EINVAL;
> > > +
> > > +       return 0;
> > > +}
> >
> > Nit: if we make this a boolean we could just do:
> >
> > bool nested_svm_check_cached_vmcb12(struct kvm_vcpu *vcpu)
> > {
> >        return nested_vmcb_check_save(vcpu) && nested_vmcb_check_controls(vcpu);
>
> I don't care one way or the other for this particular patch, but once the dust
> settles on nSVM (assuming it ever does) I do think we should align the "nested
> check" return types across nVMX and nSVM (which is likely why Paolo ended up with
> the above; I requested using -EINVAL for the nVMXx) patch.
>
> My fairly strong preference is to use 0/-errno as "return -EINVAL" is more
> obviously an error than "return true".  But we can bikeshed later :-)

No objections, I was strictly going for more concise code and it
happened to also keep the existing return type (instead of translating
boolean to int).