[MODERATED] Re: SBB V10 Bundle

[Ingo Molnar <mingo@kernel.org>]

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