[PATCH 0/2] KVM: arm64: Fix splat/misbehavior for MMIO SEA injection

[PATCH 0/2] KVM: arm64: Fix splat/misbehavior for MMIO SEA injection

[Oliver Upton]

syzkaller continues to find more interesting bugs, this time due to a
decently-sized hole in our UAPI.

Turns out we still go through the motions of completing MMIO emulation
even if userspace has pended a *synchronous* external abort in response
to an unexpected access. Oops!

In addition to the fix, I felt this warranted a selftest because a
documented UAPI flow has been broken for a while.

Marc, I'll probably take this in 6.13 since we've still got quite a bit
outstanding for 6.12 and this isn't *terribly* urgent.

Oliver Upton (2):
  KVM: arm64: Don't retire aborted MMIO instruction
  KVM: arm64: selftests: Add tests for MMIO external abort injection

 arch/arm64/include/asm/kvm_emulate.h          |  25 +++
 arch/arm64/kvm/mmio.c                         |   7 +-
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/aarch64/mmio_abort.c        | 158 ++++++++++++++++++
 4 files changed, 189 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/aarch64/mmio_abort.c


base-commit: 78a00555550042ed77b33ace7423aced228b3b4e
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH 1/2] KVM: arm64: Don't retire aborted MMIO instruction

[Oliver Upton]

Returning an abort to the guest for an unsupported MMIO access is a
documented feature of the KVM UAPI. Nevertheless, it's clear that this
plumbing has seen limited testing, since userspace can trivially cause a
WARN in the MMIO return:

  WARNING: CPU: 0 PID: 30558 at arch/arm64/include/asm/kvm_emulate.h:536 kvm_handle_mmio_return+0x46c/0x5c4 arch/arm64/include/asm/kvm_emulate.h:536
  Call trace:
   kvm_handle_mmio_return+0x46c/0x5c4 arch/arm64/include/asm/kvm_emulate.h:536
   kvm_arch_vcpu_ioctl_run+0x98/0x15b4 arch/arm64/kvm/arm.c:1133
   kvm_vcpu_ioctl+0x75c/0xa78 virt/kvm/kvm_main.c:4487
   __do_sys_ioctl fs/ioctl.c:51 [inline]
   __se_sys_ioctl fs/ioctl.c:893 [inline]
   __arm64_sys_ioctl+0x14c/0x1c8 fs/ioctl.c:893
   __invoke_syscall arch/arm64/kernel/syscall.c:35 [inline]
   invoke_syscall+0x98/0x2b8 arch/arm64/kernel/syscall.c:49
   el0_svc_common+0x1e0/0x23c arch/arm64/kernel/syscall.c:132
   do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:151
   el0_svc+0x38/0x68 arch/arm64/kernel/entry-common.c:712
   el0t_64_sync_handler+0x90/0xfc arch/arm64/kernel/entry-common.c:730
   el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:598

The splat is complaining that KVM is advancing PC while an exception is
pending, i.e. that KVM is retiring the MMIO instruction despite a
pending external abort. Womp womp.

Fix the glaring UAPI bug by skipping over all the MMIO emulation in
case there is a pending synchronous exception. Note that while userspace
is capable of pending an asynchronous exception (SError, IRQ, or FIQ),
it is still safe to retire the MMIO instruction in this case as (1) they
are by definition asynchronous, and (2) KVM relies on hardware support
for pending/delivering these exceptions instead of the software state
machine for advancing PC.

Cc: stable@vger.kernel.org
Fixes: da345174ceca ("KVM: arm/arm64: Allow user injection of external data aborts")
Reported-by: Alexander Potapenko <glider@google.com>
Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 arch/arm64/include/asm/kvm_emulate.h | 25 +++++++++++++++++++++++++
 arch/arm64/kvm/mmio.c                |  7 +++++--
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index a601a9305b10..1b229099f684 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -544,6 +544,31 @@ static __always_inline void kvm_incr_pc(struct kvm_vcpu *vcpu)
 		vcpu_set_flag((v), e);					\
 	} while (0)
 
