From 817764ea33a44867f8b8a7bbd2fa4475ebe60de7 Mon Sep 17 00:00:00 2001 From: Andrew Cooper Date: Wed, 19 Jan 2022 19:55:02 +0000 Subject: x86/msr: Fix migration compatibility issue with MSR_SPEC_CTRL This bug existed in early in 2018 between MSR_SPEC_CTRL arriving in microcode, and SSBD arriving a few months later. It went unnoticed presumably because everyone was busy rebooting everything. The same bug will reappear when adding PSFD support. Clamp the guest MSR_SPEC_CTRL value to that permitted by CPUID on migrate. The guest is already playing with reserved bits at this point, and clamping the value will prevent a migration to a less capable host from failing. Signed-off-by: Andrew Cooper Reviewed-by: Jan Beulich (cherry picked from commit 969a57f73f6b011b2ebf4c0ab1715efc65837335) --- a/xen/arch/x86/cpuid.c +++ b/xen/arch/x86/cpuid.c @@ -612,6 +612,24 @@ void __init init_guest_cpuid(void) calculate_hvm_max_policy(); } +/* + * Caller to confirm that MSR_SPEC_CTRL is available. Intel and AMD have + * separate CPUID features for this functionality, but only set will be + * active. + */ +uint64_t msr_spec_ctrl_valid_bits(const struct cpuid_policy *cp) +{ + bool ssbd = cp->feat.ssbd || cp->extd.amd_ssbd; + + /* + * Note: SPEC_CTRL_STIBP is specified as safe to use (i.e. ignored) + * when STIBP isn't enumerated in hardware. + */ + return (SPEC_CTRL_IBRS | SPEC_CTRL_STIBP | + (ssbd ? SPEC_CTRL_SSBD : 0) | + 0); +} + const uint32_t *lookup_deep_deps(uint32_t feature) { static const struct { --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -3453,7 +3453,7 @@ int hvm_msr_read_intercept(unsigned int goto gp_fault; case MSR_SPEC_CTRL: - if ( !d->arch.cpuid->feat.ibrsb ) + if ( !d->arch.cpuid->feat.ibrsb && !d->arch.cpuid->extd.ibrs ) goto gp_fault; *msr_content = v->arch.spec_ctrl; break; @@ -3637,17 +3637,9 @@ int hvm_msr_write_intercept(unsigned int break; case MSR_SPEC_CTRL: - if ( !d->arch.cpuid->feat.ibrsb ) - goto gp_fault; /* MSR available? */ - - /* - * Note: SPEC_CTRL_STIBP is specified as safe to use (i.e. ignored) - * when STIBP isn't enumerated in hardware. - */ - - if ( msr_content & ~(SPEC_CTRL_IBRS | SPEC_CTRL_STIBP | - (d->arch.cpuid->feat.ssbd ? SPEC_CTRL_SSBD : 0)) ) - goto gp_fault; /* Rsvd bit set? */ + if ( (!d->arch.cpuid->feat.ibrsb && !d->arch.cpuid->extd.ibrs) || + (msr_content & ~msr_spec_ctrl_valid_bits(d->arch.cpuid)) ) + goto gp_fault; v->arch.spec_ctrl = msr_content; break; --- a/xen/arch/x86/hvm/svm/svm.c +++ b/xen/arch/x86/hvm/svm/svm.c @@ -394,11 +394,33 @@ static int svm_load_vmcb_ctxt(struct vcp static unsigned int __init svm_init_msr(void) { - return boot_cpu_has(X86_FEATURE_DBEXT) ? 4 : 0; + return !!boot_cpu_has(X86_FEATURE_IBRS) + + (boot_cpu_has(X86_FEATURE_DBEXT) ? 4 : 0); } static void svm_save_msr(struct vcpu *v, struct hvm_msr *ctxt) { + if ( v->domain->arch.cpuid->extd.ibrs && v->arch.spec_ctrl ) + { + /* + * Guests are given full access to certain MSRs for performance + * reasons. A consequence is that Xen is unable to enforce that + * all bits disallowed by the CPUID policy yield #GP, and an + * enterprising guest may be able to set and use a bit it ought to + * leave alone. + * + * When migrating from a more capable host to a less capable one, + * such bits may be rejected by the destination, and the migration + * failed. + * + * Discard such bits here on the source side. Such bits have + * reserved behaviour, and the guest has only itself to blame. + */ + ctxt->msr[ctxt->count].index = MSR_SPEC_CTRL; + ctxt->msr[ctxt->count++].val = v->arch.spec_ctrl & + msr_spec_ctrl_valid_bits(v->domain->arch.cpuid); + } + if ( boot_cpu_has(X86_FEATURE_DBEXT) ) { ctxt->msr[ctxt->count].val = v->arch.hvm_svm.dr_mask[0]; --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -944,8 +944,23 @@ static void vmx_save_msr(struct vcpu *v, if ( v->domain->arch.cpuid->feat.ibrsb && v->arch.spec_ctrl ) { + /* + * Guests are given full access to certain MSRs for performance + * reasons. A consequence is that Xen is unable to enforce that + * all bits disallowed by the CPUID policy yield #GP, and an + * enterprising guest may be able to set and use a bit it ought to + * leave alone. + * + * When migrating from a more capable host to a less capable one, + * such bits may be rejected by the destination, and the migration + * failed. + * + * Discard such bits here on the source side. Such bits have + * reserved behaviour, and the guest has only itself to blame. + */ ctxt->msr[ctxt->count].index = MSR_SPEC_CTRL; - ctxt->msr[ctxt->count++].val = v->arch.spec_ctrl; + ctxt->msr[ctxt->count++].val = v->arch.spec_ctrl & + msr_spec_ctrl_valid_bits(v->domain->arch.cpuid); } if ( cpu_has_mpx && cpu_has_vmx_mpx ) @@ -979,13 +994,8 @@ static int vmx_load_msr(struct vcpu *v, case MSR_SPEC_CTRL: if ( !v->domain->arch.cpuid->feat.ibrsb ) err = -ENXIO; /* MSR available? */ - /* - * Note: SPEC_CTRL_STIBP is specified as safe to use (i.e. - * ignored) when STIBP isn't enumerated in hardware. - */ else if ( ctxt->msr[i].val & - ~(SPEC_CTRL_IBRS | SPEC_CTRL_STIBP | - (v->domain->arch.cpuid->feat.ssbd ? SPEC_CTRL_SSBD : 0)) ) + ~msr_spec_ctrl_valid_bits(v->domain->arch.cpuid) ) err = -ENXIO; else v->arch.spec_ctrl = ctxt->msr[i].val; --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -2671,7 +2671,7 @@ static int priv_op_read_msr(unsigned int break; case MSR_SPEC_CTRL: - if ( !currd->arch.cpuid->feat.ibrsb ) + if ( !currd->arch.cpuid->feat.ibrsb && !currd->arch.cpuid->extd.ibrs ) break; *val = curr->arch.spec_ctrl; return X86EMUL_OKAY; @@ -2908,17 +2908,10 @@ static int priv_op_write_msr(unsigned in break; case MSR_SPEC_CTRL: - if ( !currd->arch.cpuid->feat.ibrsb ) - break; /* MSR available? */ - - /* - * Note: SPEC_CTRL_STIBP is specified as safe to use (i.e. ignored) - * when STIBP isn't enumerated in hardware. - */ - - if ( val & ~(SPEC_CTRL_IBRS | SPEC_CTRL_STIBP | - (currd->arch.cpuid->feat.ssbd ? SPEC_CTRL_SSBD : 0)) ) - break; /* Rsvd bit set? */ + if ( (!currd->arch.cpuid->feat.ibrsb && + !currd->arch.cpuid->extd.ibrs) || + (val & ~msr_spec_ctrl_valid_bits(currd->arch.cpuid)) ) + break; curr->arch.spec_ctrl = val; return X86EMUL_OKAY; --- a/xen/include/asm-x86/msr.h +++ b/xen/include/asm-x86/msr.h @@ -222,6 +222,8 @@ static inline void wrmsr_tsc_aux(uint32_ } } +uint64_t msr_spec_ctrl_valid_bits(const struct cpuid_policy *cp); + /* MSR policy object for shared per-domain MSRs */ struct msr_policy { /*