[PATCH 0/3] KVM: x86: Fix XSAVE related bugs

[PATCH 0/3] KVM: x86: Fix XSAVE related bugs

[Sean Christopherson]

Patch 2 (from Dave) is the headliner and fixes a bug where KVM clear the
FP+SSE bits in user_xfeatures when XSAVE is hidden from the guest and thus
prevent userspace from saving/restoring FP+SSE state on XSAVE host.  This
most visibily manifests as a failed migration (KVM_GET_XSAVE succeeds on a
non-XSAVE host and KVM_SET_XSAVE fails on an XSAVE host), but also causes
KVM_GET_SAVE on XSAVE hosts to effectively corrupt guest FP+SSE state.

Patch 1 fixes a mostly theoretical bug, and is also a prerequisite for
patch 2.

Patch 3 fixes a bug found by inspection when staring at all of this.  KVM
fails to check CR4.OSXSAVE when emulating XSETBV (the interception case
gets away without the check because the intercept happens after hardware
checks CR4).

Dr. David Alan Gilbert (1):
  KVM: x86: Always enable legacy FP/SSE in allowed user XFEATURES

Sean Christopherson (2):
  KVM: x86: Reinstate kvm_vcpu_arch.guest_supported_xcr0
  KVM: x86: Inject #UD on emulated XSETBV if XSAVES isn't enabled

 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/cpuid.c            | 11 ++++++++---
 arch/x86/kvm/emulate.c          |  3 +++
 arch/x86/kvm/x86.c              | 10 +++-------
 4 files changed, 15 insertions(+), 10 deletions(-)


base-commit: 372d07084593dc7a399bf9bee815711b1fb1bcf2
-- 
2.37.1.595.g718a3a8f04-goog

[PATCH 1/3] KVM: x86: Reinstate kvm_vcpu_arch.guest_supported_xcr0

[Sean Christopherson]

Reinstate the per-vCPU guest_supported_xcr0 by partially reverting
commit 988896bb6182; the implicit assessment that guest_supported_xcr0 is
always the same as guest_fpu.fpstate->user_xfeatures was incorrect.

kvm_vcpu_after_set_cpuid() isn't the only place that sets user_xfeatures,
as user_xfeatures is set to fpu_user_cfg.default_features when guest_fpu
is allocated via fpu_alloc_guest_fpstate() => __fpstate_reset().
guest_supported_xcr0 on the other hand is zero-allocated.  If userspace
never invokes KVM_SET_CPUID2, supported XCR0 will be '0', whereas the
allowed user XFEATURES will be non-zero.

Practically speaking, the edge case likely doesn't matter as no sane
userspace will live migrate a VM without ever doing KVM_SET_CPUID2. The
primary motivation is to prepare for KVM intentionally and explicitly
setting bits in user_xfeatures that are not set in guest_supported_xcr0.

Because KVM_{G,S}ET_XSAVE can be used to svae/restore FP+SSE state even
if the host doesn't support XSAVE, KVM needs to set the FP+SSE bits in
user_xfeatures even if they're not allowed in XCR0, e.g. because XCR0
isn't exposed to the guest.  At that point, the simplest fix is to track
the two things separately (allowed save/restore vs. allowed XCR0).

