Re: [PATCH v3 4/7] KVM: x86/xen: Extract delivery of event to vCPU into a separate helper

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The refactored helper injects an MSI instead of forcing a VM-exit when the vcpu_info cache check fails.

Pre-existing issues:
- [Critical] The host can be forced to perform unaligned atomic operations on guest-controlled memory, leading to a Denial of Service via split lock.
--

--- Patch [4]: [PATCH v3 4/7] KVM: x86/xen: Extract delivery of event to vCPU into a separate helper ---
commit de0f33b78cabce7ad4bdfa1d85f662391c0c5256
Author: Woodhouse, David <[email protected]>
KVM: x86/xen: Extract delivery of event to vCPU into a separate helper

This commit moves the marking of an event as pending in the target vCPU's
vcpu_info into a separate helper function, __kvm_xen_set_evtchn_fast().
The intent is to improve readability without causing functional changes.

> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c

[ ... ]

> +/* Called with kvm->srcu held, as kvm_gpc_check() consults the memslots. */
> +static void __kvm_xen_set_evtchn_fast(struct kvm_vcpu *vcpu, int port_word_bit)
> +{
> +	struct gfn_to_pfn_cache *gpc = &vcpu->arch.xen.vcpu_info_cache;
> +	bool kick_vcpu = false;
> +
> +	/* Now switch to the vCPU's vcpu_info to set the index and pending_sel */
> +	rcu_read_lock();
> +	if (!kvm_gpc_check(gpc, sizeof(struct vcpu_info))) {
> +		/*
> +		 * Could not access the vcpu_info. Set the bit in-kernel and
> +		 * prod the vCPU to deliver it for itself.
> +		 */
> +		if (!test_and_set_bit(port_word_bit, &vcpu->arch.xen.evtchn_pending_sel))
> +			kick_vcpu = true;
> +		goto out_unlock;
> +	}
> +
> +	if (IS_ENABLED(CONFIG_64BIT) && vcpu->kvm->arch.xen.long_mode) {
> +		struct vcpu_info *vcpu_info = gpc->khva;
> +
> +		if (!test_and_set_bit(port_word_bit, &vcpu_info->evtchn_pending_sel)) {

[Severity: Critical]
This is a pre-existing issue, but does test_and_set_bit() here risk a
host denial of service via a hardware split lock?

If a guest or userspace sets the vcpu_info GPA via
KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO to an unaligned address such that
evtchn_pending_sel spans a 64-byte cache line boundary, the 8-byte
atomic lock bts instruction could trigger an #AC exception.

If the host kernel is configured with split_lock_detect=fatal and
panic_on_oops, this would panic the host.

This also applies to the 32-bit compat branch below where the pointer
is cast to unsigned long *.

> +			WRITE_ONCE(vcpu_info->evtchn_upcall_pending, 1);
> +			kick_vcpu = true;
> +		}
> +	} else {
> +		struct compat_vcpu_info *vcpu_info = gpc->khva;
> +
> +		if (!test_and_set_bit(port_word_bit,
> +				      (unsigned long *)&vcpu_info->evtchn_pending_sel)) {
> +			WRITE_ONCE(vcpu_info->evtchn_upcall_pending, 1);
> +			kick_vcpu = true;
> +		}
> +	}
> +
> +out_unlock:
> +	rcu_read_unlock();
> +
> +	/*
> +	 * Deliver the upcall or kick the vCPU only after dropping the RCU
> +	 * read lock. Both paths end up in kvm_vcpu_kick(), and the MSI
> +	 * delivery also walks the APIC map and takes APIC locks; none of
> +	 * that wants to be nested inside the GPC read-side critical
> +	 * section, which must be no longer than the accesses to gpc->khva
> +	 * above. Invalidation waits for a grace period, so holding the
> +	 * read lock across the kick would extend how long a memory
> +	 * invalidation is blocked.
> +	 */
> +	if (!kick_vcpu)
> +		return;
> +
> +	/* For the per-vCPU lapic vector, deliver it as MSI. */
> +	if (vcpu->arch.xen.upcall_vector) {
> +		kvm_xen_inject_vcpu_vector(vcpu);
> +		return;
> +	}

[Severity: High]
Does this new helper inadvertently bypass the required vCPU kick when the
vcpu_info cache check fails?

In the original code, if kvm_gpc_check() failed, the code jumped to the
out_rcu label. This intentionally bypassed the MSI injection block, and
fell straight through to kvm_vcpu_kick(). This forced a VM-exit so KVM
could synchronize the pending events to guest memory via
kvm_xen_inject_pending_events().

With this refactored helper, if the cache check fails and kick_vcpu is set
to true, the code jumps to out_unlock, drops the RCU read lock, and then
unconditionally injects an MSI if upcall_vector is present. Because it
returns early, the kvm_vcpu_kick() is skipped.

If APICv posted interrupts deliver the MSI directly without a VM-exit,
won't KVM fail to sync the in-kernel pending bit, causing the guest to
read stale memory and drop the interrupt?

> +
> +	kvm_make_request(KVM_REQ_UNBLOCK, vcpu);
> +	kvm_vcpu_kick(vcpu);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4