| 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: <20240419000113.2tjvyigx7nlyymrw@desk>
Date: Thu, 18 Apr 2024 17:01:13 -0700
From: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
To: "Chang S. Bae" <chang.seok.bae@...el.com>
Cc: linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org,
ebiggers@...nel.org, luto@...nel.org, dave.hansen@...ux.intel.com,
tglx@...utronix.de, bp@...en8.de, mingo@...nel.org, x86@...nel.org,
herbert@...dor.apana.org.au, ardb@...nel.org, elliott@....com,
dan.j.williams@...el.com, bernie.keany@...el.com,
charishma1.gairuboyina@...el.com,
Dave Hansen <dave.hansen@...el.com>
Subject: Re: [PATCH v9a 10/14] x86/cpu/keylocker: Check Gather Data Sampling
mitigation
On Sun, Apr 07, 2024 at 04:04:32PM -0700, Chang S. Bae wrote:
> Gather Data Sampling is a transient execution side channel issue in some
> CPU models. The stale data in registers is not guaranteed as secure when
> this vulnerability is not addressed.
>
> In the Key Locker usage during AES transformations, the temporary storage
> of the original key in registers poses a risk. The key material can be
> staled in some implementations, leading to susceptibility to leakage of
> the AES key.
>
> To mitigate this vulnerability, a qualified microcode image must be
> applied. Add code to ensure that the mitigation is installed and securely
> locked. Disable the feature, otherwise.
>
> Expand gds_ucode_mitigated() to examine the lock state.
>
> Signed-off-by: Chang S. Bae <chang.seok.bae@...el.com>
> Cc: Dave Hansen <dave.hansen@...el.com>
> Cc: Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
> ---
> Changes from v9:
> * Removed MSR reads and utilized the helper function. (Pawan Gupta)
>
> Alternatively, 'gds_mitigation' can be exported and referenced directly.
> Using 'gds_mitigation == GDS_MITIGATION_FULL_LOCKED' may also be
> readable. However, it was opted to expand gds_ucode_mitigated() for
> consistency, as it is already established.
>
> Note that this approach aligns with Intel's guidance, as the bugs.c code
> checks the following MSR bits:
> "Intel recommends that system software does not enable Key Locker (by
> setting CR4.KL) unless the GDS mitigation is enabled
> (IA32_MCU_OPT_CTRL[GDS_MITG_DIS] (bit 4) is 0) and locked
> (IA32_MCU_OPT_CTRL [GDS_MITG_LOCK](bit 5) is 1)."
>
> For more information, refer to Intel's technical documentation on Gather
> Data Sampling:
> https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/gather-data-sampling.html
> ---
> arch/x86/include/asm/processor.h | 7 ++++++-
> arch/x86/kernel/cpu/bugs.c | 5 ++++-
> arch/x86/kernel/keylocker.c | 12 ++++++++++++
> arch/x86/kvm/x86.c | 2 +-
> 4 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 811548f131f4..74eaa3a2b85b 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -721,7 +721,12 @@ enum mds_mitigations {
> MDS_MITIGATION_VMWERV,
> };
>
> -extern bool gds_ucode_mitigated(void);
> +enum mitigation_info {
> + MITG_FULL,
> + MITG_LOCKED,
> +};
> +
> +extern bool gds_ucode_mitigated(enum mitigation_info mitg);
>
> /*
> * Make previous memory operations globally visible before
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index e7ba936d798b..80f6e70619cb 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -752,8 +752,11 @@ static const char * const gds_strings[] = {
> [GDS_MITIGATION_HYPERVISOR] = "Unknown: Dependent on hypervisor status",
> };
>
> -bool gds_ucode_mitigated(void)
> +bool gds_ucode_mitigated(enum mitigation_info mitg)
> {
> + if (mitg == MITG_LOCKED)
> + return gds_mitigation == GDS_MITIGATION_FULL_LOCKED;
> +
> return (gds_mitigation == GDS_MITIGATION_FULL ||
> gds_mitigation == GDS_MITIGATION_FULL_LOCKED);
> }
> diff --git a/arch/x86/kernel/keylocker.c b/arch/x86/kernel/keylocker.c
> index 1e81d0704eea..23cf4a235f11 100644
> --- a/arch/x86/kernel/keylocker.c
> +++ b/arch/x86/kernel/keylocker.c
> @@ -113,6 +113,15 @@ void restore_keylocker(void)
> valid_wrapping_key = false;
> }
>
> +/* Check if Key Locker is secure enough to be used. */
> +static bool __init secure_keylocker(void)
> +{
> + if (boot_cpu_has_bug(X86_BUG_GDS) && !gds_ucode_mitigated(MITG_LOCKED))
> + return false;
> +
> + return true;
> +}
> +
> static int __init init_keylocker(void)
> {
> u32 eax, ebx, ecx, edx;
> @@ -126,6 +135,9 @@ static int __init init_keylocker(void)
> goto clear_cap;
> }
>
> + if (!secure_keylocker())
> + goto clear_cap;
> +
> cr4_set_bits(X86_CR4_KEYLOCKER);
>
> /* AESKLE depends on CR4.KEYLOCKER */
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 47d9f03b7778..4ab50e95fdb5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1675,7 +1675,7 @@ static u64 kvm_get_arch_capabilities(void)
> */
> }
>
> - if (!boot_cpu_has_bug(X86_BUG_GDS) || gds_ucode_mitigated())
> + if (!boot_cpu_has_bug(X86_BUG_GDS) || gds_ucode_mitigated(MITG_FULL))
> data |= ARCH_CAP_GDS_NO;
>
> return data;
Repurposing gds_ucode_mitigated() to check for the locked state is
adding a bit of a churn. We can introduce gds_mitigation_locked()
instead.
Is below looking okay:
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 811548f131f4..8ba96e8a8754 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -722,6 +722,7 @@ enum mds_mitigations {
};
extern bool gds_ucode_mitigated(void);
+extern bool gds_mitigation_locked(void);
/*
* Make previous memory operations globally visible before
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index ca295b0c1eee..a7ec26988ddb 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -753,6 +753,11 @@ bool gds_ucode_mitigated(void)
}
EXPORT_SYMBOL_GPL(gds_ucode_mitigated);
+bool gds_mitigation_locked(void)
+{
+ return gds_mitigation == GDS_MITIGATION_FULL_LOCKED;
+}
+
void update_gds_msr(void)
{
u64 mcu_ctrl_after;
diff --git a/arch/x86/kernel/keylocker.c b/arch/x86/kernel/keylocker.c
index 1b57e11d93ad..c40e72f482b1 100644
--- a/arch/x86/kernel/keylocker.c
+++ b/arch/x86/kernel/keylocker.c
@@ -112,6 +112,15 @@ void restore_keylocker(void)
valid_wrapping_key = false;
}
+/* Check if Key Locker is secure enough to be used. */
+static bool __init secure_keylocker(void)
+{
+ if (boot_cpu_has_bug(X86_BUG_GDS) && !gds_mitigation_locked())
+ return false;
+
+ return true;
+}
+
static int __init init_keylocker(void)
{
u32 eax, ebx, ecx, edx;
@@ -125,6 +134,9 @@ static int __init init_keylocker(void)
goto clear_cap;
}
+ if (!secure_keylocker())
+ goto clear_cap;
+
cr4_set_bits(X86_CR4_KEYLOCKER);
/* AESKLE depends on CR4.KEYLOCKER */
Powered by blists - more mailing lists