[PATCH v2 02/20] KVM: x86/xen: Add KVM_XEN_VCPU_ATTR_TYPE_WRITE_HYPERCALL_PAGE

[David Woodhouse <dwmw2@infradead.org>]

From: David Woodhouse <dwmw@amazon.co.uk>

Commit 3617c0ee7dec ("KVM: x86/xen: Only write Xen hypercall page for
guest writes to MSR") blocked host-initiated writes from triggering the
Xen hypercall page setup, to fix an SRCU usage violation when the
hypercall MSR index collides with a real MSR written during vCPU reset.

However, some VMMs legitimately need to trigger hypercall page setup
from host context. For example, a VMM may intercept the guest's MSR
write to track an epoch (for kexec/crash recovery), and then replay the
write as a host-initiated KVM_SET_MSRS to populate the hypercall page.
The host_initiated check breaks this use case.

Add KVM_XEN_VCPU_ATTR_TYPE_WRITE_HYPERCALL_PAGE as a vCPU attribute
which invokes the write explicitly. The work moves into a helper
called with xen_lock held — which the attribute handler already holds,
and which now covers the whole operation, serializing the writes to
guest memory — in the shape the original posting's review asked for:
handled in the attribute case-statement, with no locking games of its
own. The helper returns raw errnos for each caller to map onto its own
contract: the attribute returns them to userspace as they are, while
the guest WRMSR raises #GP for anything but -ENOMEM. That includes a
fault reading the VMM-supplied blob, which previously escaped to
userspace as -EFAULT: by that point it is indistinguishable from the
guest's own bad page_addr, and #GP serves them both. The helper also
reverts the latched long_mode when the shared_info re-init fails —
on both entry points, including the guest WRMSR path — where the old
code left it latched, letting a retry of the same write skip the
re-init and "succeed" with the wallclock still published at the
offset for the old bitness.

The Fixes: tag documents which commit regressed the use case; since
this patch adds new UAPI (an attribute and a capability bit), it is
not suitable for backporting to stable kernels.

Fixes: 3617c0ee7dec ("KVM: x86/xen: Only write Xen hypercall page for guest writes to MSR")
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 Documentation/virt/kvm/api.rst                |  22 +++-
 arch/x86/include/uapi/asm/kvm.h               |   3 +
 arch/x86/kvm/x86.c                            |   3 +-
 arch/x86/kvm/xen.c                            |  73 ++++++++---
 .../selftests/kvm/x86/xen_vmcall_test.c       | 118 ++++++++++++++++++
 5 files changed, 203 insertions(+), 16 deletions(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index e95c9cda085d..70818f1353c3 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -5848,6 +5848,23 @@ KVM_XEN_VCPU_ATTR_TYPE_UPCALL_VECTOR
   vector configured with HVM_PARAM_CALLBACK_IRQ. It is disabled by
   setting the vector to zero.
 
+KVM_XEN_VCPU_ATTR_TYPE_WRITE_HYPERCALL_PAGE
+  This attribute is available when the KVM_CAP_XEN_HVM ioctl indicates
+  support for KVM_XEN_HVM_CONFIG_WRITE_HYPERCALL_PAGE. It populates the
+  Xen hypercall page at the guest physical address given in ``gpa``,
+  just as a guest write to the hypercall MSR would. This is for VMMs
+  which intercept the guest's MSR write (e.g. to track an epoch for
+  kexec/crash recovery) and then need to perform the write themselves:
+  host-initiated writes through KVM_SET_MSRS do not populate the page.
+  As for the guest's own write, the vCPU's current long mode is latched
+  as the VM-wide Xen long_mode, and the shared_info wallclock is
+  re-published if that changed. The value is interpreted exactly as the
+  guest's MSR write would be, not as a plain address: when Xen hypercall
+  interception is enabled, any non-zero bits below the page size make
+  the write fail with -EINVAL; without interception, those bits select
+  which page of the VMM-provided hypercall blob is written, and a page
+  number beyond the blob fails with -EINVAL.
+
 
 4.129 KVM_XEN_VCPU_GET_ATTR
 ---------------------------
@@ -5861,7 +5878,8 @@ KVM_XEN_VCPU_ATTR_TYPE_UPCALL_VECTOR
 Allows Xen vCPU attributes to be read. For the structure and types,
 see KVM_XEN_VCPU_SET_ATTR above.
 
-The KVM_XEN_VCPU_ATTR_TYPE_RUNSTATE_ADJUST type may not be used
+The KVM_XEN_VCPU_ATTR_TYPE_RUNSTATE_ADJUST and
+KVM_XEN_VCPU_ATTR_TYPE_WRITE_HYPERCALL_PAGE types may not be used
 with the KVM_XEN_VCPU_GET_ATTR ioctl.
 
 4.130 KVM_ARM_MTE_COPY_TAGS
@@ -9509,6 +9527,8 @@ PVHVM guests. Valid flags are::
   #define KVM_XEN_HVM_CONFIG_EVTCHN_SEND		(1 << 5)
   #define KVM_XEN_HVM_CONFIG_RUNSTATE_UPDATE_FLAG	(1 << 6)
   #define KVM_XEN_HVM_CONFIG_PVCLOCK_TSC_UNSTABLE	(1 << 7)
+  #define KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA		(1 << 8)
+  #define KVM_XEN_HVM_CONFIG_WRITE_HYPERCALL_PAGE	(1 << 9)
 
 The KVM_XEN_HVM_CONFIG_HYPERCALL_MSR flag indicates that the KVM_XEN_HVM_CONFIG
 ioctl is available, for the guest to set its hypercall page.
diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index 00ce265391e1..e53a26370e50 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -596,6 +596,7 @@ struct kvm_x86_mce {
 #define KVM_XEN_HVM_CONFIG_RUNSTATE_UPDATE_FLAG	(1 << 6)
 #define KVM_XEN_HVM_CONFIG_PVCLOCK_TSC_UNSTABLE	(1 << 7)
 #define KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA	(1 << 8)
+#define KVM_XEN_HVM_CONFIG_WRITE_HYPERCALL_PAGE	(1 << 9)
 
 #define KVM_XEN_MSR_MIN_INDEX			0x40000000u
 #define KVM_XEN_MSR_MAX_INDEX			0x4fffffffu
@@ -704,6 +705,8 @@ struct kvm_xen_vcpu_attr {
 #define KVM_XEN_VCPU_ATTR_TYPE_UPCALL_VECTOR	0x8
 /* Available with KVM_CAP_XEN_HVM / KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA */
 #define KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO_HVA	0x9
+/* Available with KVM_CAP_XEN_HVM / KVM_XEN_HVM_CONFIG_WRITE_HYPERCALL_PAGE */
+#define KVM_XEN_VCPU_ATTR_TYPE_WRITE_HYPERCALL_PAGE	0xa
 
 /* Secure Encrypted Virtualization command */
 enum sev_cmd_id {
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 32563a91fba9..6a1f0404f029 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2315,7 +2315,8 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 		    KVM_XEN_HVM_CONFIG_EVTCHN_2LEVEL |
 		    KVM_XEN_HVM_CONFIG_EVTCHN_SEND |
 		    KVM_XEN_HVM_CONFIG_PVCLOCK_TSC_UNSTABLE |
-		    KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA;
+		    KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA |
+		    KVM_XEN_HVM_CONFIG_WRITE_HYPERCALL_PAGE;
 		if (sched_info_on())
 			r |= KVM_XEN_HVM_CONFIG_RUNSTATE |
 			     KVM_XEN_HVM_CONFIG_RUNSTATE_UPDATE_FLAG;
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 5875d10ae3a9..2718da1320e0 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -33,6 +33,7 @@
 static int kvm_xen_set_evtchn(struct kvm_xen_evtchn *xe, struct kvm *kvm);
 static int kvm_xen_setattr_evtchn(struct kvm *kvm, struct kvm_xen_hvm_attr *data);
 static bool kvm_xen_hcall_evtchn_send(struct kvm_vcpu *vcpu, u64 param, u64 *r);
+static int __kvm_xen_write_hypercall_page(struct kvm_vcpu *vcpu, u64 data);
 
 DEFINE_STATIC_KEY_DEFERRED_FALSE(kvm_xen_enabled, HZ);
 
@@ -1177,6 +1178,15 @@ int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
 		}
 		break;
 
+	case KVM_XEN_VCPU_ATTR_TYPE_WRITE_HYPERCALL_PAGE:
+		/* KVM cannot read a protected guest's bitness to latch it. */
+		if (vcpu->arch.guest_state_protected) {
+			r = -EINVAL;
+			break;
+		}
+		r = __kvm_xen_write_hypercall_page(vcpu, data->u.gpa);
+		break;
+
 	default:
 		break;
 	}
@@ -1312,30 +1322,42 @@ int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
 	return r;
 }
 
-int kvm_xen_write_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
+/*
+ * Latch the guest's bitness, re-publish the wallclock in the right
+ * place for it if needed, and fill the hypercall page. Called with
+ * xen_lock held, which serializes the writes to guest memory, and
+ * within a kvm->srcu read section.
+ *
+ * Returns raw errnos for each caller to map onto its own contract:
+ * -EINVAL for an out-of-range page_num; -EFAULT for a fault writing
+ * the page, whether through the guest's page_addr or from the VMM's
+ * blob address; -ENOMEM from the page allocations.
+ */
+static int __kvm_xen_write_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
 {
 	struct kvm *kvm = vcpu->kvm;
 	u32 page_num = data & ~PAGE_MASK;
 	u64 page_addr = data & PAGE_MASK;
 	bool lm = is_long_mode(vcpu);
-	int r = 0;
 
-	mutex_lock(&kvm->arch.xen.xen_lock);
+	lockdep_assert_held(&kvm->arch.xen.xen_lock);
+
 	if (kvm->arch.xen.long_mode != lm) {
 		kvm->arch.xen.long_mode = lm;
 
 		/*
 		 * Re-initialize shared_info to put the wallclock in the
-		 * correct place.
+		 * correct place. On failure, revert the latched long_mode:
+		 * leaving it latched would make a retry of the same write
+		 * skip this block and "succeed" with the wallclock still
+		 * published at the offset for the old bitness.
 		 */
 		if (kvm->arch.xen.shinfo_cache.active &&
-		    kvm_xen_shared_info_init(kvm))
-			r = 1;
+		    kvm_xen_shared_info_init(kvm)) {
+			kvm->arch.xen.long_mode = !lm;
+			return -EFAULT;
+		}
 	}
-	mutex_unlock(&kvm->arch.xen.xen_lock);
-
-	if (r)
-		return r;
 
 	/*
 	 * If Xen hypercall intercept is enabled, fill the hypercall
@@ -1349,7 +1371,7 @@ int kvm_xen_write_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
 		int ret, i;
 
 		if (page_num)
-			return 1;
+			return -EINVAL;
 
 		/* mov imm32, %eax */
 		instructions[0] = 0xb8;
@@ -1376,7 +1398,7 @@ int kvm_xen_write_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
 		ret = kvm_vcpu_write_guest(vcpu, page_addr, page, PAGE_SIZE);
 		kfree(page);
 		if (ret)
-			return 1;
+			return ret;
 	} else {
 		/*
 		 * Note, truncation is a non-issue as 'lm' is guaranteed to be
@@ -1390,7 +1412,7 @@ int kvm_xen_write_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
 		int ret;
 
 		if (page_num >= blob_size)
-			return 1;
+			return -EINVAL;
 
 		blob_addr += page_num * PAGE_SIZE;
 
@@ -1401,11 +1423,34 @@ int kvm_xen_write_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
 		ret = kvm_vcpu_write_guest(vcpu, page_addr, page, PAGE_SIZE);
 		kfree(page);
 		if (ret)
-			return 1;
+			return ret;
 	}
 	return 0;
 }
 
+/* The guest WRMSR entry point. */
+int kvm_xen_write_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
+{
+	struct kvm *kvm = vcpu->kvm;
+	int r;
+
+	mutex_lock(&kvm->arch.xen.xen_lock);
+	r = __kvm_xen_write_hypercall_page(vcpu, data);
+	mutex_unlock(&kvm->arch.xen.xen_lock);
+
+	/*
+	 * Any failure to populate the page raises #GP; only -ENOMEM is
+	 * signalled upwards. The one deliberate change: a fault reading
+	 * the VMM-supplied blob previously escaped to userspace as
+	 * -EFAULT, but by this point it is indistinguishable from the
+	 * guest's own bad page_addr, and #GP serves them both.
+	 */
+	if (r && r != -ENOMEM)
+		return 1;
+
+	return r;
+}
+
 int kvm_xen_hvm_config(struct kvm *kvm, struct kvm_xen_hvm_config *xhc)
 {
 	/* Only some feature flags need to be *enabled* by userspace */
diff --git a/tools/testing/selftests/kvm/x86/xen_vmcall_test.c b/tools/testing/selftests/kvm/x86/xen_vmcall_test.c
index 2585087cdf5c..85d836ddbdb8 100644
--- a/tools/testing/selftests/kvm/x86/xen_vmcall_test.c
+++ b/tools/testing/selftests/kvm/x86/xen_vmcall_test.c
@@ -12,6 +12,8 @@
 #include "processor.h"
 #include "hyperv.h"
 
+#include <string.h>
+
 #define HCALL_REGION_GPA	0xc0000000ULL
 #define HCALL_REGION_SLOT	10
 
@@ -26,6 +28,10 @@
 #define HVCALL_SIGNAL_EVENT		0x005d
 #define HV_STATUS_INVALID_ALIGNMENT	4
 
+enum {
+	TEST_WRITE_HYPERCALL_PAGE = 1,
+};
+
 static void guest_code(void)
 {
 	unsigned long rax = INPUTVALUE;
@@ -76,18 +82,74 @@ static void guest_code(void)
 			     "r"(r8));
 	GUEST_ASSERT(rax == HV_STATUS_INVALID_ALIGNMENT);
 
+	/*
+	 * Test KVM_XEN_VCPU_ATTR_TYPE_WRITE_HYPERCALL_PAGE: ask userspace
+	 * to set up MSR filtering, then write the MSR. The WRMSR will exit
+	 * to userspace (not populate the page). Userspace verifies the page
+	 * is empty, uses the attr to populate it, then resumes us.
+	 */
+	GUEST_SYNC(TEST_WRITE_HYPERCALL_PAGE);
+
+	__asm__ __volatile__("wrmsr" : : "c" (XEN_HYPERCALL_MSR),
+			     "a" (HCALL_REGION_GPA & 0xffffffff),
+			     "d" (HCALL_REGION_GPA >> 32));
+
+	/* Userspace populated the page via the attr — verify it works */
+	rax = INPUTVALUE;
+	rdi = ARGVALUE(1);
+	rsi = ARGVALUE(2);
+	rdx = ARGVALUE(3);
+	r10 = ARGVALUE(4);
+	r8 = ARGVALUE(5);
+	r9 = ARGVALUE(6);
+	__asm__ __volatile__("call *%1" : "=a"(rax) :
+			     "r"(HCALL_REGION_GPA + INPUTVALUE * 32),
+			     "a"(rax), "D"(rdi), "S"(rsi), "d"(rdx),
+			     "r"(r10), "r"(r8), "r"(r9));
+	GUEST_ASSERT(rax == RETVALUE);
+
 	GUEST_DONE();
 }
 
+static void setup_msr_filter(struct kvm_vm *vm)
+{
+	uint64_t deny_bits = 0;
+	struct kvm_msr_filter filter = {
+		.flags = KVM_MSR_FILTER_DEFAULT_ALLOW,
+		.ranges = {
+			{
+				.flags = KVM_MSR_FILTER_WRITE,
+				.nmsrs = 1,
+				.base = XEN_HYPERCALL_MSR,
+				.bitmap = (uint8_t *)&deny_bits,
+			},
+		},
+	};
+
+	vm_ioctl(vm, KVM_X86_SET_MSR_FILTER, &filter);
+}
+
 int main(int argc, char *argv[])
 {
 	unsigned int xen_caps;
 	struct kvm_vcpu *vcpu;
 	struct kvm_vm *vm;
+	bool msr_filter_ready = false;
+	bool attr_tested = false;
 
 	xen_caps = kvm_check_cap(KVM_CAP_XEN_HVM);
 	TEST_REQUIRE(xen_caps & KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL);
 
+	/*
+	 * Without these, skip only the WRITE_HYPERCALL_PAGE leg: the MSR
+	 * filter is never installed, so the guest's second WRMSR fills
+	 * the page through KVM as the first did, and the rest of the
+	 * test still runs on older kernels.
+	 */
+	bool have_whp = (xen_caps & KVM_XEN_HVM_CONFIG_WRITE_HYPERCALL_PAGE) &&
+			kvm_check_cap(KVM_CAP_X86_USER_SPACE_MSR) &&
+			kvm_check_cap(KVM_CAP_X86_MSR_FILTER);
+
 	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
 	vcpu_set_hv_cpuid(vcpu);
 
@@ -123,6 +185,42 @@ int main(int argc, char *argv[])
 			continue;
 		}
 
+		if (run->exit_reason == KVM_EXIT_X86_WRMSR) {
+			/* MSR filter caught the Xen hypercall MSR write */
+			TEST_ASSERT(msr_filter_ready,
+				    "Unexpected WRMSR exit before filter setup");
+			TEST_ASSERT_EQ(run->msr.index, XEN_HYPERCALL_MSR);
+
+			/*
+			 * The MSR filter should have prevented KVM from
+			 * populating the page. Verify it's empty.
+			 */
+			uint8_t *hcall_page = addr_gpa2hva(vm, HCALL_REGION_GPA);
+
+			TEST_ASSERT_EQ(hcall_page[0], 0);
+
+			/*
+			 * Now use the attr to populate the page, as a
+			 * VMM would after intercepting the MSR write.
+			 */
+			struct kvm_xen_vcpu_attr attr = {
+				.type = KVM_XEN_VCPU_ATTR_TYPE_WRITE_HYPERCALL_PAGE,
+				.u.gpa = HCALL_REGION_GPA,
+			};
+			vcpu_ioctl(vcpu, KVM_XEN_VCPU_SET_ATTR, &attr);
+
+			/* Verify the page is now populated */
+			TEST_ASSERT_EQ(hcall_page[0], 0xb8);
+			TEST_ASSERT_EQ(*(uint32_t *)&hcall_page[1], 0);
+			TEST_ASSERT_EQ(hcall_page[INPUTVALUE * 32], 0xb8);
+			TEST_ASSERT_EQ(*(uint32_t *)&hcall_page[INPUTVALUE * 32 + 1],
+				       INPUTVALUE);
+			attr_tested = true;
+
+			run->msr.error = 0;
+			continue;
+		}
+
 		TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_IO);
 
 		switch (get_ucall(vcpu, &uc)) {
@@ -130,6 +228,21 @@ int main(int argc, char *argv[])
 			REPORT_GUEST_ASSERT(uc);
 			/* NOT REACHED */
 		case UCALL_SYNC:
+			TEST_ASSERT_EQ(uc.args[1], TEST_WRITE_HYPERCALL_PAGE);
+
+			if (!have_whp)
+				break;
+
+			/*
+			 * Guest is about to write the Xen MSR. Clear the
+			 * hypercall page, install an MSR filter to intercept
+			 * the write, and enable userspace MSR exits.
+			 */
+			memset(addr_gpa2hva(vm, HCALL_REGION_GPA), 0, PAGE_SIZE);
+			vm_enable_cap(vm, KVM_CAP_X86_USER_SPACE_MSR,
+				      KVM_MSR_EXIT_REASON_FILTER);
+			setup_msr_filter(vm);
+			msr_filter_ready = true;
 			break;
 		case UCALL_DONE:
 			goto done;
@@ -138,6 +251,11 @@ int main(int argc, char *argv[])
 		}
 	}
 done:
+	if (have_whp)
+		TEST_ASSERT(attr_tested,
+			    "WRITE_HYPERCALL_PAGE attr was never exercised");
+	else
+		pr_info("Skipped KVM_XEN_VCPU_ATTR_TYPE_WRITE_HYPERCALL_PAGE\n");
 	kvm_vm_free(vm);
 	return 0;
 }
-- 
2.43.0