Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 16 Jun 2023 10:56:42 -0700
In-Reply-To: <147246fc-79a2-3bb5-f51f-93dfc1cffcc0@intel.com>
Mime-Version: 1.0
References: <20230511040857.6094-1-weijiang.yang@intel.com>
 <ZIufL7p/ZvxjXwK5@google.com> <147246fc-79a2-3bb5-f51f-93dfc1cffcc0@intel.com>
Message-ID: <ZIyiWr4sR+MqwmAo@google.com>
Subject: Re: [PATCH v3 00/21] Enable CET Virtualization
From:   Sean Christopherson <seanjc@google.com>
To:     Weijiang Yang <weijiang.yang@intel.com>
Cc:     pbonzini@redhat.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org, peterz@infradead.org,
        rppt@kernel.org, binbin.wu@linux.intel.com,
        rick.p.edgecombe@intel.com, john.allen@amd.com
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Jun 16, 2023, Weijiang Yang wrote:
>=20
> On 6/16/2023 7:30 AM, Sean Christopherson wrote:
> > On Thu, May 11, 2023, Yang Weijiang wrote:
> > > The last patch is introduced to support supervisor SHSTK but the feat=
ure is
> > > not enabled on Intel platform for now, the main purpose of this patch=
 is to
> > > facilitate AMD folks to enable the feature.
> > I am beyond confused by the SDM's wording of CET_SSS.
> >=20
> > First, it says that CET_SSS says the CPU isn't buggy (or maybe "less bu=
ggy" is
> > more appropriate phrasing).
> >=20
> >    Bit 18: CET_SSS. If 1, indicates that an operating system can enable=
 supervisor
> >    shadow stacks as long as it ensures that certain supervisor shadow-s=
tack pushes
> >    will not cause page faults (see Section 17.2.3 of the Intel=C2=AE 64=
 and IA-32
> >    Architectures Software Developer=E2=80=99s Manual, Volume 1).
> >=20
> > But then it says says VMMs shouldn't set the bit.
> >=20
> >    When emulating the CPUID instruction, a virtual-machine monitor shou=
ld return
> >    this bit as 0 if those pushes can cause VM exits.
> >=20
> > Based on the Xen code (which is sadly a far better source of informatio=
n than the
> > SDM), I *think* that what the SDM is trying to say is that VMMs should =
not set
> > CET_SS if VM-Exits can occur ***and*** the bit is not set in the host C=
PU.  Because
> > if the SDM really means "VMMs should never set the bit", then what on e=
arth is the
> > point of the bit.
>=20
> I need to double check for the vague description.
>=20
> From my understanding, on bare metal side, if the bit is 1, OS can enable
> SSS if pushes won't cause page fault. But for VM case, it's not recommend=
ed
> (regardless of the bit state) to set the bit as vm-exits caused by guest =
SSS
> pushes cannot be fully excluded.
>=20
> In other word, the bit is mainly for bare metal guidance now.
>=20
> > > In summary, this new series enables CET user SHSTK/IBT and kernel IBT=
, but
> > > doesn't fully support CET supervisor SHSTK, the enabling work is left=
 for
> > > the future.
> > Why?  If my interpretation of the SDM is correct, then all the pieces a=
re there.

...

> And also based on above SDM description, I don't want to add the support
> blindly now.

*sigh*

I got filled in on the details offlist.

1) In the next version of this series, please rework it to reincorporate Su=
pervisor
   Shadow Stack support into the main series, i.e. pretend Intel's implemen=
ation
   isn't horribly flawed.  KVM can't guarantee that a VM-Exit won't occur, =
i.e.
   can't advertise CET_SS, but I want the baseline support to be implemente=
d,
   otherwise the series as a whole is a big confusing mess with unanswered =
question
   left, right, and center.  And more importantly, architecturally SSS exis=
ts if
   X86_FEATURE_SHSTK is enumerated, i.e. the guest should be allowed to uti=
lize
   SSS if it so chooses, with the obvious caveat that there's a non-zero ch=
ance
   the guest risks death by doing so.  Or if userspace can ensure no VM-Exi=
t will
   occur, which is difficult but feasible (ignoring #MC), e.g. by staticall=
y
   partitioning memory, prefaulting all memory in guest firmware, and not d=
irty
   logging SSS pages.  In such an extreme setup, userspace can enumerate CE=
T_SSS
   to the guest, and KVM should support that.
=20
2) Add the below patch to document exactly why KVM doesn't advertise CET_SS=
S.
   While Intel is apparently ok with treating KVM developers like mushrooms=