Fixes: 988896bb6182 ("x86/kvm/fpu: Remove kvm_vcpu_arch.guest_supported_xcr0")
Cc: stable@vger.kernel.org
Cc: Leonardo Bras <leobras@redhat.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/kvm_host.h | 1 +
 arch/x86/kvm/cpuid.c            | 5 ++---
 arch/x86/kvm/x86.c              | 9 ++-------
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 2c96c43c313a..aa381ab69a19 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -729,6 +729,7 @@ struct kvm_vcpu_arch {
 	struct fpu_guest guest_fpu;
 
 	u64 xcr0;
+	u64 guest_supported_xcr0;
 
 	struct kvm_pio_request pio;
 	void *pio_data;
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 75dcf7a72605..2e0f27ad736a 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -315,7 +315,6 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 {
 	struct kvm_lapic *apic = vcpu->arch.apic;
 	struct kvm_cpuid_entry2 *best;
-	u64 guest_supported_xcr0;
 
 	best = kvm_find_cpuid_entry(vcpu, 1);
 	if (best && apic) {
@@ -327,10 +326,10 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 		kvm_apic_set_version(vcpu);
 	}
 
-	guest_supported_xcr0 =
+	vcpu->arch.guest_supported_xcr0 =
 		cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent);
 
-	vcpu->arch.guest_fpu.fpstate->user_xfeatures = guest_supported_xcr0;
+	vcpu->arch.guest_fpu.fpstate->user_xfeatures = vcpu->arch.guest_supported_xcr0;
 
 	kvm_update_pv_runtime(vcpu);
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index d7374d768296..97ab53046052 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1011,15 +1011,10 @@ void kvm_load_host_xsave_state(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_load_host_xsave_state);
 
-static inline u64 kvm_guest_supported_xcr0(struct kvm_vcpu *vcpu)
-{
-	return vcpu->arch.guest_fpu.fpstate->user_xfeatures;
-}
-
 #ifdef CONFIG_X86_64
 static inline u64 kvm_guest_supported_xfd(struct kvm_vcpu *vcpu)
 {
-	return kvm_guest_supported_xcr0(vcpu) & XFEATURE_MASK_USER_DYNAMIC;
+	return vcpu->arch.guest_supported_xcr0 & XFEATURE_MASK_USER_DYNAMIC;
 }
 #endif
 
@@ -1042,7 +1037,7 @@ static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
 	 * saving.  However, xcr0 bit 0 is always set, even if the
 	 * emulated CPU does not support XSAVE (see kvm_vcpu_reset()).
 	 */
-	valid_bits = kvm_guest_supported_xcr0(vcpu) | XFEATURE_MASK_FP;
+	valid_bits = vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FP;
 	if (xcr0 & ~valid_bits)
 		return 1;
 
-- 
2.37.1.595.g718a3a8f04-goog

[PATCH 2/3] KVM: x86: Always enable legacy FP/SSE in allowed user XFEATURES

[Sean Christopherson]

From: Dr. David Alan Gilbert <dgilbert@redhat.com>

Allow FP and SSE state to be saved and restored via KVM_{G,SET}_XSAVE on
XSAVE-capable hosts even if their bits are not exposed to the guest via
XCR0.

Failing to allow FP+SSE first showed up as a QEMU live migration failure,
where migrating a VM from a pre-XSAVE host, e.g. Nehalem, to an XSAVE
host failed due to KVM rejecting KVM_SET_XSAVE.  However, the bug also
causes problems even when migrating between XSAVE-capable hosts as
KVM_GET_SAVE won't set any bits in user_xfeatures if XSAVE isn't exposed
to the guest, i.e. KVM will fail to actually migrate FP+SSE.

Because KVM_{G,S}ET_XSAVE are designed to allowing migrating between
hosts with and without XSAVE, KVM_GET_XSAVE on a non-XSAVE (by way of
fpu_copy_guest_fpstate_to_uabi()) always sets the FP+SSE bits in the
header so that KVM_SET_XSAVE will work even if the new host supports
XSAVE.

Fixes: ad856280ddea ("x86/kvm/fpu: Limit guest user_xfeatures to supported bits of XCR0")
bz: https://bugzilla.redhat.com/show_bug.cgi?id=2079311
Cc: stable@vger.kernel.org
Cc: Leonardo Bras <leobras@redhat.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
[sean: add comment, massage changelog]
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/cpuid.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 2e0f27ad736a..4c1c2c06e96b 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -329,7 +329,13 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 	vcpu->arch.guest_supported_xcr0 =
 		cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent);
 
-	vcpu->arch.guest_fpu.fpstate->user_xfeatures = vcpu->arch.guest_supported_xcr0;
+	/*
+	 * FP+SSE can always be saved/restored via KVM_{G,S}ET_XSAVE, even if
+	 * XSAVE/XCRO are not exposed to the guest, and even if XSAVE isn't
+	 * supported by the host.
+	 */
+	vcpu->arch.guest_fpu.fpstate->user_xfeatures = vcpu->arch.guest_supported_xcr0 |
+						       XFEATURE_MASK_FPSSE;
 
 	kvm_update_pv_runtime(vcpu);
 
-- 
2.37.1.595.g718a3a8f04-goog

[PATCH 3/3] KVM: x86: Inject #UD on emulated XSETBV if XSAVES isn't enabled

[Sean Christopherson]

Inject #UD when emulating XSETBV if CR4.OSXSAVE is not set.  This also
covers the "XSAVE not supported" check, as setting CR4.OSXSAVE=1 #GPs if
XSAVE is not supported (and userspace gets to keep the pieces if it
forces incoherent vCPU state).

