[PATCH v2 0/5] KVM: x86/xen: Restrict hypercall MSR index

[PATCH v2 0/5] KVM: x86/xen: Restrict hypercall MSR index

[Sean Christopherson]

Harden KVM against goofy userspace by restricting the Xen hypercall MSR
index to the de facto standard synthetic range, 0x40000000 - 0x4fffffff.
This obviously has the potential to break userspace, but I'm fairly confident
it'll be fine (knock wood), and doing nothing is not an option as letting
userspace redirect any WRMSR is at best completely broken.

Patches 2-5 are tangentially related cleanups.

v2:
 - Collect reviews. [Paul, David]
 - Add proper #defines for the range. [David]
 - Drop the syzkaller/stable tags (rely on disallow host writes to fix the
   syzkaller splat]. David

v1: https://lore.kernel.org/all/20250201011400.669483-1-seanjc@google.com

Sean Christopherson (5):
  KVM: x86/xen: Restrict hypercall MSR to unofficial synthetic range
  KVM: x86/xen: Add an #ifdef'd helper to detect writes to Xen MSR
  KVM: x86/xen: Consult kvm_xen_enabled when checking for Xen MSR writes
  KVM: x86/xen: Bury xen_hvm_config behind CONFIG_KVM_XEN=y
  KVM: x86/xen: Move kvm_xen_hvm_config field into kvm_xen

 arch/x86/include/asm/kvm_host.h |  4 ++--
 arch/x86/include/uapi/asm/kvm.h |  3 +++
 arch/x86/kvm/x86.c              |  4 ++--
 arch/x86/kvm/xen.c              | 29 +++++++++++++++++++----------
 arch/x86/kvm/xen.h              | 17 +++++++++++++++--
 5 files changed, 41 insertions(+), 16 deletions(-)


base-commit: 3617c0ee7decb3db3f230b1c844126575fab4d49
-- 
2.48.1.601.g30ceb7b040-goog

[PATCH v2 1/5] KVM: x86/xen: Restrict hypercall MSR to unofficial synthetic range

[Sean Christopherson]

Reject userspace attempts to set the Xen hypercall page MSR to an index
outside of the "standard" virtualization range [0x40000000, 0x4fffffff],
as KVM is not equipped to handle collisions with real MSRs, e.g. KVM
doesn't update MSR interception, conflicts with VMCS/VMCB fields, special
case writes in KVM, etc.

While the MSR index isn't strictly ABI, i.e. can theoretically float to
any value, in practice no known VMM sets the MSR index to anything other
than 0x40000000 or 0x40000200.

Cc: Joao Martins <joao.m.martins@oracle.com>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Paul Durrant <paul@xen.org>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/uapi/asm/kvm.h | 3 +++
 arch/x86/kvm/xen.c              | 9 +++++++++
 2 files changed, 12 insertions(+)

diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index 9e75da97bce0..460306b35a4b 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -559,6 +559,9 @@ struct kvm_x86_mce {
 #define KVM_XEN_HVM_CONFIG_PVCLOCK_TSC_UNSTABLE	(1 << 7)
 #define KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA	(1 << 8)
 
+#define KVM_XEN_MSR_MIN_INDEX			0x40000000u
+#define KVM_XEN_MSR_MAX_INDEX			0x4fffffffu
+
 struct kvm_xen_hvm_config {
 	__u32 flags;
 	__u32 msr;
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index a909b817b9c0..5b94825001a7 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -1324,6 +1324,15 @@ int kvm_xen_hvm_config(struct kvm *kvm, struct kvm_xen_hvm_config *xhc)
 	     xhc->blob_size_32 || xhc->blob_size_64))
 		return -EINVAL;
 
+	/*
+	 * Restrict the MSR to the range that is unofficially reserved for
+	 * synthetic, virtualization-defined MSRs, e.g. to prevent confusing
+	 * KVM by colliding with a real MSR that requires special handling.
+	 */
+	if (xhc->msr &&
+	    (xhc->msr < KVM_XEN_MSR_MIN_INDEX || xhc->msr > KVM_XEN_MSR_MAX_INDEX))
+		return -EINVAL;
+
 	mutex_lock(&kvm->arch.xen.xen_lock);
 
 	if (xhc->msr && !kvm->arch.xen_hvm_config.msr)
