Re: SBB V10 Bundle

[Thomas Gleixner <tglx@linutronix.de>]

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