All of lore.kernel.org
 help / color / mirror / Atom feed
From: Will Deacon <will@kernel.org>
To: kvmarm@lists.cs.columbia.edu
Cc: kernel-team@android.com, Marc Zyngier <maz@kernel.org>,
	Will Deacon <will@kernel.org>,
	linux-arm-kernel@lists.infradead.org
Subject: [PATCH 7/7] KVM: arm64: Separate write faults on read-only memslots from MMIO
Date: Fri, 24 Jul 2020 15:35:06 +0100	[thread overview]
Message-ID: <20200724143506.17772-8-will@kernel.org> (raw)
In-Reply-To: <20200724143506.17772-1-will@kernel.org>

Although taking a write-fault on a read-only memslot triggers an MMIO
exit back to userspace, lumping the handling together in
kvm_handle_guest_abort() causes some of the early triage to have weird
effects on userspace. For example, if a guest generates a stage-2 fault
on a stage-1 translation fault when trying to fetch an instruction from
a read-only memslot, it will be mistakenly treated as an attempt to
execute from MMIO and a prefetch abort will be re-injected into the
guest.

Separate the MMIO handling from the read-only memslot handling, so that
the latter is left entirely up to userspace. Note that this _will_
result in more exits than before for read-only memslots, since userspace
will now see some cache maintenance and instruction-side aborts.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Quentin Perret <qperret@google.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kvm/mmu.c | 45 ++++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 9e72e7f4a2c2..2edc6f2412dc 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -2117,9 +2117,30 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	memslot = gfn_to_memslot(vcpu->kvm, gfn);
 	hva = gfn_to_hva_memslot_prot(memslot, gfn, &writable);
 	write_fault = kvm_is_write_fault(vcpu);
