Re: [PATCH v2 6/6] x86: vmx: virtualize split lock detection

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 2/3/2020 11:16 PM, Xiaoyao Li wrote:
> Due to the fact that MSR_TEST_CTRL is per-core scope, i.e., the sibling
> threads in the same physical CPU core share the same MSR, only
> advertising feature split lock detection to guest when SMT is disabled
> or unsupported for simplicitly.
> 
> Only when host is sld_off, can guest control the hardware value of
> MSR_TEST_CTL, i.e., KVM loads guest's value into hardware when vcpu is
> running.
> 
> The vmx->disable_split_lock_detect can be set to true after unhandled
> split_lock #AC in guest only when host is sld_warn mode. It's for not
> burnning old guest, of course malicous guest can exploit it for DoS
> attack.
> 
> If want to prevent DoS attack from malicious guest, it must use sld_fatal
> mode in host. When host is sld_fatal, hardware value of
> MSR_TEST_CTL.SPLIT_LOCK_DETECT never cleared.
> 
> Below summarizing how guest behaves if SMT is off and it's a linux guest:
> 
> -----------------------------------------------------------------------
>     Host	| Guest | Guest behavior
> -----------------------------------------------------------------------
> 1. off	|	| same as in bare metal
> -----------------------------------------------------------------------
> 2. warn | off	| hardware bit set initially. Once split lock happens,
> 	|	| it sets vmx->disable_split_lock_detect, which leads
> 	|	| hardware bit to be cleared when vcpu is running
>          |	| So, it's the same as in bare metal
> 	---------------------------------------------------------------
> 3.	| warn	| - user space: get #AC when split lock, then clear
> 	|	|   MSR bit, but hardware bit is not cleared. #AC again,
> 	|	|   finally sets vmx->disable_split_lock_detect, which
> 	|	|   leads hardware bit to be cleared when vcpu is running;
> 	|	|   After the userspace process finishes, it sets vcpu's
> 	|	|   MSR_TEST_CTRL.SPLIT_LOCK_DETECT bit, which causes
> 	|	|   vmx->disable_split_lock_detect to be set false
>          |	|   So it's somehow the same as in bare-metal
>          |	| - kernel: same as in bare metal.
> 	--------------------------------------------------------------
> 4.	| fatal | same as in bare metal
> ----------------------------------------------------------------------
> 5. fatal| off   | #AC reported to userspace
> 	--------------------------------------------------------------
> 6.	| warn  | - user space: get #AC when split lock, then clear
> 	|	|   MSR bit, but hardware bit is not cleared, #AC again,
>          |	|   #AC reported to userspace
>          |	| - kernel: same as in bare metal, call die();
> 	-------------------------------------------------------------
> 7.    	| fatal | same as in bare metal
> ----------------------------------------------------------------------
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>   arch/x86/kvm/vmx/vmx.c | 72 +++++++++++++++++++++++++++++++++++-------
>   arch/x86/kvm/vmx/vmx.h |  1 +
>   arch/x86/kvm/x86.c     | 13 ++++++--
>   3 files changed, 73 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 93e3370c5f84..a0c3f579ecb6 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1781,6 +1781,26 @@ static int vmx_get_msr_feature(struct kvm_msr_entry *msr)
>   	}
>   }
>   
> +/*
> + * Note: for guest, feature split lock detection can only be enumerated by
> + * MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT. The FMS enumeration is invalid.
> + */
> +static inline bool guest_has_feature_split_lock_detect(struct kvm_vcpu *vcpu)
> +{
> +	return !!(vcpu->arch.core_capabilities &
> +		  MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT);
> +}
> +
> +static inline u64 vmx_msr_test_ctrl_valid_bits(struct kvm_vcpu *vcpu)
> +{
> +	u64 valid_bits = 0;
> +
> +	if (guest_has_feature_split_lock_detect(vcpu))
> +		valid_bits |= MSR_TEST_CTRL_SPLIT_LOCK_DETECT;
> +
> +	return valid_bits;
> +}
> +
>   /*
>    * Reads an msr value (of 'msr_index') into 'pdata'.
>    * Returns 0 on success, non-0 otherwise.
> @@ -1793,6 +1813,12 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>   	u32 index;
>   
>   	switch (msr_info->index) {
> +	case MSR_TEST_CTRL:
> +		if (!msr_info->host_initiated &&
> +		    !guest_has_feature_split_lock_detect(vcpu))
> +			return 1;
> +		msr_info->data = vmx->msr_test_ctrl;
> +		break;
>   #ifdef CONFIG_X86_64
>   	case MSR_FS_BASE:
>   		msr_info->data = vmcs_readl(GUEST_FS_BASE);
> @@ -1934,6 +1960,15 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>   	u32 index;
>   
>   	switch (msr_index) {
> +	case MSR_TEST_CTRL:
> +		if (!msr_info->host_initiated &&
> +		    (!guest_has_feature_split_lock_detect(vcpu) ||
> +		     data & ~vmx_msr_test_ctrl_valid_bits(vcpu)))
> +			return 1;
> +		if (data & MSR_TEST_CTRL_SPLIT_LOCK_DETECT)
> +			vmx->disable_split_lock_detect = false;
> +		vmx->msr_test_ctrl = data;
> +		break;
>   	case MSR_EFER:
>   		ret = kvm_set_msr_common(vcpu, msr_info);
>   		break;
> @@ -4233,6 +4268,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>   
>   	vmx->msr_ia32_umwait_control = 0;
>   
> +	vmx->msr_test_ctrl = 0;
>   	vmx->disable_split_lock_detect = false;
>   
>   	vcpu->arch.microcode_version = 0x100000000ULL;
> @@ -4565,6 +4601,11 @@ static inline bool guest_cpu_alignment_check_enabled(struct kvm_vcpu *vcpu)
>   	       (kvm_get_rflags(vcpu) & X86_EFLAGS_AC);
>   }
>   
> +static inline bool guest_cpu_split_lock_detect_enabled(struct vcpu_vmx *vmx)
> +{
> +	return !!(vmx->msr_test_ctrl & MSR_TEST_CTRL_SPLIT_LOCK_DETECT);
> +}
> +
>   static int handle_exception_nmi(struct kvm_vcpu *vcpu)
>   {
>   	struct vcpu_vmx *vmx = to_vmx(vcpu);
> @@ -4660,8 +4701,8 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
>   		break;
>   	case AC_VECTOR:
>   		/*
> -		 * Inject #AC back to guest only when legacy alignment check
> -		 * enabled.
> +		 * Inject #AC back to guest only when guest is expecting it,
> +		 * i.e., legacy alignment check or split lock #AC enabled.
>   		 * Otherwise, it must be an unexpected split-lock #AC for guest
>   		 * since KVM keeps hardware SPLIT_LOCK_DETECT bit unchanged
>   		 * when vcpu is running.
> @@ -4674,12 +4715,13 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
>   		 *    similar as sending SIGBUS.
>   		 */
>   		if (guest_cpu_alignment_check_enabled(vcpu) ||
> +		    guest_cpu_split_lock_detect_enabled(vmx) ||
>   		    WARN_ON(get_split_lock_detect_state() == sld_off)) {
>   			kvm_queue_exception_e(vcpu, AC_VECTOR, error_code);
>   			return 1;
>   		}
>   		if (get_split_lock_detect_state() == sld_warn) {
> -			pr_warn("kvm: split lock #AC happened in %s [%d]\n",
> +			pr_warn_ratelimited("kvm: split lock #AC happened in %s [%d]\n",
>   				current->comm, current->pid);
>   			vmx->disable_split_lock_detect = true;
>   			return 1;
> @@ -6491,6 +6533,7 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
>   {
>   	struct vcpu_vmx *vmx = to_vmx(vcpu);
>   	unsigned long cr3, cr4;
> +	bool host_sld_enabled, guest_sld_enabled;
>   
>   	/* Record the guest's net vcpu time for enforced NMI injections. */
>   	if (unlikely(!enable_vnmi &&
> @@ -6562,10 +6605,15 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
>   	 */
>   	x86_spec_ctrl_set_guest(vmx->spec_ctrl, 0);
>   
> -	if (static_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT) &&
> -	    unlikely(vmx->disable_split_lock_detect) &&
> -	    !test_tsk_thread_flag(current, TIF_SLD))
> -		split_lock_detect_set(false);
> +	if (static_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) {
> +		host_sld_enabled = get_split_lock_detect_state() &&
> +				   !test_tsk_thread_flag(current, TIF_SLD);
> +		guest_sld_enabled = guest_cpu_split_lock_detect_enabled(vmx);
> +		if (host_sld_enabled && unlikely(vmx->disable_split_lock_detect))
> +			split_lock_detect_set(false);
> +		else if (!host_sld_enabled && guest_sld_enabled)
> +			split_lock_detect_set(true);
> +	}
>   
>   	/* L1D Flush includes CPU buffer clear to mitigate MDS */
>   	if (static_branch_unlikely(&vmx_l1d_should_flush))
> @@ -6601,10 +6649,12 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
>   
>   	x86_spec_ctrl_restore_host(vmx->spec_ctrl, 0);
>   
> -	if (static_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT) &&
> -	    unlikely(vmx->disable_split_lock_detect) &&
> -	    !test_tsk_thread_flag(current, TIF_SLD))
> -		split_lock_detect_set(true);
> +	if (static_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) {
> +		if (host_sld_enabled && unlikely(vmx->disable_split_lock_detect))
> +			split_lock_detect_set(true);
> +		else if (!host_sld_enabled && guest_sld_enabled)
> +			split_lock_detect_set(false);
> +	}
>   
>   	/* All fields are clean at this point */
>   	if (static_branch_unlikely(&enable_evmcs))
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 912eba66c5d5..c36c663f4bae 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -222,6 +222,7 @@ struct vcpu_vmx {
>   #endif
>   
>   	u64		      spec_ctrl;
> +	u64		      msr_test_ctrl;
>   	u32		      msr_ia32_umwait_control;
>   
>   	u32 secondary_exec_control;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a97a8f5dd1df..56e799981d53 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1163,7 +1163,7 @@ static const u32 msrs_to_save_all[] = {
>   #endif
>   	MSR_IA32_TSC, MSR_IA32_CR_PAT, MSR_VM_HSAVE_PA,
>   	MSR_IA32_FEAT_CTL, MSR_IA32_BNDCFGS, MSR_TSC_AUX,
> -	MSR_IA32_SPEC_CTRL,
> +	MSR_IA32_SPEC_CTRL, MSR_TEST_CTRL,
>   	MSR_IA32_RTIT_CTL, MSR_IA32_RTIT_STATUS, MSR_IA32_RTIT_CR3_MATCH,
>   	MSR_IA32_RTIT_OUTPUT_BASE, MSR_IA32_RTIT_OUTPUT_MASK,
>   	MSR_IA32_RTIT_ADDR0_A, MSR_IA32_RTIT_ADDR0_B,
> @@ -1345,7 +1345,12 @@ static u64 kvm_get_arch_capabilities(void)
>   
>   static u64 kvm_get_core_capabilities(void)
>   {
> -	return 0;
> +	u64 data = 0;
> +
> +	if (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT) && !cpu_smt_possible())
> +		data |= MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT;
> +
> +	return data;
>   }
>   
>   static int kvm_get_msr_feature(struct kvm_msr_entry *msr)
> @@ -5259,6 +5264,10 @@ static void kvm_init_msr_list(void)
>   		 * to the guests in some cases.
>   		 */
>   		switch (msrs_to_save_all[i]) {
> +		case MSR_TEST_CTRL:
> +			if (!(kvm_get_core_capabilities() &
> +			      MSR_IA32_CORE_CAPS_SPLIT_LOCK_DETECT))
> +				continue;
			
sorry, forget	 	break;
>   		case MSR_IA32_BNDCFGS:
>   			if (!kvm_mpx_supported())
>   				continue;
>