SBB V10 Bundle

SBB V10 Bundle

[Thomas Gleixner]

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

Hi!

Find attached the V10 bundle.

Changes from V9:

   - Fix the AMD CPU hotplug fallout

   - Add non vulnerable AMD models to the whitelist (Borislav)

I've update all three branches in the repository as well.

Delta patch to V9 below.

Thanks,

	tglx

8<------------------
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 6cd982709daf..50c6ba6d031b 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -555,6 +555,26 @@ static void bsp_init_amd(struct cpuinfo_x86 *c)
 		rdmsrl(MSR_FAM10H_NODE_ID, value);
 		nodes_per_socket = ((value >> 3) & 7) + 1;
 	}
+
+	if (c->x86 >= 0x15 && c->x86 <= 0x17) {
+		unsigned int bit;
+
+		switch (c->x86) {
+		case 0x15: bit = 54; break;
+		case 0x16: bit = 33; break;
+		case 0x17: bit = 10; break;
+		default: return;
+		}
+		/*
+		 * Try to cache the base value so further operations can
+		 * avoid RMW. If that faults, do not enable RDS.
+		 */
+		if (!rdmsrl_safe(MSR_AMD64_LS_CFG, &x86_amd_ls_cfg_base)) {
+			setup_force_cpu_cap(X86_FEATURE_RDS);
+			setup_force_cpu_cap(X86_FEATURE_AMD_RDS);
+			x86_amd_ls_cfg_rds_mask = (1ULL << bit);
+		}
+	}
 }
 
 static void early_detect_mem_encrypt(struct cpuinfo_x86 *c)
@@ -674,26 +694,6 @@ static void early_init_amd(struct cpuinfo_x86 *c)
 		set_cpu_bug(c, X86_BUG_AMD_E400);
 
 	early_detect_mem_encrypt(c);
-
-	if (c->x86 >= 0x15 && c->x86 <= 0x17) {
-		unsigned int bit;
-
-		switch (c->x86) {
-		case 0x15: bit = 54; break;
-		case 0x16: bit = 33; break;
-		case 0x17: bit = 10; break;
-		default: return;
-		}
-		/*
-		 * Try to cache the base value so further operations can
-		 * avoid RMW. If that faults, do not enable RDS.
-		 */
-		if (!rdmsrl_safe(MSR_AMD64_LS_CFG, &x86_amd_ls_cfg_base)) {
-			set_cpu_cap(c, X86_FEATURE_RDS);
-			set_cpu_cap(c, X86_FEATURE_AMD_RDS);
-			x86_amd_ls_cfg_rds_mask = (1ULL << bit);
-		}
-	}
 }
 
 static void init_amd_k8(struct cpuinfo_x86 *c)
@@ -919,6 +919,11 @@ static void init_amd(struct cpuinfo_x86 *c)
 	/* AMD CPUs don't reset SS attributes on SYSRET, Xen does. */
 	if (!cpu_has(c, X86_FEATURE_XENPV))
 		set_cpu_bug(c, X86_BUG_SYSRET_SS_ATTRS);
+
+	if (boot_cpu_has(X86_FEATURE_AMD_RDS)) {
+		set_cpu_cap(c, X86_FEATURE_RDS);
+		set_cpu_cap(c, X86_FEATURE_AMD_RDS);
+	}
 }
 
 #ifdef CONFIG_X86_32
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 6101e3dba2f6..f3dbdde978a4 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -939,6 +939,10 @@ static const __initconst struct x86_cpu_id cpu_no_spec_store_bypass[] = {
 	{ X86_VENDOR_CENTAUR,	5 },
 	{ X86_VENDOR_INTEL,	5 },
 	{ X86_VENDOR_NSC,	5 },
+	{ X86_VENDOR_AMD,	0xf },
+	{ X86_VENDOR_AMD,	0x10 },
+	{ X86_VENDOR_AMD,	0x11 },
+	{ X86_VENDOR_AMD,	0x12 },
 	{ X86_VENDOR_ANY,	4 },
 	{}
 };

[-- Attachment #2: Type: application/octet-stream, Size: 27800 bytes --]

[MODERATED] Re: SBB V10 Bundle

[Ingo Molnar]

A quick review of the v10 series:

==============>
b6f76374d45a: x86/nospec: Simplify alternative_msr_write()

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

==============>
3ec366cfa844: x86/bugs: Concentrate bug detection into a separate function

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

==============>
3b06f42b82ad: x86/bugs: Concentrate bug reporting into a separate function

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

==============>
edb984611076: x86/bugs: Read SPEC_CTRL MSR during boot and re-use reserved bits

This:

	if (val & ~(SPEC_CTRL_IBRS))

... can lose the parantheses I suspect?

Also:

s/Our boot-time value of SPEC_CTRL MSR.
 /Our boot-time value of the SPEC_CTRL MSR.

With those fixed:

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

Also, a bike-shed painting suggestion, could we flip over the spec_ctrl namespace 
to be top-down hiearchical, i.e. continue the nomenclature started by:

 x86_spec_ctrl_base
 x86_spec_ctrl_mask

... and make the API functions use that namespace as well:

 x86_get_spec_ctrl()  => x86_spec_ctrl_get()
 x86_set_spec_ctrl()  => x86_spec_ctrl_set()

etc.?

In a similar vein:

  PR_GET_SPECULATION_CTRL => PR_SPECULATION_CTRL_GET
  PR_SET_SPECULATION_CTRL => PR_SPECULATION_CTRL_SET

(but no strong feelings and not a condition of an Ack.)

==============>
33078b163c08: KVM/SVM/VMX/x86/bugs: Support the combination of guest and host IBRS

s/Note: This uses wrmsrl instead of native_wrmsl.
 /Note: This uses wrmsrl() instead of native_wrmsl().

s/into the functions
 /to the functions

With those fixed:

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

and if we do the renames in the previous patch then we should probably also do:

  x86_set_guest_spec_ctrl() => x86_spec_ctrl_guest_set()
  x86_restore_host_spec_ctrl() => x86_spec_ctrl_host_restore()

==============>
db77b0d7700e: x86/bugs: Expose the /sys/../spec_store_bypass and X86_BUG_SPEC_STORE_BYPASS


Title should I suspect be extended to:

  x86/bugs: Expose the /sys/../spec_store_bypass and X86_BUG_SPEC_STORE_BYPASS values

?

Also, could we change this:

+static const __initconst struct x86_cpu_id cpu_no_spec_store_bypass[] = {
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_PINEVIEW },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_LINCROFT },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_PENWELL },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_CLOVERVIEW },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_CEDARVIEW },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_SILVERMONT1 },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_AIRMONT },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_SILVERMONT2 },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_MERRIFIELD },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_CORE_YONAH },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_XEON_PHI_KNL },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_XEON_PHI_KNM },