-	if (kvm_is_error_hva(hva) || (write_fault && !writable)) {
+
+	/*
+	 * The IPA is reported as [MAX:12], so we need to complement it with
+	 * the bottom 12 bits from the faulting VA. This is always 12 bits,
+	 * irrespective of the page size.
+	 */
+	fault_ipa |= kvm_vcpu_get_hfar(vcpu) & ((1 << 12) - 1);
+
+	/*
+	 * We can perform some early fault triage based purely on the memslot
+	 * information:
+	 *
+	 * Faults on IPAs falling outside of any memslot are re-injected
+	 * into the guest as external aborts if they were either signalled as
+	 * instruction aborts or as a stage-2 fault on a translation table walk.
+	 * If the instruction was a cache maintenance instruction then it is
+	 * quietly skipped, otherwise we exit to userspace for MMIO emulation.
+	 *
+	 * Write faults on IPAs falling within a read-only memslot are reported
+	 * to userspace as MMIO exits. This includes cache maintenance and
+	 * stage-2 faults on translation table walks,
+	 */
+	if (kvm_is_error_hva(hva)) {
 		if (kvm_vcpu_trap_is_iabt(vcpu)) {
-			/* Prefetch Abort on I/O address */
 			ret = -ENOEXEC;
 			goto out;
 		}
@@ -2129,30 +2150,12 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			goto out;
 		}
 
-		/*
-		 * Check for a cache maintenance operation. Since we
-		 * ended-up here, we know it is outside of any memory
-		 * slot. But we can't find out if that is for a device,
-		 * or if the guest is just being stupid. The only thing
-		 * we know for sure is that this range cannot be cached.
-		 *
-		 * So let's assume that the guest is just being
-		 * cautious, and skip the instruction.
-		 */
 		if (kvm_vcpu_dabt_is_cm(vcpu)) {
 			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
 			ret = 1;
 			goto out_unlock;
 		}
 
-		/*
-		 * The IPA is reported as [MAX:12], so we need to
-		 * complement it with the bottom 12 bits from the
-		 * faulting VA. This is always 12 bits, irrespective
-		 * of the page size.
-		 */
-		fault_ipa |= kvm_vcpu_get_hfar(vcpu) & ((1 << 12) - 1);
-
 		/*
 		 * No valid syndrome? Ask userspace for help if it has
 		 * volunteered to do so, and bail out otherwise.
@@ -2161,7 +2164,9 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			ret = handle_error_invalid_dabt(vcpu, run, fault_ipa);
 			goto out_unlock;
 		}
+	}
 
+	if (kvm_is_error_hva(hva) || (write_fault && !writable)) {
 		ret = io_mem_abort(vcpu, run, fault_ipa);
 		goto out_unlock;
 	}
-- 
2.28.0.rc0.142.g3c755180ce-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

WARNING: multiple messages have this Message-ID (diff)
From: Will Deacon <will@kernel.org>
To: kvmarm@lists.cs.columbia.edu
Cc: Quentin Perret <qperret@google.com>,
	kernel-team@android.com, Suzuki Poulose <suzuki.poulose@arm.com>,
	James Morse <james.morse@arm.com>, Marc Zyngier <maz@kernel.org>,
	Will Deacon <will@kernel.org>,
	linux-arm-kernel@lists.infradead.org
Subject: [PATCH 7/7] KVM: arm64: Separate write faults on read-only memslots from MMIO
Date: Fri, 24 Jul 2020 15:35:06 +0100	[thread overview]
Message-ID: <20200724143506.17772-8-will@kernel.org> (raw)
In-Reply-To: <20200724143506.17772-1-will@kernel.org>

Although taking a write-fault on a read-only memslot triggers an MMIO
exit back to userspace, lumping the handling together in
kvm_handle_guest_abort() causes some of the early triage to have weird
effects on userspace. For example, if a guest generates a stage-2 fault
on a stage-1 translation fault when trying to fetch an instruction from
a read-only memslot, it will be mistakenly treated as an attempt to
execute from MMIO and a prefetch abort will be re-injected into the
guest.

Separate the MMIO handling from the read-only memslot handling, so that
the latter is left entirely up to userspace. Note that this _will_
result in more exits than before for read-only memslots, since userspace
will now see some cache maintenance and instruction-side aborts.

Cc: Marc Zyngier <maz@kernel.org>
Cc: Quentin Perret <qperret@google.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 arch/arm64/kvm/mmu.c | 45 ++++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 9e72e7f4a2c2..2edc6f2412dc 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -2117,9 +2117,30 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	memslot = gfn_to_memslot(vcpu->kvm, gfn);
 	hva = gfn_to_hva_memslot_prot(memslot, gfn, &writable);
 	write_fault = kvm_is_write_fault(vcpu);
-	if (kvm_is_error_hva(hva) || (write_fault && !writable)) {
+
+	/*
+	 * The IPA is reported as [MAX:12], so we need to complement it with
+	 * the bottom 12 bits from the faulting VA. This is always 12 bits,
+	 * irrespective of the page size.
+	 */
+	fault_ipa |= kvm_vcpu_get_hfar(vcpu) & ((1 << 12) - 1);
+
+	/*
+	 * We can perform some early fault triage based purely on the memslot
+	 * information:
+	 *
+	 * Faults on IPAs falling outside of any memslot are re-injected
+	 * into the guest as external aborts if they were either signalled as
+	 * instruction aborts or as a stage-2 fault on a translation table walk.
+	 * If the instruction was a cache maintenance instruction then it is
+	 * quietly skipped, otherwise we exit to userspace for MMIO emulation.
+	 *
+	 * Write faults on IPAs falling within a read-only memslot are reported
+	 * to userspace as MMIO exits. This includes cache maintenance and
+	 * stage-2 faults on translation table walks,
+	 */
+	if (kvm_is_error_hva(hva)) {
 		if (kvm_vcpu_trap_is_iabt(vcpu)) {
-			/* Prefetch Abort on I/O address */
 			ret = -ENOEXEC;
 			goto out;
 		}
@@ -2129,30 +2150,12 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			goto out;
 		}
 
-		/*
-		 * Check for a cache maintenance operation. Since we
-		 * ended-up here, we know it is outside of any memory
-		 * slot. But we can't find out if that is for a device,
-		 * or if the guest is just being stupid. The only thing
-		 * we know for sure is that this range cannot be cached.
-		 *
-		 * So let's assume that the guest is just being
-		 * cautious, and skip the instruction.
-		 */
 		if (kvm_vcpu_dabt_is_cm(vcpu)) {
 			kvm_skip_instr(vcpu, kvm_vcpu_trap_il_is32bit(vcpu));
 			ret = 1;
 			goto out_unlock;
 		}
 
-		/*
-		 * The IPA is reported as [MAX:12], so we need to
-		 * complement it with the bottom 12 bits from the
-		 * faulting VA. This is always 12 bits, irrespective
-		 * of the page size.
-		 */
-		fault_ipa |= kvm_vcpu_get_hfar(vcpu) & ((1 << 12) - 1);
-
 		/*
 		 * No valid syndrome? Ask userspace for help if it has
 		 * volunteered to do so, and bail out otherwise.
@@ -2161,7 +2164,9 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 			ret = handle_error_invalid_dabt(vcpu, run, fault_ipa);
 			goto out_unlock;
 		}
+	}
 
+	if (kvm_is_error_hva(hva) || (write_fault && !writable)) {
 		ret = io_mem_abort(vcpu, run, fault_ipa);
 		goto out_unlock;
 	}
-- 
2.28.0.rc0.142.g3c755180ce-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  parent reply	other threads:[~2020-07-24 14:35 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-24 14:34 [PATCH 0/7] KVM: arm64: Fixes to early stage-2 fault handling Will Deacon
2020-07-24 14:34 ` Will Deacon
2020-07-24 14:35 ` [PATCH 1/7] KVM: arm64: Update comment when skipping guest MMIO access instruction Will Deacon
2020-07-24 14:35   ` Will Deacon
2020-07-26 11:08   ` Marc Zyngier
2020-07-26 11:08     ` Marc Zyngier
2020-07-27 10:30     ` Will Deacon
2020-07-27 10:30       ` Will Deacon
2020-07-24 14:35 ` [PATCH 2/7] KVM: arm64: Rename kvm_vcpu_dabt_isextabt() Will Deacon
2020-07-24 14:35   ` Will Deacon
2020-07-26 11:15   ` Marc Zyngier
2020-07-26 11:15     ` Marc Zyngier
2020-07-27 10:30     ` Will Deacon
2020-07-27 10:30       ` Will Deacon
2020-07-24 14:35 ` [PATCH 3/7] KVM: arm64: Handle data and instruction external aborts the same way Will Deacon
2020-07-24 14:35   ` Will Deacon
2020-07-24 14:35 ` [PATCH 4/7] KVM: arm64: Remove useless local variable Will Deacon
2020-07-24 14:35   ` Will Deacon
2020-07-24 14:35 ` [PATCH 5/7] KVM: arm64: Move 'invalid syndrome' logic out of io_mem_abort() Will Deacon
2020-07-24 14:35   ` Will Deacon
2020-07-26 11:55   ` Marc Zyngier
2020-07-26 11:55     ` Marc Zyngier
2020-07-27 10:31     ` Will Deacon
2020-07-27 10:31       ` Will Deacon
2020-07-24 14:35 ` [PATCH 6/7] KVM: arm64: Handle stage-2 faults on stage-1 page-table walks earlier Will Deacon
2020-07-24 14:35   ` Will Deacon
2020-07-26 13:38   ` Marc Zyngier
2020-07-26 13:38     ` Marc Zyngier
2020-07-27 10:29     ` Will Deacon
2020-07-27 10:29       ` Will Deacon
2020-07-24 14:35 ` Will Deacon [this message]
2020-07-24 14:35   ` [PATCH 7/7] KVM: arm64: Separate write faults on read-only memslots from MMIO Will Deacon

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=20200724143506.17772-8-will@kernel.org \
    --to=will@kernel.org \
    --cc=kernel-team@android.com \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=maz@kernel.org \
    /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.