x86/CPU/AMD: Make sure EFER[AIBRSE] is set

  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(+)
  

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.