VERW based clean-up

There are several mitigations that use the VERW instruction to clean up internal CPU buffers. Currently, each of these mitigations is treated independently, but if VERW is needed for one of the mitigations, it's on for all of them. This can lead to some confusion if a user tries to disable one of the mitigations, but it is left enabled for one of the others. The user needs to disable all 4 VERW- based mitigations. Warn the user when one or more VERW mitigations are disabled but not all of them. While we're messing with VERW mitigations, might as well simplify them and remove the need to call each of them twice.

V2:
Dropped the new knob previously introduced in the first patch (Borislav) Add warning if not all 4 mitigations states match (Borislav) Removed extra comment (Josh)
Code clean-up (Josh)

Daniel Sneddon (2):
x86/bugs: Check VERW mitigations for consistency x86/bugs: Clean-up verw mitigations

 arch/x86/include/asm/processor.h |   2 +-
 arch/x86/kernel/cpu/bugs.c       | 206 +++++++++++++------------------
 2 files changed, 90 insertions(+), 118 deletions(-)

There are currently 4 mitigations that use VERW: MDS, TAA, MMIO Stale Data, and Register File Data Sampling. Because all 4 use the same mitigation path, if any one of them is enabled, they're all enabled. Normally, this is what is wanted. However, if a user wants to disable the mitigation, this can cause problems. If the user misses disabling even one of these mitigations, then none of them will be

disabled. This can cause confusion as the user expects to regain the performance lost to the mitigation but isn't seeing any improvement. Since there are already 4 knobs for controlling it, adding a 5th knob that controls all 4 mitigations together would just overcomplicate things. Instead, let the user know their mitigations are out of sync when at least one of these mitigations is disabled but not all 4.

Quoted 7 lines · Daniel Sneddon

There are currently 4 mitigations that use VERW: MDS, TAA, MMIO Stale Data, and Register File Data Sampling. Because all 4 use the same mitigation path, if any one of them is enabled, they're all enabled. Normally, this is what is wanted. However, if a user wants to disable the mitigation, this can cause problems. If the user misses disabling even one of these mitigations, then none of them will be

Quoted 8 lines

disabled. This can cause confusion as the user expects to regain the performance lost to the mitigation but isn't seeing any improvement. Since there are already 4 knobs for controlling it, adding a 5th knob that controls all 4 mitigations together would just overcomplicate things. Instead, let the user know their mitigations are out of sync when at least one of these mitigations is disabled but not all 4.

Please split this commit message into smaller chunks for better readability. For example:

There are currently 4 mitigations that use VERW: MDS, TAA, MMIO Stale Data, and Register File Data Sampling. Because all 4 use the same mitigation path, if any one of them is enabled, they're all enabled.

Normally, this is what is wanted. However, if a user wants to disable the mitigation, this can cause problems. If the user misses disabling even one of these mitigations, then none of them will be disabled.

This can cause confusion as the user expects to regain the performance lost to the mitigation but isn't seeing any improvement. Since there are already 4 knobs for controlling it, adding a 5th knob that controls all 4 mitigations together would just overcomplicate things.

Instead, let the user know their mitigations are out of sync when at least one of these mitigations is disabled but not all 4.

Thx.

arch/x86/kernel/cpu/bugs.c static void __init verw_mitigations_check(void) · line 510

Quoted 10 lines · Daniel Sneddon

arch/x86/kernel/cpu/bugs.c +3 -1

@@ -599,20 +503,70 @@ static void __init verw_mitigations_check(void)
 	}
 }
 
-static void __init md_clear_select_mitigation(void)
+static bool __init verw_mitigations_disabled(void)
 {
 	verw_mitigations_check();
+	/*
+	 * TODO: Create a single mitigation variable that will allow for setting

A patch which introduces a TODO is basically telling me, it is not ready to go anywhere yet...

arch/x86/kernel/cpu/bugs.c static void __init verw_mitigations_check(void) · line 521

Quoted 11 lines

arch/x86/kernel/cpu/bugs.c +11 -0

+	 * the location of the mitigation, i.e.:
+	 *
+	 * kernel->user
+	 * kvm->guest
+	 * kvm->guest if device passthrough
+	 * kernel->idle
+	 */
+	return (mds_mitigation == MDS_MITIGATION_OFF &&
+		taa_mitigation == TAA_MITIGATION_OFF &&
+		mmio_mitigation == MMIO_MITIGATION_OFF &&
+		rfds_mitigation == RFDS_MITIGATION_OFF);

This should be used inside verw_mitigations_check() instead of repeated here, no?

Also, pls call verw_mitigations_check() "check_verw_mitigations" - the name should start with a verb.

Actually, you can merge verw_mitigations_check() and verw_mitigations_disabled(). Please do a *minimal* patch when cleaning this up
- bugs.c is horrible. It should not get worse.

What could also help is splitting this patch - it is hard to review as it is...

Thx.

Sure, I'll split this up as much as possible.

Quoted 1 line · Daniel Sneddon

Sure, I'll split this up as much as possible.

Actually, thinking about this more and looking at David's rework:

https://lore.kernel.org/r/[email protected]

his basically is achieving what you're doing - a post-everything routine which selects the final mitigation strategy once all the mitigation options have been parsed and evaluated.

So I'm wondering if we should simply take his directly...

He removes that md_clear* function:

https://lore.kernel.org/r/[email protected]

in favor of doing the final selection in the ->apply* functions and keeping each mitigation functions simple.

Yours does this in a single function.

Practically speaking, the end result is the same.

Hmm...

There are currently 4 mitigations that use VERW: MDS, TAA, MMIO Stale Data, and Register File Data Sampling. Because all 4 use the same mitigation path, if any one of them is enabled, they're all enabled. Normally, this is what is wanted. However, if a user wants to disable the mitigation, this can cause problems. If the user misses disabling even one of these mitigations, then none of them will be disabled. This can cause confusion as the user expects to regain the performance lost to the mitigation but isn't seeing any improvement. Since there are already 4 knobs for controlling it, adding a 5th knob that controls all 4 mitigations together would just overcomplicate things. Instead, let the user know their mitigations are out of sync when at least one of these mitigations is disabled but not all 4.

Signed-off-by Daniel Sneddon <[email protected]>

---
  arch/x86/kernel/cpu/bugs.c | 18 ++++++++++++++++++
  1 file changed, 18 insertions(+)

arch/x86/kernel/cpu/bugs.c +1 -0

index d1915427b4ff..b26b3b554330 100644
@@ -582,8 +582,26 @@ static void __init md_clear_update_mitigation(void)
  		pr_info("Register File Data Sampling: %s\n", rfds_strings[rfds_mitigation]);
  }
 