-- 
2.48.1.601.g30ceb7b040-goog

[PATCH v2 2/5] KVM: x86/xen: Add an #ifdef'd helper to detect writes to Xen MSR

[Sean Christopherson]

Add a helper to detect writes to the Xen hypercall page MSR, and provide a
stub for CONFIG_KVM_XEN=n to optimize out the check for kernels built
without Xen support.

Reviewed-by: Paul Durrant <paul@xen.org>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c |  2 +-
 arch/x86/kvm/xen.h | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 462a5cd6ac4a..12c60adb7349 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3738,7 +3738,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
 	 * page setup; it could incur locking paths which are not expected
 	 * if userspace sets the MSR in an unusual location.
 	 */
-	if (msr && msr == vcpu->kvm->arch.xen_hvm_config.msr &&
+	if (kvm_xen_is_hypercall_page_msr(vcpu->kvm, msr) &&
 	    !msr_info->host_initiated)
 		return kvm_xen_write_hypercall_page(vcpu, data);
 
diff --git a/arch/x86/kvm/xen.h b/arch/x86/kvm/xen.h
index f5841d9000ae..e92e06926f76 100644
--- a/arch/x86/kvm/xen.h
+++ b/arch/x86/kvm/xen.h
@@ -56,6 +56,11 @@ static inline bool kvm_xen_msr_enabled(struct kvm *kvm)
 		kvm->arch.xen_hvm_config.msr;
 }
 
+static inline bool kvm_xen_is_hypercall_page_msr(struct kvm *kvm, u32 msr)
+{
+	return msr && msr == kvm->arch.xen_hvm_config.msr;
+}
+
 static inline bool kvm_xen_hypercall_enabled(struct kvm *kvm)
 {
 	return static_branch_unlikely(&kvm_xen_enabled.key) &&
@@ -124,6 +129,11 @@ static inline bool kvm_xen_msr_enabled(struct kvm *kvm)
 	return false;
 }
 
