From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-wr0-x230.google.com ([2a00:1450:400c:c0c::230]) by Galois.linutronix.de with esmtps (TLS1.2:RSA_AES_128_CBC_SHA1:128) (Exim 4.80) (envelope-from ) id 1fDqXc-0001bA-Bp for speck@linutronix.de; Wed, 02 May 2018 14:07:24 +0200 Received: by mail-wr0-x230.google.com with SMTP id i14-v6so10585391wre.2 for ; Wed, 02 May 2018 05:07:24 -0700 (PDT) Received: from gmail.com (2E8B0CD5.catv.pool.telekom.hu. [46.139.12.213]) by smtp.gmail.com with ESMTPSA id w11-v6sm21299791wrn.86.2018.05.02.05.07.17 for (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 02 May 2018 05:07:18 -0700 (PDT) Sender: Ingo Molnar Date: Wed, 2 May 2018 14:07:16 +0200 From: Ingo Molnar Subject: [MODERATED] Re: SBB V10 Bundle Message-ID: <20180502120716.ohnwlckp7qi7njs3@gmail.com> References: MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: A quick review of the v10 series: ==============> b6f76374d45a: x86/nospec: Simplify alternative_msr_write() Reviewed-by: Ingo Molnar ==============> 3ec366cfa844: x86/bugs: Concentrate bug detection into a separate function Reviewed-by: Ingo Molnar ==============> 3b06f42b82ad: x86/bugs: Concentrate bug reporting into a separate function Reviewed-by: Ingo Molnar ==============> 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 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 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 ==============> b6e792e6f30d: x86/cpufeatures: Add X86_FEATURE_RDS Reviewed-by: Ingo Molnar ==============> 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 ==============> 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 ==============> 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 ==============> 16544ca81942: x86/KVM/VMX: Expose SPEC_CTRL Bit(2) to the guest Reviewed-by: Ingo Molnar ==============> 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 ==============> 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 ==============> 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 ==============> 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