... to something like:

+static const __initconst struct x86_cpu_id cpu_no_spec_store_bypass[] = {
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_PINEVIEW	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_LINCROFT	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_PENWELL	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_CLOVERVIEW	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_CEDARVIEW	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_SILVERMONT1	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_AIRMONT	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_SILVERMONT2	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_MERRIFIELD	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_CORE_YONAH	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_XEON_PHI_KNL	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_XEON_PHI_KNM	},

... to make misplaced commas and other asymmetries easier to see during review?

With those fixed:

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

==============>
b6e792e6f30d: x86/cpufeatures: Add X86_FEATURE_RDS

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

==============>
33c97592f020: x86/bugs: Provide boot parameters for the spec_store_bypass_disable mitigation

s/Provide two command line control knobs:
 /As a first step to mitigate against such attacks, provide two boot command line control knobs:

s/a implementation
 /an implementation

I think the boot flags are confusing. We have:

 nospec_store_bypass_disable == Disable all mitigations for the Speculative Store 
                                Bypass vulerability, i.e. keep "speculative store 
                                bypass" enabled.

Note the 'no ... bypass disable' double negation which results in the hardware 
feature of remaining enabled.

We also have:

  spec_store_bypass_disable=off

the 'disable the disabling' results in 'enabling' the hardware feature.

I realize that this is correctly specified and explained via:

  "By default affected x86 processors will power on with Speculative
   Store Bypass enabled. Hence the provided kernel parameters are written
   from the point of view of whether to enable a mitigation or not."

... but that does not make the parameters any less confusing.

Why not control the hardware feature directly, i.e.:

  spec_store_bypass                     # equivalent to 'on'
  spec_store_bypass=off
  spec_store_bypass=on
  spec_store_bypass=auto

?

This would also remove the standalone and dissonant nospec_store_bypass_disable 
boot parameter.

Note how this is already implemented internally AFAICS:

