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. Software then verifies the mitigation state using MSRs. Add code
to ensure that the mitigation is installed and securely locked. Disable
the feature, otherwise.
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
---
Changes from v8:
* Add as a new patch.
Note that the code follows the guidance from [1]:
"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)."
[1] https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/gather-data-sampling.html
---
arch/x86/kernel/keylocker.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/arch/x86/kernel/keylocker.c b/arch/x86/kernel/keylocker.c
index 1b57e11d93ad..d4f3aa65ea8a 100644
--- a/arch/x86/kernel/keylocker.c
+++ b/arch/x86/kernel/keylocker.c
@@ -7,6 +7,7 @@
#include <linux/random.h>
#include <linux/string.h>
+#include <asm/cpu.h>
#include <asm/fpu/api.h>
#include <asm/keylocker.h>
#include <asm/msr.h>
@@ -112,6 +113,37 @@ void restore_keylocker(void)
valid_wrapping_key = false;
}
+/*
+ * The mitigation is implemented at a microcode level. Ensure that the
+ * microcode update is applied and the mitigation is locked.
+ */
+static bool __init have_gds_mitigation(void)
+{
+ u64 mcu_ctrl;
+
+ /* GDS_CTRL is set if new microcode is loaded. */
+ if (!(x86_read_arch_cap_msr() & ARCH_CAP_GDS_CTRL))
+ goto vulnerable;
+
+ /* If GDS_MITG_LOCKED is set, GDS_MITG_DIS is forced to 0. */
+ rdmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
+ if (mcu_ctrl & GDS_MITG_LOCKED)
+ return true;
+
+vulnerable:
+ pr_warn("x86/keylocker: Susceptible to the GDS vulnerability.\n");
+ return 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) && !have_gds_mitigation())
+ return false;
+
+ return true;
+}
+
static int __init init_keylocker(void)
{
u32 eax, ebx, ecx, edx;
@@ -125,6 +157,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 */
--
2.34.1On Thu, Mar 28, 2024 at 06:53:42PM -0700, Chang S. Bae wrote:
> +/*
> + * The mitigation is implemented at a microcode level. Ensure that the
> + * microcode update is applied and the mitigation is locked.
> + */
> +static bool __init have_gds_mitigation(void)
> +{
> + u64 mcu_ctrl;
> +
> + /* GDS_CTRL is set if new microcode is loaded. */
> + if (!(x86_read_arch_cap_msr() & ARCH_CAP_GDS_CTRL))
> + goto vulnerable;
> +
> + /* If GDS_MITG_LOCKED is set, GDS_MITG_DIS is forced to 0. */
> + rdmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
> + if (mcu_ctrl & GDS_MITG_LOCKED)
> + return true;
Similar to RFDS, above checks can be simplified to:
if (gds_mitigation == GDS_MITIGATION_FULL_LOCKED)
return true;
> +
> +vulnerable:
> + pr_warn("x86/keylocker: Susceptible to the GDS vulnerability.\n");
> + return false;
> +}
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@intel.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Pawan Gupta <pawan.kumar.gupta@linux.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;
--
2.40.1On 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@intel.com>
> Cc: Dave Hansen <dave.hansen@intel.com>
> Cc: Pawan Gupta <pawan.kumar.gupta@linux.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 */
On 4/18/2024 5:01 PM, Pawan Gupta wrote: > > Repurposing gds_ucode_mitigated() to check for the locked state is > adding a bit of a churn. We can introduce gds_mitigation_locked() > instead. I thought this option but I was less convinced about adding a new function for every new but slightly different check. Thanks, Chang
In order to safely enable Intel Keylocker feature, Gather Data Sampling
(GDS) mitigation should be enabled and locked. Hardware provides a way to
lock the mitigation, such that the mitigation cannot be disabled until the
CPU is reset. Currently, GDS mitigation is enabled without the lock.
Below is the recommendation from Intel:
"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). This will prevent an adversary that takes
control of the system from turning off the mitigation in order to infer
the keys behind Key Locker handles." [1]
When GDS mitigation is enabled, and Keylocker feature is present, also lock
the mitigation.
[1] Gather Data Sampling (ID# 785676)
https://www.intel.com/content/www/us/en/developer/articles/technical/software-security-guidance/technical-documentation/gather-data-sampling.html
Signed-off-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
---
This should ideally go before the patch that enables Keylocker. It is
only compile tested.
arch/x86/kernel/cpu/bugs.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index ca295b0c1eee..2777a58110e0 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -755,8 +755,8 @@ EXPORT_SYMBOL_GPL(gds_ucode_mitigated);
void update_gds_msr(void)
{
- u64 mcu_ctrl_after;
- u64 mcu_ctrl;
+ u64 mcu_ctrl, mcu_ctrl_after;
+ u64 gds_lock = 0;
switch (gds_mitigation) {
case GDS_MITIGATION_OFF:
@@ -769,6 +769,8 @@ void update_gds_msr(void)
* the same state. Make sure the mitigation is enabled on all
* CPUs.
*/
+ gds_lock = GDS_MITG_LOCKED;
+ fallthrough;
case GDS_MITIGATION_FULL:
rdmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
mcu_ctrl &= ~GDS_MITG_DIS;
@@ -779,6 +781,7 @@ void update_gds_msr(void)
return;
}
+ mcu_ctrl |= gds_lock;
wrmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
/*
@@ -840,6 +843,11 @@ static void __init gds_select_mitigation(void)
gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
}
+ /* Keylocker can only be enabled when GDS mitigation is locked */
+ if (boot_cpu_has(X86_FEATURE_KEYLOCKER) &&
+ gds_mitigation == GDS_MITIGATION_FULL)
+ gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
+
update_gds_msr();
out:
pr_info("%s\n", gds_strings[gds_mitigation]);
---
base-commit: 0bbac3facb5d6cc0171c45c9873a2dc96bea9680
change-id: 20240418-gds-lock-26ecbce88470
Best regards,
--
Thanks,
PawanOn 4/19/2024 10:47 AM, Pawan Gupta wrote:
>
> /*
> @@ -840,6 +843,11 @@ static void __init gds_select_mitigation(void)
> gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
> }
>
> + /* Keylocker can only be enabled when GDS mitigation is locked */
> + if (boot_cpu_has(X86_FEATURE_KEYLOCKER) &&
> + gds_mitigation == GDS_MITIGATION_FULL)
> + gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
> +
I'm having trouble understanding this change:
gds_select_mitigation()
{
...
if (gds_mitigation == GDS_MITIGATION_FORCE)
gds_mitigation = GDS_MITIGATION_FULL;
rdmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
if (mcu_ctrl & GDS_MITG_LOCKED) {
...
gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
}
if (boot_cpu_has(X86_FEATURE_KEYLOCKER) &&
gds_mitigation == GDS_MITIGATION_FULL)
gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
As I understand it, gds_mitigation is set to GDS_MITIGATION_FULL only if
the gds force option is enabled but IA32_MCU_OPT_CTRL[GDS_MITG_LOCK] is
not set.
Then, if the CPU has Key Locker, this code sets gds_mitigation to
GDS_MITIGATION_FULL_LOCKED, which seems contradictory. I'm not sure why
this change is necessary.
I'm also not convinced that the Key Locker series needs to modify this
function. The Key Locker setup code should simply check the current
mitigation status and enable the feature only if proper mitigation is in
place. Am I missing something here?
Thanks,
Chang
On Mon, Apr 22, 2024 at 12:35:45AM -0700, Chang S. Bae wrote:
> On 4/19/2024 10:47 AM, Pawan Gupta wrote:
> > /*
> > @@ -840,6 +843,11 @@ static void __init gds_select_mitigation(void)
> > gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
> > }
> > + /* Keylocker can only be enabled when GDS mitigation is locked */
> > + if (boot_cpu_has(X86_FEATURE_KEYLOCKER) &&
> > + gds_mitigation == GDS_MITIGATION_FULL)
> > + gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
> > +
>
> I'm having trouble understanding this change:
>
> gds_select_mitigation()
> {
> ...
> if (gds_mitigation == GDS_MITIGATION_FORCE)
> gds_mitigation = GDS_MITIGATION_FULL;
>
> rdmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
> if (mcu_ctrl & GDS_MITG_LOCKED) {
> ...
> gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
> }
>
> if (boot_cpu_has(X86_FEATURE_KEYLOCKER) &&
> gds_mitigation == GDS_MITIGATION_FULL)
> gds_mitigation = GDS_MITIGATION_FULL_LOCKED;
>
> As I understand it, gds_mitigation is set to GDS_MITIGATION_FULL only if the
> gds force option is enabled but IA32_MCU_OPT_CTRL[GDS_MITG_LOCK] is not set.
Not true, GDS_MITIGATION_FULL is the default. Cmdline
gather_data_sampling=force deploys a software fallback mitigation when
the microcode mitigation is not present. But, when microcode mitigation
is present, mitigation is set to GDS_MITIGATION_FULL.
> Then, if the CPU has Key Locker, this code sets gds_mitigation to
> GDS_MITIGATION_FULL_LOCKED, which seems contradictory. I'm not sure why this
> change is necessary.
>
> I'm also not convinced that the Key Locker series needs to modify this
> function. The Key Locker setup code should simply check the current
> mitigation status and enable the feature only if proper mitigation is in
> place. Am I missing something here?
To enable Key Locker feature, "proper mitigation" is microcode mitigation
enabled and the GDS_MITG_LOCK bit set in MSR_IA32_MCU_OPT_CTRL. Do you
agree?
If not via this patch, how is GDS_MITG_LOCK going to be set?
Below is from Intel's documentation:
"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). This will prevent an adversary that takes
control of the system from turning off the mitigation in order to infer
the keys behind Key Locker handles.
To support GDS mitigation locking for Key Locker, microcode updates
for Tiger Lake systems enable the following model-specific behavior
for GDS_MITG_LOCK. On these systems, a write to IA32_MCU_OPT_CTRL MSR
with GDS_MITG_DIS (bit 4) value 0 and GDS_MITG_LOCK (bit 5) value 1
will lock both bits at these values until reset."
On 4/22/2024 2:32 PM, Pawan Gupta wrote: > > To enable Key Locker feature, "proper mitigation" is microcode mitigation > enabled and the GDS_MITG_LOCK bit set in MSR_IA32_MCU_OPT_CTRL. Do you > agree? > > If not via this patch, how is GDS_MITG_LOCK going to be set? The lock bit seems to be set by microcode when SGX is available. However, if the lock bit is not set for Key Locker, it does seem odd. Introducing kernel code to override this situation might be seen as a workaround rather than a proper solution, potentially leading to more confusion. I'd rather investigate the behavior of the microcode further, verify its consistency, and gain a clearer understanding of the requirement for this lock bit. Thanks, Chang
On 4/19/24 10:47, Pawan Gupta wrote:
> + u64 gds_lock = 0;
>
> switch (gds_mitigation) {
> case GDS_MITIGATION_OFF:
> @@ -769,6 +769,8 @@ void update_gds_msr(void)
> * the same state. Make sure the mitigation is enabled on all
> * CPUs.
> */
> + gds_lock = GDS_MITG_LOCKED;
Can't we just drop the new gds_lock var and set mcu_ctrl |= GDS_MITG_LOCKED here?
> + fallthrough;
> case GDS_MITIGATION_FULL:
> rdmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
> mcu_ctrl &= ~GDS_MITG_DIS;
> @@ -779,6 +781,7 @@ void update_gds_msr(void)
> return;
> }
>
> + mcu_ctrl |= gds_lock;
> wrmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
On Fri, Apr 19, 2024 at 11:03:28AM -0700, Daniel Sneddon wrote:
> On 4/19/24 10:47, Pawan Gupta wrote:
> > + u64 gds_lock = 0;
> >
> > switch (gds_mitigation) {
> > case GDS_MITIGATION_OFF:
> > @@ -769,6 +769,8 @@ void update_gds_msr(void)
> > * the same state. Make sure the mitigation is enabled on all
> > * CPUs.
> > */
> > + gds_lock = GDS_MITG_LOCKED;
> Can't we just drop the new gds_lock var and set mcu_ctrl |= GDS_MITG_LOCKED here?
Unfortunately no, because ...
> > + fallthrough;
> > case GDS_MITIGATION_FULL:
> > rdmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
... mcu_ctrl is read here, it will overwrite any previous value.
> > mcu_ctrl &= ~GDS_MITG_DIS;
> > @@ -779,6 +781,7 @@ void update_gds_msr(void)
> > return;
> > }
> >
> > + mcu_ctrl |= gds_lock;
> > wrmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
© 2016 - 2026 Red Hat, Inc.