+static inline bool kvm_xen_is_hypercall_page_msr(struct kvm *kvm, u32 msr)
+{
+	return false;
+}
+
 static inline bool kvm_xen_hypercall_enabled(struct kvm *kvm)
 {
 	return false;
-- 
2.48.1.601.g30ceb7b040-goog

[PATCH v2 3/5] KVM: x86/xen: Consult kvm_xen_enabled when checking for Xen MSR writes

[Sean Christopherson]

Query kvm_xen_enabled when detecting writes to the Xen hypercall page MSR
so that the check is optimized away in the likely scenario that Xen isn't
enabled for the VM.

Deliberately open code the check instead of using kvm_xen_msr_enabled() in
order to avoid a double load of xen_hvm_config.msr (which is admittedly
rather pointless given the widespread lack of READ_ONCE() usage on the
plethora of vCPU-scoped accesses to kvm->arch.xen state).

No functional change intended.

Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Paul Durrant <paul@xen.org>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/xen.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kvm/xen.h b/arch/x86/kvm/xen.h
index e92e06926f76..1e3a913dfb94 100644
--- a/arch/x86/kvm/xen.h
+++ b/arch/x86/kvm/xen.h
@@ -58,6 +58,9 @@ static inline bool kvm_xen_msr_enabled(struct kvm *kvm)
 
 static inline bool kvm_xen_is_hypercall_page_msr(struct kvm *kvm, u32 msr)
 {
+	if (!static_branch_unlikely(&kvm_xen_enabled.key))
+		return false;
+
 	return msr && msr == kvm->arch.xen_hvm_config.msr;
 }
 
-- 
2.48.1.601.g30ceb7b040-goog

[PATCH v2 4/5] KVM: x86/xen: Bury xen_hvm_config behind CONFIG_KVM_XEN=y

[Sean Christopherson]

Now that all references to kvm_vcpu_arch.xen_hvm_config are wrapped with
CONFIG_KVM_XEN #ifdefs, bury the field itself behind CONFIG_KVM_XEN=y.

No functional change intended.

Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Paul Durrant <paul@xen.org>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/kvm_host.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index b15cde0a9b5c..f31fca4c4968 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1410,8 +1410,6 @@ struct kvm_arch {
 	struct delayed_work kvmclock_update_work;
 	struct delayed_work kvmclock_sync_work;
 
-	struct kvm_xen_hvm_config xen_hvm_config;
-
 	/* reads protected by irq_srcu, writes by irq_lock */
 	struct hlist_head mask_notifier_list;
 
@@ -1421,6 +1419,7 @@ struct kvm_arch {
 
 #ifdef CONFIG_KVM_XEN
 	struct kvm_xen xen;
+	struct kvm_xen_hvm_config xen_hvm_config;
 #endif
 
 	bool backwards_tsc_observed;
-- 
2.48.1.601.g30ceb7b040-goog

[PATCH v2 5/5] KVM: x86/xen: Move kvm_xen_hvm_config field into kvm_xen

[Sean Christopherson]

Now that all KVM usage of the Xen HVM config information is buried behind
CONFIG_KVM_XEN=y, move the per-VM kvm_xen_hvm_config field out of kvm_arch
and into kvm_xen.

No functional change intended.

Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
Reviewed-by: Paul Durrant <paul@xen.org>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/kvm_host.h |  3 ++-
 arch/x86/kvm/x86.c              |  2 +-
 arch/x86/kvm/xen.c              | 20 ++++++++++----------
 arch/x86/kvm/xen.h              |  6 +++---
 4 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index f31fca4c4968..9df725e528b1 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1188,6 +1188,8 @@ struct kvm_xen {
 	struct gfn_to_pfn_cache shinfo_cache;
 	struct idr evtchn_ports;
 	unsigned long poll_mask[BITS_TO_LONGS(KVM_MAX_VCPUS)];
+
+	struct kvm_xen_hvm_config hvm_config;
 };
 #endif
 
@@ -1419,7 +1421,6 @@ struct kvm_arch {
 
 #ifdef CONFIG_KVM_XEN
 	struct kvm_xen xen;
-	struct kvm_xen_hvm_config xen_hvm_config;
 #endif
 
 	bool backwards_tsc_observed;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 12c60adb7349..f97d4d435e7f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3188,7 +3188,7 @@ static int kvm_guest_time_update(struct kvm_vcpu *v)
 	 * problems if they observe PVCLOCK_TSC_STABLE_BIT in the pvclock flags.
 	 */
 	bool xen_pvclock_tsc_unstable =
-		ka->xen_hvm_config.flags & KVM_XEN_HVM_CONFIG_PVCLOCK_TSC_UNSTABLE;
+		ka->xen.hvm_config.flags & KVM_XEN_HVM_CONFIG_PVCLOCK_TSC_UNSTABLE;
 #endif
 
 	kernel_ns = 0;
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 5b94825001a7..8aef7cd24349 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -1280,10 +1280,10 @@ int kvm_xen_write_hypercall_page(struct kvm_vcpu *vcpu, u64 data)
 		 * Note, truncation is a non-issue as 'lm' is guaranteed to be
 		 * false for a 32-bit kernel, i.e. when hva_t is only 4 bytes.
 		 */
-		hva_t blob_addr = lm ? kvm->arch.xen_hvm_config.blob_addr_64
-				     : kvm->arch.xen_hvm_config.blob_addr_32;
-		u8 blob_size = lm ? kvm->arch.xen_hvm_config.blob_size_64
-				  : kvm->arch.xen_hvm_config.blob_size_32;
+		hva_t blob_addr = lm ? kvm->arch.xen.hvm_config.blob_addr_64
+				     : kvm->arch.xen.hvm_config.blob_addr_32;
+		u8 blob_size = lm ? kvm->arch.xen.hvm_config.blob_size_64
+				  : kvm->arch.xen.hvm_config.blob_size_32;
 		u8 *page;
 		int ret;
 
@@ -1335,13 +1335,13 @@ int kvm_xen_hvm_config(struct kvm *kvm, struct kvm_xen_hvm_config *xhc)
 
 	mutex_lock(&kvm->arch.xen.xen_lock);
 
-	if (xhc->msr && !kvm->arch.xen_hvm_config.msr)
+	if (xhc->msr && !kvm->arch.xen.hvm_config.msr)
 		static_branch_inc(&kvm_xen_enabled.key);
-	else if (!xhc->msr && kvm->arch.xen_hvm_config.msr)
+	else if (!xhc->msr && kvm->arch.xen.hvm_config.msr)
 		static_branch_slow_dec_deferred(&kvm_xen_enabled);
 
-	old_flags = kvm->arch.xen_hvm_config.flags;
-	memcpy(&kvm->arch.xen_hvm_config, xhc, sizeof(*xhc));
+	old_flags = kvm->arch.xen.hvm_config.flags;
+	memcpy(&kvm->arch.xen.hvm_config, xhc, sizeof(*xhc));
 
 	mutex_unlock(&kvm->arch.xen.xen_lock);
 
@@ -1422,7 +1422,7 @@ static bool kvm_xen_schedop_poll(struct kvm_vcpu *vcpu, bool longmode,
 	int i;
 
 	if (!lapic_in_kernel(vcpu) ||
-	    !(vcpu->kvm->arch.xen_hvm_config.flags & KVM_XEN_HVM_CONFIG_EVTCHN_SEND))
+	    !(vcpu->kvm->arch.xen.hvm_config.flags & KVM_XEN_HVM_CONFIG_EVTCHN_SEND))
 		return false;
 
 	if (IS_ENABLED(CONFIG_64BIT) && !longmode) {
@@ -2300,6 +2300,6 @@ void kvm_xen_destroy_vm(struct kvm *kvm)
 	}
 	idr_destroy(&kvm->arch.xen.evtchn_ports);
 
-	if (kvm->arch.xen_hvm_config.msr)
+	if (kvm->arch.xen.hvm_config.msr)
 		static_branch_slow_dec_deferred(&kvm_xen_enabled);
 }
diff --git a/arch/x86/kvm/xen.h b/arch/x86/kvm/xen.h
index 1e3a913dfb94..d191103d8163 100644
--- a/arch/x86/kvm/xen.h
+++ b/arch/x86/kvm/xen.h
@@ -53,7 +53,7 @@ static inline void kvm_xen_sw_enable_lapic(struct kvm_vcpu *vcpu)
 static inline bool kvm_xen_msr_enabled(struct kvm *kvm)
 {
 	return static_branch_unlikely(&kvm_xen_enabled.key) &&
-		kvm->arch.xen_hvm_config.msr;
+		kvm->arch.xen.hvm_config.msr;
 }
 
 static inline bool kvm_xen_is_hypercall_page_msr(struct kvm *kvm, u32 msr)
@@ -61,13 +61,13 @@ static inline bool kvm_xen_is_hypercall_page_msr(struct kvm *kvm, u32 msr)
 	if (!static_branch_unlikely(&kvm_xen_enabled.key))
 		return false;
 
-	return msr && msr == kvm->arch.xen_hvm_config.msr;
+	return msr && msr == kvm->arch.xen.hvm_config.msr;
 }
 
 static inline bool kvm_xen_hypercall_enabled(struct kvm *kvm)
 {
 	return static_branch_unlikely(&kvm_xen_enabled.key) &&
-		(kvm->arch.xen_hvm_config.flags &
+		(kvm->arch.xen.hvm_config.flags &
 		 KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL);
 }
 
-- 
2.48.1.601.g30ceb7b040-goog

Re: [PATCH v2 1/5] KVM: x86/xen: Restrict hypercall MSR to unofficial synthetic range

[David Woodhouse]

On 15 February 2025 02:14:33 CET, Sean Christopherson <seanjc@google.com> wrote:
>Reject userspace attempts to set the Xen hypercall page MSR to an index
>outside of the "standard" virtualization range [0x40000000, 0x4fffffff],
>as KVM is not equipped to handle collisions with real MSRs, e.g. KVM
>doesn't update MSR interception, conflicts with VMCS/VMCB fields, special
>case writes in KVM, etc.
>
>While the MSR index isn't strictly ABI, i.e. can theoretically float to
>any value, in practice no known VMM sets the MSR index to anything other
>than 0x40000000 or 0x40000200.
>
>Cc: Joao Martins <joao.m.martins@oracle.com>
>Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
>Reviewed-by: Paul Durrant <paul@xen.org>
>Signed-off-by: Sean Christopherson <seanjc@google.com>
>---
> arch/x86/include/uapi/asm/kvm.h | 3 +++
> arch/x86/kvm/xen.c              | 9 +++++++++
> 2 files changed, 12 insertions(+)
>
>diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
>index 9e75da97bce0..460306b35a4b 100644
>--- a/arch/x86/include/uapi/asm/kvm.h
>+++ b/arch/x86/include/uapi/asm/kvm.h
>@@ -559,6 +559,9 @@ struct kvm_x86_mce {
> #define KVM_XEN_HVM_CONFIG_PVCLOCK_TSC_UNSTABLE	(1 << 7)
> #define KVM_XEN_HVM_CONFIG_SHARED_INFO_HVA	(1 << 8)
> 
>+#define KVM_XEN_MSR_MIN_INDEX			0x40000000u
>+#define KVM_XEN_MSR_MAX_INDEX			0x4fffffffu
>+
> struct kvm_xen_hvm_config {
> 	__u32 flags;
> 	__u32 msr;
>diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
>index a909b817b9c0..5b94825001a7 100644
>--- a/arch/x86/kvm/xen.c
>+++ b/arch/x86/kvm/xen.c
>@@ -1324,6 +1324,15 @@ int kvm_xen_hvm_config(struct kvm *kvm, struct kvm_xen_hvm_config *xhc)
> 	     xhc->blob_size_32 || xhc->blob_size_64))
> 		return -EINVAL;
> 
>+	/*
>+	 * Restrict the MSR to the range that is unofficially reserved for
>+	 * synthetic, virtualization-defined MSRs, e.g. to prevent confusing
>+	 * KVM by colliding with a real MSR that requires special handling.
>+	 */
>+	if (xhc->msr &&
>+	    (xhc->msr < KVM_XEN_MSR_MIN_INDEX || xhc->msr > KVM_XEN_MSR_MAX_INDEX))
>+		return -EINVAL;
>+
> 	mutex_lock(&kvm->arch.xen.xen_lock);
> 
> 	if (xhc->msr && !kvm->arch.xen_hvm_config.msr)

I'd still like to restrict this to ensure it doesn't collide with MSRs that KVM expects to emulate. But that can be a separate patch, as discussed.

This patch should probably have a docs update too.

Re: [PATCH v2 1/5] KVM: x86/xen: Restrict hypercall MSR to unofficial synthetic range

[Sean Christopherson]

On Sat, Feb 15, 2025, David Woodhouse wrote:
> On 15 February 2025 02:14:33 CET, Sean Christopherson <seanjc@google.com> wrote:
> >diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> >index a909b817b9c0..5b94825001a7 100644
> >--- a/arch/x86/kvm/xen.c
> >+++ b/arch/x86/kvm/xen.c
> >@@ -1324,6 +1324,15 @@ int kvm_xen_hvm_config(struct kvm *kvm, struct kvm_xen_hvm_config *xhc)
> > 	     xhc->blob_size_32 || xhc->blob_size_64))
> > 		return -EINVAL;
> > 
> >+	/*
> >+	 * Restrict the MSR to the range that is unofficially reserved for
> >+	 * synthetic, virtualization-defined MSRs, e.g. to prevent confusing
> >+	 * KVM by colliding with a real MSR that requires special handling.
> >+	 */
> >+	if (xhc->msr &&
> >+	    (xhc->msr < KVM_XEN_MSR_MIN_INDEX || xhc->msr > KVM_XEN_MSR_MAX_INDEX))
> >+		return -EINVAL;
> >+
> > 	mutex_lock(&kvm->arch.xen.xen_lock);
> > 
> > 	if (xhc->msr && !kvm->arch.xen_hvm_config.msr)
> 
> I'd still like to restrict this to ensure it doesn't collide with MSRs that
> KVM expects to emulate. But that can be a separate patch, as discussed.

I think that has to go in userspace.  If KVM adds on-by-default, i.e. unguarded,
conflicting MSR emulation, then KVM will have broken userspace regardless of
whether or not KVM explicitly rejects KVM_XEN_HVM_CONFIG based on emulated MSRs.

If we assume future us are somewhat competent and guard new MSR emulation with a
feature bit, capability, etc., then rejecting KVM_XEN_HVM_CONFIG isn't obviously
better, or even feasible in some cases.  E.g. if the opt-in is done via guest
CPUID, then KVM is stuck because KVM_XEN_HVM_CONFIG can (and generally should?)
be called before vCPUs are even created.  Even if the opt-in is VM-scoped, e.g.
a capabilitiy, there are still ordering issues as userpace would see different
behavior depending on the order between KVM_XEN_HVM_CONFIG and the capability.

And if the MSR emulation is guarded, rejecting KVM_XEN_HVM_CONFIG without a
precise check is undesirable, because KVM would unnecessarily break userspace.

> This patch should probably have a docs update too.

Gah, forgot that.

Re: [PATCH v2 1/5] KVM: x86/xen: Restrict hypercall MSR to unofficial synthetic range

[David Woodhouse]

On 18 February 2025 17:33:14 CET, Sean Christopherson <seanjc@google.com> wrote:
>On Sat, Feb 15, 2025, David Woodhouse wrote:
>> On 15 February 2025 02:14:33 CET, Sean Christopherson <seanjc@google.com> wrote:
>> >diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
>> >index a909b817b9c0..5b94825001a7 100644
>> >--- a/arch/x86/kvm/xen.c
>> >+++ b/arch/x86/kvm/xen.c
>> >@@ -1324,6 +1324,15 @@ int kvm_xen_hvm_config(struct kvm *kvm, struct kvm_xen_hvm_config *xhc)
>> > 	     xhc->blob_size_32 || xhc->blob_size_64))
>> > 		return -EINVAL;
>> > 
>> >+	/*
>> >+	 * Restrict the MSR to the range that is unofficially reserved for
>> >+	 * synthetic, virtualization-defined MSRs, e.g. to prevent confusing
>> >+	 * KVM by colliding with a real MSR that requires special handling.
>> >+	 */
>> >+	if (xhc->msr &&
>> >+	    (xhc->msr < KVM_XEN_MSR_MIN_INDEX || xhc->msr > KVM_XEN_MSR_MAX_INDEX))
>> >+		return -EINVAL;
>> >+
>> > 	mutex_lock(&kvm->arch.xen.xen_lock);
>> > 
>> > 	if (xhc->msr && !kvm->arch.xen_hvm_config.msr)
>> 
>> I'd still like to restrict this to ensure it doesn't collide with MSRs that
>> KVM expects to emulate. But that can be a separate patch, as discussed.
>
>I think that has to go in userspace.  If KVM adds on-by-default, i.e. unguarded,
>conflicting MSR emulation, then KVM will have broken userspace regardless of
>whether or not KVM explicitly rejects KVM_XEN_HVM_CONFIG based on emulated MSRs.
>
>If we assume future us are somewhat competent and guard new MSR emulation with a
>feature bit, capability, etc., then rejecting KVM_XEN_HVM_CONFIG isn't obviously
>better, or even feasible in some cases.  E.g. if the opt-in is done via guest
>CPUID, then KVM is stuck because KVM_XEN_HVM_CONFIG can (and generally should?)
>be called before vCPUs are even created.  Even if the opt-in is VM-scoped, e.g.
>a capabilitiy, there are still ordering issues as userpace would see different
>behavior depending on the order between KVM_XEN_HVM_CONFIG and the capability.

