Received-SPF: pass (google.com: domain of linux-kernel+bounces-178302-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <55d00dc8-bfa3-4cf2-9c6a-1d81e5cfd7b3@intel.com>
Date: Tue, 14 May 2024 12:25:51 +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: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, linux-kernel@vger.kernel.org,
 kvm@vger.kernel.org, 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>
 <3b6bc6ac-276f-4a83-8972-68b98db672c7@intel.com>
 <ZkJOb4zJJnOAYnTi@google.com>
Content-Language: en-US
From: Xiaoyao Li <xiaoyao.li@intel.com>
In-Reply-To: <ZkJOb4zJJnOAYnTi@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 5/14/2024 1:31 AM, Sean Christopherson wrote:
> On Mon, May 13, 2024, Xiaoyao Li wrote:
>> 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.
> 
> Ya, it's not just the comment, the entire changelog expects this patch to land
> 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.
> 
> Hmm, how about this?
> 
> 	/*
> 	 * Ensure that the above sanity check on hardware error code bits 63:32
> 	 * also prevents false positives on KVM-defined flags.
> 	 */
> 

Maybe it's just myself inability, I still cannot interpret it well.

Can't we put it above the sanity check of error code, and just with a 
comment like

	/*
  	 * Ensure KVM-defined flags not occupied any bits below 32-bits,
          * that are used by hardware.
	 * /