linux-kernel - Re: [PATCH v2] x86/speculation/srbds: do not try to turn mitigation off when not supported

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220331084536.y4sl7qcfzltsnnew@guptapa-desk>
Date:   Thu, 31 Mar 2022 01:46:10 -0700
From:   Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>
To:     Ricardo Cañuelo <ricardo.canuelo@...labora.com>
Cc:     Borislav Petkov <bp@...en8.de>, linux-kernel@...r.kernel.org,
        Thadeu Lima de Souza Cascardo <cascardo@...onical.com>,
        Mark Gross <mgross@...ux.intel.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>, Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        John Johansen <john.johansen@...onical.com>,
        Steve Beattie <sbeattie@...ntu.com>, kernel@...labora.com
Subject: Re: [PATCH v2] x86/speculation/srbds: do not try to turn mitigation
 off when not supported

On Thu, Mar 31, 2022 at 09:48:14AM +0200, Ricardo Cañuelo wrote:
>Borislav Petkov <bp@...en8.de> writes:
>> So we could also do the below to denote what the situation is and
>> therefore clear the bug flag for such CPUs.
>>
>> The thing is: I want this to be as clear as possible because bugs.c is
>> already a nightmare and just slapping more logic to it without properly
>> thinking it through is going to be a serious pain to deal with later...
>
>Thanks Boris,
>
>I agree that the more explicit the better, I'll give this a try. I saw
>Pawan's suggestion as well but that one is similar to the originally
>proposed patch in that the logic/checks are split between two functions,
>this solution based on clearing the bug flag seems clearer considering
>the comment just before the code block:
>
>	/*
>	 * Check to see if this is one of the MDS_NO systems supporting
>	 * TSX that are only exposed to SRBDS when TSX is enabled.
>	 */

If we clear the bug flag and not write to the MSR to disable the
microcode mitigation, there will be unnecessary performance loss due to
microcode mitigation. Specifically RDRAND/RDSEED/EGET_KEY will run
slower.

Yes, the logic/checks between two functions is messy.

Does this simplify things a bit?

---
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 6296e1ebed1d..0be6c0eabb71 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -431,34 +431,22 @@ static const char * const srbds_strings[] = {
  	[SRBDS_MITIGATION_HYPERVISOR]	= "Unknown: Dependent on hypervisor status",
  };
  
-static bool srbds_off;
+static bool srbds_dis_ucode_mitigation;
  
  void update_srbds_msr(void)
  {
  	u64 mcu_ctrl;
  
-	if (!boot_cpu_has_bug(X86_BUG_SRBDS))
-		return;
-
-	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
-		return;
-
-	if (srbds_mitigation == SRBDS_MITIGATION_UCODE_NEEDED)
+	if (!boot_cpu_has_bug(X86_BUG_SRBDS) ||
+	    !boot_cpu_has(X86_FEATURE_SRBDS_CTRL))
  		return;
  
  	rdmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
  
-	switch (srbds_mitigation) {
-	case SRBDS_MITIGATION_OFF:
-	case SRBDS_MITIGATION_TSX_OFF:
+	if (srbds_dis_ucode_mitigation)
  		mcu_ctrl |= RNGDS_MITG_DIS;
-		break;
-	case SRBDS_MITIGATION_FULL:
+	else
  		mcu_ctrl &= ~RNGDS_MITG_DIS;
-		break;
-	default:
-		break;
-	}
  
  	wrmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
  }
@@ -481,9 +469,13 @@ static void __init srbds_select_mitigation(void)
  		srbds_mitigation = SRBDS_MITIGATION_HYPERVISOR;
  	else if (!boot_cpu_has(X86_FEATURE_SRBDS_CTRL))
  		srbds_mitigation = SRBDS_MITIGATION_UCODE_NEEDED;
-	else if (cpu_mitigations_off() || srbds_off)
+	else if (cpu_mitigations_off())
  		srbds_mitigation = SRBDS_MITIGATION_OFF;
  
+	if (srbds_mitigation == SRBDS_MITIGATION_OFF ||
+	    srbds_mitigation == SRBDS_MITIGATION_TSX_OFF)
+		srbds_dis_ucode_mitigation = true;
+
  	update_srbds_msr();
  	pr_info("%s\n", srbds_strings[srbds_mitigation]);
  }
@@ -496,7 +488,9 @@ static int __init srbds_parse_cmdline(char *str)
  	if (!boot_cpu_has_bug(X86_BUG_SRBDS))
  		return 0;
  
-	srbds_off = !strcmp(str, "off");
+	if (!strcmp(str, "off"))
+		srbds_mitigation = SRBDS_MITIGATION_OFF;
+
  	return 0;
  }
  early_param("srbds", srbds_parse_cmdline);