Re: [PATCH v11 7/9] KVM: X86: Add userspace access interface for CET MSRs

[Yang Weijiang]

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
> >