Re: [PATCH v7 22/26] KVM: VMX: Set up interception for CET MSRs

[Chao Gao <chao.gao@intel.com>]

On Thu, Nov 30, 2023 at 07:44:45PM +0200, Maxim Levitsky wrote:
>On Fri, 2023-11-24 at 00:53 -0500, Yang Weijiang wrote:
>> Enable/disable CET MSRs interception per associated feature configuration.
>> Shadow Stack feature requires all CET MSRs passed through to guest to make
>> it supported in user and supervisor mode while IBT feature only depends on
>> MSR_IA32_{U,S}_CETS_CET to enable user and supervisor IBT.
>> 
>> Note, this MSR design introduced an architectural limitation of SHSTK and
>> IBT control for guest, i.e., when SHSTK is exposed, IBT is also available
>> to guest from architectual perspective since IBT relies on subset of SHSTK
>> relevant MSRs.
>> 
>> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
>> ---
>>  arch/x86/kvm/vmx/vmx.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 42 insertions(+)
>> 
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index 554f665e59c3..e484333eddb0 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -699,6 +699,10 @@ static bool is_valid_passthrough_msr(u32 msr)
>>  	case MSR_LBR_CORE_TO ... MSR_LBR_CORE_TO + 8:
>>  		/* LBR MSRs. These are handled in vmx_update_intercept_for_lbr_msrs() */
>>  		return true;
>> +	case MSR_IA32_U_CET:
>> +	case MSR_IA32_S_CET:
>> +	case MSR_IA32_PL0_SSP ... MSR_IA32_INT_SSP_TAB:
>> +		return true;
>>  	}
>>  
>>  	r = possible_passthrough_msr_slot(msr) != -ENOENT;
>> @@ -7766,6 +7770,42 @@ static void update_intel_pt_cfg(struct kvm_vcpu *vcpu)
>>  		vmx->pt_desc.ctl_bitmask &= ~(0xfULL << (32 + i * 4));
>>  }
>>  
>> +static void vmx_update_intercept_for_cet_msr(struct kvm_vcpu *vcpu)
>> +{
>> +	bool incpt;
>> +
>> +	if (kvm_cpu_cap_has(X86_FEATURE_SHSTK)) {
>> +		incpt = !guest_cpuid_has(vcpu, X86_FEATURE_SHSTK);
>> +
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_U_CET,
>> +					  MSR_TYPE_RW, incpt);
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_S_CET,
>> +					  MSR_TYPE_RW, incpt);
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL0_SSP,
>> +					  MSR_TYPE_RW, incpt);
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL1_SSP,
>> +					  MSR_TYPE_RW, incpt);
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL2_SSP,
>> +					  MSR_TYPE_RW, incpt);
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_PL3_SSP,
>> +					  MSR_TYPE_RW, incpt);
>> +		if (guest_cpuid_has(vcpu, X86_FEATURE_LM))
>> +			vmx_set_intercept_for_msr(vcpu, MSR_IA32_INT_SSP_TAB,
>> +						  MSR_TYPE_RW, incpt);
>> +		if (!incpt)
>> +			return;
>> +	}
>> +
>> +	if (kvm_cpu_cap_has(X86_FEATURE_IBT)) {
>> +		incpt = !guest_cpuid_has(vcpu, X86_FEATURE_IBT);
>> +
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_U_CET,
>> +					  MSR_TYPE_RW, incpt);
>> +		vmx_set_intercept_for_msr(vcpu, MSR_IA32_S_CET,
>> +					  MSR_TYPE_RW, incpt);
>> +	}
>> +}
>> +
>>  static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>>  {
>>  	struct vcpu_vmx *vmx = to_vmx(vcpu);
>> @@ -7843,6 +7883,8 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>>  
>>  	/* Refresh #PF interception to account for MAXPHYADDR changes. */
>>  	vmx_update_exception_bitmap(vcpu);
>> +
>> +	vmx_update_intercept_for_cet_msr(vcpu);
>>  }
>>  
>>  static u64 vmx_get_perf_capabilities(void)
>
>My review feedback from the previous patch still applies as well,
>
>I still think that we should either try a best effort approach to plug
>this virtualization hole, or we at least should fail guest creation
>if the virtualization hole is present as I said:
>

>"Another, much simpler option is to fail the guest creation if the shadow stack + indirect branch tracking
>state differs between host and the guest, unless both are disabled in the guest.
>(in essence don't let the guest be created if (2) or (3) happen)"

Enforcing a "none" or "all" policy is a temporary solution. in future, if some
reserved bits in S/U_CET MSRs are extended for new features, there will be:

	platform A supports SS + IBT
	platform B supports SS + IBT + new feature

Guests running on B inevitably have the same virtualization hole. and if kvm
continues enforcing the policy on B, then VM migration from A to B would be
impossible.

To me, intercepting S/U_CET MSR and CET_S/U xsave components is intricate and
yields marginal benefits. And I also doubt any reasonable OS implementation
would depend on #GP of WRMSR to S/U_CET MSRs for functionalities. So, I vote
to leave the patch as-is.

>
>Please at least tell me what do you think about this.

>
>Best regards,
>	Maxim Levitsky
>
>
>