[MODERATED] [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[MODERATED] [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[konrad.wilk]

We do a lot of things in the check_bugs() - one of the first
things we do is identify_boot_cpu() which calls identify_cpu()
which calls this_cpu->init.

Once identify_boot_cpu() is done _then_ it walks through the
spectre_v2_select_mitigation() and alternative_assembler().

If there are some CPU fix ups _after_ spectre_v2 is done
we can't activate those on the BSP as we have already
called 'this_cpu->init'. Hence add a new function to
fixup CPUs.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/include/asm/processor.h | 1 +
 arch/x86/kernel/cpu/bugs.c       | 1 +
 arch/x86/kernel/cpu/common.c     | 8 ++++++++
 arch/x86/kernel/cpu/cpu.h        | 1 +
 4 files changed, 11 insertions(+)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index b0ccd4847a58..b22bc9be2385 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -195,6 +195,7 @@ extern void init_amd_cacheinfo(struct cpuinfo_x86 *c);
 
 extern void detect_extended_topology(struct cpuinfo_x86 *c);
 extern void detect_ht(struct cpuinfo_x86 *c);
+extern void apply_cpu_fixes(void);
 
 #ifdef CONFIG_X86_32
 extern int have_cpuid_p(void);
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index b8513a93f4b8..f02443b331f5 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -80,6 +80,7 @@ void __init check_bugs(void)
 	if (!direct_gbpages)
 		set_memory_4k((unsigned long)__va(0), 1);
 #endif
+	apply_cpu_fixes();
 }
 
 /* The kernel command line selection */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index f7d80658cced..a59f7902a7fa 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -955,6 +955,12 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
 	setup_force_cpu_bug(X86_BUG_CPU_SSB);
 };
 
+void apply_cpu_fixes(void)
+{
+	if (this_cpu->c_bug_fix)
+		this_cpu->c_bug_fix(&boot_cpu_data);
+}
+
 /*
  * Do minimum CPU detection early.
  * Fields really needed: vendor, cpuid_level, family, model, mask,
@@ -1311,6 +1317,8 @@ static void identify_cpu(struct cpuinfo_x86 *c)
 #ifdef CONFIG_NUMA
 	numa_add_cpu(smp_processor_id());
 #endif
+	if (this_cpu->c_bug_fix)
+		this_cpu->c_bug_fix(c);
 }
 
 /*
diff --git a/arch/x86/kernel/cpu/cpu.h b/arch/x86/kernel/cpu/cpu.h
index e806b11a99af..7081634caac5 100644
--- a/arch/x86/kernel/cpu/cpu.h
+++ b/arch/x86/kernel/cpu/cpu.h
@@ -15,6 +15,7 @@ struct cpu_dev {
 	void		(*c_identify)(struct cpuinfo_x86 *);
 	void		(*c_detect_tlb)(struct cpuinfo_x86 *);
 	void		(*c_bsp_resume)(struct cpuinfo_x86 *);
+	void		(*c_bug_fix)(struct cpuinfo_x86 *);
 	int		c_x86_vendor;
 #ifdef CONFIG_X86_32
 	/* Optional vendor specific routine to obtain the cache size. */
-- 
2.14.3

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Borislav Petkov]

On Thu, Apr 19, 2018 at 10:25:47PM -0400, speck for konrad.wilk_at_oracle.com wrote:
> x86/cpu: Add fix_this_cpu to be called _after_ check_bugs and at BSP.
> 
> We do a lot of things in the check_bugs() - one of the first
> things we do is identify_boot_cpu() which calls identify_cpu()
> which calls this_cpu->init.
> 
> Once identify_boot_cpu() is done _then_ it walks through the
> spectre_v2_select_mitigation() and alternative_assembler().
> 
> If there are some CPU fix ups _after_ spectre_v2 is done
> we can't activate those on the BSP as we have already
> called 'this_cpu->init'. Hence add a new function to
> fixup CPUs.

Konrad, this is still unnecessary and adding superfluous complexity to
an already crazy early boot path. Let me clarify the flow:

First you are on the BSP:

setup_arch()
|-> early_cpu_init
|-> early_identify_cpu
|-> cpu_set_bug_bits

now you have all the X86_BUG bits set so that you can test them in the
functions later.

Then, you're still on the BSP and can do check_bugs() with all cmdline
options picking apart etc etc.

setup_arch()
|-> check_bugs()
|-> ssb_select_mitigation()
|-> identify_boot_cpu()			<--- HERE you call ->c_init() on the BSP
|-> spectre_v2_select_mitigation

In that order!

So, all you wanna do works without adding this new function pointer.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Konrad Rzeszutek Wilk]

On Fri, Apr 20, 2018 at 07:42:42PM +0200, speck for Borislav Petkov wrote:
> On Thu, Apr 19, 2018 at 10:25:47PM -0400, speck for konrad.wilk_at_oracle.com wrote:
> > x86/cpu: Add fix_this_cpu to be called _after_ check_bugs and at BSP.
> > 
> > We do a lot of things in the check_bugs() - one of the first
> > things we do is identify_boot_cpu() which calls identify_cpu()
> > which calls this_cpu->init.
> > 
> > Once identify_boot_cpu() is done _then_ it walks through the
> > spectre_v2_select_mitigation() and alternative_assembler().
> > 
> > If there are some CPU fix ups _after_ spectre_v2 is done
> > we can't activate those on the BSP as we have already
> > called 'this_cpu->init'. Hence add a new function to
> > fixup CPUs.
> 
> Konrad, this is still unnecessary and adding superfluous complexity to
> an already crazy early boot path. Let me clarify the flow:
> 
> First you are on the BSP:
> 
> setup_arch()
> |-> early_cpu_init
> |-> early_identify_cpu
> |-> cpu_set_bug_bits
> 
> now you have all the X86_BUG bits set so that you can test them in the
> functions later.

I am not worrying about the X86_BUG, I am thinking about the
X86_FEATURE_MDD. See below.
> 
> Then, you're still on the BSP and can do check_bugs() with all cmdline
> options picking apart etc etc.
> 
> setup_arch()
> |-> check_bugs()
> |-> ssb_select_mitigation()
> |-> identify_boot_cpu()			<--- HERE you call ->c_init() on the BSP
> |-> spectre_v2_select_mitigation
> 
> In that order!
> 
> So, all you wanna do works without adding this new function pointer.

With that I will need to add in early_init_amd the code to
set_cpu_cap(c, X86_FEATURE_MDD) based on reading the MSR_AMD64_LS_CFG.

That is needed as the 'ssb_select_mitigation' ends calling
ssb_parse_cmdline which immediately exits if:

381 static enum ssb_mitigation_cmd __init ssb_parse_cmdline(void)
..
391         if (!boot_cpu_has(X86_FEATURE_MDD))	<===
392                 return SSB_CMD_NONE;


Adding this:
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index e207eb2e8011..6a800a71b6c0 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -676,6 +676,15 @@ static void early_init_amd(struct cpuinfo_x86 *c)
                set_cpu_bug(c, X86_BUG_AMD_E400);
 
        early_detect_mem_encrypt(c);
+
+       switch (c->x86) {
+       case 0x15:
+       case 0x16:
+       case 0x17:
+               if (!rdmsrl_safe(MSR_AMD64_LS_CFG, &msr_MSR_AMD64_LS_CFG_val))
+                       set_cpu_cap(c, X86_FEATURE_MDD);
+       default:
+       }
 }
 
 static void init_amd_k8(struct cpuinfo_x86 *c)