, I
   am not.

---
From: Sean Christopherson <seanjc@google.com>
Date: Fri, 16 Jun 2023 10:04:37 -0700
Subject: [PATCH] KVM: x86: Explicitly document that KVM must not advertise
 CET_SSS

Explicitly call out that KVM must NOT advertise CET_SSS to userspace,
i.e. must not tell userspace and thus the guest that it is safe for the
guest to enable Supervisor Shadow Stacks (SSS).

Intel's implementation of SSS is fatally flawed for virtualized
environments, as despite wording in the SDM that suggests otherwise,
Intel CPUs' handling of shadow stack switches are NOT fully atomic.  Only
the check-and-update of the supervisor shadow stack token's busy bit is
atomic.  Per the SDM:

  If the far CALL or event delivery pushes a stack frame after the token
  is acquired and any of the pushes causes a fault or VM exit, the
  processor will revert to the old shadow stack and the busy bit in the
  new shadow stack's token remains set.

Or more bluntly, any fault or VM-Exit that occurs when pushing to the
shadow stack after the busy bit is set is fatal to the kernel, i.e. to
the guest in KVM's case.  The (guest) kernel can protect itself against
faults, e.g. by ensuring that the shadow stack always has a valid mapping,
but a guest kernel obviously has no control over, or even knowledge of,
VM-Exits due to host activity.

To help software determine when it is safe to use SSS, Intel defined
CPUID.0x7.1.EDX bit (CET_SSS) and updated Intel CPUs to enumerate CET_SS,
i.e. bare metal Intel CPUs advertise to software that it is safe to enable
SSS.

  If CPUID.(EAX=3D07H,ECX=3D1H):EDX[bit 18] is enumerated as 1, it is
  sufficient for an operating system to ensure that none of the pushes can
  cause a page fault.

But CET_SS also comes with an major caveat that is kinda sorta documented
in the SDM:

  When emulating the CPUID instruction, a virtual-machine monitor should
  return this bit as 0 if those pushes can cause VM exits.

In other words, CET_SSS (bit 18) does NOT enumerate that the underlying
CPU prevents VM-Exits, only that the environment in which the software is
running will not generate VM-Exits.  I.e. CET_SSS is a stopgap to stem the
bleeding and allow kernels to enable SSS, not an indication that the
underlying CPU is immune to the VM-Exit problem.

And unfortunately, KVM itself effectively has zero chance of ensuring that
a shadow stack switch can't trigger a VM-Exit, e.g. KVM zaps *all* SPTEs
when any memslot is deleted, enabling dirty logging write-protects SPTEs,
etc.  A sufficiently motivated userspace can, at least in theory, provide
a safe environment for SSS, e.g. by statically partitioning and
prefaulting (in guest firmware) all memory, disabling PML, never
write-protecting guest shadow stacks, etc.  But such a setup is far, far
beyond typical KVM deployments.

Note, AMD CPUs have a similar erratum, but AMD CPUs *DO* perform the full
shadow stack switch atomically so long as the stack is mapped WB and does
not cross a page boundary, i.e. a "normal" KVM setup and a well-behaved
guest play nice with SSS without additional shenanigans.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/cpuid.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 1e3ee96c879b..ecf4a68aaa08 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -658,7 +658,15 @@ void kvm_set_cpu_caps(void)
 	);
=20
 	kvm_cpu_cap_init_kvm_defined(CPUID_7_1_EDX,
-		F(AVX_VNNI_INT8) | F(AVX_NE_CONVERT) | F(PREFETCHITI)
+		F(AVX_VNNI_INT8) | F(AVX_NE_CONVERT) | F(PREFETCHITI) |
+
+		/*
+		 * Do NOT advertise CET_SSS, i.e. do not tell userspace and the
+		 * guest that it is safe to use Supervisor Shadow Stacks under
+		 * KVM when running on Intel CPUs.  KVM itself cannot guarantee
+		 * that a VM-Exit won't occur during a shadow stack update.
+		 */
+		0 /* F(CET_SSS) */
 	);
=20
 	kvm_cpu_cap_mask(CPUID_D_1_EAX,

base-commit: 9305c14847719870e9e08294034861360577ce08
--=20