From: Paolo Bonzini <pbonzini@redhat.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>,
Wanpeng Li <wanpengli@tencent.com>,
Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
syzbot+fb0b6a7e8713aeb0319c@syzkaller.appspotmail.com
Subject: Re: [PATCH 0/9] KVM: x86: Fix NULL pointer #GP due to RSM bug
Date: Thu, 10 Jun 2021 15:28:54 +0200 [thread overview]
Message-ID: <685b11c1-54a6-3a52-8157-4a10a95251ca@redhat.com> (raw)
In-Reply-To: <20210609185619.992058-1-seanjc@google.com>
On 09/06/21 20:56, Sean Christopherson wrote:
> Fix a NULL pointer dereference in gfn_to_rmap() that occurs if RSM fails,
> reported by syzbot.
>
> The immediate problem is that the MMU context's role gets out of sync
> because KVM clears the SMM flag in the vCPU at the start of RSM emulation,
> but only resets the MMU context if RSM succeeds. The divergence in vCPU
> vs. MMU role with respect to the SMM flag causes explosions if the non-SMM
> memslots have gfn ranges that are not present in the SMM memslots, because
> the MMU expects that the memslot for a shadow page cannot magically
> disappear.
>
> The other obvious problem is that KVM doesn't emulate triple fault on RSM
> failure, e.g. it keeps running the vCPU in a frankenstate instead of
> exiting to userspace. Fixing that would squash the syzbot repro, but
> would not fix the underlying issue because nothing prevents userspace from
> calling KVM_RUN on a vCPU that hit shutdown (yay lack of a shutdown state).
> But, it's easy to fix and definitely worth doing.
>
> Everything after the two bug fixes is cleanup.
>
> Ben Gardon has an internal patch or two that guards against the NULL
> pointer dereference in gfn_to_rmap(). I'm planning on getting that
> functionality posted (needs a little massaging) so that these types of
> snafus don't crash the host (this isn't the first time I've introduced a
> bug that broke gfn_to_rmap(), though thankfully it's the first time such
> a bug has made it upstream, knock on wood).
>
> Amusingly, adding gfn_to_rmap() NULL memslot checks might even be a
> performance improvement. Because gfn_to_rmap() doesn't check the memslot
> before using it, and because the compiler can see the search_memslots()
> returns NULL/0, gcc often/always generates dedicated (and hilarious) code
> for NULL, e.g. this #GP was caused by an explicit load from 0:
>
> 48 8b 14 25 00 00 00 00 mov 0x0,%rdx
>
>
> Sean Christopherson (9):
> KVM: x86: Immediately reset the MMU context when the SMM flag is
> cleared
> KVM: x86: Emulate triple fault shutdown if RSM emulation fails
> KVM: x86: Replace .set_hflags() with dedicated .exiting_smm() helper
> KVM: x86: Invoke kvm_smm_changed() immediately after clearing SMM flag
> KVM: x86: Move (most) SMM hflags modifications into kvm_smm_changed()
> KVM: x86: Move "entering SMM" tracepoint into kvm_smm_changed()
> KVM: x86: Rename SMM tracepoint to make it reflect reality
> KVM: x86: Drop .post_leave_smm(), i.e. the manual post-RSM MMU reset
> KVM: x86: Drop "pre_" from enter/leave_smm() helpers
>
> arch/x86/include/asm/kvm-x86-ops.h | 4 +--
> arch/x86/include/asm/kvm_host.h | 4 +--
> arch/x86/kvm/emulate.c | 31 ++++++++++-------
> arch/x86/kvm/kvm_emulate.h | 7 ++--
> arch/x86/kvm/svm/svm.c | 8 ++---
> arch/x86/kvm/trace.h | 2 +-
> arch/x86/kvm/vmx/vmx.c | 8 ++---
> arch/x86/kvm/x86.c | 53 +++++++++++++++---------------
> 8 files changed, 61 insertions(+), 56 deletions(-)
>
Queued 2-9 too for 5.14, with Vitaly's suggested change for patch 2.
Paolo
prev parent reply other threads:[~2021-06-10 13:29 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-09 18:56 [PATCH 0/9] KVM: x86: Fix NULL pointer #GP due to RSM bug Sean Christopherson
2021-06-09 18:56 ` [PATCH 1/9] KVM: x86: Immediately reset the MMU context when the SMM flag is cleared Sean Christopherson
2021-06-10 13:19 ` Paolo Bonzini
2021-06-09 18:56 ` [PATCH 2/9] KVM: x86: Emulate triple fault shutdown if RSM emulation fails Sean Christopherson
2021-06-10 8:26 ` Vitaly Kuznetsov
2021-06-10 13:29 ` Paolo Bonzini
2021-06-10 15:28 ` Sean Christopherson
2021-06-10 15:30 ` Sean Christopherson
2021-06-11 11:42 ` Vitaly Kuznetsov
2021-06-10 13:23 ` Paolo Bonzini
2021-06-09 18:56 ` [PATCH 3/9] KVM: x86: Replace .set_hflags() with dedicated .exiting_smm() helper Sean Christopherson
2021-06-09 18:56 ` [PATCH 4/9] KVM: x86: Invoke kvm_smm_changed() immediately after clearing SMM flag Sean Christopherson
2021-06-09 18:56 ` [PATCH 5/9] KVM: x86: Move (most) SMM hflags modifications into kvm_smm_changed() Sean Christopherson
2021-06-09 18:56 ` [PATCH 6/9] KVM: x86: Move "entering SMM" tracepoint " Sean Christopherson
2021-06-09 18:56 ` [PATCH 7/9] KVM: x86: Rename SMM tracepoint to make it reflect reality Sean Christopherson
2021-06-09 18:56 ` [PATCH 8/9] KVM: x86: Drop .post_leave_smm(), i.e. the manual post-RSM MMU reset Sean Christopherson
2021-06-09 18:56 ` [PATCH 9/9] KVM: x86: Drop "pre_" from enter/leave_smm() helpers Sean Christopherson
2021-06-10 13:28 ` Paolo Bonzini [this message]
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=685b11c1-54a6-3a52-8157-4a10a95251ca@redhat.com \
--to=pbonzini@redhat.com \
--cc=jmattson@google.com \
--cc=joro@8bytes.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=seanjc@google.com \
--cc=syzbot+fb0b6a7e8713aeb0319c@syzkaller.appspotmail.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 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).