in conjunction with your flow will make it work just fine.

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Borislav Petkov]

On Fri, Apr 20, 2018 at 11:27:07PM -0400, speck for Konrad Rzeszutek Wilk wrote:
> Adding this:
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index e207eb2e8011..6a800a71b6c0 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -676,6 +676,15 @@ static void early_init_amd(struct cpuinfo_x86 *c)
>                 set_cpu_bug(c, X86_BUG_AMD_E400);
>  
>         early_detect_mem_encrypt(c);
> +
> +       switch (c->x86) {
> +       case 0x15:
> +       case 0x16:
> +       case 0x17:
> +               if (!rdmsrl_safe(MSR_AMD64_LS_CFG, &msr_MSR_AMD64_LS_CFG_val))
> +                       set_cpu_cap(c, X86_FEATURE_MDD);

	if (c->x86 >= 0x15 && c->x86 <= 0x17)
		set_cpu_cap(c, X86_FEATURE_MDD);

is enough.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Konrad Rzeszutek Wilk]

On Sat, Apr 21, 2018 at 11:03:45AM +0200, speck for Borislav Petkov wrote:
> On Fri, Apr 20, 2018 at 11:27:07PM -0400, speck for Konrad Rzeszutek Wilk wrote:
> > Adding this:
> > diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> > index e207eb2e8011..6a800a71b6c0 100644
> > --- a/arch/x86/kernel/cpu/amd.c
> > +++ b/arch/x86/kernel/cpu/amd.c
> > @@ -676,6 +676,15 @@ static void early_init_amd(struct cpuinfo_x86 *c)
> >                 set_cpu_bug(c, X86_BUG_AMD_E400);
> >  
> >         early_detect_mem_encrypt(c);
> > +
> > +       switch (c->x86) {
> > +       case 0x15:
> > +       case 0x16:
> > +       case 0x17:
> > +               if (!rdmsrl_safe(MSR_AMD64_LS_CFG, &msr_MSR_AMD64_LS_CFG_val))
> > +                       set_cpu_cap(c, X86_FEATURE_MDD);
> 
> 	if (c->x86 >= 0x15 && c->x86 <= 0x17)
> 		set_cpu_cap(c, X86_FEATURE_MDD);

<nods>

Perhaps instead of X86_FEATURE_MDD we should set on AMD only this CPU flag:
	X86_FEATURE_SSBD

And on Intel have both:
	X86_FEATURE_MDD
	X86_FEATURE_SSBD

As the MDD SPEC_CTRL is not available on AMD, at least now - it may be
in the future (<tries reading tea leaves>)
> 
> is enough.
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
> -- 

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Borislav Petkov]

On Sat, Apr 21, 2018 at 08:21:17AM -0400, speck for Konrad Rzeszutek Wilk wrote:
> Perhaps instead of X86_FEATURE_MDD we should set on AMD only this CPU flag:
> 	X86_FEATURE_SSBD
> 
> And on Intel have both:
> 	X86_FEATURE_MDD
> 	X86_FEATURE_SSBD

I prefer X86_FEATURE_MDD and X86_FEATURE_USE_MDD which is
straight-forward and in line with the other mitigations. And we query
the USE_MDD flag only. The MDD one will be for /proc/cpuinfo and USE_MDD
used by the code.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Linus Torvalds]

On Sat, 21 Apr 2018, speck for Borislav Petkov wrote:
> 
> I prefer X86_FEATURE_MDD and X86_FEATURE_USE_MDD which is
> straight-forward and in line with the other mitigations. And we query
> the USE_MDD flag only. The MDD one will be for /proc/cpuinfo and USE_MDD
> used by the code.

At the risk of bike shedding, can we *please* just write out "Store buffer 
bypass" and "Memory disambiguation" instead of using SBB amd MD?

The patches aren't so big, and these names won't be used so much that we 
can't use longer names. Yes, yes, we use PTI for the page table isolation, 
but that had a much wider impact. For Spectre v1 we at least use _half_ 
words like "nospec".

Maybe even X86_FEATURE_STBUF_BYPASS?

And X86_FEATURE_STBUF_BYPASS_MITIGATE?

And the same goes for the kernel command line. Saying "ssb=auto" is not 
helpful to *anybody*.

Nobody is going to get carpal tunnel syndrome from typing that too often. 
I guarantee it.

                Linus

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Borislav Petkov]

On Sat, Apr 21, 2018 at 02:41:13PM -0700, speck for Linus Torvalds wrote:
> At the risk of bike shedding, can we *please* just write out "Store buffer 
> bypass" and "Memory disambiguation" instead of using SBB amd MD?
> 
> The patches aren't so big, and these names won't be used so much that we 
> can't use longer names. Yes, yes, we use PTI for the page table isolation, 
> but that had a much wider impact. For Spectre v1 we at least use _half_ 
> words like "nospec".
> 
> Maybe even X86_FEATURE_STBUF_BYPASS?
> 
> And X86_FEATURE_STBUF_BYPASS_MITIGATE?

So those strings after X86_FEATURE will land in /proc/cpuinfo and
traditionally, we have kept the feature names there abbreviated. Well,
at least most of them.

The only feature where this matters, though, is the memory
disambiguation feature which should be in /proc/cpuinfo, IMO, so should
we call it

	X86_FEATURE_MEM_DISAMBG

or so?

/proc/cpuinfo will have then "mem_disambg" or we can put the string we wanna
have in "":

#define X86_FEATURE_MEM_DISAMBG		...	/* "string_we_wanna_have" memory disambiguation */

The remaining ones are synthetic ones and they can be as long as
we want them to be as we won't show them in /proc/cpuinfo. The
current state of the mitigation will be, as with the other bugs, in
/sys/devices/system/cpu/vulnerabilities/

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Jon Masters]

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

On 04/21/2018 05:41 PM, speck for Linus Torvalds wrote:

> On Sat, 21 Apr 2018, speck for Borislav Petkov wrote:
>>
>> I prefer X86_FEATURE_MDD and X86_FEATURE_USE_MDD which is
>> straight-forward and in line with the other mitigations. And we query
>> the USE_MDD flag only. The MDD one will be for /proc/cpuinfo and USE_MDD
>> used by the code.
> 
> At the risk of bike shedding, can we *please* just write out "Store buffer 
> bypass" and "Memory disambiguation" instead of using SBB amd MD?

Very reasonable.

> The patches aren't so big, and these names won't be used so much that we 
> can't use longer names. Yes, yes, we use PTI for the page table isolation, 
> but that had a much wider impact. For Spectre v1 we at least use _half_ 
> words like "nospec".
> 
> Maybe even X86_FEATURE_STBUF_BYPASS?
> 
> And X86_FEATURE_STBUF_BYPASS_MITIGATE?
> And the same goes for the kernel command line. Saying "ssb=auto" is not 
> helpful to *anybody*.

Can you make a call on the above now? We're happy to go along with
whatever but need to get the other arches aligned behind whatever we're
doing (IBM p, z, and Arm are working on patches as well as others), and
then crank on the distro patches based upon whatever is agreed here.

So might I ask your opinion of the following list of terms/uses?

(we've been previously asked by Thomas to retain the inverted logic - so
you enable a mitigation which disables a CPU performance optimization)

