| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <25da8fb143b361740660bccff3973e04f1506b39.camel@intel.com>
Date: Mon, 29 Apr 2024 11:17:12 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "tglx@...utronix.de" <tglx@...utronix.de>, "Schofield, Alison"
<alison.schofield@...el.com>, "mingo@...hat.com" <mingo@...hat.com>,
"x86@...nel.org" <x86@...nel.org>, "bp@...en8.de" <bp@...en8.de>,
"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>
CC: "hpa@...or.com" <hpa@...or.com>, "kirill.shutemov@...ux.intel.com"
<kirill.shutemov@...ux.intel.com>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "kai.huang@...ux.intel.com"
<kai.huang@...ux.intel.com>
Subject: Re: [PATCH 1/2] x86/cpu: Remove useless work in detect_tme_early()
On Thu, 2024-04-25 at 21:24 -0700, alison.schofield@...el.com wrote:
> From: Alison Schofield <alison.schofield@...el.com>
>
> TME (Total Memory Encryption) and MKTME (Multi-Key Total Memory
> Encryption) BIOS detection were introduced together here [1] and
> are loosely coupled in the Intel CPU init code.
>
> TME is a hardware only feature and its BIOS status is all that needs
> to be shared with the kernel user: enabled or disabled. The TME
> algorithm the BIOS is using and whether or not the kernel recognizes
> that algorithm is useless to the kernel user.
>
> MKTME is a hardware feature that requires kernel support. MKTME
> detection code was added in advance of broader kernel support for
> MKTME that never followed. So, rather than continuing to emit
> needless and confusing messages about BIOS MKTME status, remove
> most of the MKTME pieces from detect_tme_early().
>
> Keep one important piece: when the BIOS is configured with MKTME
> 'on' any BIOS defined KeyID bits do take away from the physaddr bits
> available in the kernel. Add a pr_info_once() informing about the
> enabled keyids so the user can address (by rebooting with MKTME off)
> if the user needs to recover the MKTME consumed bits.
Nitpickings below:
The original code checks the MSR value consistency between BSP and APs,
but the new code removed that.
I think the changelog should mention why it is OK.
I guess perhaps we can say something like the machine shouldn't be able to
boot if BIOS configures TME activate MSR inconsistently among CPUs, so
don't bother to have the consistency check.
>
> There is no functional change for the user, only this change in boot
> messages:
>
> Before:
> [] x86/tme: enabled by BIOS
> [] x86/tme: Unknown policy is active: 0x2
> [] x86/mktme: No known encryption algorithm is supported: 0x4
> [] x86/mktme: enabled by BIOS
> [] x86/mktme: 127 KeyIDs available
>
> After:
> [] x86/tme: enabled by BIOS
> [] x86/mktme: BIOS enabled 127 keyids
Regarding to what to print:
1) IMHO, it's still better to print the algorithm here. The user/admin
may want to know what algorithm is enabled by the BIOS (especially there
are more than one that can be selected in the BIOS). E.g., the user may
find the default AES-XTS-128 (0) isn't secure enough and wants the more
secure algorithm using 256-bit key.
I understand we might not want to maintain a "value to name" table for
this so the kernel can print the algorithm in name, but it would be still
helpful if we just dump the raw value here like:
x86/tme: policy activated: 0x2
2) Given the kernel doesn't support MKTME, I think people may be more
interested in the reduced physical address bits, instead of how MKTME
keyIDs are activated.
x86/tme: MKTME enabled, physical address bits reduced from <xx> to <xx>.
>
> [1] cb06d8e3d020 ("x86/tme: Detect if TME and MKTME is activated by BIOS")
>
> Signed-off-by: Alison Schofield <alison.schofield@...el.com>
> ---
> arch/x86/kernel/cpu/intel.c | 71 +++++++------------------------------
> 1 file changed, 12 insertions(+), 59 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index 3c3e7e5695ba..83865897a2a7 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -190,83 +190,36 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c)
> #define TME_ACTIVATE_LOCKED(x) (x & 0x1)
> #define TME_ACTIVATE_ENABLED(x) (x & 0x2)
>
> -#define TME_ACTIVATE_POLICY(x) ((x >> 4) & 0xf) /* Bits 7:4 */
> -#define TME_ACTIVATE_POLICY_AES_XTS_128 0
> -
> #define TME_ACTIVATE_KEYID_BITS(x) ((x >> 32) & 0xf) /* Bits 35:32 */
>
> -#define TME_ACTIVATE_CRYPTO_ALGS(x) ((x >> 48) & 0xffff) /* Bits 63:48 */
> -#define TME_ACTIVATE_CRYPTO_AES_XTS_128 1
> -
> -/* Values for mktme_status (SW only construct) */
> -#define MKTME_ENABLED 0
> -#define MKTME_DISABLED 1
> -#define MKTME_UNINITIALIZED 2
> -static int mktme_status = MKTME_UNINITIALIZED;
> -
> static void detect_tme_early(struct cpuinfo_x86 *c)
> {
> - u64 tme_activate, tme_policy, tme_crypto_algs;
> int keyid_bits = 0, nr_keyids = 0;
> - static u64 tme_activate_cpu0 = 0;
> + u64 tme_activate;
>
> rdmsrl(MSR_IA32_TME_ACTIVATE, tme_activate);
>
> - if (mktme_status != MKTME_UNINITIALIZED) {
> - if (tme_activate != tme_activate_cpu0) {
> - /* Broken BIOS? */
> - pr_err_once("x86/tme: configuration is inconsistent between CPUs\n");
> - pr_err_once("x86/tme: MKTME is not usable\n");
> - mktme_status = MKTME_DISABLED;
> -
> - /* Proceed. We may need to exclude bits from x86_phys_bits. */
> - }
> - } else {
> - tme_activate_cpu0 = tme_activate;
> - }
> -
> if (!TME_ACTIVATE_LOCKED(tme_activate) || !TME_ACTIVATE_ENABLED(tme_activate)) {
> pr_info_once("x86/tme: not enabled by BIOS\n");
> - mktme_status = MKTME_DISABLED;
> clear_cpu_cap(c, X86_FEATURE_TME);
> return;
> }
> -
> - if (mktme_status != MKTME_UNINITIALIZED)
> - goto detect_keyid_bits;
> -
> - pr_info("x86/tme: enabled by BIOS\n");
> -
> - tme_policy = TME_ACTIVATE_POLICY(tme_activate);
> - if (tme_policy != TME_ACTIVATE_POLICY_AES_XTS_128)
> - pr_warn("x86/tme: Unknown policy is active: %#llx\n", tme_policy);
> -
> - 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;
> - }
> -detect_keyid_bits:
> + pr_info_once("x86/tme: enabled by BIOS\n");
> keyid_bits = TME_ACTIVATE_KEYID_BITS(tme_activate);
> - nr_keyids = (1UL << keyid_bits) - 1;
> - if (nr_keyids) {
> - pr_info_once("x86/mktme: enabled by BIOS\n");
> - pr_info_once("x86/mktme: %d KeyIDs available\n", nr_keyids);
> - } else {
> - pr_info_once("x86/mktme: disabled by BIOS\n");
> - }
> -
> - if (mktme_status == MKTME_UNINITIALIZED) {
> - /* MKTME is usable */
> - mktme_status = MKTME_ENABLED;
> - }
> + if (!keyid_bits)
> + return;
>
> /*
> - * KeyID bits effectively lower the number of physical address
> - * bits. Update cpuinfo_x86::x86_phys_bits accordingly.
> + * KeyID bits are set by BIOS and can be present regardless
> + * of whether the kernel is using them. They effectively lower
> + * the number of physical address bits.
> + *
> + * Update cpuinfo_x86::x86_phys_bits accordingly.
> */
> c->x86_phys_bits -= keyid_bits;
> + nr_keyids = (1UL << keyid_bits) - 1;
> +
> + pr_info_once("x86/mktme: BIOS enabled %d keyids\n", nr_keyids);
If I read correctly, if you print
physical address bits reduced from <xx> to <xx>.
instead of the number of KeyIDs, you can even get rid of the 'nr_keyids'
variable.
Powered by blists - more mailing lists