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:   Tue, 12 Jul 2022 00:51:29 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Kai Huang <kai.huang@intel.com>
Cc:     Martin Fernandez <martin.fernandez@eclypsium.com>,
        linux-kernel@vger.kernel.org, bp@alien8.de,
        dave.hansen@linux.intel.com, x86@kernel.org, mingo@redhat.com,
        tglx@linutronix.de, kirill.shutemov@linux.intel.com,
        daniel.gutson@eclypsium.com, hughsient@gmail.com,
        alex.bazhaniuk@eclypsium.com
Subject: Re: [PATCH v2] x86/cpuinfo: Clear X86_FEATURE_TME if TME/MKTME is
 disabled by BIOS
Message-ID: <YszFkTZ7RtXS1rd7@google.com>
References: <20220704142250.1512726-1-martin.fernandez@eclypsium.com>
 <8d2a3175be8a3aff1d3fc535dd9ab6217cfe1e66.camel@intel.com>
 <CAKgze5azQG1mnOASbpcrs9jTejdXGkXYmezz9bTKuWQoZp5EFg@mail.gmail.com>
 <YsxZKGxVUY61zPEt@google.com>
 <ba321fad38d5f96a240f1e88a11943cea16bb4dd.camel@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ba321fad38d5f96a240f1e88a11943cea16bb4dd.camel@intel.com>
Precedence: bulk

On Tue, Jul 12, 2022, Kai Huang wrote:
> On Mon, 2022-07-11 at 17:08 +0000, Sean Christopherson wrote:
> > On Tue, Jul 05, 2022, Martin Fernandez wrote:
> > > On 7/5/22, Kai Huang <kai.huang@intel.com> wrote:
> > > > On Mon, 2022-07-04 at 11:22 -0300, Martin Fernandez wrote:
> > > > > Changelog since v1
> > > > > 
> > > > > Clear the flag not only for BSP but for every cpu in the system.
> > 
> > ...
> > 
> > > > > ---
> > > > >  arch/x86/kernel/cpu/intel.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > > 
> > > > > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> > > > > index fd5dead8371c..17f23e23f911 100644
> > > > > --- a/arch/x86/kernel/cpu/intel.c
> > > > > +++ b/arch/x86/kernel/cpu/intel.c
> > > > > @@ -570,6 +570,7 @@ static void detect_tme(struct cpuinfo_x86 *c)
> > > > > 
> > > > >  	if (!TME_ACTIVATE_LOCKED(tme_activate) ||
> > > > > !TME_ACTIVATE_ENABLED(tme_activate)) {
> > > > >  		pr_info_once("x86/tme: not enabled by BIOS\n");
> > > > > +		clear_cpu_cap(c, X86_FEATURE_TME);
> > 
> > This misses the case where the TME_ACTIVATE_KEYID_BITS() is zero.  AFAICT, that's
> > allowed, i.e. won't #GP on WRMSR.  TME_ACTIVATE_KEYID_BITS() can't be non-zero if
> > TME_ACTIVATE_ENABLED() is false, but the reverse is allowed.
> 
> But this logic applies to "whether MKTME is enabled",  but not "TME is enabled",
> right?

Ah, right, duh.

> > IMO, this entire function needs to be reworked to have a cohesive strategy for
> > enumerting TME; not just enumerating to userspace, but internal to the kernel as
> > well.
> > 
> > E.g. forcing "mktme_status = MKTME_DISABLED" on an AP is nonsensical.  If an AP's
> > basic MKTME enabling doesn't align with the BSP (activate, algorithm, and keyid0
> > bypass settings match), then there's no way an AP is going to reach detect_tme().
> > Any discrepancy in encryption for keyid0 will cause the AP will read garbage on
> > wakeup, and barring a miracle, will triple fault and never call in.
> > 
> > Conversely, if basic enabling matches but something else mismatches, e.g. an AP
> > was configured with fewer keys, then forcing "mktme_status = MKTME_DISABLED" may
> > be misleading as MKTME may be fully enabled and in use for keyid0, it just won't
> > be used for keyid!=0.  But that's a moot point because as is, the kernel _never_
> > uses keyid!=0.
> > 
> > And this code is also bogus.  Just because the kernel doesn't know the encryption
> > algorithm doesn't magically turn off encryption for keyid0.  Again, mktme_status
> > confuses "memory is encrypted" with "MKTME is theoretically usable for keyid!=0".
> > 
> > 	tme_crypto_algs = TME_ACTIVATE_CRYPTO_ALGS(tme_activate);
> > 	if (!(tme_crypto_algs & TME_ACTIVATE_CRYPTO_AES_XTS_128)) {
> > 		pr_err("x86/mktme: No known encryption algorithm is supported: %#llx\n",
> > 				tme_crypto_algs);
> > 		mktme_status = MKTME_DISABLED;
> > 	}
> > 
> > The mktme_status variable seems completely pointless.  It's not used anywhere
> > except to detect that CPU0 vs. APs.
> 
> I think your above saying makes sense, but this is a different topic and should
> be in a separate patch IMHO.

Yeah, definitely need multiple patches.

> This patch basically tries to fix the issue that TME flag isn't cleared when TME
> is disabled by BIOS.  And fir this purpose, the code change in this patch looks
> reasonable to me.  Unless I am mistaken, detect_tme() will be called for all
> cpus if TME is supported in CPUID but isn't enabled by BIOS (either LOCKED or
> ENABLED bit isn't set).

But this patch doesn't handle the bypass bit, which _does_ effectively disable
TME when set.  E.g. the MKTME spec says:

 Software must inspect the Hardware Encryption Enable (bit 1) and TME Encryption
 Bypass Enable (bit 31) to determine if TME encryption is enabled.