* spec_store_bypass_disable=auto/off/on
* /sys/devices/system/cpu/vulnerabilities/speculative_store_bypass
* X86_FEATURE_SPEC_STORE_BYPASS_DISABLE
* X86_FEATURE_USE_SPEC_STORE_BYPASS_DISABLE

> Nobody is going to get carpal tunnel syndrome from typing that too often. 
> I guarantee it.

:)

Jon.

P.S. I didn't say "stbuf" in the above because it's not required that a
uarch implement only speculation on the store buffer. It could be that a
uarch uses an integrated load/store queue (LSQ) and searches that, or it
could be that stores are tracked in another manner (e.g. IBM). All the
exploit relies upon is close-to-retirement store conflict handling. And
we have several variants of this exploit coming up that rely upon slight
twists of store-to-load forwarding. I expect those will come up here.

-- 
Computer Architect | Sent from my Fedora powered laptop

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Jon Masters]

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

On 04/21/2018 06:09 PM, speck for Borislav Petkov wrote:

<snip>

> The only feature where this matters, though, is the memory
> disambiguation feature which should be in /proc/cpuinfo, IMO, so should
> we call it
> 
> 	X86_FEATURE_MEM_DISAMBG
> 
> or so?

I propose keeping either the x86 CPU capability "Memory Disambiguation"
(as you said, via the string in the define). But only for what is
displayed in /proc/cpuinfo. It seems reasonable to expect people reading
that and caring about store buffer exploits to at least have some idea
what "MD" means without having to map it to SSB directly there.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Borislav Petkov]

On Sat, Apr 21, 2018 at 06:13:16PM -0400, speck for Jon Masters wrote:
> I propose keeping either the x86 CPU capability "Memory Disambiguation"
> (as you said, via the string in the define). But only for what is
> displayed in /proc/cpuinfo.

Actually, thinking about this more, we don't need to have any strings
in /proc/cpuinfo denoting the CPU has the memory disambiguation feature
because we only want to know whether we can *disable* the MD. And that
we can do only on a subset of CPUs. IOW, there are other CPU models
which *might* have MD but where MD cannot be disabled... yet.

So having MD or whatever string we agree upon in /proc/cpuinfo will be a
lie.

IOW, we only need two flags:

X86_FEATURE_STBUF_BYPASS
X86_FEATURE_STBUF_BYPASS_MITIGATE

or whatever those are going to be called and neither of the two should
be visible in /proc/cpuinfo.

SSB state will be visible in /sys/devices/system/cpu/vulnerabilities/

Yap, I think that should work.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Jon Masters]

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

On 04/21/2018 06:35 PM, speck for Borislav Petkov wrote:
> On Sat, Apr 21, 2018 at 06:13:16PM -0400, speck for Jon Masters wrote:
>> I propose keeping either the x86 CPU capability "Memory Disambiguation"
>> (as you said, via the string in the define). But only for what is
>> displayed in /proc/cpuinfo.
> 
> Actually, thinking about this more, we don't need to have any strings
> in /proc/cpuinfo denoting the CPU has the memory disambiguation feature
> because we only want to know whether we can *disable* the MD. And that
> we can do only on a subset of CPUs. IOW, there are other CPU models
> which *might* have MD but where MD cannot be disabled... yet.
> 
> So having MD or whatever string we agree upon in /proc/cpuinfo will be a
> lie.
> 
> IOW, we only need two flags:
> 
> X86_FEATURE_STBUF_BYPASS
> X86_FEATURE_STBUF_BYPASS_MITIGATE
> 
> or whatever those are going to be called and neither of the two should
> be visible in /proc/cpuinfo.
> 
> SSB state will be visible in /sys/devices/system/cpu/vulnerabilities/
> 
> Yap, I think that should work.

All sounds reasonable. You could display "MDD" if you like in
/proc/cpuinfo later but since that's ABI (on some level) not adding it
for now is safe for the reasons given above. Hopefully Linus can let us
know if he likes X86_FEATURE_SPEC_STORE_BYPASS vs the "STBUF" version.

I'm going over Konrad's patches at the moment trying to integrate some
of the suggestions from the past 24 hours and will send him what I come
up with tonight in case that's helpful to him. I've got syncs setup with
IBM p and z and Arm early next week so can convey whatever is agreed.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Linus Torvalds]

On Sat, 21 Apr 2018, speck for Jon Masters wrote:
> 
> All sounds reasonable. You could display "MDD" if you like in
> /proc/cpuinfo later but since that's ABI (on some level) not adding it
> for now is safe for the reasons given above. Hopefully Linus can let us
> know if he likes X86_FEATURE_SPEC_STORE_BYPASS vs the "STBUF" version.

I think "SPEC_STORE_BYPASS" is fine. Sounds better than STBUF to me - but 
I just don't want to see "sbbd" or "mdd" in user-visible places (and 
honestly, the less I see of it in the source is probably better too).

When I saw "sbb" I was like "don't we already have that"? But no, it's ssb 
(Sonics Silicon Backplane). Regardless, it's one of those very nondescript 
TLA's that aren't so common that anybody will ever remember them.

And "MD" is worse, since it means something entirely different, so now the 
extra "D" for "Disable" needs to be part of the name.

But "SPEC_STORE_BYPASS" sounds fine. I'm already over "spec" meaning 
something else to me than "speculation", it's about as well as we can 
shorten it.

              Linus

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Jon Masters]

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

On 04/21/2018 09:26 PM, speck for Linus Torvalds wrote:
> 
> 
> On Sat, 21 Apr 2018, speck for Jon Masters wrote:
>>
>> All sounds reasonable. You could display "MDD" if you like in
>> /proc/cpuinfo later but since that's ABI (on some level) not adding it
>> for now is safe for the reasons given above. Hopefully Linus can let us
>> know if he likes X86_FEATURE_SPEC_STORE_BYPASS vs the "STBUF" version.
> 
> I think "SPEC_STORE_BYPASS" is fine. Sounds better than STBUF to me - but 
> I just don't want to see "sbbd" or "mdd" in user-visible places (and 
> honestly, the less I see of it in the source is probably better too).

Ok. We'll go with SPEC_STORE_BYPASS and "spec_store_bypass_disable=".

> When I saw "sbb" I was like "don't we already have that"? But no, it's ssb 
> (Sonics Silicon Backplane). Regardless, it's one of those very nondescript 
> TLA's that aren't so common that anybody will ever remember them.

:)

> But "SPEC_STORE_BYPASS" sounds fine. I'm already over "spec" meaning 
> something else to me than "speculation", it's about as well as we can 
> shorten it.

You and all of us ;)

I'll let Arm, and IBM know to target "spec_store_bypass_disable" and
"nospec_store_bypass_disable". I've syncs with them early next week.

Btw, in case you missed it earlier, on the Arm side, Will Deacon has
point, IBM p is being lead by Michael Neuling, Anton B, and Nick P (with
I think some involvement from Ben), and z is Martin S. We also have a
bunch of Arm vendors involved but Will can be public face for that.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Borislav Petkov]

On Sat, Apr 21, 2018 at 11:18:06PM -0400, speck for Jon Masters wrote:
> Ok. We'll go with SPEC_STORE_BYPASS and "spec_store_bypass_disable=".

Well, I'd prefer

spec_store_bypass=[enable|disable|auto] etc

as it is much more flexible and versatile as an option instead of an
option having "disable" or "enable" in the name.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Jon Masters]

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

