Re: [PATCH 2/5] KVM: SVM: check validity of VMCB when returning from SMM

[Yosry Ahmed]

On Tue, Mar 10, 2026 at 1:24 PM Paolo Bonzini <pbonzini@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> ---
>  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
>
>