[PATCH] x86/bugs: wrap X86_FEATURE_RSB_CTXSW with ifdef CONFIG_RETPOLINE

[PATCH] x86/bugs: wrap X86_FEATURE_RSB_CTXSW with ifdef CONFIG_RETPOLINE

[Jon Kohler]

The only place X86_FEATURE_RSB_CTXSW is currently in use is in
arch/x86/entry/entry_{32|64}.S, where its use is wrapped with
ifdef CONFIG_RETPOLINE. If someone uses a system with
X86_FEATURE_IBRS_ENHANCED and compiles without CONFIG_RETPOLINE
but still has spectre v2 set to auto, the kernel log will
print that eIBRS is enabled and that RSB stuffing is enabled;
however, that stuffing would never occur.

To make this behavior more clear, wrap the enablement of
X86_FEATURE_RSB_CTXSW and the resulting log message with ifdef
CONFIG_RETPOLINE, such that it is compiled out along with the
actions it controls.

This way seems more correct at first glance as this was the way
the code was originally written in fdf82a7856b; however, when
enhanced IBRS was added, there was a goto added under
SPECTRE_V2_CMD_AUTO which bypasses going through retpoline_auto,
where X86_FEATURE_RETPOLINE is set.

The other option would be to remove the CONFIG_RETPOLINE from
the code in entry_{32|64}.S, such that it would always be
compiled no matter what, such that these two areas match.

Fixes: 706d51681d6 ("x86/speculation: Support Enhanced IBRS on future CPUs")
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: David Woodhouse <dwmw@amazon.co.uk>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Ravi Shankar <ravi.v.shankar@intel.com>
Cc: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Signed-off-by: Jon Kohler <jon@nutanix.com>
---
 arch/x86/kernel/cpu/bugs.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index d41b70fe4918..b72056ee21dd 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -923,6 +923,7 @@ static void __init spectre_v2_select_mitigation(void)
 	spectre_v2_enabled = mode;
 	pr_info("%s\n", spectre_v2_strings[mode]);
 
+#ifdef CONFIG_RETPOLINE
 	/*
 	 * If spectre v2 protection has been enabled, unconditionally fill
 	 * RSB during a context switch; this protects against two independent
@@ -933,6 +934,9 @@ static void __init spectre_v2_select_mitigation(void)
 	 */
 	setup_force_cpu_cap(X86_FEATURE_RSB_CTXSW);
 	pr_info("Spectre v2 / SpectreRSB mitigation: Filling RSB on context switch\n");
+#else
+	pr_info("Spectre v2 / SpectreRSB mitigation: kernel not compiled with retpoline, do not fill RSB on context switch\n");
+#endif
 
 	/*
 	 * Retpoline means the kernel is safe because it has no indirect
-- 
2.30.1 (Apple Git-130)

Re: [PATCH] x86/bugs: wrap X86_FEATURE_RSB_CTXSW with ifdef CONFIG_RETPOLINE

[Dave Hansen]

On 5/7/21 8:53 AM, Jon Kohler wrote:
> The only place X86_FEATURE_RSB_CTXSW is currently in use is in
> arch/x86/entry/entry_{32|64}.S, where its use is wrapped with
> ifdef CONFIG_RETPOLINE. If someone uses a system with
> X86_FEATURE_IBRS_ENHANCED and compiles without CONFIG_RETPOLINE
> but still has spectre v2 set to auto, the kernel log will
> print that eIBRS is enabled and that RSB stuffing is enabled;
> however, that stuffing would never occur.
> 
> To make this behavior more clear, wrap the enablement of
> X86_FEATURE_RSB_CTXSW and the resulting log message with ifdef
> CONFIG_RETPOLINE, such that it is compiled out along with the
> actions it controls.
> 
> This way seems more correct at first glance as this was the way
> the code was originally written in fdf82a7856b; however, when
> enhanced IBRS was added, there was a goto added under
> SPECTRE_V2_CMD_AUTO which bypasses going through retpoline_auto,
> where X86_FEATURE_RETPOLINE is set.
> 
> The other option would be to remove the CONFIG_RETPOLINE from
> the code in entry_{32|64}.S, such that it would always be
> compiled no matter what, such that these two areas match.

This kinda dances around the real issue: Does RSB stuffing have
mitigation value on enhanced IBRS systems?

If yes, then we should make the RSB stuffing code in entry*.S available
separately from CONFIG_RETPOLINE.

If no, is it because eIBRS systems are not vulnerable, or because RSB
stuffing has no mitigation value?

Either way, I'm not sure the approach in this patch is the one we want.

Re: [PATCH] x86/bugs: wrap X86_FEATURE_RSB_CTXSW with ifdef CONFIG_RETPOLINE

[Jon Kohler]

> On May 7, 2021, at 12:02 PM, Dave Hansen <dave.hansen@intel.com> wrote:
> 
> On 5/7/21 8:53 AM, Jon Kohler wrote:
>> The only place X86_FEATURE_RSB_CTXSW is currently in use is in
>> arch/x86/entry/entry_{32|64}.S, where its use is wrapped with
>> ifdef CONFIG_RETPOLINE. If someone uses a system with
>> X86_FEATURE_IBRS_ENHANCED and compiles without CONFIG_RETPOLINE
>> but still has spectre v2 set to auto, the kernel log will
>> print that eIBRS is enabled and that RSB stuffing is enabled;
>> however, that stuffing would never occur.
>> 
>> To make this behavior more clear, wrap the enablement of
>> X86_FEATURE_RSB_CTXSW and the resulting log message with ifdef
>> CONFIG_RETPOLINE, such that it is compiled out along with the
>> actions it controls.
>> 
>> This way seems more correct at first glance as this was the way
>> the code was originally written in fdf82a7856b; however, when
>> enhanced IBRS was added, there was a goto added under
>> SPECTRE_V2_CMD_AUTO which bypasses going through retpoline_auto,
>> where X86_FEATURE_RETPOLINE is set.
>> 
>> The other option would be to remove the CONFIG_RETPOLINE from
>> the code in entry_{32|64}.S, such that it would always be
>> compiled no matter what, such that these two areas match.
> 
> This kinda dances around the real issue: Does RSB stuffing have
> mitigation value on enhanced IBRS systems?
> 
> If yes, then we should make the RSB stuffing code in entry*.S available
> separately from CONFIG_RETPOLINE.
> 
> If no, is it because eIBRS systems are not vulnerable, or because RSB
> stuffing has no mitigation value?
> 
> Either way, I'm not sure the approach in this patch is the one we want.

Thanks, Dave, for the prompt response and thats a great question. 

The way the existing documentation reads in bugs.c, all Sklake+
processors are affected by this, so I'd lean towards "yes" to
your question; however, would love the authorative view from Intel
guru's on this, as RSB stuffing is a bit expensive, would be lovely
to drop it on eIBRS systems! :)

In running down the idea for this patch, I did look at Intel's 
"Affected Processors: Transient Execution Attacks & Related Security
Issues by CPU" guide, interestingly enough the CVE for spectre RSB
(v5 / 2018-15572) isn't listed, so I had to depend on what was written
in the various previous commits around this topic.

Anyhow, happy to take up a different approach depending on what the
powers that be at Intel say to needing RSB stuffing on eIBRS. In
addition to the chatter here, I'll see if I can poke around more on
my own as well.

Thanks again,
Jon