On 04/22/2018 05:35 AM, speck for Borislav Petkov wrote:
> On Sat, Apr 21, 2018 at 11:18:06PM -0400, speck for Jon Masters wrote:
>> Ok. We'll go with SPEC_STORE_BYPASS and "spec_store_bypass_disable=".
> 
> Well, I'd prefer
> 
> spec_store_bypass=[enable|disable|auto] etc
> 
> as it is much more flexible and versatile as an option instead of an
> option having "disable" or "enable" in the name.

I'd prefer that too. But we had this conversation with Thomas already
and he wanted a "disable" option. It was fairly clearly spelled out that
it was to be "mdd" or "ssbd" and therefore "spec_store_bypass_disable".

So that's why it's been going down that path, in line with Spectre-v2
mitigation also applying a firmware interface to disable a feature.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Borislav Petkov]

On Sun, Apr 22, 2018 at 05:53:50AM -0400, speck for Jon Masters wrote:
> I'd prefer that too. But we had this conversation with Thomas already
> and he wanted a "disable" option. It was fairly clearly spelled out that
> it was to be "mdd" or "ssbd" and therefore "spec_store_bypass_disable".

No, he wants "on,off,auto" so that we're in-line with PTI's and
spectre_v2's options. So let's do:

spec_store_bypass=[on|off|auto]

and be done with the bikeshedding. The weather's too nice outside.

:-)

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Jon Masters]

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

On 04/22/2018 06:34 AM, speck for Borislav Petkov wrote:
> On Sun, Apr 22, 2018 at 05:53:50AM -0400, speck for Jon Masters wrote:
>> I'd prefer that too. But we had this conversation with Thomas already
>> and he wanted a "disable" option. It was fairly clearly spelled out that
>> it was to be "mdd" or "ssbd" and therefore "spec_store_bypass_disable".
> 
> No, he wants "on,off,auto" so that we're in-line with PTI's and
> spectre_v2's options. So let's do:

I don't think so :)

spectre_v2 is about *disabling* speculation in order to prevent use of
branch predictor poison (or, on other arches, disabling the indirect
predictor at least across privilege levels), and performing barriers on
context switch. When you set spectre_v2=on or spectre_v2=auto you are
*disabling* indirect predictor functionality, and when you set
spectre_v2=off you're *enabling* (leaving on) indirect predictor
functionality. It's inverted logic from what you might expect if you
were thinking about it from the PoV of the indirect predictor.

Similarly, with spec_store_bypass_disable, you are *disabling*
speculative store bypassing. When you set spec_store_bypass_disable=on
you are *disabling* bypassing of store, and when you set
spec_store_bypass_disable=off, you are *enabling* (leaving on)
speculative store bypassing. It's inverted logic again.

> spec_store_bypass=[on|off|auto]

So I'm happy to change it yet again, but let's only do so if we all get
why the above was done, and if Thomas and Linus agree. There was a
specific reason we made it follow the SPECTRE_V2 inverted logic.

> and be done with the bikeshedding. The weather's too nice outside.

Hah. It's nice outside here in MA too, and I want to run, but I know the
day is going to be spent mostly on various emergency patches ;)

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Thomas Gleixner]

On Sun, 22 Apr 2018, speck for Borislav Petkov wrote:

> On Sun, Apr 22, 2018 at 05:53:50AM -0400, speck for Jon Masters wrote:
> > I'd prefer that too. But we had this conversation with Thomas already
> > and he wanted a "disable" option. It was fairly clearly spelled out that
> > it was to be "mdd" or "ssbd" and therefore "spec_store_bypass_disable".
> 
> No, he wants "on,off,auto" so that we're in-line with PTI's and
> spectre_v2's options. So let's do:
> 
> spec_store_bypass=[on|off|auto]
> 
> and be done with the bikeshedding. The weather's too nice outside.

Nope. You're reverting the meaning again.

All mitigation options so far have on|off|auto where:

 - on:   enables the mitigation
 - off:  disabled the mitigation
 - auto: lets the kernel decided whether to enable/disable it

Now your name is: spec_store_bypass which is a performance feature and not
the mitigation. So Jon is about right that spec_store_bypass_disable
describes the mitigation and the on/off/auto have then consistent behaviour
with kpti and spectre_v2, which admittedly could have named better.

Thanks,

	tglx

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Jon Masters]

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

On 04/23/2018 10:30 AM, speck for Thomas Gleixner wrote:
> On Sun, 22 Apr 2018, speck for Borislav Petkov wrote:
> 
>> On Sun, Apr 22, 2018 at 05:53:50AM -0400, speck for Jon Masters wrote:
>>> I'd prefer that too. But we had this conversation with Thomas already
>>> and he wanted a "disable" option. It was fairly clearly spelled out that
>>> it was to be "mdd" or "ssbd" and therefore "spec_store_bypass_disable".
>>
>> No, he wants "on,off,auto" so that we're in-line with PTI's and
>> spectre_v2's options. So let's do:
>>
>> spec_store_bypass=[on|off|auto]
>>
>> and be done with the bikeshedding. The weather's too nice outside.
> 
> Nope. You're reverting the meaning again.
> 
> All mitigation options so far have on|off|auto where:
> 
>  - on:   enables the mitigation
>  - off:  disabled the mitigation
>  - auto: lets the kernel decided whether to enable/disable it
> 
> Now your name is: spec_store_bypass which is a performance feature and not
> the mitigation. So Jon is about right that spec_store_bypass_disable
> describes the mitigation and the on/off/auto have then consistent behaviour
> with kpti and spectre_v2, which admittedly could have named better.

Thanks.

So, let's all agree to keep spec_store_bypass_disable. It'll help to
keep the distro knobs properly consistent with upstream.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Jon Masters]

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

On 04/23/2018 10:34 AM, speck for Jon Masters wrote:
> On 04/23/2018 10:30 AM, speck for Thomas Gleixner wrote:
>> On Sun, 22 Apr 2018, speck for Borislav Petkov wrote:
>>
>>> On Sun, Apr 22, 2018 at 05:53:50AM -0400, speck for Jon Masters wrote:
>>>> I'd prefer that too. But we had this conversation with Thomas already
>>>> and he wanted a "disable" option. It was fairly clearly spelled out that
>>>> it was to be "mdd" or "ssbd" and therefore "spec_store_bypass_disable".
>>>
>>> No, he wants "on,off,auto" so that we're in-line with PTI's and
>>> spectre_v2's options. So let's do:
>>>
>>> spec_store_bypass=[on|off|auto]
>>>
>>> and be done with the bikeshedding. The weather's too nice outside.
>>
>> Nope. You're reverting the meaning again.
>>
>> All mitigation options so far have on|off|auto where:
>>
>>  - on:   enables the mitigation
>>  - off:  disabled the mitigation
>>  - auto: lets the kernel decided whether to enable/disable it
>>
>> Now your name is: spec_store_bypass which is a performance feature and not
>> the mitigation. So Jon is about right that spec_store_bypass_disable
>> describes the mitigation and the on/off/auto have then consistent behaviour
>> with kpti and spectre_v2, which admittedly could have named better.
> 
> Thanks.
> 
> So, let's all agree to keep spec_store_bypass_disable. It'll help to
> keep the distro knobs properly consistent with upstream.

Additional: I spoke with Intel a few minutes ago to impress upon them
that we'll (RH) have to leave MD disabled by default unless they drive
the prctrl/other solutions for vulnerable processes they'd like to see.

