linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Maxim Levitsky <mlevitsk@redhat.com>
To: Vitaly Kuznetsov <vkuznets@redhat.com>,
	kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>
Cc: Sean Christopherson <seanjc@google.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>,
	Cathy Avery <cavery@redhat.com>,
	Emanuele Giuseppe Esposito <eesposit@redhat.com>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	Michael Roth <mdroth@linux.vnet.ibm.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 5/6] KVM: nSVM: Restore nested control upon leaving SMM
Date: Wed, 07 Jul 2021 13:35:33 +0300	[thread overview]
Message-ID: <e9910deb51f0b0163167b45251f7582dedeb9eed.camel@redhat.com> (raw)
In-Reply-To: <20210628104425.391276-6-vkuznets@redhat.com>

On Mon, 2021-06-28 at 12:44 +0200, Vitaly Kuznetsov wrote:
> In case nested state was saved/resored while in SMM,
> nested_load_control_from_vmcb12() which sets svm->nested.ctl was never
> called and the first nested_vmcb_check_controls() (either from
> nested_svm_vmrun() or from svm_set_nested_state() if save/restore
> cycle is repeated) is doomed to fail.

I don't like the commit description.

I propose something like that:

If the VM was migrated while in SMM, no nested state was saved/restored,
and therefore svm_leave_smm has to load both save and control area
of the vmcb12. Save area is already loaded from HSAVE area,
so now load the control area as well from the vmcb12.

However if you like to, feel free to leave the commit message as is.
My point is that while in SMM, SVM is fully disabled, so not only
svm->nested.ctl is not set but no nested state is loaded/stored at all.

Also this makes the svm_leave_smm even more dangerous versus errors,
as I said in previos patch. Since its return value is ignored,
and we are loading here the guest vmcb01 which can change under
our feet, lots of fun can happen (enter_svm_guest_mode result
isn't really checked).

Best regards,
	Maxim Levitsky 

> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  arch/x86/kvm/svm/nested.c | 4 ++--
>  arch/x86/kvm/svm/svm.c    | 7 ++++++-
>  arch/x86/kvm/svm/svm.h    | 2 ++
>  3 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index a1dec2c40181..6549e40155fa 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -304,8 +304,8 @@ static bool nested_vmcb_valid_sregs(struct kvm_vcpu *vcpu,
>  	return true;
>  }
>  
> -static void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
> -					    struct vmcb_control_area *control)
> +void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
> +				     struct vmcb_control_area *control)
>  {
>  	copy_vmcb_control_area(&svm->nested.ctl, control);
>  
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index fbf1b352a9bb..525b07873927 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4344,6 +4344,7 @@ static int svm_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
>  		u64 saved_efer = GET_SMSTATE(u64, smstate, 0x7ed0);
>  		u64 guest = GET_SMSTATE(u64, smstate, 0x7ed8);
>  		u64 vmcb12_gpa = GET_SMSTATE(u64, smstate, 0x7ee0);
> +		struct vmcb *vmcb12;
>  
>  		if (guest) {
>  			if (!guest_cpuid_has(vcpu, X86_FEATURE_SVM))
> @@ -4359,7 +4360,11 @@ static int svm_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
>  			if (svm_allocate_nested(svm))
>  				return 1;
>  
> -			ret = enter_svm_guest_mode(vcpu, vmcb12_gpa, map.hva);
> +			vmcb12 = map.hva;
> +
> +			nested_load_control_from_vmcb12(svm, &vmcb12->control);
> +
> +			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 ff2dac2b23b6..13f2d465ca36 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -481,6 +481,8 @@ int nested_svm_check_permissions(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);
> +void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
> +				     struct vmcb_control_area *control);
>  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);



  reply	other threads:[~2021-07-07 10:35 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-28 10:44 [PATCH 0/6] KVM: nSVM: Fix issues when SMM is entered from L2 Vitaly Kuznetsov
2021-06-28 10:44 ` [PATCH 1/6] KVM: nSVM: Check the value written to MSR_VM_HSAVE_PA Vitaly Kuznetsov
2021-07-07 10:28   ` Maxim Levitsky
2021-07-08 17:27     ` Paolo Bonzini
2021-07-09  6:08       ` Maxim Levitsky
2021-06-28 10:44 ` [PATCH 2/6] KVM: nSVM: Check that VM_HSAVE_PA MSR was set before VMRUN Vitaly Kuznetsov
2021-07-07 10:28   ` Maxim Levitsky
2021-06-28 10:44 ` [PATCH 3/6] KVM: nSVM: Introduce svm_copy_nonvmloadsave_state() Vitaly Kuznetsov
2021-07-05 12:08   ` Paolo Bonzini
2021-07-07 10:29     ` Maxim Levitsky
2021-06-28 10:44 ` [PATCH 4/6] KVM: nSVM: Fix L1 state corruption upon return from SMM Vitaly Kuznetsov
2021-07-07 10:32   ` Maxim Levitsky
2021-06-28 10:44 ` [PATCH 5/6] KVM: nSVM: Restore nested control upon leaving SMM Vitaly Kuznetsov
2021-07-07 10:35   ` Maxim Levitsky [this message]
2021-06-28 10:44 ` [PATCH 6/6] KVM: selftests: smm_test: Test SMM enter from L2 Vitaly Kuznetsov
2021-07-07 10:35   ` Maxim Levitsky
2021-07-08 17:40 ` [PATCH 0/6] KVM: nSVM: Fix issues when SMM is entered " Paolo Bonzini

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=e9910deb51f0b0163167b45251f7582dedeb9eed.camel@redhat.com \
    --to=mlevitsk@redhat.com \
    --cc=cavery@redhat.com \
    --cc=eesposit@redhat.com \
    --cc=jmattson@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mdroth@linux.vnet.ibm.com \
    --cc=pbonzini@redhat.com \
    --cc=seanjc@google.com \
    --cc=thomas.lendacky@amd.com \
    --cc=vkuznets@redhat.com \
    --cc=wanpengli@tencent.com \
    --subject='Re: [PATCH 5/6] KVM: nSVM: Restore nested control upon leaving SMM' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).