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@xxxxxxxxxx> 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.