We'd actually like to see an ability to keep MD on globally, and just
turn it off for things like OpenJDK (which our Java lead is still
considering all of the possible exploits against using SSB). The
browsers will use process isolation, and I know Andi is k(l)een on
seccomp being a per process knob (though that alone isn't sufficient for
every process that would need patching). That's all great, but it's up
to Intel to articulate this all now and provide the support for these.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Konrad Rzeszutek Wilk]

> 
> Additional: I spoke with Intel a few minutes ago to impress upon them
> that we'll (RH) have to leave MD disabled by default unless they drive
> the prctrl/other solutions for vulnerable processes they'd like to see.

I believe (and Linus, please correct me here) that the question of
toggling on/off SPEC_CTRL MSR on user-space entrance is a no-go.

Hence doing any work here is kind of pointless as it won't get accepted
upstream. The best we can hope is for the distros to come up with
some prctl that we need to live with and which can be reserved
in the upstream kernel.

> 
> We'd actually like to see an ability to keep MD on globally, and just
> turn it off for things like OpenJDK (which our Java lead is still
> considering all of the possible exploits against using SSB). The
> browsers will use process isolation, and I know Andi is k(l)een on
> seccomp being a per process knob (though that alone isn't sufficient for
> every process that would need patching). That's all great, but it's up
> to Intel to articulate this all now and provide the support for
> these.

They did. That is what this SPEC_CTRL MSR knob is for. But perhaps
you had something else on mind too?

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Jon Masters]

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

On 04/23/2018 01:51 PM, speck for Konrad Rzeszutek Wilk wrote:
>>
>> Additional: I spoke with Intel a few minutes ago to impress upon them
>> that we'll (RH) have to leave MD disabled by default unless they drive
>> the prctrl/other solutions for vulnerable processes they'd like to see.
> 
> I believe (and Linus, please correct me here) that the question of
> toggling on/off SPEC_CTRL MSR on user-space entrance is a no-go.

To clarify, it's not a userspace toggling of an MSR. It would be a prctl
(e.g. a new "SPECULATION_VULNERABILITY" major with some minor variants)
that would allow processes to say "I'm vulnerable to X" or somesuch.

On x86, that might then allow us to have the mechanics of globally
enabling MDD, disabling it on entry to the kernel (due to stack attack,
to be debated), and selectively disabling it for known vulnerable
processes. Yea, it's a lot more complicated.

Anyway, I'm personally ok with a global knob. It's just that we're
getting a lot of pressure from Intel and AMD to not do that. I've asked
Intel to talk with Thomas and Linus and represent their opinions here
because I don't want it to seem like this is my asking for knobs! ;)

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Jon Masters]

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

On 04/23/2018 02:01 PM, speck for Jon Masters wrote:
> On 04/23/2018 01:51 PM, speck for Konrad Rzeszutek Wilk wrote:
>>>
>>> Additional: I spoke with Intel a few minutes ago to impress upon them
>>> that we'll (RH) have to leave MD disabled by default unless they drive
>>> the prctrl/other solutions for vulnerable processes they'd like to see.
>>
>> I believe (and Linus, please correct me here) that the question of
>> toggling on/off SPEC_CTRL MSR on user-space entrance is a no-go.
> 
> To clarify, it's not a userspace toggling of an MSR. It would be a prctl
> (e.g. a new "SPECULATION_VULNERABILITY" major with some minor variants)
> that would allow processes to say "I'm vulnerable to X" or somesuch.
> 
> On x86, that might then allow us to have the mechanics of globally
> enabling MDD, disabling it on entry to the kernel (due to stack attack,
           ^^^ MD not MDD here (two all nighters in a row)
> to be debated), and selectively disabling it for known vulnerable
> processes. Yea, it's a lot more complicated.
> 
> Anyway, I'm personally ok with a global knob. It's just that we're
> getting a lot of pressure from Intel and AMD to not do that. I've asked
> Intel to talk with Thomas and Linus and represent their opinions here
> because I don't want it to seem like this is my asking for knobs! ;)
> 
> Jon.
> 


-- 
Computer Architect | Sent from my Fedora powered laptop

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Linus Torvalds]

On Mon, 23 Apr 2018, speck for Konrad Rzeszutek Wilk wrote:
> 
> I believe (and Linus, please correct me here) that the question of
> toggling on/off SPEC_CTRL MSR on user-space entrance is a no-go.

Absolutely. That would be entirely crazy. Is there a known situation where 
that would actually make sense?

                 Linus

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Jon Masters]

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

On 04/23/2018 02:05 PM, speck for Linus Torvalds wrote:
> 
> 
> On Mon, 23 Apr 2018, speck for Konrad Rzeszutek Wilk wrote:
>>
>> I believe (and Linus, please correct me here) that the question of
>> toggling on/off SPEC_CTRL MSR on user-space entrance is a no-go.
> 
> Absolutely. That would be entirely crazy.

Yep, totally nuts, and nobody is asking for that :)

> Is there a known situation where that would actually make sense?

Intel point out that globally disabling MD by default has typically a
few percent hit, but sometimes up to 30%. Therefore, they want it to be
per-process controllable. As I said, e.g. with a prctrl that happens to
control an MSR underneath, but it's not userspace setting an MSR.

Anyway, Intel should articulate the ask. I've pointed it out because
there's entirely a lack of alignment currently with both Intel and AMD
wanting MD on by default (so some parts of the system vulnerable until
we have a fine grained option available). It's great that they want it,
but they need to help find a way to make that work.

And my person opinion is that seccomp alone isn't enough of a criteria
(Andi was proposing to just whack the MDD in that case, but this relies
on all manner of userspace applications using seccomp that weren't). I
think a simple prctrl is probably all we would have time to hack in to
common stuff like OpenJDK. There's really not much time to do that, and
a total lack of folks articulating that need loudly.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Konrad Rzeszutek Wilk]

On Mon, Apr 23, 2018 at 11:05:07AM -0700, speck for Linus Torvalds wrote:
> 
> 
> On Mon, 23 Apr 2018, speck for Konrad Rzeszutek Wilk wrote:
> > 
> > I believe (and Linus, please correct me here) that the question of
> > toggling on/off SPEC_CTRL MSR on user-space entrance is a no-go.
> 
> Absolutely. That would be entirely crazy. Is there a known situation where 
> that would actually make sense?

For Skylake where they report via X86_FEATURE_ARCH_CAPABILITIES
RBSA Bit(2) (see 5.3 where they introduce it):

https://software.intel.com/sites/default/files/managed/1d/46/Retpoline-A-Branch-Target-Injection-Mitigation.pdf

Where they state that retpoline is not good enough for a list of "may"
conditions.

> 
>                  Linus

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Linus Torvalds]

On Mon, 23 Apr 2018, speck for Konrad Rzeszutek Wilk wrote:
> > 
> > Absolutely. That would be entirely crazy. Is there a known situation where 
> > that would actually make sense?
> 
> For Skylake where they report via X86_FEATURE_ARCH_CAPABILITIES
> RBSA Bit(2) (see 5.3 where they introduce it):
> 
> https://software.intel.com/sites/default/files/managed/1d/46/Retpoline-A-Branch-Target-Injection-Mitigation.pdf
> 
> Where they state that retpoline is not good enough for a list of "may"
> conditions.

Yeah, I'm not in the least interested in theory. 

Are there actual real attacks where it makes sense?

In particular since we actually have the RSB stuffing workarounds too, not 
just bare retpoline. Which leaves the "call stacks deeper than 16" case as 
the only actual possible case even in theory, and then you have to be able 
to attack it too.