+enum ssb_mitigation_cmd {
+       SPEC_STORE_BYPASS_CMD_NONE,
+       SPEC_STORE_BYPASS_CMD_AUTO,
+       SPEC_STORE_BYPASS_CMD_ON,
+};
+
+static const char *ssb_strings[] = {
+       [SPEC_STORE_BYPASS_NONE]        = "Vulnerable",
+       [SPEC_STORE_BYPASS_DISABLE]     = "Mitigation: Speculative Store Bypass disabled"
+

this already has everything simplified down to the hardware feature level.

Am I confused?

Also:

+       char arg[20];

the largest string that has to fit is "auto", so this could be 5 or just a 
self-documenting, open-coded sizeof("auto"+1), right?

==============>
0e9930e8fa7d: x86/bugs/Intel: Set proper CPU features and setup RDS

s/Intel
 /intel

because we typically write it lowercase in title prefixes.

s/setup
 /set up

Also, could we please write out "Reduced Data Speculation (RDS)" in the title, 
which is the first time people will see this acronym?


Within the changelog we can then write 'RDS' instead of writing it out every time. 
While attention span is short, it does usually last from title to end of 
changelog. This is doubly true since we use the RDS acronym in actual source 
code...

This paragraph in the changelog:

+  Note that this does not fix the KVM case where the SPEC_CTRL is exposed to
+  guests which can muck with, see patch titled : KVM/SVM/VMX/x86/spectre_v2:
+  Support the combination of guest and host IBRS.

Took me three attempts to parse correctly. Could we please clarify it to:

  Note that this does not fix the KVM case where the SPEC_CTRL is exposed to
  guests which can muck with it, see the following patch:

     KVM/SVM/VMX/x86/spectre_v2: Support the combination of guest and host IBRS

or so? (Also note the extra 'it' inserted for good measure.)

This bit in the code:

+#define ARCH_CAP_RDS_NO                        (1 << 4)   /* Not susceptible to speculative store bypass */

... is hanging in the air a bit, the specifier has 'RDS' but the comment is 
talking about 'speculative store bypass' (which should be capitalized in any 
case).

How about:

  /*
   * Not susceptible to the Speculative Store Bypass attack, so
   * no Reduced Data Speculative control required:
   */
  #define ARCH_CAP_RDS_NO                        (1 << 4)

or so?

Finally, if we do the function namespace renames for the previous patches, we 
should probably also do:

 x86_setup_ap_spec_ctrl() => x86_spec_ctrl_setup_ap()

With these fixed/changed:

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

==============>
c9c863a0e592: x86/bugs: Whitelist allowed SPEC_CTRL MSR values

  +static u64 __ro_after_init x86_spec_ctrl_mask = ~(SPEC_CTRL_IBRS);

No parantheses needed I suspect.

+                       x86_spec_ctrl_mask &= ~(SPEC_CTRL_RDS);

ditto.

With those fixed:

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

==============>
c20ee3e76f12: x86/bugs/AMD: Add support to disable RDS on Fam[15,16,17]h if requested

s/x86/bugs/AMD: Add support to disable RDS on Fam[15,16,17]h if requested
 /x86/bugs/AMD: Add support to disable RDS on Fam[15,16,17]h CPUs if requested

Also, I suspect 'Reduced Data Speculation' should be written out once in the 
changelog.

Question: does the x86_amd_ls_cfg_base only include RDS bits? If yes then I 
suspect we should rename:

   x86_amd_ls_cfg_base => x86_amd_ls_cfg_rds_base

?

If it's used for other bits then never mind.

+                       x86_amd_ls_cfg_rds_mask = (1ULL << bit);

No parentheses needed I think.

+ * AMD specific MSR info for Store Bypass control.

s/for Speculative Store Bypass control.
 /for Store Bypass control.

s/AMD64_LS_CFG msr
  The AMD64_LS_CFG MSR

With these resolved:

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

==============>
16544ca81942: x86/KVM/VMX: Expose SPEC_CTRL Bit(2) to the guest


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

==============>
83b2439da7e8: x86/speculation: Create spec-ctrl.h to avoid include hell

s/and fixup the includes
 /and fix up the includes

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

==============>
b4fe2032d782: prctl: Add speculation control prctls

+/* Per task speculation control */
+#define PR_SET_SPECULATION_CTRL                52
+#define PR_GET_SPECULATION_CTRL                53

Please flip this around, i.e.:

  /* Per task speculation control */
  #define PR_GET_SPECULATION_CTRL                52
  #define PR_SET_SPECULATION_CTRL                53

because GET/SET is the canonical ABI order, used in most early prctl()s as well, 
until someone messed it up in PR_SET_TIMERSLACK, which bad example then everyone 
after that cargo-cult-copied ... Let's go back to the original ABI sanity.


+       case PR_SET_SPECULATION_CTRL:
+               if (arg4 || arg5)
+                       return -EINVAL;

So I think returning -ENOSYS might be better, to allow future extensions to use 
-EINVAL for a real invalid value - and allow -ENOSYS to be the "older kernel" 
disambiguation. For tools that care.

I'd also add this to the changelog and to Documentation/userspace-api/spec_ctrl.rst:

  arg4 and arg5 must be zero, for future ABI compatibility. This condition is
  enforced by the system call.

Misc nits/typos:

s/The return value uses bit 0-2
 /The return value uses bits 0-2

s/There is also a class of mitigations which is very expensive,
  There is also a class of mitigations which are very expensive,

Plus the potential namespace hiearchy changes. With all of the resolved:

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

==============>
f746bf3179f2: x86/process: Allow runtime control of Speculative Store Bypass

s/Reduced Data Speculation control (RDS)
 /Reduced Data Speculation (RDS) feature

?

s/Reduced data speculation
 /Reduced Data Speculation

Also, could we write this:

+       return tifn & _TIF_RDS ? x86_amd_ls_cfg_rds_mask : 0ULL;

as:

+       return (tifn & _TIF_RDS) ? x86_amd_ls_cfg_rds_mask : 0ULL;

because while the operator priorities are unambiguous, I had to look twice.

Reviewed-by: Ingo Molnar <mingo@kernel.org>
==============>
d41eec59d419: x86/speculation: Add prctl for Speculative Store Bypass mitigation

This paragraph of the changelog does not parse for me:

 On the other hand if there is the ability to switch it freely gives more
 flexibility to do the protection which is needed for JITed code without
 impacting the overall system performance.

Could we please split this into two sentences or at least add a comma or two or 
otherwise create something more readable - and also make the 'it' more 
unambiguous?

Also, a naive reading of the boot options:

                        auto   - Kernel detects whether the CPU model contains a
                                 implementation of Speculative Store Bypass and
                                 picks the most appropriate mitigation
+                       prctl  - Control Speculative Store Bypass for a thread
+                                via prctl. By default it is enabled. The state
+                                is inherited on fork.

does not resolve the question of whether 'on', 'on', 'auto' or 'prctl' is the best 
option to use. I.e. the interaction of on/off/auto/prctl isn't clear IMHO.

Another question is:

+static int ssb_prctl_set(unsigned long ctrl)
+{
+       bool rds = !!test_tsk_thread_flag(current, TIF_RDS);
+
+       if (ssb_mode != SPEC_STORE_BYPASS_PRCTL)
+               return -ENXIO;

This might be overly permissive in the 'auto' case which defaulted to 'on', where 
an application might still want to disable it for good reasons?

I.e. we should only deny a prctl specifying a more restrictive speculation model 
when the user has forced all speculation on, i.e. "=on".

Thanks,

	Ingo

[MODERATED] Re: SBB V10 Bundle

[Ingo Molnar]

A quick review of the v10 series:

==============>
b6f76374d45a: x86/nospec: Simplify alternative_msr_write()

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

==============>
3ec366cfa844: x86/bugs: Concentrate bug detection into a separate function

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

==============>
3b06f42b82ad: x86/bugs: Concentrate bug reporting into a separate function

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

==============>
edb984611076: x86/bugs: Read SPEC_CTRL MSR during boot and re-use reserved bits

This:

	if (val & ~(SPEC_CTRL_IBRS))

... can lose the parantheses I suspect?

Also:

s/Our boot-time value of SPEC_CTRL MSR.
 /Our boot-time value of the SPEC_CTRL MSR.

With those fixed:

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

Also, a bike-shed painting suggestion, could we flip over the spec_ctrl namespace 
to be top-down hiearchical, i.e. continue the nomenclature started by:

 x86_spec_ctrl_base
 x86_spec_ctrl_mask

... and make the API functions use that namespace as well:

 x86_get_spec_ctrl()  => x86_spec_ctrl_get()
 x86_set_spec_ctrl()  => x86_spec_ctrl_set()

etc.?

In a similar vein:

  PR_GET_SPECULATION_CTRL => PR_SPECULATION_CTRL_GET
  PR_SET_SPECULATION_CTRL => PR_SPECULATION_CTRL_SET

(but no strong feelings and not a condition of an Ack.)

==============>
33078b163c08: KVM/SVM/VMX/x86/bugs: Support the combination of guest and host IBRS

s/Note: This uses wrmsrl instead of native_wrmsl.
 /Note: This uses wrmsrl() instead of native_wrmsl().

s/into the functions
 /to the functions

With those fixed:

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

and if we do the renames in the previous patch then we should probably also do:

  x86_set_guest_spec_ctrl() => x86_spec_ctrl_guest_set()
  x86_restore_host_spec_ctrl() => x86_spec_ctrl_host_restore()

==============>
db77b0d7700e: x86/bugs: Expose the /sys/../spec_store_bypass and X86_BUG_SPEC_STORE_BYPASS


Title should I suspect be extended to:

  x86/bugs: Expose the /sys/../spec_store_bypass and X86_BUG_SPEC_STORE_BYPASS values

?

Also, could we change this:

+static const __initconst struct x86_cpu_id cpu_no_spec_store_bypass[] = {
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_PINEVIEW },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_LINCROFT },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_PENWELL },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_CLOVERVIEW },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_CEDARVIEW },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_SILVERMONT1 },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_AIRMONT },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_SILVERMONT2 },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_MERRIFIELD },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_CORE_YONAH },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_XEON_PHI_KNL },
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_XEON_PHI_KNM },