+static void __init verw_mitigations_check(void)

arch/x86/kernel/cpu/bugs.c +7 -0

+
+		pr_info("MDS, TAA, MMIO Stale Data, and Register File Data Sampling all depend on VERW\n");
+		pr_info("In order to disable any one of them please ensure all 4 are disabled.\n");
+	}
+}
+
  static void __init md_clear_select_mitigation(void)
  {
+	verw_mitigations_check();
  	mds_select_mitigation();
  	taa_select_mitigation();
  	mmio_select_mitigation();

Quoted 29 lines · Daniel Sneddon

There are currently 4 mitigations that use VERW: MDS, TAA, MMIO Stale Data, and Register File Data Sampling. Because all 4 use the same mitigation path, if any one of them is enabled, they're all enabled. Normally, this is what is wanted. However, if a user wants to disable the mitigation, this can cause problems. If the user misses disabling even one of these mitigations, then none of them will be disabled. This can cause confusion as the user expects to regain the performance lost to the mitigation but isn't seeing any improvement. Since there are already 4 knobs for controlling it, adding a 5th knob that controls all 4 mitigations together would just overcomplicate things. Instead, let the user know their mitigations are out of sync when at least one of these mitigations is disabled but not all 4.

Signed-off-by Daniel Sneddon <[email protected]>

---
  arch/x86/kernel/cpu/bugs.c | 18 ++++++++++++++++++
  1 file changed, 18 insertions(+)

arch/x86/kernel/cpu/bugs.c +0 -0

index d1915427b4ff..b26b3b554330 100644
@@ -582,8 +582,26 @@ static void __init md_clear_update_mitigation(void)
  		pr_info("Register File Data Sampling: %s\n", rfds_strings[rfds_mitigation]);
  }
 

arch/x86/kernel/cpu/bugs.c static void __init md_clear_update_mitigation(void) · line 595

Quoted 11 lines

arch/x86/kernel/cpu/bugs.c +11 -0

+static void __init verw_mitigations_check(void)
+{
+	if (mds_mitigation == MDS_MITIGATION_OFF ||
+	    taa_mitigation == TAA_MITIGATION_OFF ||
+	    mmio_mitigation == MMIO_MITIGATION_OFF ||
+	    rfds_mitigation == RFDS_MITIGATION_OFF) {
+		if (mds_mitigation == MDS_MITIGATION_OFF &&
+		    taa_mitigation == TAA_MITIGATION_OFF &&
+		    mmio_mitigation == MMIO_MITIGATION_OFF &&
+		    rfds_mitigation == RFDS_MITIGATION_OFF)
+			return;

Ugh, why don't you simply XOR the 4 values and if it's 1 it means the inputs differe => there is inconsistency.

Quoted 12 lines

arch/x86/kernel/cpu/bugs.c +7 -0

+
+		pr_info("MDS, TAA, MMIO Stale Data, and Register File Data Sampling all depend on VERW\n");
+		pr_info("In order to disable any one of them please ensure all 4 are disabled.\n");
+	}
+}
+
  static void __init md_clear_select_mitigation(void)
  {
+	verw_mitigations_check();
  	mds_select_mitigation();
  	taa_select_mitigation();
  	mmio_select_mitigation();

I really like the attack vector idea David is using. I suspect people really care about "protect my kernel from bad users" or "protect my host vm from guests" more than "protect me from mds and rfds."

I was trying to get rid of the need to do a call to any kind of update function where he took the existing function and split it into one for each mitigation that needs it. Like you said, different approach same end result really.

Quoted 3 lines · Daniel Sneddon

I really like the attack vector idea David is using. I suspect people really care about "protect my kernel from bad users" or "protect my host vm from guests" more than "protect me from mds and rfds."

Yeah, exactly!

Quoted 4 lines

I was trying to get rid of the need to do a call to any kind of update function where he took the existing function and split it into one for each mitigation that needs it. Like you said, different approach same end result really.

Right.

Ok, let's concentrate on David's set, then, so that we don't do unnecessary/doubled work. I'd appreciate it if you took a look at the Intel's side of things there but please wait until he sends a new version next week.

I guess if all agree with the final result, we could look into taking it for 6.14 or so...

Thx.