Because we're not taking another 30% performance hit on kernel entry on 
theory. You may not care, and there are a lot of crazy security people who 
may not care, but real people do.  Performance matters. We're not doing 
insane things in kernel entry for insane reasons.

So let insane people take the insane patches. That is *NOT* a valid reason 
for them going upstream.

Anyway, I was wondering if there was any reason why we'd care for the 
store buffer bypass problem? The whole "run with store buffers in user 
space, disable them in kernel space" model seems insane.

                 Linus

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Jiri Kosina]

On Mon, 23 Apr 2018, speck for Linus Torvalds wrote:

> Yeah, I'm not in the least interested in theory. 
> 
> Are there actual real attacks where it makes sense?
> 
> In particular since we actually have the RSB stuffing workarounds too, not 
> just bare retpoline. Which leaves the "call stacks deeper than 16" case as 
> the only actual possible case even in theory, and then you have to be able 
> to attack it too.

BTW in addition to that, there is this gcc patch (I don't think any distro 
is shipping it yet in any way though):

	https://github.com/clearlinux-pkgs/gcc/blob/master/zero-regs.patch

that should make any such theoretical attack even harder (it zeroes all 
clobbered GPRs before ret, which is cheap, and dramatically reduces the 
potential of introducing "bad" dependencies).

> Anyway, I was wondering if there was any reason why we'd care for the 
> store buffer bypass problem? The whole "run with store buffers in user 
> space, disable them in kernel space" model seems insane.

This is all just about defining the security domain boundaries. If we 
hypothetically disable MD on kernel entry, then it's impossible for ring3 
store to badly influence ring0 load.
But absolutely yeah, there are many other scenarios that need protecting 
as well (userspace-userspace, guest-host, etc), which'd stay unprotected 
with that model.

-- 
Jiri Kosina
SUSE Labs

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Andi Kleen]

On Mon, Apr 23, 2018 at 02:23:17PM -0700, speck for Linus Torvalds wrote:
> 
> 
> On Mon, 23 Apr 2018, speck for Konrad Rzeszutek Wilk wrote:
> > > 
> > > Absolutely. That would be entirely crazy. Is there a known situation where 
> > > that would actually make sense?
> > 
> > For Skylake where they report via X86_FEATURE_ARCH_CAPABILITIES
> > RBSA Bit(2) (see 5.3 where they introduce it):
> > 
> > https://software.intel.com/sites/default/files/managed/1d/46/Retpoline-A-Branch-Target-Injection-Mitigation.pdf
> > 
> > Where they state that retpoline is not good enough for a list of "may"
> > conditions.
> 
> Yeah, I'm not in the least interested in theory. 
> 
> Are there actual real attacks where it makes sense?

We don't know of any. It's all theory. And we know if it's possible
it's really really hard.

FWIW I spent quite some time writing fairly full coverage patches
for automatic deep call chain mitigation using function patching
(see [1] and [2]), but so far nobody has managed to do a exploit.

So I agree with you there is right now no good reason to pursue
anything beyond retpoline for v2 at this point.

If someone really manages a practical exploit we have options though.

> Anyway, I was wondering if there was any reason why we'd care for the 
> store buffer bypass problem? The whole "run with store buffers in user 
> space, disable them in kernel space" model seems insane.

MDD (or now called RDS) is a bit different than IBRS. IBRS requires
barrier semantics on every entry, so you absolutely have to take
the risk.

But RDS is actually a mode so if it's needed it would be possible
to use hysteresis: keep a counter and if there are too many
kernel entries in a jiffie just keep it on until the next timer interrupt,
reducing the overhead.

That would reduce the cost quite a bit over IBRS.

But right now we're in the same situation as with deep call chain.
It's all theory, but there are no practical exploits.

Based on that I don't think it makes sense to pursue this option
at this point. But if it's needed it will be not as bad as IBRS
with the right optimizations.

Right now the only realistic risk with MD is for JITs, so we
really need a way to disable it with a per process switch. 

Disabling it globally is normally not a good idea because it can have
quite high performance overhead in some workloads.

We proposed to tie it to seccomp because many JITs already use 
seccomp, so it would work transparently. There's one problem that if 
some JIT doesn't use it already it may not want the restrictions
(in particularly the need to disable suid). But that could 
be fixed with a simple tweak: don't require the suid restriction
if the seccomp program is only a single statement that allows
the sysscall. IMHO something like this needs to be added 
to the MDD/RBS patches that are floating around.

-Andi

[1] https://git.kernel.org/pub/scm/linux/kernel/git/ak/linux-misc.git/log/?h=spec/ftrace-stuff-13
[2] https://github.com/andikleen/gcc/tree/instrument-return-gcc7-1

Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Thomas Gleixner]

On Mon, 23 Apr 2018, speck for Jon Masters wrote:

> On 04/23/2018 02:05 PM, speck for Linus Torvalds wrote:
> > 
> > 
> > On Mon, 23 Apr 2018, speck for Konrad Rzeszutek Wilk wrote:
> >>
> >> I believe (and Linus, please correct me here) that the question of
> >> toggling on/off SPEC_CTRL MSR on user-space entrance is a no-go.
> > 
> > Absolutely. That would be entirely crazy.
> 
> Yep, totally nuts, and nobody is asking for that :)
> 
> > Is there a known situation where that would actually make sense?
> 
> Intel point out that globally disabling MD by default has typically a
> few percent hit, but sometimes up to 30%. Therefore, they want it to be
> per-process controllable. As I said, e.g. with a prctrl that happens to
> control an MSR underneath, but it's not userspace setting an MSR.

As we discussed before:

 1) Yet another patch site in the entry/exit path which if patched to
    functional contains yet another conditonal.

    The amount of NOOPs in entry/exit is horrendous already if this whole
    nonsense is patched out.

 2) The prctl is a handwavy idea. The semantics are blury at best. Is it
    opt-in or opt-out? Which processes should set it? What's the chance
    that the applications get actually patched? This is the ideal target
    for bitrot.

Anyway, if Intel thinks that this is desired, then they can send patches on
top of the big hammer (MD on/off globally) and provide the answers to #2
and proper numbers.

It's fully orthogonal and does not change the plan of having the global MD
on/off switch there ASAP and first.

Thanks,

	tglx

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Jiri Kosina]

On Tue, 24 Apr 2018, speck for Thomas Gleixner wrote:

>  2) The prctl is a handwavy idea. The semantics are blury at best. Is it
>     opt-in or opt-out? Which processes should set it? What's the chance
>     that the applications get actually patched? This is the ideal target
>     for bitrot.

Exactly.

My concern with this is:

- if it's opt-in, noone will systematically keep adding support for this 
  to all applications that might need it for next XX years

- if it's opt-out, there are techniques that malicious attacker can use
  to first trick the vulnerable app to call the prctl() (which still 
  doesn't cross the security domain of the particular application) and
  then attack kernel (or other app) through MD (which does cross that
  boundary)

-- 
Jiri Kosina
SUSE Labs

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Andi Kleen]

> Is it
>     opt-in or opt-out? 

It's opt-in, but with a caveat.

> Which processes should set it? 

Any JIT relying on language based security with untrusted code.

> What's the chance
>     that the applications get actually patched? i

With tieing it to seccomp many (most?) don't need to be patched
because they already use it. For example all the major Web Browsers
are already covered.

