From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mx2.suse.de ([195.135.220.15]) by Galois.linutronix.de with esmtps (TLS1.0:DHE_RSA_CAMELLIA_256_CBC_SHA1:256) (Exim 4.80) (envelope-from ) id 1fAgJ0-0007Bw-Ty for speck@linutronix.de; Mon, 23 Apr 2018 20:35:15 +0200 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id D3803AE0F for ; Mon, 23 Apr 2018 18:35:08 +0000 (UTC) Date: Mon, 23 Apr 2018 20:34:59 +0200 From: Borislav Petkov Subject: [MODERATED] Re: [PATCH v3 06/10] [PATCH v3 6/9] Linux Patch #6 Message-ID: <20180423183459.GL24245@pd.tnic> References: <20180423171426.795385641@dhcp-10-159-147-220.vpn.oracle.com> MIME-Version: 1.0 In-Reply-To: <20180423171426.795385641@dhcp-10-159-147-220.vpn.oracle.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable To: speck@linutronix.de List-ID: On Mon, Apr 23, 2018 at 01:11:30PM -0400, speck for konrad.wilk_at_oracle.com= wrote: > x86/spec_store_bypass_disable: Provide boot parameters for the mitigation "x86/bugs: ... > Contemporary high performance processors use a common industry-wide > optimization known as "Speculative Store Bypass" in which loads from > addresses to which a recent store has occurred may (speculatively) > see an older value. Intel refers to this feature as "Memory > Disambiguation", which is part of their "Smart Memory Access" > capability in Nehalem and later generation processors. >=20 > Some processors have an implementation bug that enables a cache > side-channel attack against such speculatively read values. An > attacker can create exploit code that allows them to read memory > outside of a sandbox environment (for example, malicious JavaScript > in a web page), or to perform more complex attacks against code > running within the same privilege level, e.g. via the stack. >=20 > We provide two command line control knobs: >=20 > nospec_store_bypass_disable > spec_store_bypass_disable=3D[off,auto,on] >=20 > By default affected x86 processors will power on with Speculative > Store Bypass (Memory Disambiguation) enabled. Hence the provided > kernel parameters are written from the point of view of whether > to enable a mitigation or not. The parameters are as follows: >=20 > - auto - if possible will disable Speculative Store Bypass (Memory > Disambiguation) at boot if CPU is vulnerable to exploit This version of the "auto" explanation from below is the correct one: + auto - kernel detects whether your CPU model contains a + vulnerable implementation of Speculative Store + Bypass and picks the most appropriate mitigation > - on - disable Speculative Store Bypass (Memory Disambiguation) > - off - enable Speculative Store Bypass (Memory Disambiguation) >=20 > And as mentioned - some CPUs may have a Speculative Store Bypass > implementation which is not vulnerable to the described attacks. > For those, the 'auto' (default) option will pick the right choice. IOW, that paragraph should be after the "auto" bulletpoint above. > Signed-off-by: Konrad Rzeszutek Wilk >=20 > --- > v1.3: > - Wrong array was used when figuring mdd_v4=3D arguments > - If broken microcode found, also disable MD feature. > - s/mdd_v4/mdd/ > - s/kernel/userpsace/ > - Fix up the commit description > v1.4: > - Incorporate various feedback and suggestions from Jon Masters > - Change mdd parameters to allow only big hammer on/off for now > - Introduce "ssbd" industry standard term for cross-architecture > - Use mdd_ and MDD_ prefixes to clearly spell out that "enable" > means we are actually disabling a CPU processor feature (MD) > v2: > - s/mdd/ssb/ in the x86 code to match with the 'ssb' part > v3: > - rip out mdd. > - remove 'Haswell' and such and replace with 'contemporary' > - s/ssb/spec_store_bypass/ and various other cleanups (Jon Masters) > --- > Documentation/admin-guide/kernel-parameters.txt | 32 +++++++++ > arch/x86/include/asm/cpufeatures.h | 2 + > arch/x86/include/asm/nospec-branch.h | 6 ++ > arch/x86/kernel/cpu/bugs.c | 96 +++++++++++++++++++++= ++++ > arch/x86/kernel/cpu/intel.c | 1 + > 5 files changed, 137 insertions(+) >=20 > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentatio= n/admin-guide/kernel-parameters.txt > index 1d1d53f85ddd..340a84e05f9e 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -2647,6 +2647,9 @@ > allow data leaks with this option, which is equivalent > to spectre_v2=3Doff. > =20 > + nospec_store_disable nospec_store_bypass_disable You guys need to make up your mind. :) > + [HW] Disable all mitigations for the Speculative Store Bypass vulnerabi= lity > + > noxsave [BUGS=3DX86] Disables x86 extended register state save > and restore using xsave. The kernel will fallback to > enabling legacy floating-point and sse state. > @@ -3997,6 +4000,35 @@ > Not specifying this option is equivalent to > spectre_v2=3Dauto. > =20 > + spec_store_bypass_disable=3D > + [HW] Control Speculative Store Bypass (SSB) Disable mitigation > + (Speculative Store Bypass vulnerability) > + Certain CPUs are vulnerable to an exploit against a > + a common industry wide performance optimization known > + as "Speculative Store Bypass" in which recent stores > + to the same memory location may not be observed by > + later loads during speculative execution. The idea > + is that such stores are unlikely and that they can > + be detected prior to instruction retirement at the > + end of a particular speculation execution window. > + > + In vulnerable processors, the speculatively forwarded > + store can be used in a cache side channel attack, for > + example to read memory to which the attacker does not > + directly have access (e.g. inside sandboxed code). > + > + This parameter controls whether the Speculative Store > + Bypass optimization is used. > + > + on - unconditionally disable Speculative Store Bypass > + off - unconditionally enable Speculative Store Bypass > + auto - kernel detects whether your CPU model contains a > + vulnerable implementation of Speculative Store > + Bypass and picks the most appropriate mitigation > + > + Not specifying this option is equivalent to > + spec_store_bypass_disable=3Dauto. > + > spia_io_base=3D [HW,MTD] > spia_fio_base=3D > spia_pedr=3D > diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpuf= eatures.h > index 25c9406de6c9..1966b223e11e 100644 > --- a/arch/x86/include/asm/cpufeatures.h > +++ b/arch/x86/include/asm/cpufeatures.h > @@ -214,6 +214,8 @@ > =20 > #define X86_FEATURE_USE_IBPB ( 7*32+21) /* "" Indirect Branch Prediction = Barrier enabled */ > #define X86_FEATURE_USE_IBRS_FW ( 7*32+22) /* "" Use IBRS during runtime = firmware calls */ > +#define X86_FEATURE_STBUF_BYPASS ( 7*32+23) /* Speculative Store Bypass mi= tigation is feasible. */ Needs "" in the comment too so that it doesn't appear in /proc/cpuinfo. > +#define X86_FEATURE_STBUF_MITIGATE ( 7*32+24) /* "" Use Speculative Store = Bypass. */ X86_FEATURE_STBUF_BYPASS_MITIGATE got too long or what? > /* Virtualization flags: Linux defined, word 8 */ > #define X86_FEATURE_TPR_SHADOW ( 8*32+ 0) /* Intel TPR Shadow */ > diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/no= spec-branch.h > index 3e4b66bdf25f..c4a99c970147 100644 > --- a/arch/x86/include/asm/nospec-branch.h > +++ b/arch/x86/include/asm/nospec-branch.h > @@ -237,6 +237,12 @@ extern u64 x86_spec_ctrl_base; > extern void x86_set_guest_spec_ctrl(u64); > extern void x86_restore_host_spec_ctrl(u64); > =20 > +/* The Speculative Store Bypass disable variants */ > +enum spec_store_bypass_mitigation { > + SPEC_STORE_BYPASS_NONE, > + SPEC_STORE_BYPASS_DISABLE, > +}; > + > extern char __indirect_thunk_start[]; > extern char __indirect_thunk_end[]; > =20 > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c > index 0fc475a54aa1..b54336a42dca 100644 > --- a/arch/x86/kernel/cpu/bugs.c > +++ b/arch/x86/kernel/cpu/bugs.c > @@ -27,6 +27,7 @@ > #include > =20 > static void __init spectre_v2_select_mitigation(void); > +static void __init spec_store_bypass_select_mitigation(void); > static void __init spec_ctrl_save_msr(void); > =20 > void __init check_bugs(void) > @@ -43,6 +44,12 @@ void __init check_bugs(void) > /* Select the proper spectre mitigation before patching alternatives */ > spectre_v2_select_mitigation(); > =20 > + /* > + * Select proper mitigation for any exposure to the Speculative Store > + * Bypass vulnerability. > + */ > + spec_store_bypass_select_mitigation(); > + > #ifdef CONFIG_X86_32 > /* > * Check whether we are able to run this kernel safely on SMP. > @@ -344,6 +351,92 @@ static void __init spectre_v2_select_mitigation(void) > } > } > =20 > +#undef pr_fmt > +#define pr_fmt(fmt) "Speculative Store Bypass: " fmt > + > +static enum spec_store_bypass_mitigation spec_store_bypass_mode =3D SPEC_S= TORE_BYPASS_NONE; > + > +/* The kernel command line selection */ > +enum spec_store_bypass_mitigation_cmd { Yeah, that enum name is clearly too long and it is noticeable in the code bel= ow. > + SPEC_STORE_BYPASS_CMD_NONE, > + SPEC_STORE_BYPASS_CMD_AUTO, > + SPEC_STORE_BYPASS_CMD_ON, > +}; > + > +static const char *spec_store_bypass_strings[] =3D { > + [SPEC_STORE_BYPASS_NONE] =3D "Vulnerable", > + [SPEC_STORE_BYPASS_DISABLE] =3D "Mitigation: Speculative Store Bypass d= isabled" That's clearly too much whitespace between the element and the string. > +}; > + > +static const struct { > + const char *option; > + enum spec_store_bypass_mitigation_cmd cmd; > +} spec_store_bypass_mitigation_options[] =3D { > + { "auto", SPEC_STORE_BYPASS_CMD_AUTO }, /* Platform decides */ > + { "on", SPEC_STORE_BYPASS_CMD_ON }, /* Disable Speculative Store By= pass */ > + { "off", SPEC_STORE_BYPASS_CMD_NONE }, /* Don't touch Speculative Stor= e Bypass */ Ditto. What's wrong with: { "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 Bypa= ss */ ? > +}; > + > +static enum spec_store_bypass_mitigation_cmd __init spec_store_bypass_pars= e_cmdline(void) static enum spec_store_bypass_cmd __init spec_store_bypass_parse_cmdline(void) looks a bit better to me. Btw, that "spec_store_bypass" string is kinda everywhere and screaming at me now. :) I'm wondering if for all the functions and variables we could really use the "ssb_" prefix for shorter but keep the visible, cmdline options human readable...? > +{ > + char arg[20]; > + int ret, i; > + enum spec_store_bypass_mitigation_cmd cmd =3D SPEC_STORE_BYPASS_CMD_AUTO; > + > + if (!boot_cpu_has(X86_FEATURE_STBUF_BYPASS)) > + return SPEC_STORE_BYPASS_CMD_NONE; > + > + if (cmdline_find_option_bool(boot_command_line, "nospec_store_bypass_disa= ble")) > + return SPEC_STORE_BYPASS_CMD_NONE; > + else { > + ret =3D cmdline_find_option(boot_command_line, "spec_store_bypass_disabl= e", > + arg, sizeof(arg)); > + if (ret < 0) > + return SPEC_STORE_BYPASS_CMD_AUTO; > + > + for (i =3D 0; i < ARRAY_SIZE(spec_store_bypass_mitigation_options); i++)= { > + if (!match_option(arg, ret, spec_store_bypass_mitigation_options[i].opt= ion)) > + continue; > + > + cmd =3D spec_store_bypass_mitigation_options[i].cmd; > + break; > + } > + > + if (i >=3D ARRAY_SIZE(spec_store_bypass_mitigation_options)) { > + pr_err("unknown option (%s). Switching to AUTO select\n", arg); > + return SPEC_STORE_BYPASS_CMD_AUTO; > + } > + } > + > + return cmd; > +} > + > +static void __init spec_store_bypass_select_mitigation(void) > +{ > + enum spec_store_bypass_mitigation_cmd cmd =3D spec_store_bypass_parse_cmd= line(); > + enum spec_store_bypass_mitigation mode =3D SPEC_STORE_BYPASS_NONE; > + > + if (!boot_cpu_has_bug(X86_BUG_CPU_SPEC_STORE_BYPASS) && > + (cmd =3D=3D SPEC_STORE_BYPASS_CMD_NONE || > + cmd =3D=3D SPEC_STORE_BYPASS_CMD_AUTO)) > + return; > + > + switch (cmd) { > + case SPEC_STORE_BYPASS_CMD_AUTO: > + case SPEC_STORE_BYPASS_CMD_ON: > + mode =3D SPEC_STORE_BYPASS_DISABLE; > + break; > + case SPEC_STORE_BYPASS_CMD_NONE: > + break; > + } > + > + spec_store_bypass_mode =3D mode; > + pr_info("%s\n", spec_store_bypass_strings[mode]); > + > + if (mode !=3D SPEC_STORE_BYPASS_NONE) > + setup_force_cpu_cap(X86_FEATURE_STBUF_MITIGATE); > +} > + > #undef pr_fmt > =20 > #ifdef CONFIG_SYSFS > @@ -370,6 +463,9 @@ ssize_t cpu_show_common(struct device *dev, struct devi= ce_attribute *attr, > boot_cpu_has(X86_FEATURE_USE_IBRS_FW) ? ", IBRS_FW" : "", > spectre_v2_module_string()); > =20 > + case X86_BUG_CPU_SPEC_STORE_BYPASS: > + return sprintf(buf, "%s\n", spec_store_bypass_strings[spec_store_bypass_= mode]); > + > default: > break; > } > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c > index c3af167d0a70..16ee38d8977a 100644 > --- a/arch/x86/kernel/cpu/intel.c > +++ b/arch/x86/kernel/cpu/intel.c > @@ -189,6 +189,7 @@ static void early_init_intel(struct cpuinfo_x86 *c) > setup_clear_cpu_cap(X86_FEATURE_STIBP); > setup_clear_cpu_cap(X86_FEATURE_SPEC_CTRL); > setup_clear_cpu_cap(X86_FEATURE_INTEL_STIBP); > + setup_clear_cpu_cap(X86_FEATURE_MDD); Forgotten hunk: arch/x86/kernel/cpu/intel.c: In function =E2=80=98early_init_intel=E2=80=99: arch/x86/kernel/cpu/intel.c:192:23: error: =E2=80=98X86_FEATURE_MDD=E2=80=99 = undeclared (first use in this function); did you mean =E2=80=98X86_FEATURE_MB= A=E2=80=99? setup_clear_cpu_cap(X86_FEATURE_MDD); ^~~~~~~~~~~~~~~ X86_FEATURE_MBA arch/x86/kernel/cpu/intel.c:192:23: note: each undeclared identifier is repor= ted only once for each function it appears in make[3]: *** [arch/x86/kernel/cpu/intel.o] Error 1 make[2]: *** [arch/x86/kernel/cpu] Error 2 make[1]: *** [arch/x86/kernel] Error 2 make: *** [arch/x86] Error 2 make: *** Waiting for unfinished jobs.... --=20 Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imend=C3=B6rffer, Jane Smithard, Graham Norton, HR= B 21284 (AG N=C3=BCrnberg) --=20