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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z7S2SpH3CtqCVlBc@google.com>
Date: Tue, 18 Feb 2025 08:33:14 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: David Woodhouse <dwmw2@...radead.org>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Paul Durrant <paul@....org>, kvm@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Joao Martins <joao.m.martins@...cle.com>, 
	David Woodhouse <dwmw@...zon.co.uk>
Subject: Re: [PATCH v2 1/5] KVM: x86/xen: Restrict hypercall MSR to unofficial
 synthetic range

On Sat, Feb 15, 2025, David Woodhouse wrote:
> On 15 February 2025 02:14:33 CET, Sean Christopherson <seanjc@...gle.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.