+static inline bool kvm_pending_sync_exception(struct kvm_vcpu *vcpu)
+{
+	if (!vcpu_get_flag(vcpu, PENDING_EXCEPTION))
+		return false;
+
+	if (vcpu_el1_is_32bit(vcpu)) {
+		switch (vcpu_get_flag(vcpu, EXCEPT_MASK)) {
+		case unpack_vcpu_flag(EXCEPT_AA32_UND):
+		case unpack_vcpu_flag(EXCEPT_AA32_IABT):
+		case unpack_vcpu_flag(EXCEPT_AA32_DABT):
+			return true;
+		default:
+			return false;
+		}
+	} else {
+		switch (vcpu_get_flag(vcpu, EXCEPT_MASK)) {
+		case unpack_vcpu_flag(EXCEPT_AA64_EL1_SYNC):
+		case unpack_vcpu_flag(EXCEPT_AA64_EL2_SYNC):
+			return true;
+		default:
+			return false;
+		}
+	}
+}
+
 #define __build_check_all_or_none(r, bits)				\
 	BUILD_BUG_ON(((r) & (bits)) && ((r) & (bits)) != (bits))
 
diff --git a/arch/arm64/kvm/mmio.c b/arch/arm64/kvm/mmio.c
index cd6b7b83e2c3..0155ba665717 100644
--- a/arch/arm64/kvm/mmio.c
+++ b/arch/arm64/kvm/mmio.c
@@ -84,8 +84,11 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu)
 	unsigned int len;
 	int mask;
 
-	/* Detect an already handled MMIO return */
-	if (unlikely(!vcpu->mmio_needed))
+	/*
+	 * Detect if the MMIO return was already handled or if userspace aborted
+	 * the MMIO access.
+	 */
+	if (unlikely(!vcpu->mmio_needed || kvm_pending_sync_exception(vcpu)))
 		return 1;
 
 	vcpu->mmio_needed = 0;
-- 
2.47.0.rc1.288.g06298d1525-goog

[PATCH 2/2] KVM: arm64: selftests: Add tests for MMIO external abort injection

[Oliver Upton]

Test that the plumbing exposed to userspace for injecting aborts in
response to unexpected MMIO works as intended in two different flavors:

 - A 'normal' MMIO instruction (i.e. ESR_ELx.ISV=1)

 - An ISV=0 MMIO instruction with/without KVM_CAP_ARM_NISV_TO_USER
   enabled

Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
---
 tools/testing/selftests/kvm/Makefile          |   1 +
 .../selftests/kvm/aarch64/mmio_abort.c        | 158 ++++++++++++++++++
 2 files changed, 159 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/aarch64/mmio_abort.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 960cf6a77198..98957a99ead6 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -156,6 +156,7 @@ TEST_GEN_PROGS_aarch64 += aarch64/aarch32_id_regs
 TEST_GEN_PROGS_aarch64 += aarch64/arch_timer_edge_cases
 TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
 TEST_GEN_PROGS_aarch64 += aarch64/hypercalls
+TEST_GEN_PROGS_aarch64 += aarch64/mmio_abort
 TEST_GEN_PROGS_aarch64 += aarch64/page_fault_test
 TEST_GEN_PROGS_aarch64 += aarch64/psci_test
 TEST_GEN_PROGS_aarch64 += aarch64/set_id_regs