... to something like:

+static const __initconst struct x86_cpu_id cpu_no_spec_store_bypass[] = {
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_PINEVIEW	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_LINCROFT	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_PENWELL	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_CLOVERVIEW	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_CEDARVIEW	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_SILVERMONT1	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_AIRMONT	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_SILVERMONT2	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_ATOM_MERRIFIELD	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_CORE_YONAH	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_XEON_PHI_KNL	},
+       { X86_VENDOR_INTEL,     6, INTEL_FAM6_XEON_PHI_KNM	},

... to make misplaced commas and other asymmetries easier to see during review?

With those fixed:

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

==============>
b6e792e6f30d: x86/cpufeatures: Add X86_FEATURE_RDS

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

==============>
33c97592f020: x86/bugs: Provide boot parameters for the spec_store_bypass_disable mitigation

s/Provide two command line control knobs:
 /As a first step to mitigate against such attacks, provide two boot command line control knobs:

s/a implementation
 /an implementation

I think the boot flags are confusing. We have:

 nospec_store_bypass_disable == Disable all mitigations for the Speculative Store 
                                Bypass vulerability, i.e. keep "speculative store 
                                bypass" enabled.

Note the 'no ... bypass disable' double negation which results in the hardware 
feature of remaining enabled.

We also have:

  spec_store_bypass_disable=off

the 'disable the disabling' results in 'enabling' the hardware feature.

I realize that this is correctly specified and explained via:

  "By default affected x86 processors will power on with Speculative
   Store Bypass enabled. Hence the provided kernel parameters are written
   from the point of view of whether to enable a mitigation or not."

... but that does not make the parameters any less confusing.

Why not control the hardware feature directly, i.e.:

  spec_store_bypass                     # equivalent to 'on'
  spec_store_bypass=off
  spec_store_bypass=on
  spec_store_bypass=auto

?

This would also remove the standalone and dissonant nospec_store_bypass_disable 
boot parameter.

Note how this is already implemented internally AFAICS:

