Messages in this thread | | | Date | Tue, 12 Jul 2022 00:51:29 +0000 | From | Sean Christopherson <> | Subject | Re: [PATCH v2] x86/cpuinfo: Clear X86_FEATURE_TME if TME/MKTME is disabled by BIOS |
| |
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.
| |