diff --git a/tools/testing/selftests/kvm/aarch64/mmio_abort.c b/tools/testing/selftests/kvm/aarch64/mmio_abort.c
new file mode 100644
index 000000000000..08c4afec1f28
--- /dev/null
+++ b/tools/testing/selftests/kvm/aarch64/mmio_abort.c
@@ -0,0 +1,158 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * mmio_abort - Tests for userspace MMIO abort injection
+ *
+ * Copyright (c) 2024 Google LLC
+ */
+#include "processor.h"
+#include "test_util.h"
+
+#define MMIO_ADDR	0x8000000ULL
+
+#define ESR_ELx_FSC_EXTABT	(0x10)
+
+static u64 expected_abort_pc;
+
+static void expect_sea_handler(struct ex_regs *regs)
+{
+	GUEST_ASSERT_EQ(regs->pc, expected_abort_pc);
+	GUEST_ASSERT(read_sysreg(esr_el1) & ESR_ELx_FSC_EXTABT);
+
+	GUEST_DONE();
+}
+
+static void unexpected_dabt_handler(struct ex_regs *regs)
+{
+	GUEST_FAIL("Unexpected data abort at PC: %lx\n", regs->pc);
+}
+
+static struct kvm_vm *vm_create_with_dabt_handler(struct kvm_vcpu **vcpu, void *guest_code,
+						  handler_fn dabt_handler)
+{
+	struct kvm_vm *vm = vm_create_with_one_vcpu(vcpu, guest_code);
+
+	vm_init_descriptor_tables(vm);
+	vcpu_init_descriptor_tables(*vcpu);
+	vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT, ESR_EC_DABT, dabt_handler);
+
+	virt_map(vm, MMIO_ADDR, MMIO_ADDR, 1);
+
+	return vm;
+}
+
+static void vcpu_inject_extabt(struct kvm_vcpu *vcpu)
+{
+	struct kvm_vcpu_events events = {};
+
+	events.exception.ext_dabt_pending = true;
+	vcpu_events_set(vcpu, &events);
+}
+
+static void vcpu_run_expect_done(struct kvm_vcpu *vcpu)
+{
+	struct ucall uc;
+
+	vcpu_run(vcpu);
+	switch (get_ucall(vcpu, &uc)) {
+	case UCALL_ABORT:
+		REPORT_GUEST_ASSERT(uc);
+		break;
+	case UCALL_DONE:
+		break;
+	default:
+		TEST_FAIL("Unexpected ucall: %lu", uc.cmd);
+	}
+}
+
+extern char test_mmio_abort_insn;
+
+static void test_mmio_abort_guest(void)
+{
+	WRITE_ONCE(expected_abort_pc, (u64)&test_mmio_abort_insn);
+
+	asm volatile("test_mmio_abort_insn:\n\t"
+		     "ldr x0, [%0]\n\t"
+		     : : "r" (MMIO_ADDR) : "x0", "memory");
+
+	GUEST_FAIL("MMIO instruction should not retire");
+}
+
+/*
+ * Test that KVM doesn't complete MMIO emulation when userspace has made an
+ * external abort pending for the instruction.
+ */
+static void test_mmio_abort(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm = vm_create_with_dabt_handler(&vcpu, test_mmio_abort_guest,
+							expect_sea_handler);
+	struct kvm_run *run = vcpu->run;
+
+	vcpu_run(vcpu);
+	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_MMIO);
+	TEST_ASSERT_EQ(run->mmio.phys_addr, MMIO_ADDR);
+	TEST_ASSERT_EQ(run->mmio.len, sizeof(unsigned long));
+	TEST_ASSERT(!run->mmio.is_write, "Expected MMIO read");
+
+	vcpu_inject_extabt(vcpu);
+	vcpu_run_expect_done(vcpu);
+	kvm_vm_free(vm);
+}
+
+extern char test_mmio_nisv_insn;
+
+static void test_mmio_nisv_guest(void)
+{
+	WRITE_ONCE(expected_abort_pc, (u64)&test_mmio_nisv_insn);
+
+	asm volatile("test_mmio_nisv_insn:\n\t"
+		     "ldr x0, [%0], #8\n\t"
+		     : : "r" (MMIO_ADDR) : "x0", "memory");
+
+	GUEST_FAIL("MMIO instruction should not retire");
+}
+
+/*
+ * Test that the KVM_RUN ioctl fails for ESR_EL2.ISV=0 MMIO aborts if userspace
+ * hasn't enabled KVM_CAP_ARM_NISV_TO_USER.
+ */
+static void test_mmio_nisv(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm = vm_create_with_dabt_handler(&vcpu, test_mmio_nisv_guest,
+							unexpected_dabt_handler);
+
+	TEST_ASSERT(_vcpu_run(vcpu), "Expected nonzero return code from KVM_RUN");
+	TEST_ASSERT_EQ(errno, ENOSYS);
+
+	kvm_vm_free(vm);
+}
+
+/*
+ * Test that ESR_EL2.ISV=0 MMIO aborts reach userspace and that an injected SEA
+ * reaches the guest.
+ */
+static void test_mmio_nisv_abort(void)
+{
+	struct kvm_vcpu *vcpu;
+	struct kvm_vm *vm = vm_create_with_dabt_handler(&vcpu, test_mmio_nisv_guest,
+							expect_sea_handler);
+	struct kvm_run *run = vcpu->run;
+
+	vm_enable_cap(vm, KVM_CAP_ARM_NISV_TO_USER, 1);
+
+	vcpu_run(vcpu);
+	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_ARM_NISV);
+	TEST_ASSERT_EQ(run->arm_nisv.fault_ipa, MMIO_ADDR);
+
+	vcpu_inject_extabt(vcpu);
+	vcpu_run_expect_done(vcpu);
+	kvm_vm_free(vm);
+}
+
+int main(void)
+{
+	test_mmio_abort();
+	test_mmio_nisv();
+	test_mmio_nisv_abort();
+}
-- 
2.47.0.rc1.288.g06298d1525-goog