+enum ssb_mitigation_cmd {
+       SPEC_STORE_BYPASS_CMD_NONE,
+       SPEC_STORE_BYPASS_CMD_AUTO,
+       SPEC_STORE_BYPASS_CMD_ON,
+};
+
+static const char *ssb_strings[] = {
+       [SPEC_STORE_BYPASS_NONE]        = "Vulnerable",
+       [SPEC_STORE_BYPASS_DISABLE]     = "Mitigation: Speculative Store Bypass disabled"
+

this already has everything simplified down to the hardware feature level.

Am I confused?

Also:

+       char arg[20];

the largest string that has to fit is "auto", so this could be 5 or just a 
self-documenting, open-coded sizeof("auto"+1), right?

==============>
0e9930e8fa7d: x86/bugs/Intel: Set proper CPU features and setup RDS

s/Intel
 /intel

because we typically write it lowercase in title prefixes.

s/setup
 /set up

Also, could we please write out "Reduced Data Speculation (RDS)" in the title, 
which is the first time people will see this acronym?


Within the changelog we can then write 'RDS' instead of writing it out every time. 
While attention span is short, it does usually last from title to end of 
changelog. This is doubly true since we use the RDS acronym in actual source 
code...

This paragraph in the changelog:

+  Note that this does not fix the KVM case where the SPEC_CTRL is exposed to
+  guests which can muck with, see patch titled : KVM/SVM/VMX/x86/spectre_v2:
+  Support the combination of guest and host IBRS.

Took me three attempts to parse correctly. Could we please clarify it to:

  Note that this does not fix the KVM case where the SPEC_CTRL is exposed to
  guests which can muck with it, see the following patch:

     KVM/SVM/VMX/x86/spectre_v2: Support the combination of guest and host IBRS

or so? (Also note the extra 'it' inserted for good measure.)

This bit in the code:

+#define ARCH_CAP_RDS_NO                        (1 << 4)   /* Not susceptible to speculative store bypass */

... is hanging in the air a bit, the specifier has 'RDS' but the comment is 
talking about 'speculative store bypass' (which should be capitalized in any 
case).

How about:

  /*
   * Not susceptible to the Speculative Store Bypass attack, so
   * no Reduced Data Speculative control required:
   */
  #define ARCH_CAP_RDS_NO                        (1 << 4)

or so?

Finally, if we do the function namespace renames for the previous patches, we 
should probably also do:

 x86_setup_ap_spec_ctrl() => x86_spec_ctrl_setup_ap()

With these fixed/changed:

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

==============>
c9c863a0e592: x86/bugs: Whitelist allowed SPEC_CTRL MSR values

  +static u64 __ro_after_init x86_spec_ctrl_mask = ~(SPEC_CTRL_IBRS);

No parantheses needed I suspect.

+                       x86_spec_ctrl_mask &= ~(SPEC_CTRL_RDS);

ditto.

With those fixed:

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

==============>
c20ee3e76f12: x86/bugs/AMD: Add support to disable RDS on Fam[15,16,17]h if requested

s/x86/bugs/AMD: Add support to disable RDS on Fam[15,16,17]h if requested
 /x86/bugs/AMD: Add support to disable RDS on Fam[15,16,17]h CPUs if requested

Also, I suspect 'Reduced Data Speculation' should be written out once in the 
changelog.

Question: does the x86_amd_ls_cfg_base only include RDS bits? If yes then I 
suspect we should rename:

   x86_amd_ls_cfg_base => x86_amd_ls_cfg_rds_base

?

If it's used for other bits then never mind.

+                       x86_amd_ls_cfg_rds_mask = (1ULL << bit);

No parentheses needed I think.

+ * AMD specific MSR info for Store Bypass control.

s/for Speculative Store Bypass control.
 /for Store Bypass control.

s/AMD64_LS_CFG msr
  The AMD64_LS_CFG MSR

With these resolved:

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

==============>
16544ca81942: x86/KVM/VMX: Expose SPEC_CTRL Bit(2) to the guest


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

==============>
83b2439da7e8: x86/speculation: Create spec-ctrl.h to avoid include hell

s/and fixup the includes
 /and fix up the includes

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

==============>
b4fe2032d782: prctl: Add speculation control prctls

+/* Per task speculation control */
+#define PR_SET_SPECULATION_CTRL                52
+#define PR_GET_SPECULATION_CTRL                53

Please flip this around, i.e.:

  /* Per task speculation control */
  #define PR_GET_SPECULATION_CTRL                52
  #define PR_SET_SPECULATION_CTRL                53

because GET/SET is the canonical ABI order, used in most early prctl()s as well, 
until someone messed it up in PR_SET_TIMERSLACK, which bad example then everyone 
after that cargo-cult-copied ... Let's go back to the original ABI sanity.


+       case PR_SET_SPECULATION_CTRL:
+               if (arg4 || arg5)
+                       return -EINVAL;

So I think returning -ENOSYS might be better, to allow future extensions to use 
-EINVAL for a real invalid value - and allow -ENOSYS to be the "older kernel" 
disambiguation. For tools that care.

I'd also add this to the changelog and to Documentation/userspace-api/spec_ctrl.rst:

  arg4 and arg5 must be zero, for future ABI compatibility. This condition is
  enforced by the system call.

Misc nits/typos:

s/The return value uses bit 0-2
 /The return value uses bits 0-2

s/There is also a class of mitigations which is very expensive,
  There is also a class of mitigations which are very expensive,

Plus the potential namespace hiearchy changes. With all of the resolved:

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

==============>
f746bf3179f2: x86/process: Allow runtime control of Speculative Store Bypass

s/Reduced Data Speculation control (RDS)
 /Reduced Data Speculation (RDS) feature

?

s/Reduced data speculation
 /Reduced Data Speculation

Also, could we write this:

+       return tifn & _TIF_RDS ? x86_amd_ls_cfg_rds_mask : 0ULL;

as:

+       return (tifn & _TIF_RDS) ? x86_amd_ls_cfg_rds_mask : 0ULL;

because while the operator priorities are unambiguous, I had to look twice.

Reviewed-by: Ingo Molnar <mingo@kernel.org>
==============>
d41eec59d419: x86/speculation: Add prctl for Speculative Store Bypass mitigation

This paragraph of the changelog does not parse for me:

 On the other hand if there is the ability to switch it freely gives more
 flexibility to do the protection which is needed for JITed code without
 impacting the overall system performance.

Could we please split this into two sentences or at least add a comma or two or 
otherwise create something more readable - and also make the 'it' more 
unambiguous?

Also, a naive reading of the boot options:

                        auto   - Kernel detects whether the CPU model contains a
                                 implementation of Speculative Store Bypass and
                                 picks the most appropriate mitigation
+                       prctl  - Control Speculative Store Bypass for a thread
+                                via prctl. By default it is enabled. The state
+                                is inherited on fork.

does not resolve the question of whether 'on', 'on', 'auto' or 'prctl' is the best 
option to use. I.e. the interaction of on/off/auto/prctl isn't clear IMHO.

Another question is:

+static int ssb_prctl_set(unsigned long ctrl)
+{
+       bool rds = !!test_tsk_thread_flag(current, TIF_RDS);
+
+       if (ssb_mode != SPEC_STORE_BYPASS_PRCTL)
+               return -ENXIO;

This might be overly permissive in the 'auto' case which defaulted to 'on', where 
an application might still want to disable it for good reasons?

I.e. we should only deny a prctl specifying a more restrictive speculation model 
when the user has forced all speculation on, i.e. "=on".

Thanks,

	Ingo

Re: SBB V10 Bundle

[Thomas Gleixner]

On Wed, 2 May 2018, speck for Ingo Molnar wrote:
> Also, a bike-shed painting suggestion, could we flip over the spec_ctrl namespace 
> to be top-down hiearchical, i.e. continue the nomenclature started by:
> 
>  x86_spec_ctrl_base
>  x86_spec_ctrl_mask
> 
> ... and make the API functions use that namespace as well:
> 
>  x86_get_spec_ctrl()  => x86_spec_ctrl_get()
>  x86_set_spec_ctrl()  => x86_spec_ctrl_set()
> 
> etc.?

That's doable

> In a similar vein:
> 
>   PR_GET_SPECULATION_CTRL => PR_SPECULATION_CTRL_GET
>   PR_SET_SPECULATION_CTRL => PR_SPECULATION_CTRL_SET

The PRCTL stuff has this PR_GET_XXX PR_SET_XXX all over the place, so I
rather keep it that way.

> 33c97592f020: x86/bugs: Provide boot parameters for the spec_store_bypass_disable mitigation
> 
> s/Provide two command line control knobs:
>  /As a first step to mitigate against such attacks, provide two boot command line control knobs:
> 
> s/a implementation
>  /an implementation
> 
> I think the boot flags are confusing. We have:
> 
>  nospec_store_bypass_disable == Disable all mitigations for the Speculative Store 
>                                 Bypass vulerability, i.e. keep "speculative store 
>                                 bypass" enabled.
> 
> Note the 'no ... bypass disable' double negation which results in the hardware 
> feature of remaining enabled.
> 
> We also have:
> 
>   spec_store_bypass_disable=off
> 
> the 'disable the disabling' results in 'enabling' the hardware feature.
> 
> I realize that this is correctly specified and explained via:
> 
>   "By default affected x86 processors will power on with Speculative
>    Store Bypass enabled. Hence the provided kernel parameters are written
>    from the point of view of whether to enable a mitigation or not."
> 
> ... but that does not make the parameters any less confusing.
> 
> Why not control the hardware feature directly, i.e.:
> 
>   spec_store_bypass                     # equivalent to 'on'
>   spec_store_bypass=off
>   spec_store_bypass=on
>   spec_store_bypass=auto

You're late to that party. This has been discussed before and I shot it
down for the following reason:

 For all other vulnerabilities we control the mitigation on the kernel
 command line and we should stay consistent with that and not start to turn
 it around into a performance feature control knob for this one.

> This would also remove the standalone and dissonant nospec_store_bypass_disable 
> boot parameter.

That is redundant anyway, so it could just go away. 

> ==============>
> c20ee3e76f12: x86/bugs/AMD: Add support to disable RDS on Fam[15,16,17]h if requested
> 
> s/x86/bugs/AMD: Add support to disable RDS on Fam[15,16,17]h if requested
>  /x86/bugs/AMD: Add support to disable RDS on Fam[15,16,17]h CPUs if requested
> 
> Also, I suspect 'Reduced Data Speculation' should be written out once in the 
> changelog.
> 
> Question: does the x86_amd_ls_cfg_base only include RDS bits? If yes then I 
> suspect we should rename:
> 
>    x86_amd_ls_cfg_base => x86_amd_ls_cfg_rds_base

No, it's a magic configuration MSR with other control bits.

> ==============>
> b4fe2032d782: prctl: Add speculation control prctls
> 
> +/* Per task speculation control */
> +#define PR_SET_SPECULATION_CTRL                52
> +#define PR_GET_SPECULATION_CTRL                53
> 
> Please flip this around, i.e.:
> 
>   /* Per task speculation control */
>   #define PR_GET_SPECULATION_CTRL                52
>   #define PR_SET_SPECULATION_CTRL                53
> 
> because GET/SET is the canonical ABI order, used in most early prctl()s as well, 
> until someone messed it up in PR_SET_TIMERSLACK, which bad example then everyone 
> after that cargo-cult-copied ... Let's go back to the original ABI sanity.

> +       case PR_SET_SPECULATION_CTRL:
> +               if (arg4 || arg5)
> +                       return -EINVAL;
> 
> So I think returning -ENOSYS might be better, to allow future extensions to use 
> -EINVAL for a real invalid value - and allow -ENOSYS to be the "older kernel" 
> disambiguation. For tools that care.

Take that up with Linus. See his reply on that :)

> Also, a naive reading of the boot options:
> 
>                         auto   - Kernel detects whether the CPU model contains a
>                                  implementation of Speculative Store Bypass and
>                                  picks the most appropriate mitigation
> +                       prctl  - Control Speculative Store Bypass for a thread
> +                                via prctl. By default it is enabled. The state
> +                                is inherited on fork.
> 
> does not resolve the question of whether 'on', 'on', 'auto' or 'prctl' is the best 
> option to use. I.e. the interaction of on/off/auto/prctl isn't clear IMHO.

Can you come up with something better?

> Another question is:
> 
> +static int ssb_prctl_set(unsigned long ctrl)
> +{
> +       bool rds = !!test_tsk_thread_flag(current, TIF_RDS);
> +
> +       if (ssb_mode != SPEC_STORE_BYPASS_PRCTL)
> +               return -ENXIO;
> 
> This might be overly permissive in the 'auto' case which defaulted to 'on', where 
> an application might still want to disable it for good reasons?

Permissive? Its restrictive.

Also the default is 'prctl' now and I don't think that you want applications
let override the admin decision, which might be 'on' == global mitigation or
'off' == 'no mitigation at all'.

So when the mitigation mode is 'prctl' which is the default then
applications can fiddle with it. If the admin decided global on or off then
it's rightfully rejected.

Hmm?

Thanks,

	tglx

[MODERATED] Re: SBB V10 Bundle

[Ingo Molnar]

* speck for Thomas Gleixner <speck@linutronix.de> wrote:

> > Another question is:
> > 
> > +static int ssb_prctl_set(unsigned long ctrl)
> > +{
> > +       bool rds = !!test_tsk_thread_flag(current, TIF_RDS);
> > +
> > +       if (ssb_mode != SPEC_STORE_BYPASS_PRCTL)
> > +               return -ENXIO;
> > 
> > This might be overly permissive in the 'auto' case which defaulted to 'on', where 
> > an application might still want to disable it for good reasons?
> 
> Permissive? Its restrictive.

I mean when 'auto' set it to 'off', i.e. turned off mitigation. (Darn all the 
negatives ...)

> Also the default is 'prctl' now and I don't think that you want applications
> let override the admin decision, which might be 'on' == global mitigation or
> 'off' == 'no mitigation at all'.

Sure, explicit 'on' and 'off' is an admin choice - but 'auto' isn't really: it's 
the admin saying "whatever you think is right".

> So when the mitigation mode is 'prctl' which is the default then
> applications can fiddle with it. If the admin decided global on or off then
> it's rightfully rejected.
> 
> Hmm?

Correct, the only question is when 'auto' decides 'off' and app uses the prctl().

Do we even care, or do we think if the kernel thinks 'off' is correct then it 
should be forced?

I'd be fine with "we don't care" if "prctl" is the expected distro default.

Thanks,

	Ingo

[MODERATED] Re: SBB V10 Bundle

[Ingo Molnar]

* speck for Thomas Gleixner <speck@linutronix.de> wrote:

> > ==============>
> > b4fe2032d782: prctl: Add speculation control prctls
> > 
> > +/* Per task speculation control */
> > +#define PR_SET_SPECULATION_CTRL                52
> > +#define PR_GET_SPECULATION_CTRL                53
> > 
> > Please flip this around, i.e.:
> > 
> >   /* Per task speculation control */
> >   #define PR_GET_SPECULATION_CTRL                52
> >   #define PR_SET_SPECULATION_CTRL                53
> > 
> > because GET/SET is the canonical ABI order, used in most early prctl()s as well, 
> > until someone messed it up in PR_SET_TIMERSLACK, which bad example then everyone 
> > after that cargo-cult-copied ... Let's go back to the original ABI sanity.

JFYI, you didn't reply to this one - forgot to reply?

> > +       case PR_SET_SPECULATION_CTRL:
> > +               if (arg4 || arg5)
> > +                       return -EINVAL;
> > 
> > So I think returning -ENOSYS might be better, to allow future extensions to use 
> > -EINVAL for a real invalid value - and allow -ENOSYS to be the "older kernel" 
> > disambiguation. For tools that care.
> 
> Take that up with Linus. See his reply on that :)

Sorry, if that was on this list then his remarks are either not in my archive,
or Mutt doesn't properly search inside encrypted mails, or the list wasn't
Cc:-ed to that reply. Mind quoting that reply here?

Without having seen his reply:

- When introducing _new_ ABIs then using -ENOSYS is entirely canonical for "no 
  support exists that, yet".

- For _existing_ driver and syscall interfaces using -ENOSYS can be indeed be
  confusing to apps and thus be the wrong thing to use.

> > Also, a naive reading of the boot options:
> > 
> >                         auto   - Kernel detects whether the CPU model contains a
> >                                  implementation of Speculative Store Bypass and
> >                                  picks the most appropriate mitigation
> > +                       prctl  - Control Speculative Store Bypass for a thread
> > +                                via prctl. By default it is enabled. The state
> > +                                is inherited on fork.
> > 
> > does not resolve the question of whether 'on', 'on', 'auto' or 'prctl' is the best 
> > option to use. I.e. the interaction of on/off/auto/prctl isn't clear IMHO.
> 
> Can you come up with something better?

Sure:

                       auto   - Kernel detects whether the CPU model contains a
                                implementation of Speculative Store Bypass and
                                picks the most appropriate mitigation global
                                mitigation policy, which cannot be influenced
                                by apps even via prctl.
                       prctl  - Control Speculative Store Bypass for a thread
                                via prctl. By default mitigation is enabled.
                                The state is inherited on fork.

Btw., is it true that in the 'prctl' case mitigation is always enabled? If it's 
still CPU family/model sensitive like 'auto', which I think it is, then it might 
be more accurate to write:

                       prctl  - Control Speculative Store Bypass for a thread
                                via prctl. By default mitigation is enabled
                                on affected CPUs. The state is inherited on fork.

or so?

Thanks,

	Ingo

Re: SBB V10 Bundle

[Thomas Gleixner]

On Thu, 3 May 2018, speck for Ingo Molnar wrote:

> 
> * speck for Thomas Gleixner <speck@linutronix.de> wrote:
> 
> > > ==============>
> > > b4fe2032d782: prctl: Add speculation control prctls
> > > 
> > > +/* Per task speculation control */
> > > +#define PR_SET_SPECULATION_CTRL                52
> > > +#define PR_GET_SPECULATION_CTRL                53
> > > 
> > > Please flip this around, i.e.:
> > > 
> > >   /* Per task speculation control */
> > >   #define PR_GET_SPECULATION_CTRL                52
> > >   #define PR_SET_SPECULATION_CTRL                53
> > > 
> > > because GET/SET is the canonical ABI order, used in most early prctl()s as well, 
> > > until someone messed it up in PR_SET_TIMERSLACK, which bad example then everyone 
> > > after that cargo-cult-copied ... Let's go back to the original ABI sanity.
> 
> JFYI, you didn't reply to this one - forgot to reply?

Why so, I didn't disagree and fixed it. Like other stuff :)

Re: SBB V10 Bundle

[Thomas Gleixner]

On Thu, 3 May 2018, speck for Ingo Molnar wrote:
> * speck for Thomas Gleixner <speck@linutronix.de> wrote:
> > > So I think returning -ENOSYS might be better, to allow future extensions to use 
> > > -EINVAL for a real invalid value - and allow -ENOSYS to be the "older kernel" 
> > > disambiguation. For tools that care.
> > 
> > Take that up with Linus. See his reply on that :)
> 
> Sorry, if that was on this list then his remarks are either not in my archive,
> or Mutt doesn't properly search inside encrypted mails, or the list wasn't
> Cc:-ed to that reply. Mind quoting that reply here?
> 
> Without having seen his reply:
> 
> - When introducing _new_ ABIs then using -ENOSYS is entirely canonical for "no 
>   support exists that, yet".
> 
> - For _existing_ driver and syscall interfaces using -ENOSYS can be indeed be
>   confusing to apps and thus be the wrong thing to use.

Here is the relevant snippet:

> On Mon, 30 Apr 2018, speck for Linus Torvalds wrote:
> > On Mon, 30 Apr 2018, speck for Kees Cook wrote:
> > > 
> > > I think EINVAL is more expected in this case (and is what Michael Kerrisk
> > > has asked for in the past). In this specific case, users are able to test
> > > PR_GET_SPECULATION_CTRL to check for the prctl. Having arg3/4/5 forced
> > > to zero for that right now will ensure they're checking correctly from
> > > the start. :)
> > 
> > Yeah. If you want to test whether the support is there or not, just use 
> > the right arguments.
> 
> Fair enough. Though I would have chosen -EMORON if that would have not been
> prevented by the political correctness crowd from being merged.
> 
> tglx

