Received-SPF: pass (google.com: domain of linux-kernel+bounces-177906-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 13 May 2024 10:31:27 -0700
In-Reply-To: <3b6bc6ac-276f-4a83-8972-68b98db672c7@intel.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240507155817.3951344-1-pbonzini@redhat.com> <20240507155817.3951344-5-pbonzini@redhat.com>
 <3b6bc6ac-276f-4a83-8972-68b98db672c7@intel.com>
Message-ID: <ZkJOb4zJJnOAYnTi@google.com>
Subject: Re: [PATCH 04/17] KVM: x86: Move synthetic PFERR_* sanity checks to
 SVM's #NPF handler
From: Sean Christopherson <seanjc@google.com>
To: Xiaoyao Li <xiaoyao.li@intel.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>
Content-Type: text/plain; charset="us-ascii"

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.
	 */