linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sean Christopherson <sean.j.christopherson@intel.com>
To: "Paolo Bonzini" <pbonzini@redhat.com>,
	"Radim Krčmář" <rkrcmar@redhat.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>,
	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,
	Fuqian Huang <huangfq.daxian@gmail.com>
Subject: [PATCH v2] KVM: x86: Handle unexpected MMIO accesses using master abort semantics
Date: Thu, 12 Sep 2019 18:56:23 -0700	[thread overview]
Message-ID: <20190913015623.19869-1-sean.j.christopherson@intel.com> (raw)

Use master abort semantics, i.e. reads return all ones and writes are
dropped, to handle unexpected MMIO accesses when reading guest memory
instead of returning X86EMUL_IO_NEEDED, which in turn gets interpreted
as a guest page fault.

Emulation of certain instructions, notably VMX instructions, involves
reading or writing guest memory without going through the emulator.
These emulation flows are not equipped to handle MMIO accesses as no
sane and properly functioning guest kernel will target MMIO with such
instructions, and so simply inject a page fault in response to
X86EMUL_IO_NEEDED.

While not 100% correct, using master abort semantics is at least
sometimes correct, e.g. non-existent MMIO accesses do actually master
abort, whereas injecting a page fault is always wrong, i.e. the issue
lies in the physical address domain, not in the virtual to physical
translation.

Apply the logic to kvm_write_guest_virt_system() in addition to
replacing existing #PF logic in kvm_read_guest_virt(), as VMPTRST uses
the former, i.e. can also leak a host stack address.

Reported-by: Fuqian Huang <huangfq.daxian@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---

v2: Fix the comment for kvm_read_guest_virt_helper().

 arch/x86/kvm/x86.c | 40 +++++++++++++++++++++++++++++++---------
 1 file changed, 31 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b4cfd786d0b6..3da57f137470 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5234,16 +5234,24 @@ int kvm_read_guest_virt(struct kvm_vcpu *vcpu,
 			       struct x86_exception *exception)
 {
 	u32 access = (kvm_x86_ops->get_cpl(vcpu) == 3) ? PFERR_USER_MASK : 0;
+	int r;
+
+	r = kvm_read_guest_virt_helper(addr, val, bytes, vcpu, access,
+				       exception);
 
 	/*
-	 * FIXME: this should call handle_emulation_failure if X86EMUL_IO_NEEDED
-	 * is returned, but our callers are not ready for that and they blindly
-	 * call kvm_inject_page_fault.  Ensure that they at least do not leak
-	 * uninitialized kernel stack memory into cr2 and error code.
+	 * FIXME: this should technically call out to userspace to handle the
+	 * MMIO access, but our callers are not ready for that, so emulate
+	 * master abort behavior instead, i.e. reads return all ones.
 	 */
-	memset(exception, 0, sizeof(*exception));
-	return kvm_read_guest_virt_helper(addr, val, bytes, vcpu, access,
-					  exception);
+	if (r == X86EMUL_IO_NEEDED) {
+		memset(val, 0xff, bytes);
+		return 0;
+	}
+	if (r == X86EMUL_PROPAGATE_FAULT)
+		return -EFAULT;
+	WARN_ON_ONCE(r);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(kvm_read_guest_virt);
 
@@ -5317,11 +5325,25 @@ static int emulator_write_std(struct x86_emulate_ctxt *ctxt, gva_t addr, void *v
 int kvm_write_guest_virt_system(struct kvm_vcpu *vcpu, gva_t addr, void *val,
 				unsigned int bytes, struct x86_exception *exception)
 {
+	int r;
+
 	/* kvm_write_guest_virt_system can pull in tons of pages. */
 	vcpu->arch.l1tf_flush_l1d = true;
 
-	return kvm_write_guest_virt_helper(addr, val, bytes, vcpu,
-					   PFERR_WRITE_MASK, exception);
+	r = kvm_write_guest_virt_helper(addr, val, bytes, vcpu,
+					PFERR_WRITE_MASK, exception);
+
+	/*
+	 * FIXME: this should technically call out to userspace to handle the
+	 * MMIO access, but our callers are not ready for that, so emulate
+	 * master abort behavior instead, i.e. writes are dropped.
+	 */
+	if (r == X86EMUL_IO_NEEDED)
+		return 0;
+	if (r == X86EMUL_PROPAGATE_FAULT)
+		return -EFAULT;
+	WARN_ON_ONCE(r);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(kvm_write_guest_virt_system);
 
-- 
2.22.0


             reply	other threads:[~2019-09-13  1:56 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-13  1:56 Sean Christopherson [this message]
2019-09-13  9:10 ` [PATCH v2] KVM: x86: Handle unexpected MMIO accesses using master abort semantics 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=20190913015623.19869-1-sean.j.christopherson@intel.com \
    --to=sean.j.christopherson@intel.com \
    --cc=huangfq.daxian@gmail.com \
    --cc=jmattson@google.com \
    --cc=joro@8bytes.org \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=rkrcmar@redhat.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).