Re: [PATCH 2/2] KVM: arm64: selftests: Add tests for MMIO external abort injection

[Oliver Upton]

On Fri, Oct 18, 2024 at 07:47:57PM +0000, Oliver Upton wrote:
> Test that the plumbing exposed to userspace for injecting aborts in
> response to unexpected MMIO works as intended in two different flavors:
> 
>  - A 'normal' MMIO instruction (i.e. ESR_ELx.ISV=1)
> 
>  - An ISV=0 MMIO instruction with/without KVM_CAP_ARM_NISV_TO_USER
>    enabled
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  tools/testing/selftests/kvm/Makefile          |   1 +
>  .../selftests/kvm/aarch64/mmio_abort.c        | 158 ++++++++++++++++++
>  2 files changed, 159 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/aarch64/mmio_abort.c
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 960cf6a77198..98957a99ead6 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -156,6 +156,7 @@ TEST_GEN_PROGS_aarch64 += aarch64/aarch32_id_regs
>  TEST_GEN_PROGS_aarch64 += aarch64/arch_timer_edge_cases
>  TEST_GEN_PROGS_aarch64 += aarch64/debug-exceptions
>  TEST_GEN_PROGS_aarch64 += aarch64/hypercalls
> +TEST_GEN_PROGS_aarch64 += aarch64/mmio_abort
>  TEST_GEN_PROGS_aarch64 += aarch64/page_fault_test
>  TEST_GEN_PROGS_aarch64 += aarch64/psci_test
>  TEST_GEN_PROGS_aarch64 += aarch64/set_id_regs
> diff --git a/tools/testing/selftests/kvm/aarch64/mmio_abort.c b/tools/testing/selftests/kvm/aarch64/mmio_abort.c
> new file mode 100644
> index 000000000000..08c4afec1f28
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/aarch64/mmio_abort.c
> @@ -0,0 +1,158 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * mmio_abort - Tests for userspace MMIO abort injection
> + *
> + * Copyright (c) 2024 Google LLC
> + */
> +#include "processor.h"
> +#include "test_util.h"
> +
> +#define MMIO_ADDR	0x8000000ULL
> +
> +#define ESR_ELx_FSC_EXTABT	(0x10)
> +
> +static u64 expected_abort_pc;
> +
> +static void expect_sea_handler(struct ex_regs *regs)
> +{
> +	GUEST_ASSERT_EQ(regs->pc, expected_abort_pc);
> +	GUEST_ASSERT(read_sysreg(esr_el1) & ESR_ELx_FSC_EXTABT);
> +

This needs to extract and compare the entire FSC field, I'll fix it the
next time around.

-- 
Thanks,
Oliver

Re: [PATCH 1/2] KVM: arm64: Don't retire aborted MMIO instruction

[Marc Zyngier]

On Fri, 18 Oct 2024 20:47:56 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Returning an abort to the guest for an unsupported MMIO access is a
> documented feature of the KVM UAPI. Nevertheless, it's clear that this
> plumbing has seen limited testing, since userspace can trivially cause a
> WARN in the MMIO return:
> 
>   WARNING: CPU: 0 PID: 30558 at arch/arm64/include/asm/kvm_emulate.h:536 kvm_handle_mmio_return+0x46c/0x5c4 arch/arm64/include/asm/kvm_emulate.h:536
>   Call trace:
>    kvm_handle_mmio_return+0x46c/0x5c4 arch/arm64/include/asm/kvm_emulate.h:536
>    kvm_arch_vcpu_ioctl_run+0x98/0x15b4 arch/arm64/kvm/arm.c:1133
>    kvm_vcpu_ioctl+0x75c/0xa78 virt/kvm/kvm_main.c:4487
>    __do_sys_ioctl fs/ioctl.c:51 [inline]
>    __se_sys_ioctl fs/ioctl.c:893 [inline]
>    __arm64_sys_ioctl+0x14c/0x1c8 fs/ioctl.c:893
>    __invoke_syscall arch/arm64/kernel/syscall.c:35 [inline]
>    invoke_syscall+0x98/0x2b8 arch/arm64/kernel/syscall.c:49
>    el0_svc_common+0x1e0/0x23c arch/arm64/kernel/syscall.c:132
>    do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:151
>    el0_svc+0x38/0x68 arch/arm64/kernel/entry-common.c:712
>    el0t_64_sync_handler+0x90/0xfc arch/arm64/kernel/entry-common.c:730
>    el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:598
> 
> The splat is complaining that KVM is advancing PC while an exception is
> pending, i.e. that KVM is retiring the MMIO instruction despite a
> pending external abort. Womp womp.

nit: *synchronous* external abort.

> 
> Fix the glaring UAPI bug by skipping over all the MMIO emulation in
> case there is a pending synchronous exception. Note that while userspace
> is capable of pending an asynchronous exception (SError, IRQ, or FIQ),
> it is still safe to retire the MMIO instruction in this case as (1) they
> are by definition asynchronous, and (2) KVM relies on hardware support
> for pending/delivering these exceptions instead of the software state
> machine for advancing PC.
> 
> Cc: stable@vger.kernel.org
> Fixes: da345174ceca ("KVM: arm/arm64: Allow user injection of external data aborts")
> Reported-by: Alexander Potapenko <glider@google.com>
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/kvm_emulate.h | 25 +++++++++++++++++++++++++
>  arch/arm64/kvm/mmio.c                |  7 +++++--
>  2 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index a601a9305b10..1b229099f684 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -544,6 +544,31 @@ static __always_inline void kvm_incr_pc(struct kvm_vcpu *vcpu)
>  		vcpu_set_flag((v), e);					\
>  	} while (0)
>  
> +static inline bool kvm_pending_sync_exception(struct kvm_vcpu *vcpu)
> +{
> +	if (!vcpu_get_flag(vcpu, PENDING_EXCEPTION))
> +		return false;
> +
> +	if (vcpu_el1_is_32bit(vcpu)) {
> +		switch (vcpu_get_flag(vcpu, EXCEPT_MASK)) {
> +		case unpack_vcpu_flag(EXCEPT_AA32_UND):
> +		case unpack_vcpu_flag(EXCEPT_AA32_IABT):
> +		case unpack_vcpu_flag(EXCEPT_AA32_DABT):
> +			return true;
> +		default:
> +			return false;
> +		}
> +	} else {
> +		switch (vcpu_get_flag(vcpu, EXCEPT_MASK)) {
> +		case unpack_vcpu_flag(EXCEPT_AA64_EL1_SYNC):
> +		case unpack_vcpu_flag(EXCEPT_AA64_EL2_SYNC):
> +			return true;
> +		default:
> +			return false;
> +		}
> +	}
> +}
> +

