All of lore.kernel.org
 help / color / mirror / Atom feed
From: Maxim Levitsky <mlevitsk@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	kvm@vger.kernel.org
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>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH RFC] KVM: nSVM: Fix L1 state corruption upon return from SMM
Date: Wed, 23 Jun 2021 16:01:48 +0300	[thread overview]
Message-ID: <53a9f893cb895f4b52e16c374cbe988607925cdf.camel@redhat.com> (raw)
In-Reply-To: <a3918bfa-7b4f-c31a-448a-aa22a44d4dfd@redhat.com>

On Wed, 2021-06-23 at 11:39 +0200, Paolo Bonzini wrote:
> On 23/06/21 09:44, Vitaly Kuznetsov wrote:
> > - RFC: I'm not 100% sure my 'smart' idea to use currently-unused HSAVE area
> > is that smart. Also, we don't even seem to check that L1 set it up upon
> > nested VMRUN so hypervisors which don't do that may remain broken. A very
> > much needed selftest is also missing.
> 
> It's certainly a bit weird, but I guess it counts as smart too.  It 
> needs a few more comments, but I think it's a good solution.
> 
> One could delay the backwards memcpy until vmexit time, but that would 
> require a new flag so it's not worth it for what is a pretty rare and 
> already expensive case.
> 
> Paolo
> 

Hi!

I did some homework on this now and I would like to share few my thoughts on this:

First of all my attention caught the way we intercept the #SMI
(this isn't 100% related to the bug but still worth talking about IMHO)

A. Bare metal: Looks like SVM allows to intercept SMI, with SVM_EXIT_SMI, 
 with an intention of then entering the BIOS SMM handler manually using the SMM_CTL msr.
 On bare metal we do set the INTERCEPT_SMI but we emulate the exit as a nop.
 I guess on bare metal there are some undocumented bits that BIOS set which
 make the CPU to ignore that SMI intercept and still take the #SMI handler,
 normally but I wonder if we could still break some motherboard
 code due to that.


B. Nested: If #SMI is intercepted, then it causes nested VMEXIT.
 Since KVM does enable SMI intercept, when it runs nested it means that all SMIs 
 that nested KVM gets are emulated as NOP, and L1's SMI handler is not run.


About the issue that was fixed in this patch. Let me try to understand how
it would work on bare metal:

1. A guest is entered. Host state is saved to VM_HSAVE_PA area (or stashed somewhere
  in the CPU)

2. #SMI (without intercept) happens

3. CPU has to exit SVM, and start running the host SMI handler, it loads the SMM
    state without touching the VM_HSAVE_PA runs the SMI handler, then once it RSMs,
    it restores the guest state from SMM area and continues the guest

4. Once a normal VMexit happens, the host state is restored from VM_HSAVE_PA

So host state indeed can't be saved to VMC01.

I to be honest think would prefer not to use the L1's hsave area but rather add back our
'hsave' in KVM and store there the L1 host state on the nested entry always.

This way we will avoid touching the vmcb01 at all and both solve the issue and 
reduce code complexity.
(copying of L1 host state to what basically is L1 guest state area and back
even has a comment to explain why it (was) possible to do so.
(before you discovered that this doesn't work with SMM).

Thanks again for fixing this bug!

Best regards,
	Maxim Levitsky


  parent reply	other threads:[~2021-06-23 13:02 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-23  7:44 [PATCH RFC] KVM: nSVM: Fix L1 state corruption upon return from SMM Vitaly Kuznetsov
2021-06-23  9:39 ` Paolo Bonzini
2021-06-23 11:39   ` Maxim Levitsky
2021-06-23 12:00     ` Paolo Bonzini
2021-06-23 13:01   ` Maxim Levitsky [this message]
2021-06-23 13:07     ` Maxim Levitsky
2021-06-23 13:32       ` Vitaly Kuznetsov
2021-06-23 14:41         ` Maxim Levitsky
2021-06-23 16:10           ` Sean Christopherson
2021-06-23 16:21             ` Sean Christopherson
2021-06-23 20:37               ` Paolo Bonzini
2021-06-24  7:41                 ` Vitaly Kuznetsov
2021-06-24  8:20                 ` Maxim Levitsky
2021-06-24 10:38                   ` Paolo Bonzini
2021-06-24 14:32                     ` Tom Lendacky
2021-06-24 15:36                       ` Maxim Levitsky
2021-06-23 13:21     ` Paolo Bonzini
2021-06-23 14:06       ` Maxim Levitsky

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=53a9f893cb895f4b52e16c374cbe988607925cdf.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=pbonzini@redhat.com \
    --cc=seanjc@google.com \
    --cc=vkuznets@redhat.com \
    --cc=wanpengli@tencent.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.