On Fri, 18 Oct 2024 20:47:56 +0100,
Oliver Upton <oliver.upton@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
> Fixes: da345174ceca ("KVM: arm/arm64: Allow user injection of external data aborts")
> Reported-by: Alexander Potapenko <glider@xxxxxxxxxx>
> Signed-off-by: Oliver Upton <oliver.upton@xxxxxxxxx>
> ---
> 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.