Well, I just changed QEMU to do KVM_XEN_HVM_CONFIG from the first vCPU init because QEMU doesn't know if it needs to avoid the Hyper-V MSR space at kvm_xen_init() time. But yes, we don't want to depend on ordering either way.

Re: [PATCH v2 1/5] KVM: x86/xen: Restrict hypercall MSR to unofficial synthetic range

[Sean Christopherson]

On Sat, Feb 15, 2025, David Woodhouse wrote:
> On 15 February 2025 02:14:33 CET, Sean Christopherson <seanjc@google.com> wrote:
> >Reject userspace attempts to set the Xen hypercall page MSR to an index
> >outside of the "standard" virtualization range [0x40000000, 0x4fffffff],
> >as KVM is not equipped to handle collisions with real MSRs, e.g. KVM
> >doesn't update MSR interception, conflicts with VMCS/VMCB fields, special
> >case writes in KVM, etc.
> >
> >While the MSR index isn't strictly ABI, i.e. can theoretically float to
> >any value, in practice no known VMM sets the MSR index to anything other
> >than 0x40000000 or 0x40000200.

...

> This patch should probably have a docs update too.

To avoid sending an entirely new version only to discover I suck at writing docs,
how does this look?

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 2b52eb77e29c..5fe84f2427b5 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -1000,6 +1000,10 @@ blobs in userspace.  When the guest writes the MSR, kvm copies one
 page of a blob (32- or 64-bit, depending on the vcpu mode) to guest
 memory.
 
