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;
Message-ID: <8c628549-a388-afd5-3c6e-a956fbce7f79@linux.intel.com>
Date:   Mon, 7 Aug 2023 15:03:33 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.14.0
Subject: Re: [PATCH v2 6/8] KVM: VMX: Implement and apply
 vmx_is_lass_violation() for LASS protection
To:     Zeng Guang <guang.zeng@intel.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <seanjc@google.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        H Peter Anvin <hpa@zytor.com>, kvm@vger.kernel.org,
        x86@kernel.org, linux-kernel@vger.kernel.org
References: <20230719024558.8539-1-guang.zeng@intel.com>
 <20230719024558.8539-7-guang.zeng@intel.com>
From:   Binbin Wu <binbin.wu@linux.intel.com>
In-Reply-To: <20230719024558.8539-7-guang.zeng@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 7/19/2023 10:45 AM, Zeng Guang wrote:
> Implement and wire up vmx_is_lass_violation() in kvm_x86_ops for VMX.
>
> LASS violation check takes effect in KVM emulation of instruction fetch
> and data access including implicit access when vCPU is running in long
> mode, and also involved in emulation of VMX instruction and SGX ENCLS
> instruction to enforce the mode-based protections before paging.
>
> But the target memory address of emulation of TLB invalidation and branch
> instructions aren't subject to LASS as exceptions.
>
> Signed-off-by: Zeng Guang <guang.zeng@intel.com>
> Tested-by: Xuelian Guo <xuelian.guo@intel.com>
> ---
>   arch/x86/kvm/vmx/nested.c |  3 ++-
>   arch/x86/kvm/vmx/sgx.c    |  4 ++++
>   arch/x86/kvm/vmx/vmx.c    | 35 +++++++++++++++++++++++++++++++++++
>   arch/x86/kvm/vmx/vmx.h    |  3 +++
>   4 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index e35cf0bd0df9..72e78566a3b6 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -4985,7 +4985,8 @@ int get_vmx_mem_address(struct kvm_vcpu *vcpu, unsigned long exit_qualification,
>   		 * non-canonical form. This is the only check on the memory
>   		 * destination for long mode!
>   		 */
> -		exn = is_noncanonical_address(*ret, vcpu);
> +		exn = is_noncanonical_address(*ret, vcpu) ||
> +		      vmx_is_lass_violation(vcpu, *ret, len, 0);
>   	} else {
>   		/*
>   		 * When not in long mode, the virtual/linear address is
> diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c
> index 2261b684a7d4..f8de637ce634 100644
> --- a/arch/x86/kvm/vmx/sgx.c
> +++ b/arch/x86/kvm/vmx/sgx.c
> @@ -46,6 +46,10 @@ static int sgx_get_encls_gva(struct kvm_vcpu *vcpu, unsigned long offset,
>   			((s.base != 0 || s.limit != 0xffffffff) &&
>   			(((u64)*gva + size - 1) > s.limit + 1));
>   	}
> +
> +	if (!fault)
> +		fault = vmx_is_lass_violation(vcpu, *gva, size, 0);
> +
>   	if (fault)
>   		kvm_inject_gp(vcpu, 0);
>   	return fault ? -EINVAL : 0;
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 44fb619803b8..15a7c6e7a25d 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -8127,6 +8127,40 @@ static void vmx_vm_destroy(struct kvm *kvm)
>   	free_pages((unsigned long)kvm_vmx->pid_table, vmx_get_pid_table_order(kvm));
>   }
>   
> +bool vmx_is_lass_violation(struct kvm_vcpu *vcpu, unsigned long addr,
> +			   unsigned int size, unsigned int flags)
> +{
> +	const bool is_supervisor_address = !!(addr & BIT_ULL(63));
> +	const bool implicit = !!(flags & X86EMUL_F_IMPLICIT);
> +	const bool fetch = !!(flags & X86EMUL_F_FETCH);
> +	const bool is_wraparound_access = size ? (addr + size - 1) < addr : false;
> +
> +	if (!kvm_is_cr4_bit_set(vcpu, X86_CR4_LASS) || !is_long_mode(vcpu))
> +		return false;
> +
> +	/*
> +	 * INVTLB isn't subject to LASS, e.g. to allow invalidating userspace
> +	 * addresses without toggling RFLAGS.AC.  Branch targets aren't subject
> +	 * to LASS in order to simplifiy far control transfers (the subsequent
s/simplifiy/simplifiy

> +	 * fetch will enforce LASS as appropriate).
> +	 */
> +	if (flags & (X86EMUL_F_BRANCH | X86EMUL_F_INVTLB))
> +		return false;
> +
> +	if (!implicit && vmx_get_cpl(vcpu) == 3)
> +		return is_supervisor_address;
> +
> +	/* LASS is enforced for supervisor-mode access iff SMAP is enabled. */
To be more accurate, supervisor-mode data access.
Also, "iff" here is not is a typo for "if" or it stands for "if and only 
if"?
It is not accureate to use "if and only if" here because beside SMAP, 
there are other conditions, i.e. implicit or RFLAGS.AC.

> +	if (!fetch && !kvm_is_cr4_bit_set(vcpu, X86_CR4_SMAP))
> +		return false;
> +
> +	/* Like SMAP, RFLAGS.AC disables LASS checks in supervisor mode. */
> +	if (!fetch && !implicit && (kvm_get_rflags(vcpu) & X86_EFLAGS_AC))
> +		return false;
> +
> +	return is_wraparound_access ? true : !is_supervisor_address;
> +}
> +
>   static struct kvm_x86_ops vmx_x86_ops __initdata = {
>   	.name = KBUILD_MODNAME,
>   
> @@ -8266,6 +8300,7 @@ static struct kvm_x86_ops vmx_x86_ops __initdata = {
>   	.complete_emulated_msr = kvm_complete_insn_gp,
>   
>   	.vcpu_deliver_sipi_vector = kvm_vcpu_deliver_sipi_vector,
> +	.is_lass_violation = vmx_is_lass_violation,
>   };
>   
>   static unsigned int vmx_handle_intel_pt_intr(void)
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 9e66531861cf..c1e541a790bb 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -433,6 +433,9 @@ void vmx_enable_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr, int type);
>   u64 vmx_get_l2_tsc_offset(struct kvm_vcpu *vcpu);
>   u64 vmx_get_l2_tsc_multiplier(struct kvm_vcpu *vcpu);
>   
> +bool vmx_is_lass_violation(struct kvm_vcpu *vcpu, unsigned long addr,
> +			   unsigned int size, unsigned int flags);
> +
>   static inline void vmx_set_intercept_for_msr(struct kvm_vcpu *vcpu, u32 msr,
>   					     int type, bool value)
>   {