Add a comment to kvm_emulate_xsetbv() to call out that the CPU checks
CR4.OSXSAVE before checking for intercepts.  AMD'S APM implies that #UD
has priority (says that intercepts are checked before #GP exceptions),
while Intel's SDM says nothing about interception priority.  However,
testing on hardware shows that both AMD and Intel CPUs prioritize the #UD
over interception.

Fixes: 02d4160fbd76 ("x86: KVM: add xsetbv to the emulator")
Cc: stable@vger.kernel.org
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/emulate.c | 3 +++
 arch/x86/kvm/x86.c     | 1 +
 2 files changed, 4 insertions(+)

diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index f092c54d1a2f..8ce5ae61fc41 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -4132,6 +4132,9 @@ static int em_xsetbv(struct x86_emulate_ctxt *ctxt)
 {
 	u32 eax, ecx, edx;
 
+	if (!(ctxt->ops->get_cr(ctxt, 4) & X86_CR4_OSXSAVE))
+		return emulate_ud(ctxt);
+
 	eax = reg_read(ctxt, VCPU_REGS_RAX);
 	edx = reg_read(ctxt, VCPU_REGS_RDX);
 	ecx = reg_read(ctxt, VCPU_REGS_RCX);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 97ab53046052..356d0475ab6d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1065,6 +1065,7 @@ static int __kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr)
 
 int kvm_emulate_xsetbv(struct kvm_vcpu *vcpu)
 {
+	/* Note, #UD due to CR4.OSXSAVE=0 has priority over the intercept. */
 	if (static_call(kvm_x86_get_cpl)(vcpu) != 0 ||
 	    __kvm_set_xcr(vcpu, kvm_rcx_read(vcpu), kvm_read_edx_eax(vcpu))) {
 		kvm_inject_gp(vcpu, 0);
-- 
2.37.1.595.g718a3a8f04-goog

Re: [PATCH 0/3] KVM: x86: Fix XSAVE related bugs

[Dr. David Alan Gilbert]

* Sean Christopherson (seanjc@google.com) wrote:
> Patch 2 (from Dave) is the headliner and fixes a bug where KVM clear the
> FP+SSE bits in user_xfeatures when XSAVE is hidden from the guest and thus
> prevent userspace from saving/restoring FP+SSE state on XSAVE host.  This
> most visibily manifests as a failed migration (KVM_GET_XSAVE succeeds on a
> non-XSAVE host and KVM_SET_XSAVE fails on an XSAVE host), but also causes
> KVM_GET_SAVE on XSAVE hosts to effectively corrupt guest FP+SSE state.
> 
> Patch 1 fixes a mostly theoretical bug, and is also a prerequisite for
> patch 2.
> 
> Patch 3 fixes a bug found by inspection when staring at all of this.  KVM
> fails to check CR4.OSXSAVE when emulating XSETBV (the interception case
> gets away without the check because the intercept happens after hardware
> checks CR4).

Thanks for pulling those together; the set of 3 passes my same (light) smoke test.

Dave
> 
> Dr. David Alan Gilbert (1):
>   KVM: x86: Always enable legacy FP/SSE in allowed user XFEATURES
> 
> Sean Christopherson (2):
>   KVM: x86: Reinstate kvm_vcpu_arch.guest_supported_xcr0
>   KVM: x86: Inject #UD on emulated XSETBV if XSAVES isn't enabled
> 
>  arch/x86/include/asm/kvm_host.h |  1 +
>  arch/x86/kvm/cpuid.c            | 11 ++++++++---
>  arch/x86/kvm/emulate.c          |  3 +++
>  arch/x86/kvm/x86.c              | 10 +++-------
>  4 files changed, 15 insertions(+), 10 deletions(-)
> 
> 
> base-commit: 372d07084593dc7a399bf9bee815711b1fb1bcf2
> -- 
> 2.37.1.595.g718a3a8f04-goog
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 0/3] KVM: x86: Fix XSAVE related bugs

[Sean Christopherson]

On Wed, Aug 24, 2022, Dr. David Alan Gilbert wrote:
> * Sean Christopherson (seanjc@google.com) wrote:
> > Patch 2 (from Dave) is the headliner and fixes a bug where KVM clear the
> > FP+SSE bits in user_xfeatures when XSAVE is hidden from the guest and thus
> > prevent userspace from saving/restoring FP+SSE state on XSAVE host.  This
> > most visibily manifests as a failed migration (KVM_GET_XSAVE succeeds on a
> > non-XSAVE host and KVM_SET_XSAVE fails on an XSAVE host), but also causes
> > KVM_GET_SAVE on XSAVE hosts to effectively corrupt guest FP+SSE state.
> > 
> > Patch 1 fixes a mostly theoretical bug, and is also a prerequisite for
> > patch 2.
> > 
> > Patch 3 fixes a bug found by inspection when staring at all of this.  KVM
> > fails to check CR4.OSXSAVE when emulating XSETBV (the interception case
> > gets away without the check because the intercept happens after hardware
> > checks CR4).
> 
> Thanks for pulling those together; the set of 3 passes my same (light) smoke test.

Paolo, do you want to grab this series for 6.0?

Re: [PATCH 0/3] KVM: x86: Fix XSAVE related bugs

[Paolo Bonzini]

Queued, thanks.

Paolo