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 <[email protected]>
---
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,
Pawan
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);
On 4/19/24 13:19, Pawan Gupta wrote:
> ... mcu_ctrl is read here, it will overwrite any previous value.Ah, yep. Bummer.
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.
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