Is there any advantage in adding this to an otherwise unsuspecting
include file, given that this is only used in a single spot?

>  #define __build_check_all_or_none(r, bits)				\
>  	BUILD_BUG_ON(((r) & (bits)) && ((r) & (bits)) != (bits))
>  
> diff --git a/arch/arm64/kvm/mmio.c b/arch/arm64/kvm/mmio.c
> index cd6b7b83e2c3..0155ba665717 100644
> --- a/arch/arm64/kvm/mmio.c
> +++ b/arch/arm64/kvm/mmio.c
> @@ -84,8 +84,11 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu)
>  	unsigned int len;
>  	int mask;
>  
> -	/* Detect an already handled MMIO return */
> -	if (unlikely(!vcpu->mmio_needed))
> +	/*
> +	 * Detect if the MMIO return was already handled or if userspace aborted
> +	 * the MMIO access.
> +	 */
> +	if (unlikely(!vcpu->mmio_needed || kvm_pending_sync_exception(vcpu)))
>  		return 1;
>  
>  	vcpu->mmio_needed = 0;

Otherwise looks good to me!

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/2] KVM: arm64: Fix splat/misbehavior for MMIO SEA injection

[Marc Zyngier]

On Fri, 18 Oct 2024 20:47:55 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> syzkaller continues to find more interesting bugs, this time due to a
> decently-sized hole in our UAPI.
> 
> Turns out we still go through the motions of completing MMIO emulation
> even if userspace has pended a *synchronous* external abort in response
> to an unexpected access. Oops!
> 
> In addition to the fix, I felt this warranted a selftest because a
> documented UAPI flow has been broken for a while.

