Messages in this thread Patch in this message | | | Date | Wed, 30 Mar 2022 22:02:00 +0200 | From | Borislav Petkov <> | Subject | Re: [PATCH v2] x86/speculation/srbds: do not try to turn mitigation off when not supported |
| |
+ 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 <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; > > 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
| |