linux-kernel - Re: [PATCH 04/17] KVM: x86: Move synthetic PFERR

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]

Date: Tue, 14 May 2024 12:25:51 +0800
From: Xiaoyao Li <xiaoyao.li@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, linux-kernel@...r.kernel.org,
 kvm@...r.kernel.org, Kai Huang <kai.huang@...el.com>,
 Binbin Wu <binbin.wu@...ux.intel.com>
Subject: Re: [PATCH 04/17] KVM: x86: Move synthetic PFERR_* sanity checks to
 SVM's #NPF handler

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@...gle.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@...gle.com>
>>> Reviewed-by: Kai Huang <kai.huang@...el.com>
>>> Reviewed-by: Binbin Wu <binbin.wu@...ux.intel.com>
>>> Message-ID: <20240228024147.41573-8-seanjc@...gle.com>
>>> Signed-off-by: Paolo Bonzini <pbonzini@...hat.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.
	 * /