I just went with EINVAL. I guess EMORON would have raised that discussion.

> > > Also, a naive reading of the boot options:
> > > 
> > >                         auto   - Kernel detects whether the CPU model contains a
> > >                                  implementation of Speculative Store Bypass and
> > >                                  picks the most appropriate mitigation
> > > +                       prctl  - Control Speculative Store Bypass for a thread
> > > +                                via prctl. By default it is enabled. The state
> > > +                                is inherited on fork.
> > > 
> > > does not resolve the question of whether 'on', 'on', 'auto' or 'prctl' is the best 
> > > option to use. I.e. the interaction of on/off/auto/prctl isn't clear IMHO.
> > 
> > Can you come up with something better?
> 
> Sure:
> 
>                        auto   - Kernel detects whether the CPU model contains a
>                                 implementation of Speculative Store Bypass and
>                                 picks the most appropriate mitigation global
>                                 mitigation policy, which cannot be influenced
>                                 by apps even via prctl.
>                        prctl  - Control Speculative Store Bypass for a thread
>                                 via prctl. By default mitigation is enabled.
>                                 The state is inherited on fork.
> 
> Btw., is it true that in the 'prctl' case mitigation is always enabled? If it's 
> still CPU family/model sensitive like 'auto', which I think it is, then it might 
> be more accurate to write:

