On Thu, Apr 23, 2020 at 11:14:06AM -0700, Sean Christopherson wrote:
> On Thu, Mar 26, 2020 at 04:18:44PM +0800, Yang Weijiang wrote:
> > +#define CET_MSR_RSVD_BITS_1 GENMASK(1, 0)
> > +#define CET_MSR_RSVD_BITS_2 GENMASK(9, 6)
> > +
> > +static bool cet_check_msr_write(struct kvm_vcpu *vcpu,
>
> s/cet_check_msr_write/is_cet_msr_valid
>
> Otherwise the polarity of the return value isn't obvious.
>
> > + struct msr_data *msr,
>
> Unnecessary newline.
>
> > + u64 mask)
>
> s/mask/rsvd_bits
>
Sure, will change them, thank you!
> > +{
> > + u64 data = msr->data;
> > + u32 high_word = data >> 32;
> > +
> > + if (data & mask)
> > + return false;
> > +
> > + if (!is_64_bit_mode(vcpu) && high_word)
> > + return false;
>
> As I called out before, this is wrong. AFAIK, the CPU never depends on
> WRMSR to prevent loading bits 63:32, software can simply do WRMSR and then
> transition back to 32-bit mode. Yes, the shadow stack itself is 32 bits,
> but the internal value is still 64 bits. This is backed up by the CALL
> pseudocode:
>
So I'll remove this invalid check, thanks for the comments!
> IF ShadowStackEnabled(CPL)
> IF (EFER.LMA and DEST(CodeSegmentSelector).L) = 0
> (* If target is legacy or compatibility mode then the SSP must be in low 4GB *)
> IF (SSP & 0xFFFFFFFF00000000 != 0)
> THEN #GP(0); FI;
> FI;
>
> as well as RDSSP:
>
> IF CPL = 3
> IF CR4.CET & IA32_U_CET.SH_STK_EN
> IF (operand size is 64 bit)
> THEN
> Dest ← SSP;
> ELSE
> Dest ← SSP[31:0];
> FI;
> FI;
> ELSE
>
> > +
> > + return true;
> > +}
> > +
> > +static bool cet_check_ssp_msr_access(struct kvm_vcpu *vcpu,
> > + struct msr_data *msr)
>
> Similar to above, the polarity of the return isn't obvious. Maybe
> is_cet_ssp_msr_accessible()?
>
> I'd prefer to pass in @index, passing the full @msr makes it look like
> this helper might also check msr->data.
>
Sure, will follow it.
> > +{
> > + u32 index = msr->index;
> > +
> > + if (!boot_cpu_has(X86_FEATURE_SHSTK))
> > + return false;
> > +
> > + if (!msr->host_initiated &&
> > + !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK))
> > + return false;
> > +
> > + if (index == MSR_IA32_INT_SSP_TAB)
> > + return true;
> > +
> > + if (index == MSR_IA32_PL3_SSP) {
> > + if (!(supported_xss & XFEATURE_MASK_CET_USER))
> > + return false;
> > + } else if (!(supported_xss & XFEATURE_MASK_CET_KERNEL)) {
> > + return false;
> > + }
>
> if (index == MSR_IA32_PL3_SSP)
> return supported_xss & XFEATURE_MASK_CET_USER;
>
> /* MSR_IA32_PL[0-2]_SSP */
> return supported_xss & XFEATURE_MASK_CET_KERNEL;
Nice! ;-))
> > +
> > + return true;
> > +}
> > +
> > +static bool cet_check_ctl_msr_access(struct kvm_vcpu *vcpu,
>
> is_cet_ctl_msr_accessible?
>
OK.
> > + struct msr_data *msr)
> > +{
> > + u32 index = msr->index;
> > +
> > + if (!boot_cpu_has(X86_FEATURE_SHSTK) &&
> > + !boot_cpu_has(X86_FEATURE_IBT))
> > + return false;
> > +
> > + if (!msr->host_initiated &&
> > + !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK) &&
> > + !guest_cpuid_has(vcpu, X86_FEATURE_IBT))
> > + return false;
> > +
> > + if (index == MSR_IA32_U_CET) {
> > + if (!(supported_xss & XFEATURE_MASK_CET_USER))
> > + return false;
> > + } else if (!(supported_xss & XFEATURE_MASK_CET_KERNEL)) {
> > + return false;
> > + }
>
> Same as above:
>
> if (index == MSR_IA32_U_CET)
> return supported_xss & XFEATURE_MASK_CET_USER;
>
> return supported_xss & XFEATURE_MASK_CET_KERNEL;
Got it!
> > +
> > + return true;
> > +}
> > /*
> > * Reads an msr value (of 'msr_index') into 'pdata'.
> > * Returns 0 on success, non-0 otherwise.
> > @@ -1941,6 +2026,26 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > else
> > msr_info->data = vmx->pt_desc.guest.addr_a[index / 2];
> > break;
> > + case MSR_IA32_S_CET:
> > + if (!cet_check_ctl_msr_access(vcpu, msr_info))
> > + return 1;
> > + msr_info->data = vmcs_readl(GUEST_S_CET);
> > + break;
> > + case MSR_IA32_INT_SSP_TAB:
> > + if (!cet_check_ssp_msr_access(vcpu, msr_info))
> > + return 1;
> > + msr_info->data = vmcs_readl(GUEST_INTR_SSP_TABLE);
> > + break;
> > + case MSR_IA32_U_CET:
> > + if (!cet_check_ctl_msr_access(vcpu, msr_info))
> > + return 1;
> > + vmx_get_xsave_msr(msr_info);
> > + break;
> > + case MSR_IA32_PL0_SSP ... MSR_IA32_PL3_SSP:
> > + if (!cet_check_ssp_msr_access(vcpu, msr_info))
> > + return 1;
> > + vmx_get_xsave_msr(msr_info);
> > + break;
> > case MSR_TSC_AUX:
> > if (!msr_info->host_initiated &&
> > !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))
> > @@ -2197,6 +2302,34 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > else
> > vmx->pt_desc.guest.addr_a[index / 2] = data;
> > break;
> > + case MSR_IA32_S_CET:
> > + if (!cet_check_ctl_msr_access(vcpu, msr_info))
> > + return 1;
> > + if (!cet_check_msr_write(vcpu, msr_info, CET_MSR_RSVD_BITS_2))
> > + return 1;
> > + vmcs_writel(GUEST_S_CET, data);
> > + break;
> > + case MSR_IA32_INT_SSP_TAB:
> > + if (!cet_check_ctl_msr_access(vcpu, msr_info))
> > + return 1;
> > + if (!is_64_bit_mode(vcpu))
>
> This is wrong, the SDM explicitly calls out the !64 case:
>
> IA32_INTERRUPT_SSP_TABLE_ADDR (64 bits; 32 bits on processors that do not
> support Intel 64 architecture).
So the check is also unnecessary as it's natual size?
>
> > + return 1;
> > + vmcs_writel(GUEST_INTR_SSP_TABLE, data);
> > + break;
> > + case MSR_IA32_U_CET:
> > + if (!cet_check_ctl_msr_access(vcpu, msr_info))
> > + return 1;
> > + if (!cet_check_msr_write(vcpu, msr_info, CET_MSR_RSVD_BITS_2))
> > + return 1;
> > + vmx_set_xsave_msr(msr_info);
> > + break;
> > + case MSR_IA32_PL0_SSP ... MSR_IA32_PL3_SSP:
> > + if (!cet_check_ssp_msr_access(vcpu, msr_info))
> > + return 1;
> > + if (!cet_check_msr_write(vcpu, msr_info, CET_MSR_RSVD_BITS_1))
> > + return 1;
> > + vmx_set_xsave_msr(msr_info);
> > + break;
> > case MSR_TSC_AUX:
> > if (!msr_info->host_initiated &&
> > !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 9654d779bdab..9e89ee6a09e1 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -1229,6 +1229,10 @@ static const u32 msrs_to_save_all[] = {
> > MSR_ARCH_PERFMON_EVENTSEL0 + 12, MSR_ARCH_PERFMON_EVENTSEL0 + 13,
> > MSR_ARCH_PERFMON_EVENTSEL0 + 14, MSR_ARCH_PERFMON_EVENTSEL0 + 15,
> > MSR_ARCH_PERFMON_EVENTSEL0 + 16, MSR_ARCH_PERFMON_EVENTSEL0 + 17,
> > +
> > + MSR_IA32_XSS, MSR_IA32_U_CET, MSR_IA32_S_CET,
> > + MSR_IA32_PL0_SSP, MSR_IA32_PL1_SSP, MSR_IA32_PL2_SSP,
> > + MSR_IA32_PL3_SSP, MSR_IA32_INT_SSP_TAB,
> > };
> >
> > static u32 msrs_to_save[ARRAY_SIZE(msrs_to_save_all)];
> > @@ -1504,6 +1508,13 @@ static int __kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data,
> > * invokes 64-bit SYSENTER.
> > */
> > data = get_canonical(data, vcpu_virt_addr_bits(vcpu));
> > + break;
> > + case MSR_IA32_PL0_SSP ... MSR_IA32_PL3_SSP:
> > + case MSR_IA32_U_CET:
> > + case MSR_IA32_S_CET:
> > + case MSR_IA32_INT_SSP_TAB:
> > + if (is_noncanonical_address(data, vcpu))
>
> IMO the canonical check belongs in cet_check_msr_write(). The above checks
> are for MSRs that are common to VMX and SVM, i.e. the common check saves
> having to duplicate the logic. If SVM picks up CET support, then they'll
> presumably want to share all of the checks, not just the canonical piece.
OK, I'll move them back.
>
> > + return 1;
> > }
> >
> > msr.data = data;
> > --
> > 2.17.2
> >
