Received-SPF: pass (google.com: domain of linux-kernel+bounces-177178-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <3b6bc6ac-276f-4a83-8972-68b98db672c7@intel.com>
Date: Mon, 13 May 2024 13:50:22 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 04/17] KVM: x86: Move synthetic PFERR_* sanity checks to
 SVM's #NPF handler
To: Paolo Bonzini <pbonzini@redhat.com>, linux-kernel@vger.kernel.org,
 kvm@vger.kernel.org
Cc: Sean Christopherson <seanjc@google.com>, Kai Huang <kai.huang@intel.com>,
 Binbin Wu <binbin.wu@linux.intel.com>
References: <20240507155817.3951344-1-pbonzini@redhat.com>
 <20240507155817.3951344-5-pbonzini@redhat.com>
Content-Language: en-US
From: Xiaoyao Li <xiaoyao.li@intel.com>
In-Reply-To: <20240507155817.3951344-5-pbonzini@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 5/7/2024 11:58 PM, Paolo Bonzini wrote:
> From: Sean Christopherson <seanjc@google.com>
> 
> Move the sanity check that hardware never sets bits that collide with KVM-
> define synthetic bits from kvm_mmu_page_fault() to npf_interception(),
> i.e. make the sanity check #NPF specific.  The legacy #PF path already
> WARNs if _any_ of bits 63:32 are set, and the error code that comes from
> VMX's EPT Violatation and Misconfig is 100% synthesized (KVM morphs VMX's
> EXIT_QUALIFICATION into error code flags).
> 
> Add a compile-time assert in the legacy #PF handler to make sure that KVM-
> define flags are covered by its existing sanity check on the upper bits.
> 
> Opportunistically add a description of PFERR_IMPLICIT_ACCESS, since we
> are removing the comment that defined it.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
> Message-ID: <20240228024147.41573-8-seanjc@google.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   arch/x86/include/asm/kvm_host.h |  6 ++++++
>   arch/x86/kvm/mmu/mmu.c          | 14 +++-----------
>   arch/x86/kvm/svm/svm.c          |  9 +++++++++
>   3 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 58bbcf76ad1e..12e727301262 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -267,7 +267,13 @@ enum x86_intercept_stage;
>   #define PFERR_GUEST_ENC_MASK	BIT_ULL(34)
>   #define PFERR_GUEST_SIZEM_MASK	BIT_ULL(35)
>   #define PFERR_GUEST_VMPL_MASK	BIT_ULL(36)
> +
> +/*
> + * IMPLICIT_ACCESS is a KVM-defined flag used to correctly perform SMAP checks
> + * when emulating instructions that triggers implicit access.
> + */
>   #define PFERR_IMPLICIT_ACCESS	BIT_ULL(48)
> +#define PFERR_SYNTHETIC_MASK	(PFERR_IMPLICIT_ACCESS)
>   
>   #define PFERR_NESTED_GUEST_PAGE (PFERR_GUEST_PAGE_MASK |	\
>   				 PFERR_WRITE_MASK |		\
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index c72a2033ca96..5562d693880a 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -4502,6 +4502,9 @@ int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code,
>   		return -EFAULT;
>   #endif
>   
> +	/* Ensure the above sanity check also covers KVM-defined flags. */

1. There is no sanity check above related to KVM-defined flags yet. It 
has to be after Patch 6.

2. I somehow cannot parse the comment properly, though I know it's to 
ensure KVM-defined PFERR_SYNTHETIC_MASK not contain any bit below 32-bits.

> +	BUILD_BUG_ON(lower_32_bits(PFERR_SYNTHETIC_MASK));
> +
>   	vcpu->arch.l1tf_flush_l1d = true;
>   	if (!flags) {
>   		trace_kvm_page_fault(vcpu, fault_address, error_code);
> @@ -5786,17 +5789,6 @@ int noinline kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 err
>   	int r, emulation_type = EMULTYPE_PF;
>   	bool direct = vcpu->arch.mmu->root_role.direct;
>   
> -	/*
> -	 * IMPLICIT_ACCESS is a KVM-defined flag used to correctly perform SMAP
> -	 * checks when emulating instructions that triggers implicit access.
> -	 * WARN if hardware generates a fault with an error code that collides
> -	 * with the KVM-defined value.  Clear the flag and continue on, i.e.
> -	 * don't terminate the VM, as KVM can't possibly be relying on a flag
> -	 * that KVM doesn't know about.
> -	 */
> -	if (WARN_ON_ONCE(error_code & PFERR_IMPLICIT_ACCESS))
> -		error_code &= ~PFERR_IMPLICIT_ACCESS;
> -
>   	if (WARN_ON_ONCE(!VALID_PAGE(vcpu->arch.mmu->root.hpa)))
>   		return RET_PF_RETRY;
>   
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 0f3b59da0d4a..535018f152a3 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2047,6 +2047,15 @@ static int npf_interception(struct kvm_vcpu *vcpu)
>   	u64 fault_address = svm->vmcb->control.exit_info_2;
>   	u64 error_code = svm->vmcb->control.exit_info_1;
>   
> +	/*
> +	 * WARN if hardware generates a fault with an error code that collides
> +	 * with KVM-defined sythentic flags.  Clear the flags and continue on,
> +	 * i.e. don't terminate the VM, as KVM can't possibly be relying on a
> +	 * flag that KVM doesn't know about.
> +	 */
> +	if (WARN_ON_ONCE(error_code & PFERR_SYNTHETIC_MASK))
> +		error_code &= ~PFERR_SYNTHETIC_MASK;
> +
>   	trace_kvm_page_fault(vcpu, fault_address, error_code);
>   	return kvm_mmu_page_fault(vcpu, fault_address, error_code,
>   			static_cpu_has(X86_FEATURE_DECODEASSISTS) ?