No. It's tthe other way round. Here is how I reworded it in V11

                        on     - Unconditionally disable Speculative Store Bypass
                        off    - Unconditionally enable Speculative Store Bypass
                        auto   - Kernel detects whether the CPU model contains an
                                 implementation of Speculative Store Bypass and
                                 picks the most appropriate mitigation.
                        prctl  - Control Speculative Store Bypass per thread
                                 via prctl. Speculative Store Bypass is enabled
                                 for a process by default. The state of the control
                                 is inherited on fork.

Thanks,

	tglx

Re: SBB V10 Bundle

[Thomas Gleixner]

On Thu, 3 May 2018, speck for Thomas Gleixner wrote:
> On Thu, 3 May 2018, speck for Ingo Molnar wrote:
> Here is the relevant snippet:
> 
> > On Mon, 30 Apr 2018, speck for Linus Torvalds wrote:
> > > On Mon, 30 Apr 2018, speck for Kees Cook wrote:
> > > > 
> > > > I think EINVAL is more expected in this case (and is what Michael Kerrisk
> > > > has asked for in the past). In this specific case, users are able to test
> > > > PR_GET_SPECULATION_CTRL to check for the prctl. Having arg3/4/5 forced
> > > > to zero for that right now will ensure they're checking correctly from
> > > > the start. :)
> > > 
> > > Yeah. If you want to test whether the support is there or not, just use 
> > > the right arguments.
> > 
> > Fair enough. Though I would have chosen -EMORON if that would have not been
> > prevented by the political correctness crowd from being merged.
> > 
> > tglx
> 
> I just went with EINVAL. I guess EMORON would have raised that discussion.

wouldn't :)