Some additional processes will need to be patched (e.g. JVM),
but to start that process requires defining a kernel API
first. So it's important that we define a kernel API.

> Anyway, if Intel thinks that this is desired, 

It is desired.

> then they can send patches on
> top of the big hammer (MD on/off globally) and provide the answers to #2
> and proper numbers.
> 
> It's fully orthogonal and does not change the plan of having the global MD
> on/off switch there ASAP and first.

They key point is what is the default. Defaulting MD to off by default
can cause large performance problems. So it's important to not turn
it off by default.  We should never ship a patch kit that does that.
It always should be a user decision.

But we still need to have options for the processes that actually
need it, that is why seccomp/prctl is required.

-Andi

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Andi Kleen]

On Tue, Apr 24, 2018 at 12:30:50AM +0200, speck for Jiri Kosina wrote:
> On Tue, 24 Apr 2018, speck for Thomas Gleixner wrote:
> 
> >  2) The prctl is a handwavy idea. The semantics are blury at best. Is it
> >     opt-in or opt-out? Which processes should set it? What's the chance
> >     that the applications get actually patched? This is the ideal target
> >     for bitrot.
> 
> Exactly.
> 
> My concern with this is:
> 
> - if it's opt-in, noone will systematically keep adding support for this 
>   to all applications that might need it for next XX years

Vulnerable applications that are not maintained will be vulnerable to other issues
anyways. e.g. Spectre v1 always needs application specific fixes,
and v1 is far easier to exploit anyways the speculative store bypass.

So yes if something is not maintained it will not be fixed.

The key point is to have the right options for applications that are properly
maintained.

For distributions you would be on the hook for backporting the right patches.

> 
> - if it's opt-out, there are techniques that malicious attacker can use
>   to first trick the vulnerable app to call the prctl() (which still 
>   doesn't cross the security domain of the particular application) and
>   then attack kernel (or other app) through MD (which does cross that
>   boundary)

That's silly. If you can execute arbitary code like prctl already then 
you don't need anything of Spectre. You already have far easier
options to take over the program.

-Andi

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Tim Chen]

[-- Attachment #1.1: Type: text/plain, Size: 8983 bytes --]


> 
> As we discussed before:
> 
>  1) Yet another patch site in the entry/exit path which if patched to
>     functional contains yet another conditonal.
> 
>     The amount of NOOPs in entry/exit is horrendous already if this whole
>     nonsense is patched out.
> 
>  2) The prctl is a handwavy idea. The semantics are blury at best. Is it
>     opt-in or opt-out? Which processes should set it? What's the chance
>     that the applications get actually patched? This is the ideal target
>     for bitrot.
> 
> Anyway, if Intel thinks that this is desired, then they can send patches on
> top of the big hammer (MD on/off globally) and provide the answers to #2
> and proper numbers.
> 


Here's a crack from me on top of Conrad's patches to disable speculation only
for SECCOMP process.  I know that there is still some debate going forward
on refining the prctl but this is to illustrate an example of how it can be
done. We still need to switch the default to SECCOMP or some other prctl 
mechanism and the memory disambiguation disable and reduced data
speculation names need to be put in sync.

I've tested it with browser app that has seccomp turned on.

I still need to collect some performance numbers.

Thanks.

Tim

------>8-------

From e6aaa3123f9c42db4936d705341f799ef8a698f6 Mon Sep 17 00:00:00 2001
From: Tim Chen <tim.c.chen@linux.intel.com>
Date: Mon, 23 Apr 2018 05:57:53 -0700
Subject: [PATCH] x86/ssb: Enable option to mitigate SBB only for seccomp
 processes

For many usage cases, the administrator may not want to disable
speculative store bypass for the whole system for performance reason.
He may choose to enable speculative store bypass mitigation only
for seccomp processes, which have the highest risk for this kind of
vulnerability.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  2 ++
 arch/x86/include/asm/nospec-branch.h            | 21 ++++++++++++--
 arch/x86/kernel/cpu/bugs.c                      | 37 ++++++++++++++++++++++++-
 arch/x86/kernel/cpu/intel.c                     |  3 +-
 arch/x86/kernel/process_64.c                    | 11 ++++++++
 5 files changed, 70 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 0574105..bd1baf9 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4053,6 +4053,8 @@
 			auto - kernel detects whether your CPU model contains a
 			       vulnerable implementation of Speculative Store
 			       Bypass and picks the most appropriate mitigation
+			seccomp - disable Speculative Store Bypass only for seccomp
+				  processes.
 
 			Not specifying this option is equivalent to
 			spec_store_bypass_disable=auto.
diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index c4a99c9..c718929 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -3,10 +3,12 @@
 #ifndef _ASM_X86_NOSPEC_BRANCH_H_
 #define _ASM_X86_NOSPEC_BRANCH_H_
 
+#include <linux/jump_label.h>
 #include <asm/alternative.h>
 #include <asm/alternative-asm.h>
 #include <asm/cpufeatures.h>
 #include <asm/msr-index.h>
