Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 26 May 2022 07:27:44 +0800
From:   Yuan Yao <yuan.yao@linux.intel.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Chenyi Qiang <chenyi.qiang@intel.com>,
        Lei Wang <lei4.wang@intel.com>
Subject: Re: [PATCH 1/2] KVM: VMX: Sanitize VM-Entry/VM-Exit control pairs at
 kvm_intel load time
Message-ID: <20220525232744.e6g77merw7pita3s@yy-desk-7060>
References: <20220525210447.2758436-1-seanjc@google.com>
 <20220525210447.2758436-2-seanjc@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220525210447.2758436-2-seanjc@google.com>
User-Agent: NeoMutt/20171215
Precedence: bulk

On Wed, May 25, 2022 at 09:04:46PM +0000, Sean Christopherson wrote:
> Sanitize the VM-Entry/VM-Exit control pairs (load+load or load+clear)
> during setup instead of checking both controls in a pair at runtime.  If
> only one control is supported, KVM will report the associated feature as
> not available, but will leave the supported control bit set in the VMCS
> config, which could lead to corruption of host state.  E.g. if only the
> VM-Entry control is supported and the feature is not dynamically toggled,
> KVM will set the control in all VMCSes and load zeros without restoring
> host state.
>
> Note, while this is technically a bug fix, practically speaking no sane
> CPU or VMM would support only one control.  KVM's behavior of checking
> both controls is mostly pedantry.
>
> Cc: Chenyi Qiang <chenyi.qiang@intel.com>
> Cc: Lei Wang <lei4.wang@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/capabilities.h | 13 ++++--------
>  arch/x86/kvm/vmx/vmx.c          | 35 +++++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
> index dc2cb8a16e76..464bf39e4835 100644
> --- a/arch/x86/kvm/vmx/capabilities.h
> +++ b/arch/x86/kvm/vmx/capabilities.h
> @@ -97,20 +97,17 @@ static inline bool cpu_has_vmx_posted_intr(void)
>
>  static inline bool cpu_has_load_ia32_efer(void)
>  {
> -	return (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_EFER) &&
> -	       (vmcs_config.vmexit_ctrl & VM_EXIT_LOAD_IA32_EFER);
> +	return vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_EFER;
>  }
>
>  static inline bool cpu_has_load_perf_global_ctrl(void)
>  {
> -	return (vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL) &&
> -	       (vmcs_config.vmexit_ctrl & VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL);
> +	return vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
>  }
>
>  static inline bool cpu_has_vmx_mpx(void)
>  {
> -	return (vmcs_config.vmexit_ctrl & VM_EXIT_CLEAR_BNDCFGS) &&
> -		(vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_BNDCFGS);
> +	return vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_BNDCFGS;
>  }
>
>  static inline bool cpu_has_vmx_tpr_shadow(void)
> @@ -377,7 +374,6 @@ static inline bool cpu_has_vmx_intel_pt(void)
>  	rdmsrl(MSR_IA32_VMX_MISC, vmx_msr);
>  	return (vmx_msr & MSR_IA32_VMX_MISC_INTEL_PT) &&
>  		(vmcs_config.cpu_based_2nd_exec_ctrl & SECONDARY_EXEC_PT_USE_GPA) &&
> -		(vmcs_config.vmexit_ctrl & VM_EXIT_CLEAR_IA32_RTIT_CTL) &&
>  		(vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_RTIT_CTL);
>  }
>
> @@ -406,8 +402,7 @@ static inline bool vmx_pebs_supported(void)
>
>  static inline bool cpu_has_vmx_arch_lbr(void)
>  {
> -	return (vmcs_config.vmexit_ctrl & VM_EXIT_CLEAR_IA32_LBR_CTL) &&
> -		(vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_LBR_CTL);
> +	return vmcs_config.vmentry_ctrl & VM_ENTRY_LOAD_IA32_LBR_CTL;
>  }
>
>  static inline u64 vmx_get_perf_capabilities(void)
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 6927f6e8ec31..2ea256de9aba 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2462,6 +2462,9 @@ static __init u64 adjust_vmx_controls64(u64 ctl_opt, u32 msr)
>  	return  ctl_opt & allowed;
>  }
>
> +#define VMCS_ENTRY_EXIT_PAIR(name, entry_action, exit_action) \
> +	{ VM_ENTRY_##entry_action##_##name, VM_EXIT_##exit_action##_##name }
> +
>  static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
>  				    struct vmx_capability *vmx_cap)
>  {
> @@ -2473,6 +2476,24 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
>  	u64 _cpu_based_3rd_exec_control = 0;
>  	u32 _vmexit_control = 0;
>  	u32 _vmentry_control = 0;
> +	int i;
> +
> +	/*
> +	 * LOAD/SAVE_DEBUG_CONTROLS are absent because both are mandatory.
> +	 * SAVE_IA32_PAT and SAVE_IA32_EFER are absent because KVM always
> +	 * intercepts writes to PAT and EFER, i.e. never enables those controls.
> +	 */
> +	struct {
> +		u32 entry_control;
> +		u32 exit_control;
> +	} vmcs_entry_exit_pairs[] = {
> +		VMCS_ENTRY_EXIT_PAIR(IA32_PERF_GLOBAL_CTRL, LOAD, LOAD),
> +		VMCS_ENTRY_EXIT_PAIR(IA32_PAT, LOAD, LOAD),
> +		VMCS_ENTRY_EXIT_PAIR(IA32_EFER, LOAD, LOAD),
> +		VMCS_ENTRY_EXIT_PAIR(BNDCFGS, LOAD, CLEAR),
> +		VMCS_ENTRY_EXIT_PAIR(IA32_RTIT_CTL, LOAD, CLEAR),
> +		VMCS_ENTRY_EXIT_PAIR(IA32_LBR_CTL, LOAD, CLEAR),
> +	};
>
>  	memset(vmcs_conf, 0, sizeof(*vmcs_conf));
>  	min = CPU_BASED_HLT_EXITING |
> @@ -2614,6 +2635,20 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
>  				&_vmentry_control) < 0)
>  		return -EIO;
>
> +	for (i = 0; i < ARRAY_SIZE(vmcs_entry_exit_pairs); i++) {
> +		u32 n_ctrl = vmcs_entry_exit_pairs[i].entry_control;
> +		u32 x_ctrl = vmcs_entry_exit_pairs[i].exit_control;
> +
> +		if (!(_vmentry_control & n_ctrl) == !(_vmexit_control & x_ctrl))
> +			continue;
> +
> +		pr_warn_once("Inconsistent VM-Entry/VM-Exit pair, entry = %x, exit = %x\n",
> +			     _vmentry_control & n_ctrl, _vmexit_control & x_ctrl);

How about "n_ctrl, x_ctrl);" ? In 0/1 or 1/0 case this
outputs all information of real inconsistent bits but not 0.

> +
> +		_vmentry_control &= ~n_ctrl;
> +		_vmexit_control &= ~x_ctrl;
> +	}
> +
>  	/*
>  	 * Some cpus support VM_{ENTRY,EXIT}_IA32_PERF_GLOBAL_CTRL but they
>  	 * can't be used due to an errata where VM Exit may incorrectly clear
> --
> 2.36.1.124.g0e6072fb45-goog
>