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

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]

On Fri, 2025-01-31 at 17:13 -0800, Sean Christopherson wrote:
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -1324,6 +1324,14 @@ 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 < 0x40000000 || xhc->msr > 0x4fffffff))
> +		return -EINVAL;
> +
>  	mutex_lock(&kvm->arch.xen.xen_lock);
>  
>  	if (xhc->msr && !kvm->arch.xen_hvm_config.msr)

I'd prefer to see #defines for those magic values. Especially as there
is a corresponding requirement that they never be set from host context
(which is where the potential locking issues come in). Which train of
thought leads me to ponder this as an alternative (or additional)
solution:

--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3733,7 +3733,13 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
        u32 msr = msr_info->index;
        u64 data = msr_info->data;
 
-       if (msr && msr == vcpu->kvm->arch.xen_hvm_config.msr)
+       /*
+        * Do not allow host-initiated writes to trigger the Xen hypercall
+        * 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 &&
+           !msr_info->host_initiated)
                return kvm_xen_write_hypercall_page(vcpu, data);
 
        switch (msr) {


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]