Message-ID: <ZJXS7tU6iukBXSuv@google.com>
Date:   Fri, 23 Jun 2023 10:18:09 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Isaku Yamahata <isaku.yamahata@...il.com>
Cc:     Kai Huang <kai.huang@...el.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Isaku Yamahata <isaku.yamahata@...el.com>,
        "michael.roth@....com" <michael.roth@....com>,
        Vishal Annapurve <vannapurve@...gle.com>,
        "ackerleytng@...gle.com" <ackerleytng@...gle.com>,
        Bo2 Chen <chen.bo@...el.com>, Sagi Shahar <sagis@...gle.com>,
        Erdem Aktas <erdemaktas@...gle.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "dmatlack@...gle.com" <dmatlack@...gle.com>,
        "zhi.wang.linux@...il.com" <zhi.wang.linux@...il.com>,
        "linux-coco@...ts.linux.dev" <linux-coco@...ts.linux.dev>,
        "chao.p.peng@...ux.intel.com" <chao.p.peng@...ux.intel.com>
Subject: Re: [RFC PATCH v2 3/6] KVM: x86/mmu: Pass round full 64-bit error
 code for the KVM page fault

On Thu, Jun 22, 2023, Isaku Yamahata wrote:
> On Thu, Jun 22, 2023 at 11:28:22PM +0000,
> Kai Huang <kai.huang@...el.com> wrote:
> 
> > On Thu, 2023-06-22 at 16:16 -0700, Yamahata, Isaku wrote:
> > > The upper 32 bits of error code are discarded at kvm_mmu_page_fault()
> > > by lower_32_bits().� Now it's passed down as full 64 bits.
> > > Because only FNAME(page_fault) depends on it, move lower_32_bits() into
> > > FNAME(page_fault).
> > 
> > I haven't looked into the code yet, but the last sentence around
> > FNAME(page_fault) doesn't make a lot sense IIUC?
> > 
> > For instance, we can have a shadow EPT table when EPT is enabled in L0 and
> > exposed to L1.  If we want to pass 64-bit error code to the handler, how can
> > FRAME(page_fault)() depend on the lower 32-bit value? 
> 
> Probably "depend" was too strong. In short, I wanted to not change the value
> passed down as error_code from FNAME(page_fault).
> 
> FNAME(page_fault) calls helper function to walk page tables. Some check
> PFERR_IMPLICIT_ACCESS_MASK(48 bit).

Argh, IMPLICIT_ACCESS is a KVM-defined value without any sanity checks against it
being used by hardware.

> If we don't mask lower_32_bits(), it can
> pass accidentally the bit.  Maybe we can audit the code carefully to check if
> IMPLICIT_ACCESS bit doesn't matter or fix it.  But I don't want to do it with
> this patch series.

No.  Going halfway on something like this is exactly how we end up with code that
no one understands and exists only because of "hysterical raisins".

There's no reason not to fix this properly.  It's obviously not your fault that
commit 147277540bbc ("kvm: svm: Add support for additional SVM NPF error codes")
took a shortcut, but that doesn't mean it's ok to keep punting the proper fix to
a future developer.

One of the things I've been trying to drill into people's heads by harping on
writing tests is that trying to do the least amount of work to get some newfangled
feature merged is not a winning strategy.  The more work that is done upfront by
developers, the less work that needs to be done by reviewers and maintainers.  It's
definitely not a 1:1 ratio, i.e. you doing an extra hour of work doesn't save me an
hour of work, but since reviewers and maintainers are a relatively scarce resource,
shifting work to developers almost always results in moving faster overall.  And
many times, doing more work upfront *does* require less effort overall, e.g. in
this case, we'll probably have spent more time discussing this than it would have
taken to audit the code in the first place.

As for IMPLICIT_ACCESS, that can and should be handled by asserting that the flag
isn't set by hardware, and if it is, suppressing the bit so that KVM doesn't
misinterpret an unknown hardware flag as IMPLICIT_ACCESS.

I'll test the below and post it separately, but feel free to incorporate it into
this series, I'll make sure everything gets ordered correctly if/when all of this
gets applied.

--
From: Sean Christopherson <seanjc@...gle.com>
Date: Fri, 23 Jun 2023 09:46:38 -0700
Subject: [PATCH] KVM: x86/mmu: Guard against collision with KVM-defined
 PFERR_IMPLICIT_ACCESS

Add an assertion in kvm_mmu_page_fault() to ensure the error code provided
by hardware doesn't conflict with KVM's software-defined IMPLICIT_ACCESS
flag.  In the unlikely scenario that future hardware starts using bit 48
for a hardware-defined flag, preserving the bit could result in KVM
incorrectly interpreting the unknown flag as KVM's IMPLICIT_ACCESS flag.

WARN so that any such conflict can be surfaced to KVM developers and
resolved, but otherwise ignore the bit as KVM can't possibly rely on a
flag it knows nothing about.

Fixes: 4f4aa80e3b88 ("KVM: X86: Handle implicit supervisor access with SMAP")
Signed-off-by: Sean Christopherson <seanjc@...gle.com>
---
 arch/x86/kvm/mmu/mmu.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 60397a1beda3..228a483d3746 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -5742,6 +5742,17 @@ 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(!VALID_PAGE(vcpu->arch.mmu->root.hpa)))
 		return RET_PF_RETRY;
 

base-commit: 293375bf2fd333e5563dd80b894725b90cd84c5d
--

Powered by blists - more mailing lists