Indeed. A good indication that nobody is using it.

> Marc, I'll probably take this in 6.13 since we've still got quite a bit
> outstanding for 6.12 and this isn't *terribly* urgent.

Agreed. And the current set of fixes is not going anywhere at the
moment. I may have to resort to an alternative routing for that.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 1/2] KVM: arm64: Don't retire aborted MMIO instruction

[Oliver Upton]

On Sat, Oct 19, 2024 at 10:10:04AM +0100, Marc Zyngier wrote:
> On Fri, 18 Oct 2024 20:47:56 +0100,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> > 
> > Returning an abort to the guest for an unsupported MMIO access is a
> > documented feature of the KVM UAPI. Nevertheless, it's clear that this
> > plumbing has seen limited testing, since userspace can trivially cause a
> > WARN in the MMIO return:
> > 
> >   WARNING: CPU: 0 PID: 30558 at arch/arm64/include/asm/kvm_emulate.h:536 kvm_handle_mmio_return+0x46c/0x5c4 arch/arm64/include/asm/kvm_emulate.h:536
> >   Call trace:
> >    kvm_handle_mmio_return+0x46c/0x5c4 arch/arm64/include/asm/kvm_emulate.h:536
> >    kvm_arch_vcpu_ioctl_run+0x98/0x15b4 arch/arm64/kvm/arm.c:1133
> >    kvm_vcpu_ioctl+0x75c/0xa78 virt/kvm/kvm_main.c:4487
> >    __do_sys_ioctl fs/ioctl.c:51 [inline]
> >    __se_sys_ioctl fs/ioctl.c:893 [inline]
> >    __arm64_sys_ioctl+0x14c/0x1c8 fs/ioctl.c:893
> >    __invoke_syscall arch/arm64/kernel/syscall.c:35 [inline]
> >    invoke_syscall+0x98/0x2b8 arch/arm64/kernel/syscall.c:49
> >    el0_svc_common+0x1e0/0x23c arch/arm64/kernel/syscall.c:132
> >    do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:151
> >    el0_svc+0x38/0x68 arch/arm64/kernel/entry-common.c:712
> >    el0t_64_sync_handler+0x90/0xfc arch/arm64/kernel/entry-common.c:730
> >    el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:598
> > 
> > The splat is complaining that KVM is advancing PC while an exception is
> > pending, i.e. that KVM is retiring the MMIO instruction despite a
> > pending external abort. Womp womp.
> 
> nit: *synchronous* external abort.

Doh!

> > +static inline bool kvm_pending_sync_exception(struct kvm_vcpu *vcpu)
> > +{
> > +	if (!vcpu_get_flag(vcpu, PENDING_EXCEPTION))
> > +		return false;
> > +
> > +	if (vcpu_el1_is_32bit(vcpu)) {
> > +		switch (vcpu_get_flag(vcpu, EXCEPT_MASK)) {
> > +		case unpack_vcpu_flag(EXCEPT_AA32_UND):
> > +		case unpack_vcpu_flag(EXCEPT_AA32_IABT):
> > +		case unpack_vcpu_flag(EXCEPT_AA32_DABT):
> > +			return true;
> > +		default:
> > +			return false;
> > +		}
> > +	} else {
> > +		switch (vcpu_get_flag(vcpu, EXCEPT_MASK)) {
> > +		case unpack_vcpu_flag(EXCEPT_AA64_EL1_SYNC):
> > +		case unpack_vcpu_flag(EXCEPT_AA64_EL2_SYNC):
> > +			return true;
> > +		default:
> > +			return false;
> > +		}
> > +	}
> > +}
> > +
> 
> Is there any advantage in adding this to an otherwise unsuspecting
> include file, given that this is only used in a single spot?

v0 of this was a bit more involved, which is why I had this in a header.
I'll move it.

> Otherwise looks good to me!

Thanks!

-- 
Best,
Oliver