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

[Marc Zyngier <maz@kernel.org>]

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.