+#include <asm/percpu.h>
 
 /*
  * Fill the CPU return stack buffer.
@@ -241,11 +243,15 @@ extern void x86_restore_host_spec_ctrl(u64);
 enum spec_store_bypass_mitigation {
 	SPEC_STORE_BYPASS_NONE,
 	SPEC_STORE_BYPASS_DISABLE,
+	SPEC_STORE_BYPASS_SECCOMP,
 };
 
 extern char __indirect_thunk_start[];
 extern char __indirect_thunk_end[];
 
+/* cpu's MSR_IA32_SPEC_CTRL state for speculation store bypass mitigation */
+DECLARE_PER_CPU(int, rds_msr_val);
+
 /*
  * On VMEXIT we must ensure that no RSB predictions learned in the guest
  * can be followed in the host, by overwriting the RSB completely. Both
@@ -300,12 +306,23 @@ do {									\
 
 #define firmware_restrict_branch_speculation_end()			\
 do {									\
-	alternative_msr_write(MSR_IA32_SPEC_CTRL,			\
-			      x86_spec_ctrl_base,			\
+	u64 val = x86_spec_ctrl_base | __this_cpu_read(rds_msr_val);	\
+	alternative_msr_write(MSR_IA32_SPEC_CTRL, val,			\
 			      X86_FEATURE_USE_IBRS_FW);			\
 	preempt_enable();						\
 } while (0)
 
+/*
+ * speculation store bypass vulnerabilities mitigation functions.
+ *
+ * Should be called only from pre-emption off paths.
+ *
+ */
+
+void enable_speculative_store_bypass_mitigation(void);
+void disable_speculative_store_bypass_mitigation(void);
+
+DECLARE_STATIC_KEY_FALSE(specctrl_rds);
 #endif /* __ASSEMBLY__ */
 
 /*
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 03f13a6..9e811cd 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -361,16 +361,26 @@ static void __init spectre_v2_select_mitigation(void)
 
 static enum spec_store_bypass_mitigation spec_store_bypass_mode = SPEC_STORE_BYPASS_NONE;
 
+/* dynamic reduced data speculation in use */
+DEFINE_STATIC_KEY_FALSE(specctrl_rds);
+EXPORT_SYMBOL(specctrl_rds);
+
+/* current MSR_IA32_SPEC_CTRL state for ssb mitigation */
+DEFINE_PER_CPU(int, rds_msr_val);
+EXPORT_SYMBOL(rds_msr_val);
+
 /* The kernel command line selection */
 enum spec_store_bypass_mitigation_cmd {
 	SPEC_STORE_BYPASS_CMD_NONE,
 	SPEC_STORE_BYPASS_CMD_AUTO,
 	SPEC_STORE_BYPASS_CMD_ON,
+	SPEC_STORE_BYPASS_CMD_SECCOMP,
 };
 
 static const char *spec_store_bypass_strings[] = {
 	[SPEC_STORE_BYPASS_NONE]			= "Vulnerable",
-	[SPEC_STORE_BYPASS_DISABLE]			= "Mitigation: Speculative Store Bypass disabled"
+	[SPEC_STORE_BYPASS_DISABLE]			= "Mitigation: Speculative Store Bypass disabled",
+	[SPEC_STORE_BYPASS_SECCOMP]			= "Mitigation: Speculative Store Bypass disabled for seccomp processes"
 };
 
 static const struct {
@@ -380,8 +390,29 @@ static const struct {
 	{ "auto",				SPEC_STORE_BYPASS_CMD_AUTO }, /* Platform decides */
 	{ "on",					SPEC_STORE_BYPASS_CMD_ON },   /* Disable Speculative Store Bypass */
 	{ "off",				SPEC_STORE_BYPASS_CMD_NONE }, /* Don't touch Speculative Store Bypass */
+	{ "seccomp",				SPEC_STORE_BYPASS_CMD_SECCOMP }, /* disable Speculative Store Bypass for seccomp processes*/
 };
 
+void enable_speculative_store_bypass_mitigation(void)
+{
+       if (static_branch_unlikely(&specctrl_rds) &&
+           /* need to toggle msr? */
+           __this_cpu_read(rds_msr_val) != SPEC_CTRL_MDD) {
+               wrmsrl(MSR_IA32_SPEC_CTRL, SPEC_CTRL_MDD | x86_spec_ctrl_base);
+               __this_cpu_write(rds_msr_val, SPEC_CTRL_MDD);
+       }
+}
+
+void disable_speculative_store_bypass_mitigation(void)
+{
+       if (static_branch_unlikely(&specctrl_rds) &&
+           /* need to toggle msr? */
+           __this_cpu_read(rds_msr_val) != 0) {
+               wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
+               __this_cpu_write(rds_msr_val, 0);
+       }
+}
+
 static enum spec_store_bypass_mitigation_cmd __init spec_store_bypass_parse_cmdline(void)
 {
 	char arg[20];
@@ -440,6 +471,10 @@ static void __init spec_store_bypass_select_mitigation(void)
 	case SPEC_STORE_BYPASS_CMD_ON:
 		mode = SPEC_STORE_BYPASS_DISABLE;
 		break;
+	case SPEC_STORE_BYPASS_CMD_SECCOMP:
+		mode = SPEC_STORE_BYPASS_SECCOMP;
+		static_branch_enable(&specctrl_rds);
+		break;
 	case SPEC_STORE_BYPASS_CMD_NONE:
 		break;
 	}
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index c66a8e7..ca6e7f3 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -774,7 +774,8 @@ static void init_intel(struct cpuinfo_x86 *c)
 	init_intel_misc_features(c);
 
 	if (cpu_has(c, X86_FEATURE_STBUF_MITIGATE)) {
-		x86_spec_ctrl_base |= SPEC_CTRL_MDD;
+		if (!static_branch_unlikely(&specctrl_rds))
+			x86_spec_ctrl_base |= SPEC_CTRL_MDD;
 		wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
 	}
 }
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 4b100fe..9e1236d 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -54,6 +54,7 @@
 #include <asm/vdso.h>
 #include <asm/intel_rdt_sched.h>
 #include <asm/unistd.h>
+#include <asm/nospec-branch.h>
 #ifdef CONFIG_IA32_EMULATION
 /* Not included via unistd.h */
 #include <asm/unistd_32_ia32.h>
@@ -529,6 +530,16 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 	/* Load the Intel cache allocation PQR MSR. */
 	intel_rdt_sched_in();
 
+	if (next_p->mm && test_tsk_thread_flag(next_p, TIF_SECCOMP))
+		/*
+		 * Reduce data speculation for sandboxed
+		 * seccomp process for protection against speculation
+		 * store bypass attack.
+		 */
+		enable_speculative_store_bypass_mitigation();
+	else
+		disable_speculative_store_bypass_mitigation();
+
 	return prev_p;
 }
 
-- 
2.9.4

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Jon Masters]

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

On 04/23/2018 05:23 PM, speck for Linus Torvalds wrote:

> Anyway, I was wondering if there was any reason why we'd care for the 
> store buffer bypass problem? The whole "run with store buffers in user 
> space, disable them in kernel space" model seems insane.

In theory there's a stack attack that can be performed using v4. To pull
it off requires untrusted user-provided values on the kernel stack
(which we do in saving all of the registers, e.g. due to v1). We have
lfences in the retpoline entry code so we're definitely safe at least
until the point of calling each actual syscall function. But, in theory,
it's possible that at least one syscall could contain gadget code. We
don't yet have a scanner that can adequately detect this in reality.

We originally had code that toggled on kernel entry. Several of us
pushed back saying it was overkill, especially given the cost of the MSR
writes vs just disabling MD globally (Konrad has numbers). But a problem
may come if folks like Intel and AMD want to argue for keeping MD on by
default. The syscall attack is only a remotely vague theory at this
point, so if we kept MD on by default, you could just do the MSR write
when entering/leaving a process that has specifically asked for it.

[ On the RHEL side, we already have downstream-only IBRS patches that
already frob the IBRS on entry/exit (Skylake+) so adding MDD won't cost
us more if we decide to go and be crazy on the distro kernel side, at
least on parts where we're already doing that SPEC_CTRL write anyway. ]

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Jon Masters]

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

On 04/23/2018 06:31 PM, speck for Andi Kleen wrote:

> With tieing it to seccomp many (most?) don't need to be patched
> because they already use it. For example all the major Web Browsers
> are already covered.

As I mentioned on our call earlier, seccomp alone won't be sufficient. I
know you know that, and it's been repeated by Tim, but just to be clear.
We would either need seccomp to change, or a new prctl (e.g. one that
controls speculation options for a process).

> Some additional processes will need to be patched (e.g. JVM),
> but to start that process requires defining a kernel API
> first. So it's important that we define a kernel API.

Indeed, that was my ask earlier, that Intel help drive this urgently if
you'd like to save MD on by default by May 21. If we only have the big
hammer, as I said on the other call with AMD/Microsoft/SuSE/Canonical
etc. earlier this pm, we'll have to go with MD off by default in RHEL.
We'd like to give you what you want, but we need a way to go and patch
things like OpenJDK in the next couple of weeks, and we're out of time.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

[MODERATED] Re: [patch 07/11] [PATCH v2 07/10] Linux Patch #7

[Jiri Kosina]

On Mon, 23 Apr 2018, speck for Andi Kleen wrote:

> That's silly. If you can execute arbitary code like prctl already then 
> you don't need anything of Spectre. You already have far easier options 
> to take over the program.

By spectre you're not necessarily taking over only the program, it opens 
sidechannel to the kernel (which wasn't there with the vulnerable 
application that allows attacker to do arbitrary library call). That's 
what I meant by the

	"(which still doesn't cross the security domain of the particular 
	application) and then attack kernel (or other app) through MD 
	(which does cross that boundary)"

-- 
Jiri Kosina
SUSE Labs