+The MSR index must be in the range [0x40000000, 0x4fffffff], i.e. must reside
+in the range that is unofficially reserved for use by hypervisors.  The min/max
+values are enumerated via KVM_XEN_MSR_MIN_INDEX and KVM_XEN_MSR_MAX_INDEX.
+
 ::
 
   struct kvm_xen_hvm_config {

Re: [PATCH v2 1/5] KVM: x86/xen: Restrict hypercall MSR to unofficial synthetic range

[David Woodhouse]

On 20 February 2025 19:36:41 CET, Sean Christopherson <seanjc@google.com> wrote:
>On Sat, Feb 15, 2025, David Woodhouse wrote:
>> On 15 February 2025 02:14:33 CET, Sean Christopherson <seanjc@google.com> wrote:
>> >Reject userspace attempts to set the Xen hypercall page MSR to an index
>> >outside of the "standard" virtualization range [0x40000000, 0x4fffffff],
>> >as KVM is not equipped to handle collisions with real MSRs, e.g. KVM
>> >doesn't update MSR interception, conflicts with VMCS/VMCB fields, special
>> >case writes in KVM, etc.
>> >
>> >While the MSR index isn't strictly ABI, i.e. can theoretically float to
>> >any value, in practice no known VMM sets the MSR index to anything other
>> >than 0x40000000 or 0x40000200.
>
>...
>
>> This patch should probably have a docs update too.
>
>To avoid sending an entirely new version only to discover I suck at writing docs,
>how does this look?
>
>diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
>index 2b52eb77e29c..5fe84f2427b5 100644
>--- a/Documentation/virt/kvm/api.rst
>+++ b/Documentation/virt/kvm/api.rst
>@@ -1000,6 +1000,10 @@ blobs in userspace.  When the guest writes the MSR, kvm copies one
> page of a blob (32- or 64-bit, depending on the vcpu mode) to guest
> memory.
> 
>+The MSR index must be in the range [0x40000000, 0x4fffffff], i.e. must reside
>+in the range that is unofficially reserved for use by hypervisors.  The min/max
>+values are enumerated via KVM_XEN_MSR_MIN_INDEX and KVM_XEN_MSR_MAX_INDEX.
>+
> ::
> 
>   struct kvm_xen_hvm_config {

LGTM, thanks

Re: [PATCH v2 0/5] KVM: x86/xen: Restrict hypercall MSR index

[Sean Christopherson]

On Fri, 14 Feb 2025 17:14:32 -0800, Sean Christopherson wrote:
> Harden KVM against goofy userspace by restricting the Xen hypercall MSR
> index to the de facto standard synthetic range, 0x40000000 - 0x4fffffff.
> This obviously has the potential to break userspace, but I'm fairly confident
> it'll be fine (knock wood), and doing nothing is not an option as letting
> userspace redirect any WRMSR is at best completely broken.
> 
> Patches 2-5 are tangentially related cleanups.
> 
> [...]

Applied to kvm-x86 xen, with the docs change.  Thanks for the reviews!

[1/5] KVM: x86/xen: Restrict hypercall MSR to unofficial synthetic range
      https://github.com/kvm-x86/linux/commit/5c17848134ab
[2/5] KVM: x86/xen: Add an #ifdef'd helper to detect writes to Xen MSR
      https://github.com/kvm-x86/linux/commit/bb0978d95a55
[3/5] KVM: x86/xen: Consult kvm_xen_enabled when checking for Xen MSR writes
      https://github.com/kvm-x86/linux/commit/a5d7700af6b0
[4/5] KVM: x86/xen: Bury xen_hvm_config behind CONFIG_KVM_XEN=y
      https://github.com/kvm-x86/linux/commit/69e5a7dde965
[5/5] KVM: x86/xen: Move kvm_xen_hvm_config field into kvm_xen
      https://github.com/kvm-x86/linux/commit/26e228ec1695

--
https://github.com/kvm-x86/linux/tree/next