linux-kernel - Re: [RFC PATCH 06/18] KVM: VMX: Wire up Intel MBEC enable/disable logic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aCI8pGJbn3l99kq8@google.com>
Date: Mon, 12 May 2025 11:23:32 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Jon Kohler <jon@...anix.com>
Cc: pbonzini@...hat.com, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, 
	dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com, 
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 06/18] KVM: VMX: Wire up Intel MBEC enable/disable logic

On Thu, Mar 13, 2025, Jon Kohler wrote:
> Add logic to enable / disable Intel Mode Based Execution Control (MBEC)
> based on specific conditions.
> 
> MBEC depends on:
> - User space exposing secondary execution control bit 22
> - Extended Page Tables (EPT)
> - The KVM module parameter `enable_pt_guest_exec_control`
> 
> If any of these conditions are not met, MBEC will be disabled
> accordingly.

Why?  I know why, but I know why despite the changeloge, not because of the
changelog.

> Store runtime enablement within `kvm_vcpu_arch.pt_guest_exec_control`.

Again, why?  If you actually tried to explain this, I think/hope you would realize
why it's wrong.

> Signed-off-by: Jon Kohler <jon@...anix.com>
> 
> ---
>  arch/x86/kvm/vmx/vmx.c | 11 +++++++++++
>  arch/x86/kvm/vmx/vmx.h |  7 +++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 7a98f03ef146..116910159a3f 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2694,6 +2694,7 @@ static int setup_vmcs_config(struct vmcs_config *vmcs_conf,
>  			return -EIO;
>  
>  		vmx_cap->ept = 0;
> +		_cpu_based_2nd_exec_control &= ~SECONDARY_EXEC_MODE_BASED_EPT_EXEC;
>  		_cpu_based_2nd_exec_control &= ~SECONDARY_EXEC_EPT_VIOLATION_VE;
>  	}
>  	if (!(_cpu_based_2nd_exec_control & SECONDARY_EXEC_ENABLE_VPID) &&
> @@ -4641,11 +4642,15 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
>  		exec_control &= ~SECONDARY_EXEC_ENABLE_VPID;
>  	if (!enable_ept) {
>  		exec_control &= ~SECONDARY_EXEC_ENABLE_EPT;
> +		exec_control &= ~SECONDARY_EXEC_MODE_BASED_EPT_EXEC;
>  		exec_control &= ~SECONDARY_EXEC_EPT_VIOLATION_VE;
>  		enable_unrestricted_guest = 0;
>  	}
>  	if (!enable_unrestricted_guest)
>  		exec_control &= ~SECONDARY_EXEC_UNRESTRICTED_GUEST;
> +	if (!enable_pt_guest_exec_control)
> +		exec_control &= ~SECONDARY_EXEC_MODE_BASED_EPT_EXEC;

This is wrong and unnecessary.  As mentioned early, the input that matters is
vmcs12.  This flag should *never* be set for vmcs01.

>  	if (kvm_pause_in_guest(vmx->vcpu.kvm))
>  		exec_control &= ~SECONDARY_EXEC_PAUSE_LOOP_EXITING;
>  	if (!kvm_vcpu_apicv_active(vcpu))
> @@ -4770,6 +4775,9 @@ static void init_vmcs(struct vcpu_vmx *vmx)
>  		if (vmx->ve_info)
>  			vmcs_write64(VE_INFORMATION_ADDRESS,
>  				     __pa(vmx->ve_info));
> +
> +		vmx->vcpu.arch.pt_guest_exec_control =
> +			enable_pt_guest_exec_control && vmx_has_mbec(vmx);

This should effectively be dead code, because vmx_has_mbec() should never be
true at vCPU creation.