[MODERATED] Re: [patch V8 15/15] SSB 15

[Kees Cook <kees@outflux.net>]

On Mon, Apr 30, 2018 at 05:04:38PM +0200, speck for Thomas Gleixner wrote:
> Add prctl based control for Speculative Store Bypass mitigation and make it
> the default mitigation for Intel.

Maybe "default mitigation strategy" ? Because this kind of changes things
pretty dramatically: now the mitigation is explicitly opt-in.

> Andi Kleen provided the following rationale (slightly redacted):
> 
>  There are multiple levels of impact of Speculative Store Bypass:
> 
>  1) JITed sandbox.
>     It cannot invoke system calls, but can do PRIME+PROBE and may have call
>     interfaces to other code
> 
>  2) Native code process.
>     No protection inside the process at this level.
> 
>  3) Kernel.
> 
>  4) Between processes. 
> 
>  The prctl tries to protect against case (1) doing attacks.
> 
>  If the untrusted code can do random system calls then control is already
>  lost in a much worse way. So there needs to be system call protection in
>  some way (using a JIT not allowing them or seccomp). Or rather if the
>  process can subvert its environment somehow to do the prctl it can already
>  execute arbitrary code, which is much worse than SSB.
> 
>  To put it differently, the point of the prctl is to not allow JITed code
>  to read data it shouldn't read from its JITed sandbox. If it already has
>  escaped its sandbox then it can already read everything it wants in its
>  address space, and do much worse.
> 
>  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.

I basically agree with the analysis, but I feel like there's more to
consider.

I'm uncomfortable with the idea that people can only fix SSB by having both
the right hardware AND having software that knows to call the needed
prctl. That is double opt-in. :( It leaves their old browsers vulnerable,
for example. This kind of thing should be opt-out, yes?

And if we're going to do opt-in, I think we could do it a bit better. (And
sorry if this came up before, I see some reference to this in other
threads, but I was late to this list.) If we want to cover sandboxes, it
seems like looking at seccomp and no_new_privs is a good way to know if
something is expecting to deal with untrusted input. On modern Ubuntu,
331 processes running on the desktop, 17 have seccomp and NNP, most of
which is Chrome:

# egrep 'Name|Seccomp|NoNewPriv' /proc/*/status > /tmp/status.txt
# grep Name /tmp/status.txt | wc -l
331
# grep Seccomp /tmp/status.txt | grep -v '0$' | wc -l
17
# grep NoNewPriv /tmp/status.txt | grep -v '0$' | wc -l
17
# grep -B1 'NoNewPrivs:.*1$' /tmp/status.txt | grep Name | cut -d: -f3- |
    sort | uniq -c | sort -n
 1 irqbalance
 1 nacl_helper
 1 systemd-resolve
 1 systemd-timesyn
13 chrome

(and yes, if I run Firefox, I then see "Web Content" as listed too)


Other notes below...

> 
> Based on an initial patch from Tim Chen. Completely rewritten.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |    3 
>  arch/x86/include/asm/nospec-branch.h            |    1 
>  arch/x86/kernel/cpu/bugs.c                      |   78 ++++++++++++++++++++++--
>  3 files changed, 77 insertions(+), 5 deletions(-)
> 
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4054,6 +4054,9 @@
>  			auto   - Kernel detects whether the CPU model contains a
>  			         vulnerable 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.

"By default it is enabled." I find confusing here. And making it the
default while there is still something named "auto" hurts my head too.
Perhaps add the clarifier "global" to the "auto" description: "... the
most appropriate global mitigation." And change prctl to:

			prctl  - Control Speculative Store Bypass for a thread
			         via prctl. The state is inherited on fork. This
			         is the default mitigation strategy if none is
			         selected on the kernel cmdline.

> +static int sbb_prctl_set(unsigned long ctrl)
> +{
> +	bool rds = !!test_tsk_thread_flag(current, TIF_RDS);

With at least both seccomp and no_new_privs, we've had people want to be
able to externally query the state of processes, and a line was added to
/proc/$pid/status. I think this would be of similar interest, though it
likely needs to report the global state too, not just the process state
in other modes. For example:

diff --git a/fs/proc/array.c b/fs/proc/array.c
index ae2c807fd719..4a5cdf199fde 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -335,6 +335,25 @@ static inline void task_seccomp(struct seq_file *m, struct task_struct *p)
 #ifdef CONFIG_SECCOMP
 	seq_put_decimal_ull(m, "\nSeccomp:\t", p->seccomp.mode);
 #endif
+	seq_printf(m, "\nSpeculation, Store Bypass:\t");
+	switch (arch_prctl_get_spec_ctrl(PR_SPEC_STORE_BYPASS)) {
+	case -EINVAL:
+		seq_printf(m, "unknown");
+		break;
+	case PR_SPEC_NOT_AFFECTED:
+		seq_printf(m, "not vulnerable");
+		break;
+	case PR_SPEC_PRCTL:
+		seq_printf(m, test_tsk_thread_flag(p, TIF_RDS) ?
+				"thread mitigated" : "thread vulnerable");
+		break;
+	case PR_SPEC_DISABLE:
+		seq_printf(m, "globally mitigated");
+		break;
+	default:
+		seq_printf(m, "vulnerable");
+		break;
+	}
 	seq_putc(m, '\n');
 }
 


-Kees

-- 
Kees Cook                                            @outflux.net