Re: [PATCH] KVM: arm64: Clear pending exception state before injecting a new one

[Marc Zyngier <maz@kernel.org>]

On Tue, 15 Jul 2025 07:51:09 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Hey,
> 
> On Mon, Jul 14, 2025 at 03:46:36PM +0100, Marc Zyngier wrote:
> > Repeatedly injecting an exception from userspace without running
> > the vcpu between calls results in a nasty warning, as we're not
> > really keen on losing already pending exceptions.
> > 
> > But this precaution doesn't really apply to userspace, who can
> > do whatever it wants (within reason). So let's simply clear any
> > previous exception state before injecting a new one.
> > 
> > Note that this is done unconditionally, even if the injection
> > ultimately fails.
> > 
> > Reported-by: syzbot+4e09b1432de3774b86ae@syzkaller.appspotmail.com
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> 
> Thanks for taking a look at this. I think the correct fix is a bit more
> involved, as:
> 
>  - ABI prior to my patches allowed dumb things like injecting both an
>    SEA and SError from the same ioctl. With your patch I think you could
>    still get the warning to fire with serror_pending && ext_dabt_pending
> 
>  - KVM_GET_VCPU_EVENTS is broken for 'pending' SEAs, as we assume
>    they're committed in the vCPU state immediately when they're actually
>    deferred to the next KVM_RUN.
> 
> I thoroughly hate the fix I have but it should address both of these
> issues. Although the pending PC adjustment flags seem more like a
> liability than anything else if ioctls need to flush them before
> returning to userspace. Might look at a larger cleanup down the road.
> 
> Thanks,
> Oliver
> 
> From 149262689dfe881542f5c5b60f9ee308a00f0596 Mon Sep 17 00:00:00 2001
> From: Oliver Upton <oliver.upton@linux.dev>
> Date: Mon, 14 Jul 2025 23:25:07 -0700
> Subject: [PATCH] KVM: arm64: Commit exceptions from KVM_SET_VCPU_EVENTS
>  immediately
> 
> syzkaller has found that it can trip a warning in KVM's exception
> emulation infrastructure by repeatedly injecting exceptions into the
> guest.
> 
> While it's unlikely that a reasonable VMM will do this, further
> investigation of the issue reveals that KVM can potentially discard the
> "pending" SEA state. While the handling of KVM_GET_VCPU_EVENTS presumes
> that userspace-injected SEAs are realized immediately, in reality the
> emulated exception entry is deferred until the next call to KVM_RUN.
> 
> Hack-a-fix the immediate issues by committing the pending exceptions to
> the vCPU's architectural state immediately in KVM_SET_VCPU_EVENTS. This
> is no different to the way KVM-injected exceptions are handled in
> KVM_RUN where we potentially call __kvm_adjust_pc() before returning to
> userspace.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/kvm/guest.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index e2702718d56d..16ba5e9ac86c 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -834,6 +834,19 @@ int __kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
>  	return 0;
>  }
>  
> +static void commit_pending_events(struct kvm_vcpu *vcpu)
> +{
> +	if (!vcpu_get_flag(vcpu, PENDING_EXCEPTION))
> +		return;
> +
> +	/*
> +	 * Reset the MMIO emulation state to avoid stepping PC after emulating
> +	 * the exception entry.
> +	 */
> +	vcpu->mmio_needed = false;
> +	kvm_call_hyp(__kvm_adjust_pc, vcpu);
> +}
> +
>  int __kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
>  			      struct kvm_vcpu_events *events)
>  {
> @@ -843,8 +856,15 @@ int __kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
>  	u64 esr = events->exception.serror_esr;
>  	int ret = 0;
>  
> -	if (ext_dabt_pending)
> +	/*
> +	 * Immediately commit the pending SEA to the vCPU's architectural
> +	 * state which is necessary since we do not return a pending SEA
> +	 * to userspace via KVM_GET_VCPU_EVENTS.
> +	 */
> +	if (ext_dabt_pending) {
>  		ret = kvm_inject_sea_dabt(vcpu, kvm_vcpu_get_hfar(vcpu));
> +		commit_pending_events(vcpu);
> +	}
>  
>  	if (ret < 0)
>  		return ret;
> @@ -863,6 +883,12 @@ int __kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
>  	else
>  		ret = kvm_inject_serror(vcpu);
>  
> +	/*
> +	 * We could've decided that the SError is due for immediate software
> +	 * injection; commit the exception in case userspace decides it wants
> +	 * to inject more exceptions for some strange reason.
> +	 */
> +	commit_pending_events(vcpu);
>  	return (ret < 0) ? ret : 0;
>  }

Yup, this seems looks reasonable. I would have liked the commit helper
to be used just before vcpu_put(), but obviously, that's not the right
thing to do because of the mmio_needed field.

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.

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