linux-kernel - Re: [PATCH 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] [day] [month] [year] [list]

Message-ID: <Z6ZAZjBj9jv-VKgS@google.com>
Date: Fri, 7 Feb 2025 09:18:30 -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, 
	syzbot+cdeaeec70992eca2d920@...kaller.appspotmail.com, 
	Joao Martins <joao.m.martins@...cle.com>
Subject: Re: [PATCH 1/5] KVM: x86/xen: Restrict hypercall MSR to unofficial
 synthetic range

On Thu, Feb 06, 2025, David Woodhouse wrote:
> On Wed, 2025-02-05 at 11:20 -0800, Sean Christopherson wrote:
> > On Wed, Feb 05, 2025, David Woodhouse wrote:
> > > On Wed, 2025-02-05 at 16:18 +0000, David Woodhouse wrote:
> > > > 
> > > > > Oh!  It doesn't help KVM avoid breaking userspace, but a way for QEMU to avoid a
> > > > > future collision would be to have QEMU start at 0x40000200 when Hyper-V is enabled,
> > > > > but then use KVM_GET_MSR_INDEX_LIST to detect a collision with KVM Hyper-V, e.g.
> > > > > increment the index until an available index is found (with sanity checks and whatnot).
> > > > 
> > > > Makes sense. I think that's a third separate patch, yes?
> > > 
> > > To be clear, I think I mean a third patch which further restricts
> > > kvm_xen_hvm_config() to disallow indices for which
> > > kvm_is_advertised_msr() returns true?
> > > 
> > > We could roll that into your original patch instead, if you prefer.
> > 
> > Nah, I like the idea of separate patch.
> 
> Helpfully, kvm_is_advertised_msr() doesn't actually return true for
> MSR_IA32_XSS. Is that a bug?

Technically, no.  KVM doesn't support a non-zero XSS, yet, and so there's nothing
for userspace to save/restore.  But the word "yet"...

> And kvm_vcpu_reset() attempts to set MSR_IA32_XSS even if the guest
> doesn't have X86_FEATURE_XSAVES. Is that a bug?

Ugh, sort of.  Functionally, it's fine.  Though it's quite the mess.  The write
can be straight up deleted, as the vCPU structure is zero-allocated and the CPUID
side effects that using __kvm_set_msr() is intended to deal with are irrelevant
because the CPUID array can't yet exist.

The code came about due to an SDM bug and racing patches, and we missed that the
__kvm_set_msr() would be pointless.

The SDM had a bug that said XSS was cleared to '0' on INIT, and then KVM had a
bug in its emulation of the buggy INIT logic where KVM open coded clearing ia32_xss,
which led to stale CPUID information (XSTATE sizes weren't updated).

While the patch[1] that became commit 05a9e065059e ("KVM: x86: Sync the states
size with the XCR0/IA32_XSS at, any time") was in flight, Xiayoao reported the
SDM bug and sent a fix[2].

I merged the two changes, but overlooked that at RESET, the CPUID array is
guaranteed to be null/empty, and so calling into kvm_update_cpuid_runtime() is
technically pointless.  And because guest CPUID is empty, the vCPU can't
possibly have X86_FEATURE_XSAVES, so gating the write on XSAVES would be even
weirder and more confusing.

I'm not sure what the best answer is.  I'm leaning towards simply deleting the
write.  I'd love to have a better MSR framework in KVM, e.g. to document which
MSRs are modified by INIT, but at this point I think writing '0' to an MSR during
RESET (a.k.a. vCPU creation) does more harm than good.

[1] https://lore.kernel.org/all/20220117082631.86143-1-likexu@tencent.com
[2] https://lore.kernel.org/all/20220126034750.2495371-1-xiaoyao.li@intel.com