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

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

[Thadeu Lima de Souza Cascardo]

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>
Acked-by: John Johansen <john.johansen@canonical.com>
Acked-by: Steve Beattie <sbeattie@ubuntu.com>
Cc: stable@vger.kernel.org
---
 arch/x86/kernel/cpu/bugs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index b6f887be440c..ee5bdca7fd30 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -432,7 +432,7 @@ void update_srbds_msr(void)
 	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
 		return;
 
-	if (srbds_mitigation == SRBDS_MITIGATION_UCODE_NEEDED)
+	if (!boot_cpu_has(X86_FEATURE_SRBDS_CTRL))
 		return;
 
 	rdmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
-- 
2.25.1

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

[Borislav Petkov]

On Tue, Jun 09, 2020 at 02:43:13PM -0300, Thadeu Lima de Souza Cascardo wrote:
> When SRBDS is mitigated by TSX OFF, update_srbds_msr will still read and

Are you talking about this case in srbds_select_mitigation():

        if ((ia32_cap & ARCH_CAP_MDS_NO) && !boot_cpu_has(X86_FEATURE_RTM))
                srbds_mitigation = SRBDS_MITIGATION_TSX_OFF;

?

and you have a system which:

         * Check to see if this is one of the MDS_NO systems supporting
         * TSX that are only exposed to SRBDS when TSX is enabled.

i.e., no SRBDS microcode for it and the fix is to disable TSX?

If so, I think the right fix should be to do:

	if (!boot_cpu_has(X86_FEATURE_SRBDS_CTRL))
		return;

in update_srbds_msr() with a comment above it explaining why that check
is being done.

Hmmm.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Thadeu Lima de Souza Cascardo]

On Mon, Jun 15, 2020 at 10:28:58AM +0200, Borislav Petkov wrote:
> On Tue, Jun 09, 2020 at 02:43:13PM -0300, Thadeu Lima de Souza Cascardo wrote:
> > When SRBDS is mitigated by TSX OFF, update_srbds_msr will still read and
> 
> Are you talking about this case in srbds_select_mitigation():
> 
>         if ((ia32_cap & ARCH_CAP_MDS_NO) && !boot_cpu_has(X86_FEATURE_RTM))
>                 srbds_mitigation = SRBDS_MITIGATION_TSX_OFF;
> 
> ?
> 

That's the case that it hits, correct.

> and you have a system which:
> 
>          * Check to see if this is one of the MDS_NO systems supporting
>          * TSX that are only exposed to SRBDS when TSX is enabled.
> 
> i.e., no SRBDS microcode for it and the fix is to disable TSX?

It was booted without the microcode update. There was microcode available, but
systems may be booted without it, thus causing warnings due to the MSR
read/write.

> 
> If so, I think the right fix should be to do:
> 
> 	if (!boot_cpu_has(X86_FEATURE_SRBDS_CTRL))
> 		return;
> 
> in update_srbds_msr() with a comment above it explaining why that check
> is being done.

That's exactly the fix in the patch I sent, right? Do you want me to resend
with a comment, then?

Thanks.
Cascardo.

> 
> Hmmm.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

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

[Borislav Petkov]

On Mon, Jun 15, 2020 at 07:27:38AM -0300, Thadeu Lima de Souza Cascardo wrote:
> It was booted without the microcode update. There was microcode available, but
> systems may be booted without it, thus causing warnings due to the MSR
> read/write.

Right.

> That's exactly the fix in the patch I sent, right? Do you want me to resend
> with a comment, then?

Your patch replaced

-       if (srbds_mitigation == SRBDS_MITIGATION_UCODE_NEEDED)

Thinking about this more, I think the proper thing to do is this:

---
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 0b71970d2d3d..ce2931563f8f 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -432,14 +432,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;
 
 	rdmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
 
 	switch (srbds_mitigation) {
 	case SRBDS_MITIGATION_OFF:
-	case SRBDS_MITIGATION_TSX_OFF:
 		mcu_ctrl |= RNGDS_MITG_DIS;
 		break;
 	case SRBDS_MITIGATION_FULL:
---

because looking at:

  7e5b3c267d25 ("x86/speculation: Add Special Register Buffer Data Sampling (SRBDS) mitigation")

it says:

    While it is present on all affected CPU models, the microcode mitigation
    is not needed on models that enumerate ARCH_CAPABILITIES[MDS_NO] in the
    cases where TSX is not supported or has been disabled with TSX_CTRL.

which could be also understood as "there won't be microcode for those
CPUs and thus MSR_IA32_MCU_OPT_CTRL won't be there."

Because if that is the case, then SRBDS_MITIGATION_TSX_OFF means the MSR
is not there and therefore we should not touch it.

And you've actually shown that without the microcode loaded, you
can have a system which is MDS_NO and which hasn't generated
MSR_IA32_MCU_OPT_CTRL (because the microcode is not loaded) and thus
can't touch said MSR.

Mark?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette