[patch RFC 0/5] x86/spectre_v2: Initial integration of IBRS into the spectre_v2 mechanics

[patch RFC 0/5] x86/spectre_v2: Initial integration of IBRS into the spectre_v2 mechanics

[Thomas Gleixner]

I started to look at the IBRS series and this thing really needs a thorough
treatment with a steam blaster.

So I started that effort by integrating it proper into the existing spectre_v2
command line parsing and mitigation method selection.

It's not having any of the actual IBRS bits yet and is completely untested,
but I wanted to send it out before falling completely asleep so the folks
still awake can take a look and comment or expand/modify.

The first patch is from Boris to simplify the cpu feature bits sync after a
late microcode load, but it's not yet used. I just added it for reference
so people can see where this is heading.

I'll continue on that tomorrow morning after picking up a few patches here
and there.

Applies on top of

  git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/pti

Thanks,

	tglx

[patch RFC 1/5] x86/CPU: Sync CPU feature flags late

[Thomas Gleixner]

[-- Attachment #1: x86-CPU--Sync_CPU_feature_flags_late.patch --]
[-- Type: text/plain, Size: 1396 bytes --]

From: Borislav Petkov <bp@suse.de>

This is for the case where we need to set feature flags late, like, for
example, after late microcode patch has been loaded which has enabled
new CPUID bits.

This has no effect on alternatives patching.

Signed-off-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>

---
 arch/x86/kernel/cpu/common.c |   17 +++++++++++++++++
 arch/x86/kernel/cpu/cpu.h    |    3 +++
 2 files changed, 20 insertions(+)

--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -724,6 +724,23 @@ static void apply_forced_caps(struct cpu
 	}
 }
 
+/*
+ * This late synchronization of CPU caps has no effect on alternatives patching
+ * but updates the visible feature bits per CPU.
+ */
+void cpu_caps_sync_late(void)
+{
+	int cpu;
+
+	lockdep_assert_cpus_held();
+
+	for_each_online_cpu(cpu) {
+		struct cpuinfo_x86 *c = &cpu_data(cpu);
+
+		apply_forced_caps(c);
+	}
+}
+
 void get_cpu_cap(struct cpuinfo_x86 *c)
 {
 	u32 eax, ebx, ecx, edx;
--- a/arch/x86/kernel/cpu/cpu.h
+++ b/arch/x86/kernel/cpu/cpu.h
@@ -46,5 +46,8 @@ extern const struct cpu_dev *const __x86
 			    *const __x86_cpu_dev_end[];
 
 extern void get_cpu_cap(struct cpuinfo_x86 *c);
+
+extern void cpu_caps_sync_late(void);
+
 extern void cpu_detect_cache_sizes(struct cpuinfo_x86 *c);
 #endif /* ARCH_X86_CPU_H */

Re: [patch RFC 1/5] x86/CPU: Sync CPU feature flags late

[Dave Hansen]

On 01/09/2018 05:06 PM, Thomas Gleixner wrote:
> This is for the case where we need to set feature flags late, like, for
> example, after late microcode patch has been loaded which has enabled
> new CPUID bits.
> 
> This has no effect on alternatives patching.

In other words, if you use late microcode loading for getting IBRS, you
don't get ALTERNATIVE patching and its benefits?

I'll also profess some microcode ignorance here.  Is "late microcode
patching" *all* of the stuff we do from the OS, or do we have early and
late Linux loading in addition to what the BIOS can do?

RE: [patch RFC 1/5] x86/CPU: Sync CPU feature flags late

[Van De Ven, Arjan]

 
> In other words, if you use late microcode loading for getting IBRS, you
> don't get ALTERNATIVE patching and its benefits?
> 
> I'll also profess some microcode ignorance here.  Is "late microcode
> patching" *all* of the stuff we do from the OS, or do we have early and
> late Linux loading in addition to what the BIOS can do?

the early boot loader level stuff is much better generally (but does not work when the microcode comes out after the system booted... like really long uptimes)

RE: [patch RFC 1/5] x86/CPU: Sync CPU feature flags late

[Thomas Gleixner]

On Wed, 10 Jan 2018, Van De Ven, Arjan wrote:
> > In other words, if you use late microcode loading for getting IBRS, you
> > don't get ALTERNATIVE patching and its benefits?
> > 
> > I'll also profess some microcode ignorance here.  Is "late microcode
> > patching" *all* of the stuff we do from the OS, or do we have early and
> > late Linux loading in addition to what the BIOS can do?
> 
> the early boot loader level stuff is much better generally (but does not
> work when the microcode comes out after the system booted... like really
> long uptimes)

That stuff indeed would be way simpler w/o the late support, but the fact
that the microcode for this might reach the user way later than the kernel
support makes it almost a must to support the late loading.

Thanks,

	tglx

Re: [patch RFC 1/5] x86/CPU: Sync CPU feature flags late

[Andy Lutomirski]

> On Jan 9, 2018, at 5:47 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> On Wed, 10 Jan 2018, Van De Ven, Arjan wrote:
>>> In other words, if you use late microcode loading for getting IBRS, you
>>> don't get ALTERNATIVE patching and its benefits?
>>> 
>>> I'll also profess some microcode ignorance here.  Is "late microcode
>>> patching" *all* of the stuff we do from the OS, or do we have early and
>>> late Linux loading in addition to what the BIOS can do?
>> 
>> the early boot loader level stuff is much better generally (but does not
>> work when the microcode comes out after the system booted... like really
>> long uptimes)
> 
> That stuff indeed would be way simpler w/o the late support, but the fact
> that the microcode for this might reach the user way later than the kernel
> support makes it almost a must to support the late loading.

How hard would it be to add a late alternative feature?  Concretely, we'd have a list of "late" cpufeatures.  When we scan the alternative list, if we find a late feature, we copy it to some other list that isn't discarded, and we also copy its replacement (and relocate it eagerly, since we'll lose the offset).

Re: [patch RFC 1/5] x86/CPU: Sync CPU feature flags late

[Thomas Gleixner]

On Tue, 9 Jan 2018, Andy Lutomirski wrote:
> > On Jan 9, 2018, at 5:47 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > On Wed, 10 Jan 2018, Van De Ven, Arjan wrote:
> >>> In other words, if you use late microcode loading for getting IBRS, you
> >>> don't get ALTERNATIVE patching and its benefits?
> >>> 
> >>> I'll also profess some microcode ignorance here.  Is "late microcode
> >>> patching" *all* of the stuff we do from the OS, or do we have early and
> >>> late Linux loading in addition to what the BIOS can do?
> >> 
> >> the early boot loader level stuff is much better generally (but does not
> >> work when the microcode comes out after the system booted... like really
> >> long uptimes)
> > 
> > That stuff indeed would be way simpler w/o the late support, but the fact
> > that the microcode for this might reach the user way later than the kernel
> > support makes it almost a must to support the late loading.
> 

> How hard would it be to add a late alternative feature?  Concretely, we'd
> have a list of "late" cpufeatures.  When we scan the alternative list, if
> we find a late feature, we copy it to some other list that isn't
> discarded, and we also copy its replacement (and relocate it eagerly,
> since we'll lose the offset).

It shouldn't be rocket science, but that's not for now. And yes, we should
do it.

Thanks,

	tglx

Re: [patch RFC 1/5] x86/CPU: Sync CPU feature flags late

[Thomas Gleixner]

On Tue, 9 Jan 2018, Dave Hansen wrote:

> On 01/09/2018 05:06 PM, Thomas Gleixner wrote:
> > This is for the case where we need to set feature flags late, like, for
> > example, after late microcode patch has been loaded which has enabled
> > new CPUID bits.
> > 
> > This has no effect on alternatives patching.
> 
> In other words, if you use late microcode loading for getting IBRS, you
> don't get ALTERNATIVE patching and its benefits?
> 
> I'll also profess some microcode ignorance here.  Is "late microcode
> patching" *all* of the stuff we do from the OS, or do we have early and
> late Linux loading in addition to what the BIOS can do?

IBRS wont use alternatives for that and various other reasons. It has to be
static key based so we can patch it late

Thanks,

	tglx

Re: [patch RFC 1/5] x86/CPU: Sync CPU feature flags late

[Ingo Molnar]

* Dave Hansen <dave.hansen@intel.com> wrote:

> On 01/09/2018 05:06 PM, Thomas Gleixner wrote:
> > This is for the case where we need to set feature flags late, like, for
> > example, after late microcode patch has been loaded which has enabled
> > new CPUID bits.
> > 
> > This has no effect on alternatives patching.
> 
> In other words, if you use late microcode loading for getting IBRS, you
> don't get ALTERNATIVE patching and its benefits?
> 
> I'll also profess some microcode ignorance here.  Is "late microcode
> patching" *all* of the stuff we do from the OS, or do we have early and
> late Linux loading in addition to what the BIOS can do?

So would it be really unreasonable to say that if a microcode update changes CPU 
flags an initrd rebuild and a reboot is required? It's not like microcode updates 
are _that_ frequent - in fact they tend to be much _less_ frequent in a system's 
life time than kernel updates.

So all of this 'late loading' and CPU flag splitting complexity seems unnecessary 
to me: we should be glad we do early microcode loading now, and should embrace it.

Changing CPU features way after the CPU has booted up is possible, and we could in 
theory extend code patching to work 'late' as well, but given how infrequent all 
this is bound to be in practice I fear it's all going to be a big, seldom tested, 
often broken mess, with no real benefit to users.

Thanks,

	Ingo

Re: [patch RFC 1/5] x86/CPU: Sync CPU feature flags late

[Borislav Petkov]

On Wed, Jan 10, 2018 at 07:20:13AM +0100, Ingo Molnar wrote:
> it be really unreasonable to say that if a microcode update changes CPU 
> flags an initrd rebuild and a reboot is required? It's not like microcode updates 
> are _that_ frequent - in fact they tend to be much _less_ frequent in a system's 
> life time than kernel updates.
> 
> So all of this 'late loading' and CPU flag splitting complexity seems unnecessary 
> to me: we should be glad we do early microcode loading now, and should embrace it.
> 
> Changing CPU features way after the CPU has booted up is possible, and we could in 
> theory extend code patching to work 'late' as well, but given how infrequent all 
> this is bound to be in practice I fear it's all going to be a big, seldom tested, 
> often broken mess, with no real benefit to users.

Agreed: we support that late patching for those use cases where machines
run for a long time, simulating all kinds of crap. And frankly, if
those things need to get IBRS all of a sudden and *not* reboot, then
something's wrong with the whole contraption setup.

So yes, I'd vote too for supporting only early IBRS and not do the late
thing now. Maybe later, if there's, like, a really compelling use case.

I will have to do the late thing for our old kernels which don't have
early loading but that would be a one-off and my problem.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [patch RFC 1/5] x86/CPU: Sync CPU feature flags late

[Thomas Gleixner]

On Wed, 10 Jan 2018, Borislav Petkov wrote:
> On Wed, Jan 10, 2018 at 07:20:13AM +0100, Ingo Molnar wrote:
> > it be really unreasonable to say that if a microcode update changes CPU 
> > flags an initrd rebuild and a reboot is required? It's not like microcode updates 
> > are _that_ frequent - in fact they tend to be much _less_ frequent in a system's 
> > life time than kernel updates.
> > 
> > So all of this 'late loading' and CPU flag splitting complexity seems unnecessary 
> > to me: we should be glad we do early microcode loading now, and should embrace it.
> > 
> > Changing CPU features way after the CPU has booted up is possible, and we could in 
> > theory extend code patching to work 'late' as well, but given how infrequent all 
> > this is bound to be in practice I fear it's all going to be a big, seldom tested, 
> > often broken mess, with no real benefit to users.
> 
> Agreed: we support that late patching for those use cases where machines
> run for a long time, simulating all kinds of crap. And frankly, if
> those things need to get IBRS all of a sudden and *not* reboot, then
> something's wrong with the whole contraption setup.
> 
> So yes, I'd vote too for supporting only early IBRS and not do the late
> thing now. Maybe later, if there's, like, a really compelling use case.

/me exposes it to the flame-thrower

[patch RFC 2/5] x86/spectre: Simplify spectre code a bit

[Thomas Gleixner]

[-- Attachment #1: x86-spectre--Simplify-spectre-code-a-bit.patch --]
[-- Type: text/plain, Size: 2300 bytes --]

Make the sysfs print use a string array, use __defined(RETPOLINE) instead
of the ugly ifdefs.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/bugs.c |   31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -78,10 +78,18 @@ enum spectre_v2_mitigation {
 	SPECTRE_V2_RETPOLINE_AMD,
 };
 
+static const char *spectre_v2_strings[] = {
+	[SPECTRE_V2_NONE]			= "Vulnerable",
+	[SPECTRE_V2_RETPOLINE_MINIMAL]		= "Mitigation: Minimal generic ASM retpoline",
+	[SPECTRE_V2_RETPOLINE_MINIMAL_AMD]	= "Mitigation: Minimal AMD ASM retpoline",
+	[SPECTRE_V2_RETPOLINE_GENERIC]		= "Mitigation: Full generic retpoline",
+	[SPECTRE_V2_RETPOLINE_AMD]		= "Mitigation: Full AMD retpoline",
+};
+
 #undef pr_fmt
 #define pr_fmt(fmt)     "Spectre V2 mitigation: " fmt
 
-static int spectre_v2_enabled = SPECTRE_V2_NONE;
+static enum spectre_v2_mitigation spectre_v2_enabled = SPECTRE_V2_NONE;
 
 static void __init spec2_print_if_insecure(const char *reason)
 {
@@ -97,11 +105,7 @@ static void __init spec2_print_if_secure
 
 static inline bool retp_compiler(void)
 {
-#ifdef RETPOLINE
-	return true;
-#else
-	return false;
-#endif
+	return __is_defined(RETPOLINE);
 }
 
 static inline bool match_option(const char *arg, int arglen, const char *opt)
@@ -182,6 +186,8 @@ static void __init spectre_v2_check_boot
 	return;
 }
 
+#undef pr_fmt
+
 #ifdef CONFIG_SYSFS
 ssize_t cpu_show_meltdown(struct device *dev,
 			  struct device_attribute *attr, char *buf)
@@ -207,17 +213,6 @@ ssize_t cpu_show_spectre_v2(struct devic
 	if (!boot_cpu_has_bug(X86_BUG_SPECTRE_V2))
 		return sprintf(buf, "Not affected\n");
 
-	switch (spectre_v2_enabled) {
-	case SPECTRE_V2_RETPOLINE_MINIMAL:
-		return sprintf(buf, "Mitigation: Minimal generic ASM retpoline\n");
-	case SPECTRE_V2_RETPOLINE_MINIMAL_AMD:
-		return sprintf(buf, "Mitigation: Minimal AMD ASM retpoline\n");
-	case SPECTRE_V2_RETPOLINE_GENERIC:
-		return sprintf(buf, "Mitigation: Full generic retpoline\n");
-	case SPECTRE_V2_RETPOLINE_AMD:
-		return sprintf(buf, "Mitigation: Full AMD retpoline\n");
-	default:
-		return sprintf(buf, "Vulnerable\n");
-	}
+	return sprintf(buf, "%s\n", spectre_v2_strings[spectre_v2_enabled]);
 }
 #endif

Re: [patch RFC 2/5] x86/spectre: Simplify spectre code a bit

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> Make the sysfs print use a string array, use __defined(RETPOLINE) instead
> of the ugly ifdefs.

Capitalization fix for the title:

s/spectre
 /Spectre

Reviewed-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

[patch RFC 3/5] x86/spectre: Prepare for IBRS selection

[Thomas Gleixner]

[-- Attachment #1: x86-spectre--Prepare-for-IBRS-selection.patch --]
[-- Type: text/plain, Size: 7004 bytes --]

IBRS (Indirect Branch Restricted Speculation) is another mitigation method
for Spectre V2. So it needs to be integrated into the command line options.

That would be simple, but the new microcode which provides the
functionality is not widely deployed and gets potentially loaded late, so
the selection process might have to be repeated after micro code has loaded.

Split the command line parsing and the actual selection of the mitigation
method into two parts to prepare for IBRS support.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/nospec-branch.h |   13 +++
 arch/x86/kernel/cpu/bugs.c           |  133 +++++++++++++++++++++++------------
 2 files changed, 101 insertions(+), 45 deletions(-)

--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -152,5 +152,18 @@
 # define THUNK_TARGET(addr) [thunk_target] "rm" (addr)
 #endif
 
+/* The Spectre V2 mitigation variants */
+enum spectre_v2_mitigation {
+	SPECTRE_V2_NONE,
+	SPECTRE_V2_RETPOLINE_MINIMAL,
+	SPECTRE_V2_RETPOLINE_MINIMAL_AMD,
+	SPECTRE_V2_RETPOLINE_GENERIC,
+	SPECTRE_V2_RETPOLINE_AMD,
+	SPECTRE_V2_IBRS,
+};
+
+enum spectre_v2_mitigation spectre_v2_enabled;
+void spectre_v2_select_mitigation(void);
+
 #endif /* __ASSEMBLY__ */
 #endif /* __NOSPEC_BRANCH_H__ */
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -37,6 +37,7 @@ void __init check_bugs(void)
 
 	/* Select the proper spectre mitigation before patching alternatives */
 	spectre_v2_check_boottime_disable();
+	spectre_v2_select_mitigation();
 
 #ifdef CONFIG_X86_32
 	/*
@@ -70,12 +71,14 @@ void __init check_bugs(void)
 #endif
 }
 
-enum spectre_v2_mitigation {
-	SPECTRE_V2_NONE,
-	SPECTRE_V2_RETPOLINE_MINIMAL,
-	SPECTRE_V2_RETPOLINE_MINIMAL_AMD,
-	SPECTRE_V2_RETPOLINE_GENERIC,
-	SPECTRE_V2_RETPOLINE_AMD,
+/* The kernel command line selection */
+enum spectre_v2_mitigation_cmd {
+	SPECTRE_V2_CMD_NONE,
+	SPECTRE_V2_CMD_AUTO,
+	SPECTRE_V2_CMD_FORCE,
+	SPECTRE_V2_CMD_RETPOLINE,
+	SPECTRE_V2_CMD_RETPOLINE_GENERIC,
+	SPECTRE_V2_CMD_RETPOLINE_AMD,
 };
 
 static const char *spectre_v2_strings[] = {
@@ -89,7 +92,8 @@ static const char *spectre_v2_strings[]
 #undef pr_fmt
 #define pr_fmt(fmt)     "Spectre V2 mitigation: " fmt
 
-static enum spectre_v2_mitigation spectre_v2_enabled = SPECTRE_V2_NONE;
+static enum spectre_v2_mitigation_cmd spectre_v2_cmdline = SPECTRE_V2_CMD_AUTO;
+enum spectre_v2_mitigation spectre_v2_enabled = SPECTRE_V2_NONE;
 
 static void __init spec2_print_if_insecure(const char *reason)
 {
@@ -124,66 +128,105 @@ static void __init spectre_v2_check_boot
 				  sizeof(arg));
 	if (ret > 0)  {
 		if (match_option(arg, ret, "off")) {
-			spec2_print_if_insecure("disabled on command line.");
 			goto disable;
 		} else if (match_option(arg, ret, "on")) {
 			spec2_print_if_secure("force enabled on command line.");
-			goto force;
+			spectre_v2_cmdline = SPECTRE_V2_CMD_FORCE;
+			return;
 		} else if (match_option(arg, ret, "retpoline")) {
 			spec2_print_if_insecure("retpoline selected on command line.");
-			goto retpoline;
+			spectre_v2_cmdline = SPECTRE_V2_CMD_RETPOLINE;
+			return;
 		} else if (match_option(arg, ret, "retpoline,amd")) {
+			if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
+				pr_err("retpoline,amd selected but CPU is not AMD. Switching to AUTO select\n");
+				return;
+			}
 			spec2_print_if_insecure("AMD retpoline selected on command line.");
-			goto retpoline_amd;
+			spectre_v2_cmdline = SPECTRE_V2_CMD_RETPOLINE_AMD;
+			return;
 		} else if (match_option(arg, ret, "retpoline,generic")) {
 			spec2_print_if_insecure("generic retpoline selected on command line.");
-			goto retpoline_generic;
+			spectre_v2_cmdline = SPECTRE_V2_CMD_RETPOLINE_GENERIC;
+			return;
 		} else if (match_option(arg, ret, "auto")) {
-			goto autosel;
+			return;
 		}
 	}
 
-	if (cmdline_find_option_bool(boot_command_line, "nospectre_v2")) {
-		spec2_print_if_insecure("disabled on command line.");
-		goto disable;
-	}
+	if (!cmdline_find_option_bool(boot_command_line, "nospectre_v2"))
+		return;
+disable:
+	spec2_print_if_insecure("disabled on command line.");
+	spectre_v2_cmdline = SPECTRE_V2_CMD_NONE;
+}
 
-autosel:
-	if (!boot_cpu_has_bug(X86_BUG_SPECTRE_V2))
-		goto disable;
+void spectre_v2_select_mitigation(void)
+{
+	enum spectre_v2_mitigation mode = SPECTRE_V2_NONE;
 
-force:
-#ifdef CONFIG_RETPOLINE
-retpoline:
+	/*
+	 * If the CPU is not affected and the command line mode is NONE or AUTO
+	 * then nothing to do.
+	 */
+	if (!boot_cpu_has_bug(X86_BUG_SPECTRE_V2) &&
+	    (spectre_v2_cmdline == SPECTRE_V2_CMD_NONE ||
+	     spectre_v2_cmdline == SPECTRE_V2_CMD_AUTO))
+		return;
+
+	switch (spectre_v2_cmdline) {
+	case SPECTRE_V2_CMD_NONE:
+		return;
+
+	case SPECTRE_V2_CMD_FORCE:
+		/* FALLTRHU */
+	case SPECTRE_V2_CMD_AUTO:
+		goto retpoline_auto;
+
+	case SPECTRE_V2_CMD_RETPOLINE_AMD:
+		if (IS_ENABLED(CONFIG_RETPOLINE))
+			goto retpoline_amd;
+		break;
+	case SPECTRE_V2_CMD_RETPOLINE_GENERIC:
+		if (IS_ENABLED(CONFIG_RETPOLINE))
+			goto retpoline_generic;
+		break;
+	case SPECTRE_V2_CMD_RETPOLINE:
+		if (IS_ENABLED(CONFIG_RETPOLINE))
+			goto retpoline_auto;
+		break;
+	}
+	pr_err("kernel not compiled with retpoline; no mitigation available!");
+	return;
+
+retpoline_auto:
+	/*
+	 * Note: This only takes effect during boot before alternatives
+	 * have been patched. Later invocations after late micro code
+	 * update wont change the boot selection even if IBRS has not been
+	 * made available by the microcode loading.
+	 */
 	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
 	retpoline_amd:
-		if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD ||
-		    !boot_cpu_has(X86_FEATURE_LFENCE_RDTSC)) {
-			pr_info("AMD retpoline not supported, fall back to generic\n");
+		if (!boot_cpu_has(X86_FEATURE_LFENCE_RDTSC)) {
+			pr_info("LFENCE not serializing, using to generic mode\n");
 			goto retpoline_generic;
 		}
-
-		spectre_v2_enabled = retp_compiler() ?
-			SPECTRE_V2_RETPOLINE_AMD : SPECTRE_V2_RETPOLINE_MINIMAL_AMD;
+		mode = retp_compiler() ? SPECTRE_V2_RETPOLINE_AMD :
+					 SPECTRE_V2_RETPOLINE_MINIMAL_AMD;
 		setup_force_cpu_cap(X86_FEATURE_RETPOLINE_AMD);
 		setup_force_cpu_cap(X86_FEATURE_RETPOLINE);
-		return;
+	} else {
+	retpoline_generic:
+		mode = retp_compiler() ? SPECTRE_V2_RETPOLINE_GENERIC :
+					 SPECTRE_V2_RETPOLINE_MINIMAL;
+		setup_force_cpu_cap(X86_FEATURE_RETPOLINE);
 	}
-retpoline_generic:
-	spectre_v2_enabled = retp_compiler() ?
-		SPECTRE_V2_RETPOLINE_GENERIC : SPECTRE_V2_RETPOLINE_MINIMAL;
-	setup_force_cpu_cap(X86_FEATURE_RETPOLINE);
-	return;
-#else
-retpoline:
-retpoline_amd:
-retpoline_generic:
-	pr_err("kernel not compiled with retpoline; no mitigation available!");
-#endif
-disable:
-	setup_clear_cpu_cap(X86_FEATURE_RETPOLINE);
-	setup_clear_cpu_cap(X86_FEATURE_RETPOLINE_AMD);
-	return;
+
+	if (spectre_v2_enabled == mode)
+		return;
+	spectre_v2_enabled = mode;
+	pr_info("%s\n", spectre_v2_strings[mode]);
 }
 
 #undef pr_fmt

Re: [patch RFC 3/5] x86/spectre: Prepare for IBRS selection

[Dave Hansen]

On 01/09/2018 05:06 PM, Thomas Gleixner wrote:
...
> +	case SPECTRE_V2_CMD_RETPOLINE:
> +		if (IS_ENABLED(CONFIG_RETPOLINE))
> +			goto retpoline_auto;
> +		break;
> +	}
> +	pr_err("kernel not compiled with retpoline; no mitigation available!");
> +	return;
> +
> +retpoline_auto:
> +	/*
> +	 * Note: This only takes effect during boot before alternatives
> +	 * have been patched. Later invocations after late micro code
> +	 * update wont change the boot selection even if IBRS has not been
> +	 * made available by the microcode loading.
> +	 */
>  	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
>  	retpoline_amd:

Should the "IBRS has not been" be "IBRS has been"?

[patch RFC 4/5] x86/cpufeatures: Detect Speculation control feature

[Thomas Gleixner]

[-- Attachment #1: x86-feature--Detect_the_x86_IBRS_feature_to_control_Speculation.patch --]
[-- Type: text/plain, Size: 3504 bytes --]

From: Tim Chen <tim.c.chen@linux.intel.com>

CPUs can expose a MSR to control speculation. The initial function of this
MSR is to control Indirect Branch Speculation, which is required to
mitigate the Spectre_V2 attack on certain CPU generations.

If CPUID(7).RDX[26] is set then MSR_IA32_SPEC_CTRL (0x48) is available and
bit 0 of that MSR controls whether Indirect Branch Speculation is
restricted or not. The control bit is named IBRS (Indirect Branch
Restricted Speculation). The IBSR bit can be unconditionally set to 1
without clearing it before.

If IBRS is set, near returns and near indirect jumps/calls will not allow
their predicted target address to be controlled by code that executed in a
less privileged prediction mode before the IBRS mode was last written with
a value of 1 or on another logical processor so long as all Return Stack
Buffer (RSB) entries from the previous less privileged prediction mode are
overwritten.

Thus a near indirect jump/call/return may be affected by code in a less
privileged prediction mode that executed AFTER IBRS mode was last written
with a value of 1.

Code executed by a sibling logical processor cannot control indirect
jump/call/return predicted target when IBRS is set

IBRS is not required in order to isolate branch predictions for SMM or SGX
enclaves.

Enabling IBRS can cause a measurable and depending on the workload
significant CPU performance penalty.

[ tglx: Steam blastered changelog ]

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
Cc: Dave Hansen <dave.hansen@intel.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Arjan Van De Ven <arjan.van.de.ven@intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: David Woodhouse <dwmw@amazon.co.uk>

---
 arch/x86/include/asm/cpufeatures.h |    1 +
 arch/x86/include/asm/msr-index.h   |    4 ++++
 arch/x86/kernel/cpu/scattered.c    |    1 +
 3 files changed, 6 insertions(+)

--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -211,6 +211,7 @@
 #define X86_FEATURE_AVX512_4FMAPS	( 7*32+17) /* AVX-512 Multiply Accumulation Single precision */
 
 #define X86_FEATURE_MBA			( 7*32+18) /* Memory Bandwidth Allocation */
+#define X86_FEATURE_SPEC_CTRL		( 7*32+19) /* Speculation Control */
 
 /* Virtualization flags: Linux defined, word 8 */
 #define X86_FEATURE_TPR_SHADOW		( 8*32+ 0) /* Intel TPR Shadow */
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -42,6 +42,10 @@
 #define MSR_PPIN_CTL			0x0000004e
 #define MSR_PPIN			0x0000004f
 
+#define MSR_IA32_SPEC_CTRL		0x00000048
+#define SPEC_CTRL_DISABLE_IBRS		(0UL << 0)
+#define SPEC_CTRL_ENABLE_IBRS		(1UL << 0)
+
 #define MSR_IA32_PERFCTR0		0x000000c1
 #define MSR_IA32_PERFCTR1		0x000000c2
 #define MSR_FSB_FREQ			0x000000cd
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -24,6 +24,7 @@ static const struct cpuid_bit cpuid_bits
 	{ X86_FEATURE_INTEL_PT,		CPUID_EBX, 25, 0x00000007, 0 },
 	{ X86_FEATURE_AVX512_4VNNIW,    CPUID_EDX,  2, 0x00000007, 0 },
 	{ X86_FEATURE_AVX512_4FMAPS,    CPUID_EDX,  3, 0x00000007, 0 },
+	{ X86_FEATURE_SPEC_CTRL,	CPUID_EDX, 26, 0x00000007, 0 },
 	{ X86_FEATURE_CAT_L3,		CPUID_EBX,  1, 0x00000010, 0 },
 	{ X86_FEATURE_CAT_L2,		CPUID_EBX,  2, 0x00000010, 0 },
 	{ X86_FEATURE_CDP_L3,		CPUID_ECX,  2, 0x00000010, 1 },

Re: [patch RFC 4/5] x86/cpufeatures: Detect Speculation control feature

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> From: Tim Chen <tim.c.chen@linux.intel.com>
> 
> CPUs can expose a MSR to control speculation. The initial function of this
> MSR is to control Indirect Branch Speculation, which is required to
> mitigate the Spectre_V2 attack on certain CPU generations.

s/a MSR
 /an MSR

> If CPUID(7).RDX[26] is set then MSR_IA32_SPEC_CTRL (0x48) is available and
> bit 0 of that MSR controls whether Indirect Branch Speculation is
> restricted or not. The control bit is named IBRS (Indirect Branch
> Restricted Speculation). The IBSR bit can be unconditionally set to 1
> without clearing it before.

Argh for inverted logic: why was the control bit defined for a _negated_ value, 
i.e. why does '0' mean "don't don't speculate"?

And yes, I know what's behind it: this way 'IBRS' can be called a 'mitigation 
feature' that can be 'enabled', instead of calling it a 'broken CPU feature 
feature' that has to be disabled ...

That's nonsense that causes confusion to no end:

> If IBRS is set, near returns and near indirect jumps/calls will not allow
> their predicted target address to be controlled by code that executed in a
> less privileged prediction mode before the IBRS mode was last written with
> a value of 1 or on another logical processor so long as all Return Stack
> Buffer (RSB) entries from the previous less privileged prediction mode are
> overwritten.
> 
> Thus a near indirect jump/call/return may be affected by code in a less
> privileged prediction mode that executed AFTER IBRS mode was last written
> with a value of 1.
> 
> Code executed by a sibling logical processor cannot control indirect
> jump/call/return predicted target when IBRS is set
> 
> IBRS is not required in order to isolate branch predictions for SMM or SGX
> enclaves.
> 
> Enabling IBRS can cause a measurable and depending on the workload
> significant CPU performance penalty.

This is bound to be really confusing due to the logic negation, in particular:

> +#define SPEC_CTRL_DISABLE_IBRS		(0UL << 0)
> +#define SPEC_CTRL_ENABLE_IBRS		(1UL << 0)

"SPEC_CTRL_ENABLE_IBRS" will _disable_ speculation!

Then that brokenness is propagated into higher code as well by the next patch, 
i.e. "SPECTRE_V2_CMD_IBRS" et al.

This is totally brain-dead and should be inverted to follow natural logic instead. 
The lowest level hardware ABI will obviously stay broken,

Why not define a sane name instead? Something like:

	CTRL_DISABLE_BR_SPECULATION
	CTRL_ENABLE_BR_SPECULATION

and only explain the broken negated Intel naming and flag once in the comments for 
the definition and the lowest level MSR write.

Thanks,

	Ingo

Re: [patch RFC 4/5] x86/cpufeatures: Detect Speculation control feature

[Thomas Gleixner]

On Wed, 10 Jan 2018, Ingo Molnar wrote:
> > If CPUID(7).RDX[26] is set then MSR_IA32_SPEC_CTRL (0x48) is available and
> > bit 0 of that MSR controls whether Indirect Branch Speculation is
> > restricted or not. The control bit is named IBRS (Indirect Branch
> > Restricted Speculation). The IBSR bit can be unconditionally set to 1
> > without clearing it before.
> 
> Argh for inverted logic: why was the control bit defined for a _negated_ value, 
> i.e. why does '0' mean "don't don't speculate"?
> 
> And yes, I know what's behind it: this way 'IBRS' can be called a 'mitigation 
> feature' that can be 'enabled', instead of calling it a 'broken CPU feature 
> feature' that has to be disabled ...
> 
> That's nonsense that causes confusion to no end:

Yes, it's horrible and on my todo list to clean that up at least on any
layer which is above the pure MSR definition level, which we can't change.

The steam blasting of that mess is not even halfways done.

Thanks,

	tglx

[patch RFC 5/5] x86/speculation: Add basic speculation control code

[Thomas Gleixner]

[-- Attachment #1: x86-speculation--Add-basic-speculation-control-code.patch --]
[-- Type: text/plain, Size: 5218 bytes --]

Add the minimal infrastructure to control the speculation control feature.

 - Integrate it into the spectre_v2 coammand line parser and the mitigation
   selector function. The conditional selector function is a placeholder
   right now, which needs to be expanded with CPU specific decision
   functions.

 - Provide a static key for the actual code control.  

 - Provide a init function which is called after jump label patching is
   functional.

 - Provide an interface for the late micro code loader to allow late
   discovery of the IBRS support. Not yet functional.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/include/asm/nospec-branch.h |    9 +++++
 arch/x86/kernel/cpu/Makefile         |    1 
 arch/x86/kernel/cpu/bugs.c           |   17 +++++++++
 arch/x86/kernel/cpu/specctrl.c       |   62 +++++++++++++++++++++++++++++++++++
 4 files changed, 89 insertions(+)

--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -3,6 +3,8 @@
 #ifndef __NOSPEC_BRANCH_H__
 #define __NOSPEC_BRANCH_H__
 
+#include <linux/static_key.h>
+
 #include <asm/alternative.h>
 #include <asm/alternative-asm.h>
 #include <asm/cpufeatures.h>
@@ -165,5 +167,12 @@ enum spectre_v2_mitigation {
 enum spectre_v2_mitigation spectre_v2_enabled;
 void spectre_v2_select_mitigation(void);
 
+DECLARE_STATIC_KEY_FALSE(specctrl_ibrs);
+
+void specctrl_init_features(void);
+void specctrl_update_features(void);
+bool specctrl_force_enable_ibrs(void);
+bool specctrl_cond_enable_ibrs(bool full_retpoline);
+
 #endif /* __ASSEMBLY__ */
 #endif /* __NOSPEC_BRANCH_H__ */
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -24,6 +24,7 @@ obj-y			+= match.o
 obj-y			+= bugs.o
 obj-$(CONFIG_CPU_FREQ)	+= aperfmperf.o
 obj-y			+= cpuid-deps.o
+obj-y			+= specctrl.c
 
 obj-$(CONFIG_PROC_FS)	+= proc.o
 obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.o
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -79,6 +79,7 @@ enum spectre_v2_mitigation_cmd {
 	SPECTRE_V2_CMD_RETPOLINE,
 	SPECTRE_V2_CMD_RETPOLINE_GENERIC,
 	SPECTRE_V2_CMD_RETPOLINE_AMD,
+	SPECTRE_V2_CMD_IBRS,
 };
 
 static const char *spectre_v2_strings[] = {
@@ -87,6 +88,7 @@ static const char *spectre_v2_strings[]
 	[SPECTRE_V2_RETPOLINE_MINIMAL_AMD]	= "Mitigation: Minimal AMD ASM retpoline",
 	[SPECTRE_V2_RETPOLINE_GENERIC]		= "Mitigation: Full generic retpoline",
 	[SPECTRE_V2_RETPOLINE_AMD]		= "Mitigation: Full AMD retpoline",
+	[SPECTRE_V2_IBRS]			= "Mitigation: Indirect Branch Restricted Speculation",
 };
 
 #undef pr_fmt
@@ -163,6 +165,7 @@ static void __init spectre_v2_check_boot
 
 void spectre_v2_select_mitigation(void)
 {
+	bool full_retpoline = IS_ENABLED(CONFIG_RETPOLINE) && retp_compiler();
 	enum spectre_v2_mitigation mode = SPECTRE_V2_NONE;
 
 	/*
@@ -178,9 +181,22 @@ void spectre_v2_select_mitigation(void)
 	case SPECTRE_V2_CMD_NONE:
 		return;
 
+	case SPECTRE_V2_CMD_IBRS:
+		/* Command line requested IBRS. Try to enable it */
+		if (specctrl_force_enable_ibrs()) {
+			mode = SPECTRE_V2_IBRS;
+			goto set_mode;
+		}
+		/* FALLTRHU */
+
 	case SPECTRE_V2_CMD_FORCE:
 		/* FALLTRHU */
 	case SPECTRE_V2_CMD_AUTO:
+		/* Check whether the CPU prefers to have IBRS */
+		if (specctrl_cond_enable_ibrs(full_retpoline)) {
+			mode = SPECTRE_V2_IBRS;
+			goto set_mode;
+		}
 		goto retpoline_auto;
 
 	case SPECTRE_V2_CMD_RETPOLINE_AMD:
@@ -223,6 +239,7 @@ void spectre_v2_select_mitigation(void)
 		setup_force_cpu_cap(X86_FEATURE_RETPOLINE);
 	}
 
+set_mode:
 	if (spectre_v2_enabled == mode)
 		return;
 	spectre_v2_enabled = mode;
--- /dev/null
+++ b/arch/x86/kernel/cpu/specctrl.c
@@ -0,0 +1,62 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <asm/nospec-branch.h>
+#include <asm/cpufeatures.h>
+
+/* Static key to control enablement of IBRS */
+DEFINE_STATIC_KEY_FALSE(specctrl_ibrs);
+
+/**
+ * specctrl_init_features - Init speculation control features
+ *
+ * Called after static key patching is functional. The decision which
+ * mitigation to use has been made already in check_bugs() before patching
+ * the alternatives.
+ */
+void __init specctrl_init_features(void)
+{
+	if (spectre_v2_enabled != SPECTRE_V2_IBRS)
+		return;
+
+	static_branch_enable(&specctrl_ibrs);
+}
+
+/**
+ * specctrl_update_features - Update the speculation control features
+ *
+ * Called after a late microcode load changed CPU feature bits.
+ *
+ * Note: This is called with CPU hotplug lock and microcode mutex held.
+ */
+void specctrl_update_features(void)
+{
+	if (static_key_enabled(&specctrl_ibrs))
+		return;
+
+	/*
+	 * FIXME: Either the CPU bits need to be reevaluated here or its
+	 * done in the late microcode loader. Borislav ?
+	 */
+	spectre_v2_select_mitigation();
+	if (spectre_v2_enabled != SPECTRE_V2_IBRS)
+		return;
+	static_branch_enable_cpuslocked(&specctrl_ibrs);
+}
+
+bool specctrl_force_enable_ibrs(void)
+{
+	if (!boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+		return false;
+	return true;
+}
+
+bool specctrl_cond_enable_ibrs(bool full_retpoline)
+{
+	if (!boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+		return false;
+	/*
+	 * FIXME: Add logic here to decide what the best option is for a
+	 * particular CPU.
+	 */
+	return true;
+}

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Dave Hansen]

On 01/09/2018 05:06 PM, Thomas Gleixner wrote:
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -79,6 +79,7 @@ enum spectre_v2_mitigation_cmd {
>  	SPECTRE_V2_CMD_RETPOLINE,
>  	SPECTRE_V2_CMD_RETPOLINE_GENERIC,
>  	SPECTRE_V2_CMD_RETPOLINE_AMD,
> +	SPECTRE_V2_CMD_IBRS,
>  };

A few nits on this:

IBRS should not default on anywhere, which goes double when retpolines
are available.

I think I'd also prefer that we separate the IBRS and retpoline enabling
so that you can do both if you want.  They do nearly the same thing in
practice, but I can't convince myself that you never ever need IBRS once
retpolines are in place.

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Justin Forbes]

On Tue, Jan 9, 2018 at 8:02 PM, Dave Hansen <dave.hansen@intel.com> wrote:
> On 01/09/2018 05:06 PM, Thomas Gleixner wrote:
>> --- a/arch/x86/kernel/cpu/bugs.c
>> +++ b/arch/x86/kernel/cpu/bugs.c
>> @@ -79,6 +79,7 @@ enum spectre_v2_mitigation_cmd {
>>       SPECTRE_V2_CMD_RETPOLINE,
>>       SPECTRE_V2_CMD_RETPOLINE_GENERIC,
>>       SPECTRE_V2_CMD_RETPOLINE_AMD,
>> +     SPECTRE_V2_CMD_IBRS,
>>  };
>
> A few nits on this:
>
> IBRS should not default on anywhere, which goes double when retpolines
> are available.
>
> I think I'd also prefer that we separate the IBRS and retpoline enabling
> so that you can do both if you want.  They do nearly the same thing in
> practice, but I can't convince myself that you never ever need IBRS once
> retpolines are in place.

Fairly strong agreement here. IBRS being separately configurable gives
us an option for the paranoid, and allows distros to ship with it off
by default.

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Peter Zijlstra]

On Tue, Jan 09, 2018 at 06:02:53PM -0800, Dave Hansen wrote:
> On 01/09/2018 05:06 PM, Thomas Gleixner wrote:
> > --- a/arch/x86/kernel/cpu/bugs.c
> > +++ b/arch/x86/kernel/cpu/bugs.c
> > @@ -79,6 +79,7 @@ enum spectre_v2_mitigation_cmd {
> >  	SPECTRE_V2_CMD_RETPOLINE,
> >  	SPECTRE_V2_CMD_RETPOLINE_GENERIC,
> >  	SPECTRE_V2_CMD_RETPOLINE_AMD,
> > +	SPECTRE_V2_CMD_IBRS,
> >  };
> 
> A few nits on this:
> 
> IBRS should not default on anywhere, which goes double when retpolines
> are available.
> 
> I think I'd also prefer that we separate the IBRS and retpoline enabling
> so that you can do both if you want.  They do nearly the same thing in
> practice, but I can't convince myself that you never ever need IBRS once
> retpolines are in place.

As per:

https://lkml.kernel.org/r/1515460999.4423.104.camel@amazon.co.uk

IBRS=2 (always on) is preferred for SKL+ over retpoline.

And from what I gather IBRS=1 is never better than retpoline, IBRS=1 is
both slower and covers less AFAIU (please educate if I'm wrong).

>From this point, I would prefer to not even have the IBRS=1 code.

The only question I have is if retpoline works at all on SKL (with ucode
update); BDW needs the ucode update for retpoline to work because of the
RSB fallback.

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1646 bytes --]

On Wed, 2018-01-10 at 10:22 +0100, Peter Zijlstra wrote:
> On Tue, Jan 09, 2018 at 06:02:53PM -0800, Dave Hansen wrote:
> > 
> > On 01/09/2018 05:06 PM, Thomas Gleixner wrote:
> > > 
> > > --- a/arch/x86/kernel/cpu/bugs.c
> > > +++ b/arch/x86/kernel/cpu/bugs.c
> > > @@ -79,6 +79,7 @@ enum spectre_v2_mitigation_cmd {
> > >  	SPECTRE_V2_CMD_RETPOLINE,
> > >  	SPECTRE_V2_CMD_RETPOLINE_GENERIC,
> > >  	SPECTRE_V2_CMD_RETPOLINE_AMD,
> > > +	SPECTRE_V2_CMD_IBRS,
> > >  };
> > A few nits on this:
> > 
> > IBRS should not default on anywhere, which goes double when retpolines
> > are available.
> > 
> > I think I'd also prefer that we separate the IBRS and retpoline enabling
> > so that you can do both if you want.  They do nearly the same thing in
> > practice, but I can't convince myself that you never ever need IBRS once
> > retpolines are in place.
> As per:
> 
> https://lkml.kernel.org/r/1515460999.4423.104.camel@amazon.co.uk
> 
> IBRS=2 (always on) is preferred for SKL+ over retpoline.
> 
> And from what I gather IBRS=1 is never better than retpoline, IBRS=1 is
> both slower and covers less AFAIU (please educate if I'm wrong).
> 
> From this point, I would prefer to not even have the IBRS=1 code.
> 
> The only question I have is if retpoline works at all on SKL (with ucode
> update); BDW needs the ucode update for retpoline to work because of the
> RSB fallback.

As I understand it, Skylake is never getting the IBRS_ATT (all the
time) feature. That is for future CPUs only.

I don't know why you're calling that 'IBRS=2'; are you getting confused
by Andrea's distro horridness?

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Peter Zijlstra]

On Wed, Jan 10, 2018 at 09:27:59AM +0000, David Woodhouse wrote:
> > The only question I have is if retpoline works at all on SKL (with ucode
> > update); BDW needs the ucode update for retpoline to work because of the
> > RSB fallback.
> 
> As I understand it, Skylake is never getting the IBRS_ATT (all the
> time) feature. That is for future CPUs only.

Ah, so then I got confused by your earlier email. So retpoline will work
on SKL, but it will, like BDW need the ucode update?

> I don't know why you're calling that 'IBRS=2'; are you getting confused
> by Andrea's distro horridness?

Yes, continuation of the nomenclature of that earlier thread.

Still, point remains, do we ever need that MSR fiddling dynamic IBRS
stuff now that we have retpolines? I would prefer to not have it if
there is no compelling reason for it.

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2957 bytes --]

On Wed, 2018-01-10 at 11:03 +0100, Peter Zijlstra wrote:
> On Wed, Jan 10, 2018 at 09:27:59AM +0000, David Woodhouse wrote:
> > 
> > > 
> > > The only question I have is if retpoline works at all on SKL (with ucode
> > > update); BDW needs the ucode update for retpoline to work because of the
> > > RSB fallback.
> > As I understand it, Skylake is never getting the IBRS_ATT (all the
> > time) feature. That is for future CPUs only.
>
> Ah, so then I got confused by your earlier email. So retpoline will work
> on SKL, but it will, like BDW need the ucode update?

Er, ... I'm not sure if SKL does need the µcode update. As I understand
it, the µcode update for BDW stops it from taking predictions for 'ret'
from anything but the RSB, which means that RSB-stuffing is sufficient.

On SKL I don't think there *is* a way to stop it doing that, which is
why we were originally saying we'd use IBRS on SKL instead.

Do ask someone who works at Intel for confirmation of the above :)

> > I don't know why you're calling that 'IBRS=2'; are you getting confused
> > by Andrea's distro horridness?
> Yes, continuation of the nomenclature of that earlier thread.

Except I don't think it is; I think the various IBRS= options there
were about pointlessly fine-tuning when you frob the *existing* IBRS
bit, not about the future IBRS_ATT bit which really is set-and-forget.

> Still, point remains, do we ever need that MSR fiddling dynamic IBRS
> stuff now that we have retpolines? I would prefer to not have it if
> there is no compelling reason for it.

We do want IBPB if we want to protect userspace processes (and VM
guests) from each other. Although the attack mode there is relatively
hard so *perhaps* we can resort to prayer instead?

For IBRS though... yeah, good question. It *does* give additional
protection on SKL where 'ret' might get mispredicted and the RSB-
stuffing is not always guaranteed to be sufficient, but appealing to
the deity of your choice might also be an acceptable mitigation there?

When IBRS came first and retpoline was purely a performance
optimisation, it seemed to be a no-brainer: performance optimisations
don't get to say "well, it's only a *little* bit less secure but yay
it's faster!". Now with retpoline working first, it does seem
*psychologically* to be a slightly different question, but really it's
the same choice.

I don't have a real answer for precisely how hard it would be to
exploit the theoretical loopholes that retpoline opens on SKL.

One observation is that for an attacker to find a gadget is slightly
easier than it used to be, in some ways. Previously, if you found
gadgets you wanted to use, they had to *work*. They had to return to
some sane address and not just crash the system. That's not true in
speculation. They can leak the data and then crash or go off into the
weeds, but the crash will never really happen. The leak will.


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Thomas Gleixner]

[-- Attachment #1: Type: text/plain, Size: 3651 bytes --]

On Wed, 10 Jan 2018, David Woodhouse wrote:
> On Wed, 2018-01-10 at 11:03 +0100, Peter Zijlstra wrote:
> > On Wed, Jan 10, 2018 at 09:27:59AM +0000, David Woodhouse wrote:
> > > 
> > > > 
> > > > The only question I have is if retpoline works at all on SKL (with ucode
> > > > update); BDW needs the ucode update for retpoline to work because of the
> > > > RSB fallback.
> > > As I understand it, Skylake is never getting the IBRS_ATT (all the
> > > time) feature. That is for future CPUs only.
> >
> > Ah, so then I got confused by your earlier email. So retpoline will work
> > on SKL, but it will, like BDW need the ucode update?
> 
> Er, ... I'm not sure if SKL does need the µcode update. As I understand
> it, the µcode update for BDW stops it from taking predictions for 'ret'
> from anything but the RSB, which means that RSB-stuffing is sufficient.
> 
> On SKL I don't think there *is* a way to stop it doing that, which is
> why we were originally saying we'd use IBRS on SKL instead.
> 
> Do ask someone who works at Intel for confirmation of the above :)

To get how many contradicting answers? I'm really fed up with the
desinformation politics in this matter.

> > > I don't know why you're calling that 'IBRS=2'; are you getting confused
> > > by Andrea's distro horridness?
> > Yes, continuation of the nomenclature of that earlier thread.
> 
> Except I don't think it is; I think the various IBRS= options there
> were about pointlessly fine-tuning when you frob the *existing* IBRS
> bit, not about the future IBRS_ATT bit which really is set-and-forget.

So we can offer the spectre_v2=ibrs option now, which just enforces it and
be done with it. The auto mode will not select IBRS.

The question remains, whether retpoline needs to be still in place even
with IBRS enabled. I'd say yes, unless the IBRS always on mechanics fully
works as intended. But that's not a question we have to answer today.

As Ingo stated, we also can require to have microcode early, which is in
fact a single initrd/reboot required. With the flood of reboots we've seen
in the last week and which will continue a while that really should not
matter at all. But we can avoid all the complexity of late switching and
other crap.

> > Still, point remains, do we ever need that MSR fiddling dynamic IBRS
> > stuff now that we have retpolines? I would prefer to not have it if
> > there is no compelling reason for it.
> 
> We do want IBPB if we want to protect userspace processes (and VM
> guests) from each other. 

IBPB is an orthogonal feature and that's the one which is also implemented
by AMD, who did not bother with the IBRS voodoo, if my limited knowledge
(READ: Where is the damned documentation (both Intel and AMD)?) is correct.

> Although the attack mode there is relatively hard so *perhaps* we can
> resort to prayer instead?

That's the only thing we can do anyway due to lack of authorative
statements. That lack is either caused by lawyer weaseling or by
fundamental lack of confidence that the operation principles of the CPU are
fully understood. Maybe both. Not very useful in any case.

> For IBRS though... yeah, good question. It *does* give additional
> protection on SKL where 'ret' might get mispredicted and the RSB-
> stuffing is not always guaranteed to be sufficient, but appealing to
> the deity of your choice might also be an acceptable mitigation there?

So the right thing to do is:

   if (spectre_v2_commandline == "ibrs" || !retpoline)
      	ibrs_enable_if_ucode_supports();

   if (retpoline)
        retpoline_select();

We definitely should add spectre_v2=pray as a valid boot option choice.

Thanks,

	tglx

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Peter Zijlstra]

On Wed, Jan 10, 2018 at 11:22:53AM +0000, David Woodhouse wrote:
> On Wed, 2018-01-10 at 11:03 +0100, Peter Zijlstra wrote:
> > On Wed, Jan 10, 2018 at 09:27:59AM +0000, David Woodhouse wrote:
> > > 
> > > > 
> > > > The only question I have is if retpoline works at all on SKL (with ucode
> > > > update); BDW needs the ucode update for retpoline to work because of the
> > > > RSB fallback.
> > > As I understand it, Skylake is never getting the IBRS_ATT (all the
> > > time) feature. That is for future CPUs only.
> >
> > Ah, so then I got confused by your earlier email. So retpoline will work
> > on SKL, but it will, like BDW need the ucode update?
> 
> Er, ... I'm not sure if SKL does need the µcode update. As I understand
> it, the µcode update for BDW stops it from taking predictions for 'ret'
> from anything but the RSB, which means that RSB-stuffing is sufficient.

OK, that is what I understood as well.

> On SKL I don't think there *is* a way to stop it doing that, which is
> why we were originally saying we'd use IBRS on SKL instead.

Ah, so we need the dynamic IBRS cruft for SKL :/ I really detest the
IBRS 'feature' it would be ever so much better if we can otherwise fix
that RSB issue, but I suppose people have looked at that already.

> > Yes, continuation of the nomenclature of that earlier thread.
> 
> Except I don't think it is; I think the various IBRS= options there
> were about pointlessly fine-tuning when you frob the *existing* IBRS
> bit, not about the future IBRS_ATT bit which really is set-and-forget.

IBRS=1 was the dynamic mumbo jumbo, IBRS=2 set it once at CPU bringup
and leave it on (or rather, that was the intention, patch had holes)
IIRC.

> > Still, point remains, do we ever need that MSR fiddling dynamic IBRS
> > stuff now that we have retpolines? I would prefer to not have it if
> > there is no compelling reason for it.
> 
> We do want IBPB if we want to protect userspace processes (and VM
> guests) from each other. Although the attack mode there is relatively
> hard so *perhaps* we can resort to prayer instead?

Sure, not arguing about IBPB, I understand that bit.

> For IBRS though... yeah, good question. It *does* give additional
> protection on SKL where 'ret' might get mispredicted and the RSB-
> stuffing is not always guaranteed to be sufficient, but appealing to
> the deity of your choice might also be an acceptable mitigation there?

I'm just trying to understand the various undocumented options here. As
pjt [1] put it nicely:

"The majority of the confusion does not stem from this being complicated.
 It's that there's been great reluctance to document the details"

There just isn't clear and coherent information on this stuff around, so
everybody gets confused.


[1] https://lkml.kernel.org/r/CAPM31RKV-P40P0yUy7QjvsWT7sr2wUrBzqj_ma1trAjNiBWM3w@mail.gmail.com

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Andrea Arcangeli]

On Wed, Jan 10, 2018 at 09:27:59AM +0000, David Woodhouse wrote:
> I don't know why you're calling that 'IBRS=2'; are you getting confused
> by Andrea's distro horridness?

Eh, yes he's got confused. ibrs_enabled 2 simply means to leave IBRS
set in SPEC_CTLR 100% of the time, except in guest mode.

IBRS is the "1" value to write in SPEC_CTRL MSR, SPEC_CTRL = 2 is not
IBRS.

"ibrs_enabled ibpb_enabled" semantics are practically the only thing we
didn't change from the code that we've been presented with (aside from
adding ibrs_enable 0 ibpb_enabled 2 new mode to fix spectre variant#2
on CPUs with no SPEC_CTRL/IBRS and only with IBPB_SUPPORT, that
otherwise would have no kernel protection at all [of course modulo
HT/SMT but that's not the PoC and ibrs_enabled 1 ibpb_enabled 1 also
won't protect guest/user vs guest/user for HT/SMT and CPU
pinning/isolation or ibrs 2 or HT/SMT disablement is needed for that]).

NOTE: we moved ibrs_enabled ibpb_enable to debugfs, they were in more
problematic location before we moved them there.

We could have done something entirely different, but ibrs_enabled
ibpb_enabled semantics as they were presented, looked as good as it
could get to be able to achieve all the below on 0day:

1) ship a full fix for all CPUs (including those CPUs that cannot be
   fixed with reptolines) that protected the host kernel memory 100%
   from spectre variant#2 by default

2) to be able to get as much performance back as possible for all
   those cases where these attacks are irrelevant and performance is
   critical by setting ibrs_enabled 0 ibpb_enabled 0 (pti_enabled 0 is
   available too in the same debug/x86 location as well in fact).

   If you see complains on ibrs_enabled 1 slowing down ffmpeg or
   something compared to ibrs_enabled 0, well it's because we shipped
   those tunables in the first place that precisely allows so easy
   comparison, and that makes it so easy to set ibrs_enabled 0 and run
   at full CPU power in kernel mode too if spectre variant#2 is not a
   concern.

   Compare that to having to reboot the system and modify grub every
   time before running a test... that would have been truly horrid as
   a solution to get the all performance back where spectre variant#2
   is no concern.

   If you're rendering a movie or you're transcoding with ffmpeg and
   nothing else and you're behind several level of firewalls or even
   disconnected from the network, it can all be disabled (including
   setting pti_enabled 0 if that's the only app in guest or host,
   i.e. a single memcached microservice running in a KVM guest).

3) allow to optionally make it impossible for a KVM guest userland to
   read KVM guest kernel memory by attacking qemu host userland, by
   simply setting ibrs_enabled 2 ibpb_enabled 1 for a subset of
   customers that may be especially concerned about that theoretical
   issue. Also allow to fix other theoretical HT/SMT attacks affecting
   guest/user mode vs guest/user mode with the same ibrs_enabled 2
   ibpb_enabled 1.

   We don't know if those issues are practical but ibrs_enabled 2 will
   provide math guarantee for those as well.

   For the KVM guest userland attack on qemu userland not even SMEP
   helps as kindly confirmed by Dave. For that issue also the
   ibrs_enabled 0 ibpb_enabled 2 on host or even only in guest is
   enough, but likely it would perform worse than ibrs_enabled 2
   ibpb_enabled 1, but then it depends on the CPU which is again why
   tunables are so handy so you can optimize for the best tradeoff.

Not exactly sure how you could have accommodated for all 3 points
above in an even more flexible way by removing those three debugfs
tunables.

If you don't want to deal with them because it's very complex to
understand exactly what they do, there's a simple solution too: ignore
them knowing it boots secure by default if either
SPEC_CTRL/IBPB_SUPPORT are present in boot "dmesg" logs and to disable
it all to get the performance back: echo 0 >ibrs_enabled echo 0
>ibpb_enabled.

Even Dave just said that even with reptolines optimization patched in
by default at boot, he'd still like to be able to enable the
equivalent of ibrs_enabled 1, let alone those CPUs where reptolines
can't fix spectre variant#2. Furthermore ibrs_enabled 2 is not
obsoleted at all by kernel repotlines unless you rebuild the whole
userland including all qemu dependencies and glibc with reptolines
which would have been impossible to achieve on 0day no matter what.

Retpolines are an optimization for later for a subset of CPUs so that
we can stop setting IBRS in SPEC_CTRL while in kernel mode. On some
CPUs IBRS or in general disabling the IBP is so fast I wouldn't even
bother to enable reptolines at boot in if they don't even work safe to
begin with.

Thanks,
Andrea

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 673 bytes --]

On Wed, 2018-01-10 at 12:54 +0100, Andrea Arcangeli wrote:
> On Wed, Jan 10, 2018 at 09:27:59AM +0000, David Woodhouse wrote:
> > I don't know why you're calling that 'IBRS=2'; are you getting
> confused
> > by Andrea's distro horridness?
> 
> Eh, yes he's got confused. ibrs_enabled 2 simply means to leave IBRS
> set in SPEC_CTLR 100% of the time, except in guest mode.

On all current hardware, if you only set IBRS when you exit a guest,
then you are not protecting yourself from userspace at all. IBRS acts
as a *barrier* in all current hardware.

Future CPUs will have a new feature where you *can* do something like
this, but this is not available yet.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Andrea Arcangeli]

On Wed, Jan 10, 2018 at 11:58:54AM +0000, David Woodhouse wrote:
> On Wed, 2018-01-10 at 12:54 +0100, Andrea Arcangeli wrote:
> > On Wed, Jan 10, 2018 at 09:27:59AM +0000, David Woodhouse wrote:
> > > I don't know why you're calling that 'IBRS=2'; are you getting
> > confused
> > > by Andrea's distro horridness?
> > 
> > Eh, yes he's got confused. ibrs_enabled 2 simply means to leave IBRS
> > set in SPEC_CTLR 100% of the time, except in guest mode.
> 
> On all current hardware, if you only set IBRS when you exit a guest,
> then you are not protecting yourself from userspace at all. IBRS acts
> as a *barrier* in all current hardware.

Kernel memory is 100% protected if you set only IBRS at vmexit.

Once IBRS is set, there's no way any userland (nor host nor guest) can
attack the kernel memory through spectre variant#2.

What is not protected is host userland from guest userland which is
point 3 in the email I posted earlier and I already provided all
details there on how to fix that purely theoretical issue not part of
the PoC with the provided debugfs tunables, so I won't repeat here.

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Andrea Arcangeli]

On Wed, Jan 10, 2018 at 01:01:58PM +0100, Andrea Arcangeli wrote:
> On Wed, Jan 10, 2018 at 11:58:54AM +0000, David Woodhouse wrote:
> > On Wed, 2018-01-10 at 12:54 +0100, Andrea Arcangeli wrote:
> > > On Wed, Jan 10, 2018 at 09:27:59AM +0000, David Woodhouse wrote:
> > > > I don't know why you're calling that 'IBRS=2'; are you getting
> > > confused
> > > > by Andrea's distro horridness?
> > > 
> > > Eh, yes he's got confused. ibrs_enabled 2 simply means to leave IBRS
> > > set in SPEC_CTLR 100% of the time, except in guest mode.
> > 
> > On all current hardware, if you only set IBRS when you exit a guest,
> > then you are not protecting yourself from userspace at all. IBRS acts
> > as a *barrier* in all current hardware.
> 
> Kernel memory is 100% protected if you set only IBRS at vmexit.
> 
> Once IBRS is set, there's no way any userland (nor host nor guest) can
> attack the kernel memory through spectre variant#2.
> 
> What is not protected is host userland from guest userland which is
> point 3 in the email I posted earlier and I already provided all
> details there on how to fix that purely theoretical issue not part of
> the PoC with the provided debugfs tunables, so I won't repeat here.

Also I read in another email you thought IBRS is a barrier or
something, it's not, it's purely temporarily preventing the CPU to
speculate through IBP BTB whatever, it will do the least possible
amount of work without altering the internal state that can still be
poisoned by the attacker but it's no problem because kernel mode won't
make any use of that "poison". IBRS won't flush anything, the poison
stays there, it's not barrier, and IBPB is the only barrier that
flushes the content.

IBPB is expensive but after IBPB you return running at core full speed
but you can't be influenced by any poison somebody put in the CPU to
influence your runtime. IBRS as opposed is not expensive to execute
(relatively speaking) but then the CPU runs slower as it can't use
those parts of the CPU that may have been poisoned by the attacker.

So you can also flush the whole content of IBP/BTB at vmexit with IBPB
instead of setting SPECT_CTRL to IBRS. Same thing at kernel entry. If
you do that and you don't care about SMT/HT effect it's equivalent to
setting IBRS at vmexit/kernelentry and in fact it'll also protect host
userland from guest userland.

Hope this clarifies what those IBRS IBPB features achieve.

Thanks,
Andrea

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1784 bytes --]

On Wed, 2018-01-10 at 13:07 +0100, Andrea Arcangeli wrote:
> On Wed, Jan 10, 2018 at 01:01:58PM +0100, Andrea Arcangeli wrote:
> > On Wed, Jan 10, 2018 at 11:58:54AM +0000, David Woodhouse wrote:
> > > On Wed, 2018-01-10 at 12:54 +0100, Andrea Arcangeli wrote:
> > > > On Wed, Jan 10, 2018 at 09:27:59AM +0000, David Woodhouse wrote:
> > > > > I don't know why you're calling that 'IBRS=2'; are you getting
> > > > confused
> > > > > by Andrea's distro horridness?
> > > > 
> > > > Eh, yes he's got confused. ibrs_enabled 2 simply means to leave IBRS
> > > > set in SPEC_CTLR 100% of the time, except in guest mode.
> > > 
> > > On all current hardware, if you only set IBRS when you exit a guest,
> > > then you are not protecting yourself from userspace at all. IBRS acts
> > > as a *barrier* in all current hardware.
> > 
> > Kernel memory is 100% protected if you set only IBRS at vmexit.
> > 
> > Once IBRS is set, there's no way any userland (nor host nor guest) can
> > attack the kernel memory through spectre variant#2.
> > 
> > What is not protected is host userland from guest userland which is
> > point 3 in the email I posted earlier and I already provided all
> > details there on how to fix that purely theoretical issue not part of
> > the PoC with the provided debugfs tunables, so I won't repeat here.
> 
> Also I read in another email you thought IBRS is a barrier or
> something, it's not, it's purely temporarily preventing the CPU to
> speculate through IBP BTB whatever,

No.

IBRS is like a barrier. You must write it between the 'problematic'
loading of the branch targets, and the kernel code which might be
affected.

You cannot, on current hardware, merely set it once and forget about
it. That is not sufficient.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Andrea Arcangeli]

On Wed, Jan 10, 2018 at 12:12:53PM +0000, David Woodhouse wrote:
> IBRS is like a barrier. You must write it between the 'problematic'
> loading of the branch targets, and the kernel code which might be
> affected.
> 
> You cannot, on current hardware, merely set it once and forget about
> it. That is not sufficient.

I think you've got it all wrong...

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Andrea Arcangeli]

On Wed, Jan 10, 2018 at 01:20:45PM +0100, Andrea Arcangeli wrote:
> On Wed, Jan 10, 2018 at 12:12:53PM +0000, David Woodhouse wrote:
> > IBRS is like a barrier. You must write it between the 'problematic'
> > loading of the branch targets, and the kernel code which might be
> > affected.
> > 
> > You cannot, on current hardware, merely set it once and forget about
> > it. That is not sufficient.
> 
> I think you've got it all wrong...

On a side note, I hope once you get it right, the debugfs tunable will
suddenly look less horrid.

RE: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Van De Ven, Arjan]

 
> On Wed, Jan 10, 2018 at 12:12:53PM +0000, David Woodhouse wrote:
> > IBRS is like a barrier. You must write it between the 'problematic'
> > loading of the branch targets, and the kernel code which might be
> > affected.
> >
> > You cannot, on current hardware, merely set it once and forget about
> > it. That is not sufficient.
> 
> I think you've got it all wrong...

Andrea: David is right. The specification draft that you have also makes this clear.

You can't just set IBRS once and call it good; you do need to write it on entering the kernel.

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 977 bytes --]

On Wed, 2018-01-10 at 13:01 +0100, Andrea Arcangeli wrote:
> 
> > On all current hardware, if you only set IBRS when you exit a guest,
> > then you are not protecting yourself from userspace at all. IBRS acts
> > as a *barrier* in all current hardware.
> 
> Kernel memory is 100% protected if you set only IBRS at vmexit.
> 
> Once IBRS is set, there's no way any userland (nor host nor guest) can
> attack the kernel memory through spectre variant#2.

That is not consistent with the documentation I've seen, which Intel
have so far utterly failed to publish AFAICT.

"a near indirect jump/call/return may be affected by code in a less privileged
prediction mode that executed AFTER IBRS mode was last written with a value of 1"

The kernel is only protected from branch targets set in userspace
*BEFORE* the IBRS mode was last set to 1. If you set it to 1, then
leave it like that while you run userspace and then kernel code again,
you are not protected.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Andrea Arcangeli]

On Wed, Jan 10, 2018 at 12:09:34PM +0000, David Woodhouse wrote:
> That is not consistent with the documentation I've seen, which Intel
> have so far utterly failed to publish AFAICT.
> 
> "a near indirect jump/call/return may be affected by code in a less privileged
> prediction mode that executed AFTER IBRS mode was last written with a value of 1"

You must have misunderstood the context there, or the above text is
wrong to begin with.

> The kernel is only protected from branch targets set in userspace
> *BEFORE* the IBRS mode was last set to 1. If you set it to 1, then
> leave it like that while you run userspace and then kernel code again,
> you are not protected.

I'm sure you've got it wrong, that would be crazy if it would be the
case.

Even from an implementation standpoint IBRS just means stop
speculating through branch prediction, there's no way they can add
this separation between ring 0 and ring 3 when it didn't exist to
begin with.

RSB had it with SMEP and in fact there's no need to do stuff_RSB with
SMEP enabled in cr4 and we alternative it out in kernel entry on host
and guest, but we still have to do that in vmexit.

IBP/BTB had nothing like, which is why user ring 3 can attack host
ring 0.

It will have it and also separating guest from host but in the future.

Plus IBRS can be set at will by guest mode, if guest kernel is
malicious it will let guest userland run with IBRS on. You can't trust
the guest kernel to begin with. So it would make IBRS totally useless
for KVM to set in vmexit if you were right about that.

In any case, we've a ibrs_enabled 0 ibpb_enabled 2 that would achieve
full security if you were right, but I think you got it all wrong
about IBRS and rings.

Second: IBRS means Indirect Branch Restricted Speculation, the
speculation through branch prediction is restricted while it's
set. It's inconceivable that they add ring knowledge to a part that is
couldn't be helped by SMEP in the first place that had at least some
ring separation when enabled.

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1244 bytes --]

On Wed, 2018-01-10 at 13:17 +0100, Andrea Arcangeli wrote:
> On Wed, Jan 10, 2018 at 12:09:34PM +0000, David Woodhouse wrote:
> > That is not consistent with the documentation I've seen, which Intel
> > have so far utterly failed to publish AFAICT.
> > 
> > "a near indirect jump/call/return may be affected by code in a less privileged
> > prediction mode that executed AFTER IBRS mode was last written with a value of 1"
> 
> You must have misunderstood the context there, or the above text is
> wrong to begin with.

That's a quote from the Intel documentation for the IBRS feature.
Go read it, please.

> > The kernel is only protected from branch targets set in userspace
> > *BEFORE* the IBRS mode was last set to 1. If you set it to 1, then
> > leave it like that while you run userspace and then kernel code again,
> > you are not protected.
> 
> I'm sure you've got it wrong, that would be crazy if it would be the
> case.

Andrea, what part of this whole fucking mess isn't entirely batshit
insane to start with? :)

I think you are confused with the future IBRS_ATT option which will
exist on new hardware. 

Right now, IBRS works as I described it because that's the best they
could do with microcode.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Andrea Arcangeli]

On Wed, Jan 10, 2018 at 12:29:44PM +0000, David Woodhouse wrote:
> On Wed, 2018-01-10 at 13:17 +0100, Andrea Arcangeli wrote:
> > On Wed, Jan 10, 2018 at 12:09:34PM +0000, David Woodhouse wrote:
> > > That is not consistent with the documentation I've seen, which Intel
> > > have so far utterly failed to publish AFAICT.
> > > 
> > > "a near indirect jump/call/return may be affected by code in a less privileged
> > > prediction mode that executed AFTER IBRS mode was last written with a value of 1"
> > 
> > You must have misunderstood the context there, or the above text is
> > wrong to begin with.
> 
> That's a quote from the Intel documentation for the IBRS feature.
> Go read it, please.

Perhaps the confusing come from "less privileged prediction mode" and
you thought that meant "less privileged ring mode". It says "predction
mode" not ring 3.

Frankly I found that documentation very confusing like the inversion
of IBRS enabled really meaning IBP speculation disabled like Ingo
pointed out.

It's clear all done in a rush but we've to live with it. After all the
electric current also really flows in the opposite direction of the
arrow..

> Andrea, what part of this whole fucking mess isn't entirely batshit
> insane to start with? :)

Absolutely :).

> I think you are confused with the future IBRS_ATT option which will
> exist on new hardware. 

No, the only difference is that such option will perform best.

This is why the current default ibrs_enabled 1 ibpb_enabled 1.

That future CPUID bit will simply make the kernel boot by default with
ibrs_enabled 2 ibpb_enabled 1 and it'll perform best as it won't have
to write to SPEC_CTRL in kernel entry kernel exit vmenter/vmexit like
it commonly has to do with ibrs_enabled 1.

The only difference is that ibrs_enabled 2 will perform best, while
currently ibrs_enabled 1 performs best.

> Right now, IBRS works as I described it because that's the best they
> could do with microcode.

It works as I described but instead of arguing about specs above,
Intel should clarify that IBRS can already be set 100% of the time, be
left alone and set always, with all CPUs with SPEC_CTRL, and it's the
most secure mode available and matches the IBRS patchset with
ibrs_enabled 2.

Hope this helps clarify and of course this is so complex it's
perfectly normal to misunderstand something, but I'm confident it's
not me who misunderstood because if I did, the whole ibrs_enabled 2
that was just posted would make zero sense, that is for current CPUs
and it's the most secure option available (not less secure as you
seem to think).

Thanks,
Andrea

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Jiri Kosina]

On Wed, 10 Jan 2018, Andrea Arcangeli wrote:

> Perhaps the confusing come from "less privileged prediction mode" and
> you thought that meant "less privileged ring mode". It says "predction
> mode" not ring 3.

Well, prediction mode is defined by "CPL3 vs CPL0-2" and "VMX root vs VMX 
non-root", with obvious ordering of privileges.

So if IBRS is set, branch predictor will not allow the predicted target to 
be influenced by code that executed in less privileged prediction mode 
before value of '1' IBRS mode was last written to, and that's pretty much 
it.

-- 
Jiri Kosina
SUSE Labs

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1027 bytes --]

On Wed, 2018-01-10 at 13:47 +0100, Jiri Kosina wrote:
> On Wed, 10 Jan 2018, Andrea Arcangeli wrote:
> 
> > Perhaps the confusing come from "less privileged prediction mode" and
> > you thought that meant "less privileged ring mode". It says "predction
> > mode" not ring 3.
> 
> Well, prediction mode is defined by "CPL3 vs CPL0-2" and "VMX root vs VMX 
> non-root", with obvious ordering of privileges.
> 
> So if IBRS is set, branch predictor will not allow the predicted target to 
> be influenced by code that executed in less privileged prediction mode 
> before value of '1' IBRS mode was last written to, and that's pretty much 
> it.

The operative words in that sentence being, "before the IBRS mode was
last written with a value of 1".

If it worked as Andrea suggests, then there would be absolutely no
point in the patches we've seen which add the IBRS-frobbing on syscall
entry and vmexit.

The "IBRS all the time" feature is something we get on *future*
hardware, not current hardware.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Andrea Arcangeli]

On Wed, Jan 10, 2018 at 12:51:13PM +0000, David Woodhouse wrote:
> If it worked as Andrea suggests, then there would be absolutely no
> point in the patches we've seen which add the IBRS-frobbing on syscall
> entry and vmexit.

This is perhaps what you're missing I think, there is a huge point:
performance.

ibrs_enabled 1 ibpb_enabled 1 is much faster than ibrs_enabled 2
ibpb_enabled 1.

The current IBRS is disabling IBP, it can't do the "finegrined" thing
that leaves IBP speculation enabled.

Kernel is not a 100% C++ virtual template, so for kernel IBRS enabled
is not as a big deal as userland with IBRS enabled.

Once the new CPUid bit will be set it'll tell us IBRS is really
restricting speculation in a finegrined way with all privilege levels
ordered, so it simply tells is ibrs_enabled 2 ibpb_enabled 1 will be
faster than ibrs_enabled 1 ibpb_enabled 1, so the only thing we've to
do with the new CPUID bit you're talking about from the future, is to
change the default to ibrs_enabled 2 and it'll perform even better
with zero overhead in kernel entry points.

We already optimized for the future silicon, we've only to add a tweak
to detect cpuid and set the default ibrs_enabled to 2.

> The "IBRS all the time" feature is something we get on *future*
> hardware, not current hardware.

No, for current hardware it's the most secure option available and
required in fact if you want to fix guest userland attack on host
userland with math certainty which can matter for some customer.

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Andrea Arcangeli]

On Wed, Jan 10, 2018 at 02:02:02PM +0100, Andrea Arcangeli wrote:
> On Wed, Jan 10, 2018 at 12:51:13PM +0000, David Woodhouse wrote:
> > If it worked as Andrea suggests, then there would be absolutely no
> > point in the patches we've seen which add the IBRS-frobbing on syscall
> > entry and vmexit.
> 
> This is perhaps what you're missing I think, there is a huge point:
> performance.

And note, if you run getppid in a loop you're perfectly right
ibrs_enabled 2 will perform better than ibrs_enabled 1, but we run
multiple set of macro benchmarks and ibrs_enabled 1 ibpb_enabled 1
always wins. So ibrs_enabled 2 ibpb_enabled 1 is left as an optional
"paranoid" security mode, more secure but even slower.

We draw a line for the default to stop the PoC which we know can be
mounted (at least in theory :) and is practical attack.

Also note, the slowdown of setting IBRS varies with older CPUs being
more affected, but for all current silicon ibrs_enabled 1 ibpb_enabled
1 is faster in 99% of real life workloads.

The future silicon will set the CPUID that will tell us set
"ibrs_enabled 2" by default, but you can do it right and it's even
more secure as it completely solves any HT effect in userland and it
prevents the KVM guest kernel to be attacked by guest userland through
a spectre variant#2 attack from guest userland to qemu host userland.

Hope this helps,
Andrea

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Andrea Arcangeli]

On Wed, Jan 10, 2018 at 02:05:51PM +0100, Andrea Arcangeli wrote:
> Also note, the slowdown of setting IBRS varies with older CPUs being

To give a further detail, older CPUs to provide IBRS semantics have to
do something even less finegrined that doesn't just restricts
speculation across IBRS less privileged prediction level, that doesn't
just restrict speculation across indirect branches, that doesn't just
restrict indirect branch prediction, but that shuts down a whole block
of the CPU to achieve those super finegrined theoretical IBRS
semantics that matches future silicon.

IBRS is doing a lot more than what it would be needed because there
was no other way to achieve it on some microcode
implementations. Which is why ibrs_enabled 1 is needed to achieve
better performance as you don't want to run slower while in user mode,
much better to pay the cost of setting IBRS and shutting down that
part of the CPU.

It's still incredibly faster to shutdown part of the CPU temporarily
than to flush its internal state as a whole with IBPB. If it wouldn't
be the case ibrs_enabled 0 ibpb_enabled 2 special mode would perform
better (but that's only enabled by default if SPEC_CTRL is not
available and only IBPB_SUPPORT is).

Thanks,
Andrea

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Andrea Arcangeli]

On Wed, Jan 10, 2018 at 02:10:10PM +0100, Andrea Arcangeli wrote:
> It's still incredibly faster to shutdown part of the CPU temporarily
> than to flush its internal state as a whole with IBPB. If it wouldn't
> be the case ibrs_enabled 0 ibpb_enabled 2 special mode would perform
> better (but that's only enabled by default if SPEC_CTRL is not
> available and only IBPB_SUPPORT is).

Yet another bit of perhaps useful info: if you run 100% kernel
intensive computation (i.e. only interrupts on idle task or a kernel
driver doing all in kernel) ibrs_enabled 0 ibpb_enabled 2 of course is
much faster than ibrs_enabled 1 ibpb_enabled 1. As long as you don't
have frequent ring change, ibrs_enabled 0 ibpb_enabled 2 is faster.

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Andrea Arcangeli]

On Wed, Jan 10, 2018 at 01:47:22PM +0100, Jiri Kosina wrote:
> On Wed, 10 Jan 2018, Andrea Arcangeli wrote:
> 
> > Perhaps the confusing come from "less privileged prediction mode" and
> > you thought that meant "less privileged ring mode". It says "predction
> > mode" not ring 3.
> 
> Well, prediction mode is defined by "CPL3 vs CPL0-2" and "VMX root vs VMX 
> non-root", with obvious ordering of privileges.
> 
> So if IBRS is set, branch predictor will not allow the predicted target to 
> be influenced by code that executed in less privileged prediction mode 
> before value of '1' IBRS mode was last written to, and that's pretty much 
> it.

Which in current silicon IBP speculation is turned off always, and the
above specification really is to provide more finegrined semantics
for future silicon where it'll perform best to leave it always on and
it'll be still as secure as it is now despite the IBP speculation may
not always be turned off like it happens right now.

With all the prediction modes ordered right for the respective
guest/ring and CPUID will tell us when it's higher perf to enable
ibrs_enabled 2 ibpb_enabled 1 by default.

Again I see zero issues with leaving IBRS always on in current and
future silicon and I see absolutely zero problems in setting IBRS in
vmexit to prevent the whole guest mode to attack the kernel memory,
and in fact ibrs_enabled 2 will even more secure and it'll prevent the
gust mode userland even to attack the host qemu userland through
spectre variant#2.

As long as the "with obvious ordering of privileges" is maintained
when IBRS is not a total turn off of IBP speculation, everything works
as intended.

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2420 bytes --]

On Wed, 2018-01-10 at 13:57 +0100, Andrea Arcangeli wrote:
> On Wed, Jan 10, 2018 at 01:47:22PM +0100, Jiri Kosina wrote:
> > 
> > On Wed, 10 Jan 2018, Andrea Arcangeli wrote:
> > 
> > > 
> > > Perhaps the confusing come from "less privileged prediction mode" and
> > > you thought that meant "less privileged ring mode". It says "predction
> > > mode" not ring 3.
> > Well, prediction mode is defined by "CPL3 vs CPL0-2" and "VMX root vs VMX 
> > non-root", with obvious ordering of privileges.
> > 
> > So if IBRS is set, branch predictor will not allow the predicted target to 
> > be influenced by code that executed in less privileged prediction mode 
> > before value of '1' IBRS mode was last written to, and that's pretty much 
> > it.
> Which in current silicon IBP speculation is turned off always, and the
> above specification really is to provide more finegrined semantics
> for future silicon where it'll perform best to leave it always on and
> it'll be still as secure as it is now despite the IBP speculation may
> not always be turned off like it happens right now.
> 
> With all the prediction modes ordered right for the respective
> guest/ring and CPUID will tell us when it's higher perf to enable
> ibrs_enabled 2 ibpb_enabled 1 by default.
> 
> Again I see zero issues with leaving IBRS always on in current and
> future silicon and I see absolutely zero problems in setting IBRS in
> vmexit to prevent the whole guest mode to attack the kernel memory,
> and in fact ibrs_enabled 2 will even more secure and it'll prevent the
> gust mode userland even to attack the host qemu userland through
> spectre variant#2.
> 
> As long as the "with obvious ordering of privileges" is maintained
> when IBRS is not a total turn off of IBP speculation, everything works
> as intended.

Andrea, what you're saying is directly contradicting what I've heard
from Intel.

The documentation already distinguishes between IBRS on current
hardware, and IBRS_ATT on future hardware. If it was the case that IBRS
on current hardware is a set-and-forget option and completely disables
branch prediction, then they would say that. Rather than explicitly
saying the *opposite*, specifically for the case of current hardware,
as they do.

Rather than continuing to debate it, perhaps it's best just to wake for
the US to wake up, and Intel to give a definitive answer.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

RE: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Van De Ven, Arjan]

 
> Andrea, what you're saying is directly contradicting what I've heard
> from Intel.
> 
> The documentation already distinguishes between IBRS on current
> hardware, and IBRS_ATT on future hardware. If it was the case that IBRS
> on current hardware is a set-and-forget option and completely disables
> branch prediction, then they would say that. Rather than explicitly
> saying the *opposite*, specifically for the case of current hardware,
> as they do.
> 
> Rather than continuing to debate it, perhaps it's best just to wake for
> the US to wake up, and Intel to give a definitive answer.

On current hardware, you cannot just set IBRS always.


(In practice, on some you might get lucky if you try. Intel does not guarantee it. Intel does not test it. The model is to write the msr on privilege change, e.g. ring transition)

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Andrea Arcangeli]

On Wed, Jan 10, 2018 at 01:45:52PM +0000, Van De Ven, Arjan wrote:
>  
> > Andrea, what you're saying is directly contradicting what I've heard
> > from Intel.
> > 
> > The documentation already distinguishes between IBRS on current
> > hardware, and IBRS_ATT on future hardware. If it was the case that IBRS
> > on current hardware is a set-and-forget option and completely disables
> > branch prediction, then they would say that. Rather than explicitly
> > saying the *opposite*, specifically for the case of current hardware,
> > as they do.
> > 
> > Rather than continuing to debate it, perhaps it's best just to wake for
> > the US to wake up, and Intel to give a definitive answer.
> 
> On current hardware, you cannot just set IBRS always.

ibrs_enabled 1:

sets IBRS at vmexit and at kernel entry.
clears IBRS at kernel exit (return to usermode)
restores whatever IBRS value the guest was using at vmenter

ibrs_enabled 2:

sets IBRS always in host
sets IBRS if it wasn't already set by the guest in vmexit
restores whatever IBRS valeu the guest was using at vmenter

This matches the semantics described here by Tim patchset on lkml:

https://marc.info/?l=linux-kernel&m=151520606320646

If you what you say is correct, then you should discuss with Tim what
"echo 2 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in both
userspace and kernel" is doing or is supposed to achieve.

> (In practice, on some you might get lucky if you try. Intel does not guarantee it. Intel does not test it. The model is to write the msr on privilege change, e.g. ring transition)

ibrs_enabled 1 is the default always with SPEC_CTRL in cpuid.

The question is ibrs_enabled 2 optional mode but what is implemented
matches the semantics of the above patchset in link from Tim so again
you should talk with Tim and adjourn on the status of leaving IBRS
always on with current silicon.

I can tell in practice it works as I described in all microcodes
I tested.

Thanks,
Andrea

RE: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Van De Ven, Arjan]

> ibrs_enabled 2:
> 
> sets IBRS always in host

this is not secure

> This matches the semantics described here by Tim patchset on lkml:
> 
> https://marc.info/?l=linux-kernel&m=151520606320646

I will talk to Tim, it's not right.


 
> I can tell in practice it works as I described in all microcodes
> I tested.

have you tested all 80 ?

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Tim Chen]

On 01/10/2018 05:53 AM, Van De Ven, Arjan wrote:
>> ibrs_enabled 2:
>>
>> sets IBRS always in host
> 
> this is not secure
> 
>> This matches the semantics described here by Tim patchset on lkml:
>>
>> https://marc.info/?l=linux-kernel&m=151520606320646
> 
> I will talk to Tim, it's not right.
> 
> 

Yes, there's a misunderstanding on my part.  Leaving IBRS=1 all the
time may not provide full protection on all cpu models.

Tim

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Andrea Arcangeli]

On Wed, Jan 10, 2018 at 01:35:45PM -0800, Tim Chen wrote:
> time may not provide full protection on all cpu models.

All right no problem at all, it's fixed up.

Until very recently the majority of microcodes wasn't available in the
first place so I guess it's no big issue if in a subset of those the
IBRS barrier-like behavior wasn't immediately fully leveraged in all
cases. I'm just glad this detail was clarified sooner than later.

The IBRS barrier-like behavior and need to be set even when it's
already set, when changing mode to an higher privilege, is crystal
clear now.

Thanks,
Andrea

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Thomas Gleixner]

On Wed, 10 Jan 2018, David Woodhouse wrote:
> Andrea, what you're saying is directly contradicting what I've heard
> from Intel.
> 
> The documentation already distinguishes between IBRS on current
> hardware, and IBRS_ATT on future hardware. If it was the case that IBRS
> on current hardware is a set-and-forget option and completely disables
> branch prediction, then they would say that. Rather than explicitly
> saying the *opposite*, specifically for the case of current hardware,
> as they do.
> 
> Rather than continuing to debate it, perhaps it's best just to wake for
> the US to wake up, and Intel to give a definitive answer.

So here is the simple list of questions all to be answered with YES or
NO. I don't want to see any of the 'but, though ...'. We all know by now
that it's CPU dependent and slow and whatever and that IBRS_ATT will be in
future CPUs. So get your act together and tell a clear YES or NO.

1) Does IBRS=1 when set once act as a set-and-forget option ?

 1a) If the answer to #1 is yes, is it more secure than toggling it?

 1b) If the answer to #1 is yes, is retpoline required ?

 1c) If the answer to #1 is yes, is RSB stuffing required ?

2) Does toggle mode of IBRS require retpoline ?

3) Does toggle mode of IBRS require RSB stuffing ?

4) Exist CPUs which require IBRS to be selected automatically ?

   4b) If yes, provide the list as a separate answer please

Thanks,

	tglx

RE: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Van De Ven, Arjan]

> So here is the simple list of questions all to be answered with YES or
> NO. I don't want to see any of the 'but, though ...'. We all know by now
> that it's CPU dependent and slow and whatever and that IBRS_ATT will be in
> future CPUs. So get your act together and tell a clear YES or NO.

I will answer for current cpus, so those not having IBRS_ATT
> 
> 1) Does IBRS=1 when set once act as a set-and-forget option ?

NO

 
>  1a) If the answer to #1 is yes, is it more secure than toggling it?

NO
>
>  1b) If the answer to #1 is yes, is retpoline required ?

NO - Once you have IBRS done PROPERLY (toggled) you don't need retpoline
> 
>  1c) If the answer to #1 is yes, is RSB stuffing required ?

Only for the VM exit case

> 
> 2) Does toggle mode of IBRS require retpoline ?

NO

> 
> 3) Does toggle mode of IBRS require RSB stuffing ?

Only for the VM exit case
> 
> 4) Exist CPUs which require IBRS to be selected automatically ?

I do not understand your question exactly

RE: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Thomas Gleixner]

On Wed, 10 Jan 2018, Van De Ven, Arjan wrote:
> > So here is the simple list of questions all to be answered with YES or
> > NO. I don't want to see any of the 'but, though ...'. We all know by now
> > that it's CPU dependent and slow and whatever and that IBRS_ATT will be in
> > future CPUs. So get your act together and tell a clear YES or NO.

> > 2) Does toggle mode of IBRS require retpoline ?
> 
> NO
> 
> > 
> > 3) Does toggle mode of IBRS require RSB stuffing ?
> 
> Only for the VM exit case

Ok.

> > 
> > 4) Exist CPUs which require IBRS to be selected automatically ?
> 
> I do not understand your question exactly

Whether the kernel should enable IBRS by default on certain models. And if
yes, which ones.

Thanks,

	tglx

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1663 bytes --]

On Wed, 2018-01-10 at 14:46 +0100, Thomas Gleixner wrote:
> 
> So here is the simple list of questions all to be answered with YES or
> NO. I don't want to see any of the 'but, though ...'. We all know by now
> that it's CPU dependent and slow and whatever and that IBRS_ATT will be in
> future CPUs. So get your act together and tell a clear YES or NO.

This is actually covered by the documentation. Someone really should
send you a copy.

> 1) Does IBRS=1 when set once act as a set-and-forget option ?

Never on current hardware. In future with IBRS_ATT, yes.

>  1a) If the answer to #1 is yes, is it more secure than toggling it?

No, just faster.

>  1b) If the answer to #1 is yes, is retpoline required ?

No. We'll ALTERNATIVE it away if we have IBRS_ATT.

>  1c) If the answer to #1 is yes, is RSB stuffing required ?

Not for IBRS_ATT, with weasel words about requiring SMEP.

> 2) Does toggle mode of IBRS require retpoline ?

No. Retpoline is an *alternative* to IBRS, for protecting the kernel.

> 3) Does toggle mode of IBRS require RSB stuffing ?

Yes for kernel entry if you have no SMEP. And yes on vmexit.

> 4) Exist CPUs which require IBRS to be selected automatically ?
> 
>    4b) If yes, provide the list as a separate answer please

You mean CPUs on which retpoline isn't sufficient and thus the  kernel
should prefer IBRS "automatically" without a command line option?

As discussed, yes on Skylake and anything after it that doesn't have
IBRS_ATT, because there are tiny theoretical gaps that retpoline
doesn't handle. But the option of sacrificing goats may be perfectly
acceptable.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Andrea Arcangeli]

Hello,

On Wed, Jan 10, 2018 at 02:46:22PM +0100, Thomas Gleixner wrote:
> So here is the simple list of questions all to be answered with YES or
> NO. I don't want to see any of the 'but, though ...'. We all know by now
> that it's CPU dependent and slow and whatever and that IBRS_ATT will be in
> future CPUs. So get your act together and tell a clear YES or NO.

Other comments/code from Tim Chen, and Dave Hansen and most important
the ibrs_enabled 2 description and implementation on lkml, makes me
still wonder if even Arjan may have misunderstood some detail about
IBRS semantics too.

Tim and Dave please comment too, Tim you originally wrote that code
that leaves IBRS always on and never toggles it in the kernel entry
point so you must know full well if Arjan is correct that you must
toggle IBRS every time you enter kernel and in turn ibrs_enabled 2
isn't valid mode.

I can answer my understanding of IBRS so far that would make perfect
sense if that code posted made sense in the first place...

https://marc.info/?l=linux-kernel&m=151520606320646

> 
> 1) Does IBRS=1 when set once act as a set-and-forget option ?

Yes it can. Once set makes spectre variant#2 go away but only for IBP
BTB, stuff_RSB still needed and register hygiene too.

> 
>  1a) If the answer to #1 is yes, is it more secure than toggling it?

It's equivalent, not more secure nor less secure.

Simply IBRS only protects the code that runs with IBRS set. So you if
you leave it always set, you protect more code.

IBRS set is equivalent to the code having been compiled with reptoline
on CPUS where reptoline is equaivalent to IBRS.

The act of setting IBRS does nothing, it's not a barrier (IBPB is a
barrier and flushes the CPU internal state). After IBRS is set the
code is like if it's built with reptoline.

IBRS may reduce the performance measurably and this is why it has to
be disabled in userland and that is the motivation for using
ibrs_enabled 1 the default.

The toggling at kernel entry is purely required so userland keeps
running fast with IBRS not set by default. But it would be more secure
to leave IBRS always on which is what ibrs_enabled 2, it's just
incredibly slower with current silicon in some microcode
implementations, which is why it's not the default.

> 
>  1b) If the answer to #1 is yes, is retpoline required ?

reptoline is definitely not required whenever IBRS is set. IBRS set if
something is more secure than repotline as it's sure thing variant#2
is fixed and the microcode was applied if IBRS could be set.

> 
>  1c) If the answer to #1 is yes, is RSB stuffing required ?

RBS is totally different piece of the CPU not covered by IBRS
AFIK. I'm not sure about this though but doing also stuff_RSB wouldn't
hurt if the CPU has no microcode update applied.

SMEP set in cr4 obviates the need of stuff_RSB in kernel entry but not
in vmexit.

> 2) Does toggle mode of IBRS require retpoline ?

Toggle mode means nothing in terms of added security. Toggle mode
simply protects only kernel space.

If you use the toggle mode (ibrs_enabled 1 default) only kernel is
protected by IBRS and in turn you don't have to build the kernel with
reptoline.

If you leave IBRS always on it's effectively like having built
everything (kernel and all userland) with reptoline.

> 3) Does toggle mode of IBRS require RSB stuffing ?

Yes, again IBRS does nothing to prevent the RSB stuffing. Only SMEP
helps there if set in cr4 but only for kernel entry, vmexit still
needs stuff_RSB.

The stuff_RSB part I'm no certain but again it won't hurt to do it and
we do it even with IBRS on. It's not measurable though.

> 4) Exist CPUs which require IBRS to be selected automatically ?
> 
>    4b) If yes, provide the list as a separate answer please

Not sure if I get the question so not answering this one.

Future CPUs will have a new CPUID bit where enabling IBRS will perform
better than toggling it at kernel entry, so in those CPUs the most
secure mode (ibrs_enabled 2) that leaves IBRS always on, will perform
even better than the current default (ibrs_enabled 1). So in presence
of such future CPUID bit we should prefer ibrs_enabled 2 as default
and leave IBRS always on as default.

Thanks,
Andrea

RE: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Van De Ven, Arjan]

> Hello,
> 
> On Wed, Jan 10, 2018 at 02:46:22PM +0100, Thomas Gleixner wrote:
> > So here is the simple list of questions all to be answered with YES or
> > NO. I don't want to see any of the 'but, though ...'. We all know by now
> > that it's CPU dependent and slow and whatever and that IBRS_ATT will be in
> > future CPUs. So get your act together and tell a clear YES or NO.
> 
> Other comments/code from Tim Chen, and Dave Hansen and most important
> the ibrs_enabled 2 description and implementation on lkml, makes me
> still wonder if even Arjan may have misunderstood some detail about
> IBRS semantics too.

I spent the better part of the last 6 months in dungeons with CPU designers trying to to figure out what we could and could not do. I'm pretty darn sure I know the details.

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Dave Hansen]

On 01/10/2018 06:10 AM, Andrea Arcangeli wrote:
> Tim and Dave please comment too, Tim you originally wrote that code
> that leaves IBRS always on and never toggles it in the kernel entry
> point so you must know full well if Arjan is correct that you must
> toggle IBRS every time you enter kernel and in turn ibrs_enabled 2
> isn't valid mode.

Hi Andrea,

The "writing IBRS=1 acts as a barrier when it is already IBRS=1"
behavior is something which I misunderstood in the past.  Thanks, Arjan,
for clearing it up.

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Andrea Arcangeli]

On Wed, Jan 10, 2018 at 06:59:54AM -0800, Dave Hansen wrote:
> On 01/10/2018 06:10 AM, Andrea Arcangeli wrote:
> > Tim and Dave please comment too, Tim you originally wrote that code
> > that leaves IBRS always on and never toggles it in the kernel entry
> > point so you must know full well if Arjan is correct that you must
> > toggle IBRS every time you enter kernel and in turn ibrs_enabled 2
> > isn't valid mode.
> 
> Hi Andrea,
> 
> The "writing IBRS=1 acts as a barrier when it is already IBRS=1"
> behavior is something which I misunderstood in the past.  Thanks, Arjan,
> for clearing it up.

"writing IBRS=1 acts as a barrier when it is already IBRS=1" would
have been much clearer wording frankly. IBPB is IBP "Barrier", but
also IBRS is a barrier, no problem :).

So we'll add a dummy IBRS write to SPEC_CTRL in kernel entry and
vmexit so that it is compliant with all released microcodes that may
require it, also when ibrs_enabled is 2. Can you confirm?

Can you also tell if IBRS must be written as a barrier to SPEC_CTRL in
return to userland (kernel exit) when ibrs_enabled 2? Generally we
wouldn't run a barrier there with ibrs_enabled 2, but absolutely
nothing is intuitive here so I need to ask explicitly.

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 572 bytes --]

On Wed, 2018-01-10 at 16:13 +0100, Andrea Arcangeli wrote:
> 
> Can you also tell if IBRS must be written as a barrier to SPEC_CTRL in
> return to userland (kernel exit) when ibrs_enabled 2? Generally we
> wouldn't run a barrier there with ibrs_enabled 2, but absolutely
> nothing is intuitive here so I need to ask explicitly.

Since it achieves nothing¹ but to make userspace run slower, there's no
need to write it again on returning to userspace. It will perform that
function just fine without doing so.


(¹ that STIBP doesn't already do, and cheaper)

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Andrea Arcangeli]

On Wed, Jan 10, 2018 at 03:24:17PM +0000, David Woodhouse wrote:
> Since it achieves nothing¹ but to make userspace run slower, there's no
> need to write it again on returning to userspace. It will perform that
> function just fine without doing so.

Ok, very glad we are on the same page now.

Note that as far as I can tell there was no way to answer the above
question by reading the spec.

You also explicitly used the word barrier in association with IBRS
before, but there was no word barrier in the aforementioned specs in
association with IBRS (every word barrier was always and only in
association with IBPB).

I hope this discussion helped clear the additional barrier semantics
of IBRS in more understandable way for the current/future upstream
code.

Thanks,
Andrea

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1239 bytes --]

On Wed, 2018-01-10 at 16:47 +0100, Andrea Arcangeli wrote:
> On Wed, Jan 10, 2018 at 03:24:17PM +0000, David Woodhouse wrote:
> > Since it achieves nothing¹ but to make userspace run slower, there's no
> > need to write it again on returning to userspace. It will perform that
> > function just fine without doing so.
> 
> Ok, very glad we are on the same page now.
> 
> Note that as far as I can tell there was no way to answer the above
> question by reading the spec.

The spec does, I concede, leave something to be desired. This sentence
in particular — and it really is a single sentence — caused me to throw
my toys out of the pram and demand verbal explanations (which is
perhaps the only reason I managed to work it out):

"If IBRS is set, near returns and near indirect jumps/calls will not allow their
speculative target address to be controlled by code that executed in a less
privileged prediction mode before the IBRS mode was last written with a
value of 1 or on another logical processor so long as all RSB entries from the
previous less privileged prediction mode are overwritten"

But it *does* have the words "before the IBRS mode was last written
with a value of 1" in there somewhere... ;)

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]

Re: [patch RFC 5/5] x86/speculation: Add basic speculation control code

[Jiri Kosina]

On Wed, 10 Jan 2018, Andrea Arcangeli wrote:

> Which in current silicon IBP speculation is turned off always, 

So this is clearly the source of confusion.

If that's the case, what you are saying makes sense. Where is this 
information coming from? It definitely is not in the Intel documentation 
I've seen.

Thanks,

-- 
Jiri Kosina
SUSE Labs