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

[Pawan Gupta <pawan.kumar.gupta@linux.intel.com>]

On Wed, Mar 30, 2022 at 10:20:26AM +0200, Ricardo Cañuelo wrote:
>When SRBDS is mitigated by TSX OFF, update_srbds_msr will still read and
>write to MSR_IA32_MCU_OPT_CTRL even when that is not supported by the
>microcode.
>
>Checking for X86_FEATURE_SRBDS_CTRL as a CPU feature available makes more
>sense than checking for SRBDS_MITIGATION_UCODE_NEEDED as the found
>"mitigation".
>
>Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
>Signed-off-by: Borislav Petkov <bp@alien8.de>
>Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com>
>Tested-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com>
>---
>Hi all,
>
>This patch was originally posted here:
>
>https://lore.kernel.org/all/20200609174313.2600320-1-cascardo@canonical.com/#t
>
>by Boris, based on the original patch by Cascardo, I didn't make any
>changes to it. I didn't see it merged or discussed further and I can
>still reproduce the issue on a Samsung Galaxy Chromebook 2 (Intel
>Cometlake-U). When booted without the proper CPU u-codes, TSX is
>disabled (so the CPU isn't vulnerable to SRDBS) but this code still
>tries to access an unavailable MSR register so I get these two warning
>messages:
>
>unchecked MSR access error: RDMSR from 0x123 at rIP: 0xffffffff8203707e (update_srbds_msr+0x2e/0xa0)
>Call Trace:
> <TASK>
> check_bugs+0x994/0xa6e
> ? __get_locked_pte+0x8f/0x100
> start_kernel+0x630/0x664
> secondary_startup_64_no_verify+0xd5/0xdb
> </TASK>
>unchecked MSR access error: WRMSR to 0x123 (tried to write 0x0000000000000001) at rIP: 0xffffffff820370a9 (update_srbds_msr+0x59/0xa0)
>Call Trace:
> <TASK>
> check_bugs+0x994/0xa6e
> ? __get_locked_pte+0x8f/0x100
> start_kernel+0x630/0x664
> secondary_startup_64_no_verify+0xd5/0xdb
> </TASK>
>
>This patch avoids them.
>
> arch/x86/kernel/cpu/bugs.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
>index 6296e1ebed1d..9b14cb2ec693 100644
>--- a/arch/x86/kernel/cpu/bugs.c
>+++ b/arch/x86/kernel/cpu/bugs.c
>@@ -443,14 +443,14 @@ void update_srbds_msr(void)
> 	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
> 		return;
>
>-	if (srbds_mitigation == SRBDS_MITIGATION_UCODE_NEEDED)
>+	if (srbds_mitigation == SRBDS_MITIGATION_UCODE_NEEDED ||
>+	    srbds_mitigation == SRBDS_MITIGATION_TSX_OFF)
> 		return;

Returning for TSX OFF case doesn't seem right. System is mitigated
already and there is no need to incur performance loss due to microcode
mitigation. So we need to write to the MSR for TSX OFF case to disable
the microcode mitigation.

IIUC root of the issue is the logic in srbds_select_mitigation() that
gives precedence to TSX_OFF over UCODE_NEEDED:

   srbds_select_mitigation()
   {
   [...]
   	if ((ia32_cap & ARCH_CAP_MDS_NO) && !boot_cpu_has(X86_FEATURE_RTM))
   		srbds_mitigation = SRBDS_MITIGATION_TSX_OFF;
   	else if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
   		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)
		srbds_mitigation = SRBDS_MITIGATION_OFF;

If we don't want to touch this logic, below can be a simple fix:

---
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 6296e1ebed1d..575817163354 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -437,7 +437,8 @@ void update_srbds_msr(void)
  {
  	u64 mcu_ctrl;
  
-	if (!boot_cpu_has_bug(X86_BUG_SRBDS))
+	if (!boot_cpu_has_bug(X86_BUG_SRBDS) ||
+	    !boot_cpu_has(X86_FEATURE_SRBDS_CTRL))
  		return;
  
  	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))

---
Thanks,
Pawan