From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from hsi-kbw-5-158-153-52.hsi19.kabel-badenwuerttemberg.de ([5.158.153.52] helo=nanos.tec.linutronix.de) by Galois.linutronix.de with esmtpsa (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1fC5v9-0004VT-8J for speck@linutronix.de; Fri, 27 Apr 2018 18:08:27 +0200 Date: Fri, 27 Apr 2018 18:08:26 +0200 (CEST) From: Thomas Gleixner Subject: Re: [PATCH v2] Linux Patch 1/1 In-Reply-To: <92c45587-5eb3-2421-6fc1-42e74a715e30@linux.intel.com> Message-ID: References: <92c45587-5eb3-2421-6fc1-42e74a715e30@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII To: speck@linutronix.de List-ID: On Wed, 25 Apr 2018, speck for Tim Chen wrote: > +/* dynamic reduced data speculation in use */ > +DEFINE_STATIC_KEY_FALSE(specctrl_rds); > +EXPORT_SYMBOL(specctrl_rds); Why needs this to be exported? > +/* current MSR_IA32_SPEC_CTRL state for ssb mitigation */ > +DEFINE_PER_CPU(int, rds_msr_val); > +EXPORT_SYMBOL(rds_msr_val); Ditto > /* The kernel command line selection */ > enum ssb_mitigation_cmd { > SPEC_STORE_BYPASS_CMD_NONE, > SPEC_STORE_BYPASS_CMD_AUTO, > SPEC_STORE_BYPASS_CMD_ON, > + SPEC_STORE_BYPASS_CMD_PRCTL, > }; > > static const char *ssb_strings[] = { > [SPEC_STORE_BYPASS_NONE] = "Vulnerable", > - [SPEC_STORE_BYPASS_DISABLE] = "Mitigation: Speculative Store Bypass disabled" > + [SPEC_STORE_BYPASS_DISABLE] = "Mitigation: Speculative Store Bypass disabled", > + [SPEC_STORE_BYPASS_PRCTL] = "Mitigation: Speculative Store Bypass disabled via prctl" > }; > > static const struct { > @@ -395,8 +408,61 @@ static const struct { > { "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 Bypass */ > + { "prctl", SPEC_STORE_BYPASS_CMD_PRCTL }, /* disable Speculative Store Bypass via prctl */ > }; > > +void enable_speculative_store_bypass_mitigation(void) The declaration of those functions says: > +/* > + * speculation store bypass vulnerabilities mitigation functions. > + * > + * Should be called only from pre-emption off paths. > + * > + */ Can you please explain how the PRCTL code fulfils the preempt off prerequisite? It does not, which clearly shows that you never tested that code with the full set of debug stuff enabled, because otherwise the preempt check in __this_cpu_read/write would have triggered. > +{ > + if (static_branch_unlikely(&specctrl_rds) && > + /* need to toggle msr? */ > + __this_cpu_read(rds_msr_val) != SPEC_CTRL_RDS) { Bah. This is horrible to read, really. if (static_branch_unlikely(&specctrl_rds)) { if (__this_cpu_read(rds_msr_val) != SPEC_CTRL_RDS) { wrmsrl(MSR_IA32_SPEC_CTRL, SPEC_CTRL_RDS | x86_spec_ctrl_base); __this_cpu_write(rds_msr_val, SPEC_CTRL_RDS); } } But see below. > +int arch_prctl_set_rds(struct task_struct *p, unsigned long val) > +{ > + if (val == PR_RDS_MODE_ENABLE) { > + set_tsk_thread_flag(p, TIF_RDS); > + if (p == current) > + enable_speculative_store_bypass_mitigation(); > + } else if (val == PR_RDS_MODE_DISABLE) { > + clear_tsk_thread_flag(p, TIF_RDS); > + if (p == current) > + disable_speculative_store_bypass_mitigation(); In an earlier discussion it was said, that the prtcl - if at all - is a one way operation, i.e. set it end be done with it. Why is this different again? And allowing to disable it once it is set is broken, really. > + } else > + return -EINVAL; Missing parentheses. Dammit, why do I have say that over and over? > + > + return 0; > +} > + > +int arch_prctl_get_rds(struct task_struct *p) > +{ > + if (test_tsk_thread_flag(p, TIF_RDS)) > + return PR_RDS_MODE_ENABLE; > + > + return PR_RDS_MODE_DISABLE; > +} > + > +void arch_prctl_copy_rds_mode(struct task_struct *parent, struct task_struct *child) What the heck has this to do with prctl? Its about TIF_RDS where ever that flag has been set from. > +{ > + if (test_tsk_thread_flag(parent, TIF_RDS)) > + set_tsk_thread_flag(child, TIF_RDS); > + else > + clear_tsk_thread_flag(child, TIF_RDS); > +} > + > static enum ssb_mitigation_cmd __init ssb_parse_cmdline(void) > { > char arg[20]; > @@ -445,6 +511,7 @@ static void __init ssb_select_mitigation(void) > cmd == SPEC_STORE_BYPASS_CMD_AUTO)) > return; > > +again: > switch (cmd) { > case SPEC_STORE_BYPASS_CMD_AUTO: > /* > @@ -452,9 +519,17 @@ static void __init ssb_select_mitigation(void) > */ > if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) > break; > + /* Choose prctl as the default mode */ > + cmd = SPEC_STORE_BYPASS_CMD_PRCTL; > + goto again; > + break; WTF? This is just crap. The goto is not required at all if done right. > case SPEC_STORE_BYPASS_CMD_ON: > @@ -529,6 +530,17 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p) > /* Load the Intel cache allocation PQR MSR. */ > intel_rdt_sched_in(); > > + if (test_tsk_thread_flag(next_p, TIF_RDS)) > + /* > + * Reduce data speculation for processes that > + * have reduced data speculation option turned > + * on via prctl. This provides protection against speculation > + * store bypass attack for untrusted code within the process. > + */ > + enable_speculative_store_bypass_mitigation(); > + else > + disable_speculative_store_bypass_mitigation(); So this does an unconditional function call on every context switch independent of whether the prctl mode is enabled or not. Crap, really. What the heck is wrong with putting the TIF flag into _TIF_WORK_CTXSW and put this into __switch_to_extra() and do: if ((tifp ^ tifn) & _TIF_RDS) speculative_bypass_update(tifn); and that would be: static inline void speculative_bypass_update(unsigned long tifn) { u64 msr = x86_spec_ctrl_base | ((tifn & _TIF_RDS) << TIF_TO_RDS_SHIFT); wrmsrl(MSR_IA32_SPEC_CTRL, msr); } Which would get rid of the static key because the only place where the mitigation mode and therefore the PRCTL enablement needs to be checked is the prctl itself. int arch_prctl_set_rds(struct task_struct *p, unsigned long val) { if (!spec_store_bypass_prctl) return -ENOTSUPP; if (val != PR_RDS_MODE_ENABLE) return -EINVAL; set_tsk_thread_flag(p, TIF_RDS); if (p == current) { preempt_disable(); speculative_bypass_update(task_thread_info(p)->flags); preempt_enable(); } return 0; } See? No static key in the context switch path and no unconditional function calls, a single conditional which is evaluated already in switch_to. If RDS is never set then speculative_bypass_update() is never ever invoked. If it needs to be toggled, then the MSR write is worse than the extra hoop through __switch_to_extra(). It also spares the per cpu state variable for this RDS bit. That would be too simple and too performant and would magically make this work for 32bit as well, right? It even would make the seccomp magic simple because all it has to do is to set TIF_RDS on the thread once and not check for seccomp on every fricking context switch. And most important it has absolutely ZERO impact if the prctl is not enabled. That said, the above might be acceptable when Linus does not veto the whole thing in general. Thanks, tglx