| Message ID | Y/lUSC5x2ZkTIGu4@zn.tnic (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/CPU/AMD: Make sure EFER[AIBRSE] is set | expand |
On Sat, Feb 25, 2023 at 01:20:24AM +0100, Borislav Petkov wrote: > On Fri, Feb 24, 2023 at 04:09:31PM -0800, Josh Poimboeuf wrote: > > Ah, I had to stare it that for a bit to figure out how it works. > > Yeah, it is a bit "hidden". :) > > > setup_real_mode() reads MSR_EFER from the boot CPU and stores it in > > trampoline_header->efer. Then the other CPUs read that stored value in > > startup_32() and write it into their MSR. > > Exactly. > > > Yeah, I think that would be good. Otherwise it's rather magical. > > Yap, see below. > > > That EFER MSR is a surprising place to put that bit. > > That MSR is very important on AMD. Consider it AMD's CR4. :-) > > Thx. > > --- > From: "Borislav Petkov (AMD)" <bp@alien8.de> > Date: Sat, 25 Feb 2023 01:11:31 +0100 > Subject: [PATCH] x86/CPU/AMD: Make sure EFER[AIBRSE] is set > > The AutoIBRS bit gets set only on the BSP as part of determining which > mitigation to enable on AMD. Setting on the APs relies on the > circumstance that the APs get booted through the trampoline and EFER > - the MSR which contains that bit - gets replicated on every AP from the > BSP. > > However, this can change in the future and considering the security > implications of this bit not being set on every CPU, make sure it is set > by verifying EFER later in the boot process and on every AP. > > Reported-by: Josh Poimboeuf <jpoimboe@kernel.org> > Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> > Link: https://lore.kernel.org/r/20230224185257.o3mcmloei5zqu7wa@treble > --- > arch/x86/kernel/cpu/amd.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c > index 380753b14cab..de624c1442c2 100644 > --- a/arch/x86/kernel/cpu/amd.c > +++ b/arch/x86/kernel/cpu/amd.c > @@ -996,6 +996,16 @@ static void init_amd(struct cpuinfo_x86 *c) > msr_set_bit(MSR_K7_HWCR, MSR_K7_HWCR_IRPERF_EN_BIT); > > check_null_seg_clears_base(c); > + > + /* > + * Make sure EFER[AIBRSE - Automatic IBRS Enable] is set. The APs are brought up > + * using the trampoline code and as part of it, EFER gets prepared there in order > + * to be replicated onto them. Regardless, set it here again, if not set, to protect > + * against any future refactoring/code reorganization which might miss setting > + * this important bit. > + */ > + if (cpu_has(c, X86_FEATURE_AUTOIBRS)) > + msr_set_bit(MSR_EFER, _EFER_AUTOIBRS); Is it intended to be set regardless of the spectre_v2 mitigation status?
On Fri, Feb 24, 2023 at 04:52:21PM -0800, Pawan Gupta wrote: > On Sat, Feb 25, 2023 at 01:20:24AM +0100, Borislav Petkov wrote: > > On Fri, Feb 24, 2023 at 04:09:31PM -0800, Josh Poimboeuf wrote: > > > Ah, I had to stare it that for a bit to figure out how it works. > > > > Yeah, it is a bit "hidden". :) > > > > > setup_real_mode() reads MSR_EFER from the boot CPU and stores it in > > > trampoline_header->efer. Then the other CPUs read that stored value in > > > startup_32() and write it into their MSR. > > > > Exactly. > > > > > Yeah, I think that would be good. Otherwise it's rather magical. > > > > Yap, see below. > > > > > That EFER MSR is a surprising place to put that bit. > > > > That MSR is very important on AMD. Consider it AMD's CR4. :-) > > > > Thx. > > > > --- > > From: "Borislav Petkov (AMD)" <bp@alien8.de> > > Date: Sat, 25 Feb 2023 01:11:31 +0100 > > Subject: [PATCH] x86/CPU/AMD: Make sure EFER[AIBRSE] is set > > > > The AutoIBRS bit gets set only on the BSP as part of determining which > > mitigation to enable on AMD. Setting on the APs relies on the > > circumstance that the APs get booted through the trampoline and EFER > > - the MSR which contains that bit - gets replicated on every AP from the > > BSP. > > > > However, this can change in the future and considering the security > > implications of this bit not being set on every CPU, make sure it is set > > by verifying EFER later in the boot process and on every AP. > > > > Reported-by: Josh Poimboeuf <jpoimboe@kernel.org> > > Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> > > Link: https://lore.kernel.org/r/20230224185257.o3mcmloei5zqu7wa@treble > > --- > > arch/x86/kernel/cpu/amd.c | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c > > index 380753b14cab..de624c1442c2 100644 > > --- a/arch/x86/kernel/cpu/amd.c > > +++ b/arch/x86/kernel/cpu/amd.c > > @@ -996,6 +996,16 @@ static void init_amd(struct cpuinfo_x86 *c) > > msr_set_bit(MSR_K7_HWCR, MSR_K7_HWCR_IRPERF_EN_BIT); > > > > check_null_seg_clears_base(c); > > + > > + /* > > + * Make sure EFER[AIBRSE - Automatic IBRS Enable] is set. The APs are brought up > > + * using the trampoline code and as part of it, EFER gets prepared there in order > > + * to be replicated onto them. Regardless, set it here again, if not set, to protect > > + * against any future refactoring/code reorganization which might miss setting > > + * this important bit. > > + */ > > + if (cpu_has(c, X86_FEATURE_AUTOIBRS)) > > + msr_set_bit(MSR_EFER, _EFER_AUTOIBRS); > > Is it intended to be set regardless of the spectre_v2 mitigation status? Right, it needs to check spectre_v2_enabled. Also, this code might be a better fit in identify_secondary_cpu() with the other MSR-writing bug-related code.
On Fri, Feb 24, 2023 at 05:32:02PM -0800, Josh Poimboeuf wrote: > > Is it intended to be set regardless of the spectre_v2 mitigation status? > > Right, it needs to check spectre_v2_enabled. Right, I realized this too this morning, while sleeping, so I made me a note on the nightstand to fix it... :-) > Also, this code might be a better fit in identify_secondary_cpu() with > the other MSR-writing bug-related code. Same path: identify_secondary_cpu->identify_cpu->this_cpu->c_init(c)->init_amd Plus, it keeps the vendor code where it belongs. v2 below, still untested. --- From: "Borislav Petkov (AMD)" <bp@alien8.de> Date: Sat, 25 Feb 2023 01:11:31 +0100 Subject: [PATCH] x86/CPU/AMD: Make sure EFER[AIBRSE] is set The AutoIBRS bit gets set only on the BSP as part of determining which mitigation to enable on AMD. Setting on the APs relies on the circumstance that the APs get booted through the trampoline and EFER - the MSR which contains that bit - gets replicated on every AP from the BSP. However, this can change in the future and considering the security implications of this bit not being set on every CPU, make sure it is set by verifying EFER later in the boot process and on every AP. Reported-by: Josh Poimboeuf <jpoimboe@kernel.org> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> Link: https://lore.kernel.org/r/20230224185257.o3mcmloei5zqu7wa@treble --- arch/x86/kernel/cpu/amd.c | 11 +++++++++++ arch/x86/kernel/cpu/bugs.c | 14 ++------------ arch/x86/kernel/cpu/cpu.h | 10 ++++++++++ 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c index 380753b14cab..aba1b43ed6fd 100644 --- a/arch/x86/kernel/cpu/amd.c +++ b/arch/x86/kernel/cpu/amd.c @@ -996,6 +996,17 @@ static void init_amd(struct cpuinfo_x86 *c) msr_set_bit(MSR_K7_HWCR, MSR_K7_HWCR_IRPERF_EN_BIT); check_null_seg_clears_base(c); + + /* + * Make sure EFER[AIBRSE - Automatic IBRS Enable] is set. The APs are brought up + * using the trampoline code and as part of it, EFER gets prepared there in order + * to be replicated onto them. Regardless, set it here again, if not set, to protect + * against any future refactoring/code reorganization which might miss setting + * this important bit. + */ + if (spectre_v2_in_ibrs_mode(spectre_v2_enabled) && + cpu_has(c, X86_FEATURE_AUTOIBRS)) + msr_set_bit(MSR_EFER, _EFER_AUTOIBRS); } #ifdef CONFIG_X86_32 diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c index 4fd43d25b483..407c73d3beb9 100644 --- a/arch/x86/kernel/cpu/bugs.c +++ b/arch/x86/kernel/cpu/bugs.c @@ -784,8 +784,7 @@ static int __init nospectre_v1_cmdline(char *str) } early_param("nospectre_v1", nospectre_v1_cmdline); -static enum spectre_v2_mitigation spectre_v2_enabled __ro_after_init = - SPECTRE_V2_NONE; +enum spectre_v2_mitigation spectre_v2_enabled __ro_after_init = SPECTRE_V2_NONE; #undef pr_fmt #define pr_fmt(fmt) "RETBleed: " fmt @@ -1133,16 +1132,7 @@ spectre_v2_parse_user_cmdline(void) return SPECTRE_V2_USER_CMD_AUTO; } -static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode) -{ - return mode == SPECTRE_V2_IBRS || - mode == SPECTRE_V2_EIBRS || - mode == SPECTRE_V2_EIBRS_RETPOLINE || - mode == SPECTRE_V2_EIBRS_LFENCE; -} - -static void __init -spectre_v2_user_select_mitigation(void) +static void __init spectre_v2_user_select_mitigation(void) { enum spectre_v2_user_mitigation mode = SPECTRE_V2_USER_NONE; bool smt_possible = IS_ENABLED(CONFIG_SMP); diff --git a/arch/x86/kernel/cpu/cpu.h b/arch/x86/kernel/cpu/cpu.h index 57a5349e6954..99c507c42901 100644 --- a/arch/x86/kernel/cpu/cpu.h +++ b/arch/x86/kernel/cpu/cpu.h @@ -83,4 +83,14 @@ unsigned int aperfmperf_get_khz(int cpu); extern void x86_spec_ctrl_setup_ap(void); extern void update_srbds_msr(void); +extern enum spectre_v2_mitigation spectre_v2_enabled; + +static inline bool spectre_v2_in_ibrs_mode(enum spectre_v2_mitigation mode) +{ + return mode == SPECTRE_V2_IBRS || + mode == SPECTRE_V2_EIBRS || + mode == SPECTRE_V2_EIBRS_RETPOLINE || + mode == SPECTRE_V2_EIBRS_LFENCE; +} + #endif /* ARCH_X86_CPU_H */
On Sat, Feb 25, 2023 at 01:21:49PM +0100, Borislav Petkov wrote: > On Fri, Feb 24, 2023 at 05:32:02PM -0800, Josh Poimboeuf wrote: > > > Is it intended to be set regardless of the spectre_v2 mitigation status? > > > > Right, it needs to check spectre_v2_enabled. > > Right, I realized this too this morning, while sleeping, so I made me > a note on the nightstand to fix it... :-) > > > Also, this code might be a better fit in identify_secondary_cpu() with > > the other MSR-writing bug-related code. > > Same path: > > identify_secondary_cpu->identify_cpu->this_cpu->c_init(c)->init_amd > > Plus, it keeps the vendor code where it belongs. All the other "bug" code in identify_secondary_cpu() *is* vendor-specific. And for that matter, so is most of the code in bugs.c. I'm thinking we should just move all this MSR-writing bug-related code into a new cpu_init_bugs() function in bugs.c which can be called by identify_secondary_cpu(). Then we have more "bug" code together and all the local variables/functions like spectre_v2_in_ibrs_mode() can remain local.
On Sat, Feb 25, 2023 at 09:28:32AM -0800, Josh Poimboeuf wrote: > All the other "bug" code in identify_secondary_cpu() *is* > vendor-specific. I meant "vendor-specific" in the sense that AMD code goes to amd.c, etc. As to the identify_secondary_cpu() code - I didn't like it being slapped there either but it got stuck in there hastily during the mitigations upstreaming as back then we had bigger fish to fry than paying too much attention to clean design... > And for that matter, so is most of the code in bugs.c. > > I'm thinking we should just move all this MSR-writing bug-related code > into a new cpu_init_bugs() function in bugs.c which can be called by > identify_secondary_cpu(). I guess. > Then we have more "bug" code together and all the local > variables/functions like spectre_v2_in_ibrs_mode() can remain local. They're still local, more or less. Note the special cpu.h header which is private to arch/x86/kernel/cpu/ Thx.
On Sat, Feb 25, 2023 at 11:56:37PM +0100, Borislav Petkov wrote: > On Sat, Feb 25, 2023 at 09:28:32AM -0800, Josh Poimboeuf wrote: > > All the other "bug" code in identify_secondary_cpu() *is* > > vendor-specific. > > I meant "vendor-specific" in the sense that AMD code goes to amd.c, etc. Hm? So code in bugs.c is not vendor-specific? That seems circular and I don't get your point. > As to the identify_secondary_cpu() code - I didn't like it being > slapped there either but it got stuck in there hastily during the > mitigations upstreaming as back then we had bigger fish to fry than > paying too much attention to clean design... Right, so rather than spreading all the bug-related MSR logic around, just do it in one spot.
On Sat, Feb 25, 2023 at 03:43:30PM -0800, Josh Poimboeuf wrote: > Hm? So code in bugs.c is not vendor-specific? That seems circular and > I don't get your point. Lemme try that again... So there's an obvious benefit of keeping vendor-specific CPU code in one place: Intel stuff in cpu/intel*, AMD stuff in cpu/amd.c The sekjority stuff is still vendor-specific CPU code. Now, if you wanna add a function pointer ->bugs_init or so, say, to struct cpu_dev and keep the respective code in amd.c or intel.c, then we get the best of both worlds: - vendor-specific code remains in the respective file - you have a vendor-specific function which does hw vuln-specific work *without* vendor checks and so on > Right, so rather than spreading all the bug-related MSR logic around, > just do it in one spot. It is all CPU init code and I'm wondering if splitting stuff by vendor wouldn't make all that maze in bugs.c a lot more palatable. And get rid of $ git grep VENDOR arch/x86/kernel/cpu/bugs.c | wc -l 11 those, for starters. There's this trade-off of 1. keeping bugs setup code in one place - but then you need to do vendor checks and the other CPU setup code is somewhere else and it is probably related, MSR_AMD64_DE_CFG_LFENCE_SERIALIZE_BIT in amd.c for example. or 2. separating it into their respective files. Then the respective vendor code is simple because you don't need vendor checks. It would need to be done in a slick way, though, so that it remains maintainable.
On Sun, Feb 26, 2023 at 12:18:06PM +0100, Borislav Petkov wrote: > > Right, so rather than spreading all the bug-related MSR logic around, > > just do it in one spot. > > It is all CPU init code and I'm wondering if splitting stuff by vendor > wouldn't make all that maze in bugs.c a lot more palatable. And get rid > of > > $ git grep VENDOR arch/x86/kernel/cpu/bugs.c | wc -l > 11 > > those, for starters. > > There's this trade-off of > > 1. keeping bugs setup code in one place - but then you need to do vendor > checks and the other CPU setup code is somewhere else and it is > probably related, MSR_AMD64_DE_CFG_LFENCE_SERIALIZE_BIT in amd.c for > example. > > or > > 2. separating it into their respective files. Then the respective vendor > code is simple because you don't need vendor checks. It would need to > be done in a slick way, though, so that it remains maintainable. At least now it's a (mostly) self-contained hornets nest. I'm not sure we want to poke it :-) And I'm not sure spreading the mess around would be an improvement.
On Sun, Feb 26, 2023 at 09:27:26AM -0800, Josh Poimboeuf wrote: > At least now it's a (mostly) self-contained hornets nest. I'm not sure > we want to poke it :-) > > And I'm not sure spreading the mess around would be an improvement. Yah, if anything, I wanna see the change first and it has to be an obvious and good one. What I think we should finish doing, though, is documenting it. Because there are aspects/mitigations that are missing, e.g., there's no retbleed.rst in Documentation/admin-guide/hw-vuln/
On 2/25/23 04:21, Borislav Petkov wrote: > + /* > + * Make sure EFER[AIBRSE - Automatic IBRS Enable] is set. The APs are brought up > + * using the trampoline code and as part of it, EFER gets prepared there in order > + * to be replicated onto them. Regardless, set it here again, if not set, to protect > + * against any future refactoring/code reorganization which might miss setting > + * this important bit. > + */ > + if (spectre_v2_in_ibrs_mode(spectre_v2_enabled) && > + cpu_has(c, X86_FEATURE_AUTOIBRS)) > + msr_set_bit(MSR_EFER, _EFER_AUTOIBRS); > } I guess the belt and suspenders could be justified here by how important the bit is. But, if EFER[AIBRSE] gets clear somehow shouldn't we also dump a warning out here so the fool who botched it can fix it? Even if AIBRSE is fixed up, some less important bit could still be botched. It will freak some users out, but it does seem like the kind of thing we _want_ a bug report for.
On Mon, Feb 27, 2023 at 07:25:00AM -0800, Dave Hansen wrote: > It will freak some users out, but it does seem like the kind of thing we > _want_ a bug report for. You mean, something like: if (spectre_v2_in_ibrs_mode(spectre_v2_enabled) && cpu_has(c, X86_FEATURE_AUTOIBRS)) WARN_ON_ONCE(msr_set_bit(MSR_EFER, _EFER_AUTOIBRS)); ?
On 2/27/23 07:40, Borislav Petkov wrote: > On Mon, Feb 27, 2023 at 07:25:00AM -0800, Dave Hansen wrote: >> It will freak some users out, but it does seem like the kind of thing we >> _want_ a bug report for. > You mean, something like: > > if (spectre_v2_in_ibrs_mode(spectre_v2_enabled) && > cpu_has(c, X86_FEATURE_AUTOIBRS)) > WARN_ON_ONCE(msr_set_bit(MSR_EFER, _EFER_AUTOIBRS)); > > ? Yep, that looks sane.
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c index 380753b14cab..de624c1442c2 100644 --- a/arch/x86/kernel/cpu/amd.c +++ b/arch/x86/kernel/cpu/amd.c @@ -996,6 +996,16 @@ static void init_amd(struct cpuinfo_x86 *c) msr_set_bit(MSR_K7_HWCR, MSR_K7_HWCR_IRPERF_EN_BIT); check_null_seg_clears_base(c); + + /* + * Make sure EFER[AIBRSE - Automatic IBRS Enable] is set. The APs are brought up + * using the trampoline code and as part of it, EFER gets prepared there in order + * to be replicated onto them. Regardless, set it here again, if not set, to protect + * against any future refactoring/code reorganization which might miss setting + * this important bit. + */ + if (cpu_has(c, X86_FEATURE_AUTOIBRS)) + msr_set_bit(MSR_EFER, _EFER_AUTOIBRS); } #ifdef CONFIG_X86_32