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 <[email protected]>
Signed-off-by: Borislav Petkov <[email protected]>
Signed-off-by: Ricardo Cañuelo <[email protected]>
Tested-by: Ricardo Cañuelo <[email protected]>
---
Hi all,
This patch was originally posted here:
https://lore.kernel.org/all/[email protected]/#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;
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:
--
2.25.1
+ Pawan who's been poking at TSX recently...
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 <[email protected]>
> Signed-off-by: Borislav Petkov <[email protected]>
> Signed-off-by: Ricardo Cañuelo <[email protected]>
> Tested-by: Ricardo Cañuelo <[email protected]>
> ---
> Hi all,
>
> This patch was originally posted here:
>
> https://lore.kernel.org/all/[email protected]/#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;
>
> 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:
> --
So I'm not yet 100% sure as to how to model this properly. The fact
is, the CPU is not affected by SRBDS when it is a MDS_NO CPU with TSX
disabled.
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...
Thx.
---
diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 66d3e3b1d24d..9fa7a6ba09c7 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -217,6 +217,7 @@ static __always_inline bool _static_cpu_has(u16 bit)
#define static_cpu_has_bug(bit) static_cpu_has((bit))
#define boot_cpu_has_bug(bit) cpu_has_bug(&boot_cpu_data, (bit))
#define boot_cpu_set_bug(bit) set_cpu_cap(&boot_cpu_data, (bit))
+#define boot_cpu_clear_bug(bit) clear_cpu_cap(&boot_cpu_data, (bit))
#define MAX_CPU_FEATURES (NCAPINTS * 32)
#define cpu_have_feature boot_cpu_has
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 6296e1ebed1d..02fdfe5e2f2a 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -475,8 +475,10 @@ static void __init srbds_select_mitigation(void)
* TSX that are only exposed to SRBDS when TSX is enabled.
*/
ia32_cap = x86_read_arch_cap_msr();
- if ((ia32_cap & ARCH_CAP_MDS_NO) && !boot_cpu_has(X86_FEATURE_RTM))
+ if ((ia32_cap & ARCH_CAP_MDS_NO) && !boot_cpu_has(X86_FEATURE_RTM)) {
srbds_mitigation = SRBDS_MITIGATION_TSX_OFF;
+ boot_cpu_clear_bug(X86_BUG_SRBDS);
+ }
else if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
srbds_mitigation = SRBDS_MITIGATION_HYPERVISOR;
else if (!boot_cpu_has(X86_FEATURE_SRBDS_CTRL))
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
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 <[email protected]>
>Signed-off-by: Borislav Petkov <[email protected]>
>Signed-off-by: Ricardo Cañuelo <[email protected]>
>Tested-by: Ricardo Cañuelo <[email protected]>
>---
>Hi all,
>
>This patch was originally posted here:
>
>https://lore.kernel.org/all/[email protected]/#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
Borislav Petkov <[email protected]> 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.
*/
Cheers,
Ricardo
On Thu, Mar 31, 2022 at 09:48:14AM +0200, Ricardo Cañuelo wrote:
>Borislav Petkov <[email protected]> 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);
On Thu, Mar 31, 2022 at 09:48:14AM +0200, Ricardo Cañuelo wrote:
> 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:
Yeah, and I have some reservations with clearing that flag because,
technically speaking, that CPU still has X86_BUG_SRBDS - it's just that
it hasn't been exposed due to TSX being disabled. And no SRBDS microcode
has been uploaded.
Btw this is exactly the reason I want this to be crystal clear -
the insane conditionals around those things just to salvage *some*
performance with a lot of "but but" make everyone who deals with bugs.c
cringe...
Anyway, Pawan's suggestion makes more sense with the aspect that, yes,
the CPU is affected but the MSR is not there. And we already have
similar logic when dealing with TSX so that no new territory.
So yeah, let's do his but *actually* document why and put it in a
separate line:
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 6296e1ebed1d..d879a6c93609 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -446,6 +446,13 @@ void update_srbds_msr(void)
if (srbds_mitigation == SRBDS_MITIGATION_UCODE_NEEDED)
return;
+ /*
+ * A MDS_NO CPU for which SRBDS mitigation is not needed due to TSX
+ * being disabled and it hasn't received the SRBDS MSR microcode.
+ */
+ if (!boot_cpu_has(X86_FEATURE_SRBDS_CTRL))
+ return;
+
rdmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
switch (srbds_mitigation) {
---
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Pawan Gupta <[email protected]> writes:
Hi Pawan,
> 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;
Just to clarify, this changes the behavior wrt the hypervisor case:
currently it just bails out of update_srbds_msr(), with your patch it'd
clear RNGDS_MITG_DIS from MSR_IA32_MCU_OPT_CTRL. Is that what you
intended?
Ricardo
On Thu, Mar 31, 2022 at 05:25:27PM +0200, Borislav Petkov wrote:
>On Thu, Mar 31, 2022 at 03:18:42PM +0200, Ricardo Cañuelo wrote:
>> Just to clarify, this changes the behavior wrt the hypervisor case:
>> currently it just bails out of update_srbds_msr(), with your patch it'd
>> clear RNGDS_MITG_DIS from MSR_IA32_MCU_OPT_CTRL. Is that what you
>> intended?
Yes, this should be fine as update_srbds_msr() would also check for
X86_FEATURE_SRBDS_CTRL. If a hypervisor doesn't want a guest to write to
the MSR it should not export X86_FEATURE_SRBDS_CTRL.
>Just do the simple thing I pasted earlier - no need to rewrite the whole
>function for no good reason.
I was trying to address earlier comment on split logic in two functions.
I am okay with keeping it as is (and just adding X86_FEATURE_SRBDS_CTRL
check in update_srbds_msr()).
Thanks,
Pawan
On Thu, Mar 31, 2022 at 03:18:42PM +0200, Ricardo Cañuelo wrote:
> Just to clarify, this changes the behavior wrt the hypervisor case:
> currently it just bails out of update_srbds_msr(), with your patch it'd
> clear RNGDS_MITG_DIS from MSR_IA32_MCU_OPT_CTRL. Is that what you
> intended?
Just do the simple thing I pasted earlier - no need to rewrite the whole
function for no good reason.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette