All of lore.kernel.org
 help / color / mirror / Atom feed
* [MODERATED] [PATCH SSBv11 3/3] seccomp 0
  2018-05-03  0:44 [MODERATED] [PATCH SSBv11 0/3] seccomp 1 Kees Cook
@ 2018-05-01 22:07 ` Kees Cook
  2018-05-01 22:19 ` [MODERATED] [PATCH SSBv11 1/3] seccomp 2 Kees Cook
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Kees Cook @ 2018-05-01 22:07 UTC (permalink / raw)
  To: speck

When speculation flaw mitigations are opt-in (via prctl), using seccomp
will automatically opt-in to these protections, since using seccomp
indicates at least some level of sandboxing is desired.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 kernel/seccomp.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index dc77548167ef..88933d05e81b 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -19,6 +19,8 @@
 #include <linux/compat.h>
 #include <linux/coredump.h>
 #include <linux/kmemleak.h>
+#include <linux/nospec.h>
+#include <linux/prctl.h>
 #include <linux/sched.h>
 #include <linux/sched/task_stack.h>
 #include <linux/seccomp.h>
@@ -227,6 +229,19 @@ static inline bool seccomp_may_assign_mode(unsigned long seccomp_mode)
 	return true;
 }
 
+/*
+ * If a given speculation mitigation is opt-in (prctl()-controlled),
+ * select it, by disabling speculation (enabling mitigation).
+ */
+static inline void spec_mitigate(struct task_struct *task,
+				 unsigned long which)
+{
+	int state = arch_prctl_spec_ctrl_get(task, which);
+
+	if (state > 0 && (state & PR_SPEC_PRCTL))
+		arch_prctl_spec_ctrl_set(task, which, PR_SPEC_DISABLE);
+}
+
 static inline void seccomp_assign_mode(struct task_struct *task,
 				       unsigned long seccomp_mode)
 {
@@ -239,6 +254,9 @@ static inline void seccomp_assign_mode(struct task_struct *task,
 	 */
 	smp_mb__before_atomic();
 	set_tsk_thread_flag(task, TIF_SECCOMP);
+
+	/* Assume seccomp processes want speculation flaw mitigation. */
+	spec_mitigate(task, PR_SPEC_STORE_BYPASS);
 }
 
 #ifdef CONFIG_SECCOMP_FILTER
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [MODERATED] [PATCH SSBv11 1/3] seccomp 2
  2018-05-03  0:44 [MODERATED] [PATCH SSBv11 0/3] seccomp 1 Kees Cook
  2018-05-01 22:07 ` [MODERATED] [PATCH SSBv11 3/3] seccomp 0 Kees Cook
@ 2018-05-01 22:19 ` Kees Cook
  2018-05-01 22:31 ` [MODERATED] [PATCH SSBv11 2/3] seccomp 3 Kees Cook
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Kees Cook @ 2018-05-01 22:19 UTC (permalink / raw)
  To: speck

This adjusts arch_prctl_get/set_spec_ctrl() to operate on tasks other
than current. This is needed both for /proc/$pid/status queries and
for seccomp (since thread-syncing can trigger seccomp in non-current
threads).

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/kernel/cpu/bugs.c | 27 ++++++++++++++++-----------
 include/linux/nospec.h     |  7 +++++--
 kernel/sys.c               | 10 ++++++----
 3 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 15f77d4518c7..a390325ce552 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -526,31 +526,35 @@ static void ssb_select_mitigation()
 
 #undef pr_fmt
 
-static int ssb_prctl_set(unsigned long ctrl)
+static int ssb_prctl_set(struct task_struct *task, unsigned long ctrl)
 {
-	bool rds = !!test_tsk_thread_flag(current, TIF_RDS);
+	bool rds = !!test_tsk_thread_flag(task, TIF_RDS);
 
 	if (ssb_mode != SPEC_STORE_BYPASS_PRCTL)
 		return -ENXIO;
 
 	if (ctrl == PR_SPEC_ENABLE)
-		clear_tsk_thread_flag(current, TIF_RDS);
+		clear_tsk_thread_flag(task, TIF_RDS);
 	else
-		set_tsk_thread_flag(current, TIF_RDS);
+		set_tsk_thread_flag(task, TIF_RDS);
 
-	if (rds != !!test_tsk_thread_flag(current, TIF_RDS))
+	/*
+	 * If being set on non-current task, delay setting the CPU
+	 * mitigation until it is next scheduled.
+	 */
+	if (task == current && rds != !!test_tsk_thread_flag(task, TIF_RDS))
 		speculative_store_bypass_update();
 
 	return 0;
 }
 
-static int ssb_prctl_get(void)
+static int ssb_prctl_get(struct task_struct *task)
 {
 	switch (ssb_mode) {
 	case SPEC_STORE_BYPASS_DISABLE:
 		return PR_SPEC_DISABLE;
 	case SPEC_STORE_BYPASS_PRCTL:
-		if (test_tsk_thread_flag(current, TIF_RDS))
+		if (test_tsk_thread_flag(task, TIF_RDS))
 			return PR_SPEC_PRCTL | PR_SPEC_DISABLE;
 		return PR_SPEC_PRCTL | PR_SPEC_ENABLE;
 	default:
@@ -560,24 +564,25 @@ static int ssb_prctl_get(void)
 	}
 }
 
-int arch_prctl_spec_ctrl_set(unsigned long which, unsigned long ctrl)
+int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long which,
+			     unsigned long ctrl)
 {
 	if (ctrl != PR_SPEC_ENABLE && ctrl != PR_SPEC_DISABLE)
 		return -ERANGE;
 
 	switch (which) {
 	case PR_SPEC_STORE_BYPASS:
-		return ssb_prctl_set(ctrl);
+		return ssb_prctl_set(task, ctrl);
 	default:
 		return -ENODEV;
 	}
 }
 
-int arch_prctl_spec_ctrl_get(unsigned long which)
+int arch_prctl_spec_ctrl_get(struct task_struct *task, unsigned long which)
 {
 	switch (which) {
 	case PR_SPEC_STORE_BYPASS:
-		return ssb_prctl_get();
+		return ssb_prctl_get(task);
 	default:
 		return -ENODEV;
 	}
diff --git a/include/linux/nospec.h b/include/linux/nospec.h
index 700bb8a4e4ea..a908c954484d 100644
--- a/include/linux/nospec.h
+++ b/include/linux/nospec.h
@@ -7,6 +7,8 @@
 #define _LINUX_NOSPEC_H
 #include <asm/barrier.h>
 
+struct task_struct;
+
 /**
  * array_index_mask_nospec() - generate a ~0 mask when index < size, 0 otherwise
  * @index: array element index
@@ -57,7 +59,8 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
 })
 
 /* Speculation control prctl */
-int arch_prctl_spec_ctrl_get(unsigned long which);
-int arch_prctl_spec_ctrl_set(unsigned long which, unsigned long ctrl);
+int arch_prctl_spec_ctrl_get(struct task_struct *task, unsigned long which);
+int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long which,
+			     unsigned long ctrl);
 
 #endif /* _LINUX_NOSPEC_H */
diff --git a/kernel/sys.c b/kernel/sys.c
index b76dee23bdc9..defd513e6ea2 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2244,12 +2244,14 @@ static int propagate_has_child_subreaper(struct task_struct *p, void *data)
 	return 1;
 }
 
-int __weak arch_prctl_spec_ctrl_get(unsigned long which)
+int __weak arch_prctl_spec_ctrl_get(struct task_struct *task,
+				    unsigned long which)
 {
 	return -EINVAL;
 }
 
-int __weak arch_prctl_spec_ctrl_set(unsigned long which, unsigned long ctrl)
+int __weak arch_prctl_spec_ctrl_set(struct task_struct *task,
+				    unsigned long which, unsigned long ctrl)
 {
 	return -EINVAL;
 }
@@ -2465,12 +2467,12 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 	case PR_GET_SPECULATION_CTRL:
 		if (arg3 || arg4 || arg5)
 			return -EINVAL;
-		error = arch_prctl_spec_ctrl_get(arg2);
+		error = arch_prctl_spec_ctrl_get(me, arg2);
 		break;
 	case PR_SET_SPECULATION_CTRL:
 		if (arg4 || arg5)
 			return -EINVAL;
-		error = arch_prctl_spec_ctrl_set(arg2, arg3);
+		error = arch_prctl_spec_ctrl_set(me, arg2, arg3);
 		break;
 	default:
 		error = -EINVAL;
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [MODERATED] [PATCH SSBv11 2/3] seccomp 3
  2018-05-03  0:44 [MODERATED] [PATCH SSBv11 0/3] seccomp 1 Kees Cook
  2018-05-01 22:07 ` [MODERATED] [PATCH SSBv11 3/3] seccomp 0 Kees Cook
  2018-05-01 22:19 ` [MODERATED] [PATCH SSBv11 1/3] seccomp 2 Kees Cook
@ 2018-05-01 22:31 ` Kees Cook
  2018-05-03  8:58 ` [MODERATED] Re: [PATCH SSBv11 3/3] seccomp 0 Peter Zijlstra
  2018-05-03 12:29 ` [MODERATED] Re: [PATCH SSBv11 0/3] seccomp 1 Andi Kleen
  4 siblings, 0 replies; 15+ messages in thread
From: Kees Cook @ 2018-05-01 22:31 UTC (permalink / raw)
  To: speck

As done with seccomp and no_new_privs, also show speculation flaw
mitigation state in /proc/$pid/status.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 fs/proc/array.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index ae2c807fd719..303c155f9b04 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -85,6 +85,7 @@
 #include <linux/delayacct.h>
 #include <linux/seq_file.h>
 #include <linux/pid_namespace.h>
+#include <linux/prctl.h>
 #include <linux/ptrace.h>
 #include <linux/tracehook.h>
 #include <linux/string_helpers.h>
@@ -335,6 +336,27 @@ 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_spec_ctrl_get(p, 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 | PR_SPEC_DISABLE:
+		seq_printf(m, "thread mitigated");
+		break;
+	case PR_SPEC_PRCTL | PR_SPEC_ENABLE:
+		seq_printf(m, "thread vulnerable");
+		break;
+	case PR_SPEC_DISABLE:
+		seq_printf(m, "globally mitigated");
+		break;
+	default:
+		seq_printf(m, "vulnerable");
+		break;
+	}
 	seq_putc(m, '\n');
 }
 
-- 
2.17.0

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [MODERATED] [PATCH SSBv11 0/3] seccomp 1
@ 2018-05-03  0:44 Kees Cook
  2018-05-01 22:07 ` [MODERATED] [PATCH SSBv11 3/3] seccomp 0 Kees Cook
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Kees Cook @ 2018-05-03  0:44 UTC (permalink / raw)
  To: speck

As seccomp use overlaps best (though not perfectly) with applications
most likely to want speculation flaw mitigations enabled, seccomp will
enable them when seccomp is enabled for a task. Also adds a line to
/proc/$pid/status for examining the mitigation state of a task.

-Kees


Kees Cook (3):
  nospec: Allow getting/setting on non-current task
  proc: Provide details on speculation flaw mitigations
  seccomp: Enable speculation flaw mitigations

 arch/x86/kernel/cpu/bugs.c | 27 ++++++++++++++++-----------
 fs/proc/array.c            | 22 ++++++++++++++++++++++
 include/linux/nospec.h     |  7 +++++--
 kernel/seccomp.c           | 18 ++++++++++++++++++
 kernel/sys.c               | 10 ++++++----
 5 files changed, 67 insertions(+), 17 deletions(-)

-- 
2.17.0

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [MODERATED] Re: [PATCH SSBv11 3/3] seccomp 0
  2018-05-03  0:44 [MODERATED] [PATCH SSBv11 0/3] seccomp 1 Kees Cook
                   ` (2 preceding siblings ...)
  2018-05-01 22:31 ` [MODERATED] [PATCH SSBv11 2/3] seccomp 3 Kees Cook
@ 2018-05-03  8:58 ` Peter Zijlstra
  2018-05-03  9:21   ` Thomas Gleixner
  2018-05-03 12:29 ` [MODERATED] Re: [PATCH SSBv11 0/3] seccomp 1 Andi Kleen
  4 siblings, 1 reply; 15+ messages in thread
From: Peter Zijlstra @ 2018-05-03  8:58 UTC (permalink / raw)
  To: speck

On Tue, May 01, 2018 at 03:07:31PM -0700, speck for Kees Cook wrote:
> @@ -239,6 +254,9 @@ static inline void seccomp_assign_mode(struct task_struct *task,
>  	 */
>  	smp_mb__before_atomic();
>  	set_tsk_thread_flag(task, TIF_SECCOMP);
> +
> +	/* Assume seccomp processes want speculation flaw mitigation. */
> +	spec_mitigate(task, PR_SPEC_STORE_BYPASS);
>  }

What about the ordering there? That function appears to explicitly set
TIF_SECCOMP last, such that when that is observed, the complete
environment is observed.

But now you add something after it. Does this not mean that if you set
this on a remote task, this task can execute TIF_SECCOMP thing before it
disables SSB.

Is that OK?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH SSBv11 3/3] seccomp 0
  2018-05-03  8:58 ` [MODERATED] Re: [PATCH SSBv11 3/3] seccomp 0 Peter Zijlstra
@ 2018-05-03  9:21   ` Thomas Gleixner
  2018-05-03 16:03     ` [MODERATED] " Kees Cook
  0 siblings, 1 reply; 15+ messages in thread
From: Thomas Gleixner @ 2018-05-03  9:21 UTC (permalink / raw)
  To: speck

On Thu, 3 May 2018, speck for Peter Zijlstra wrote:
> On Tue, May 01, 2018 at 03:07:31PM -0700, speck for Kees Cook wrote:
> > @@ -239,6 +254,9 @@ static inline void seccomp_assign_mode(struct task_struct *task,
> >  	 */
> >  	smp_mb__before_atomic();
> >  	set_tsk_thread_flag(task, TIF_SECCOMP);
> > +
> > +	/* Assume seccomp processes want speculation flaw mitigation. */
> > +	spec_mitigate(task, PR_SPEC_STORE_BYPASS);
> >  }
> 
> What about the ordering there? That function appears to explicitly set
> TIF_SECCOMP last, such that when that is observed, the complete
> environment is observed.
> 
> But now you add something after it. Does this not mean that if you set
> this on a remote task, this task can execute TIF_SECCOMP thing before it
> disables SSB.
> 
> Is that OK?

It probably should be the other way round.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [MODERATED] Re: [PATCH SSBv11 0/3] seccomp 1
  2018-05-03  0:44 [MODERATED] [PATCH SSBv11 0/3] seccomp 1 Kees Cook
                   ` (3 preceding siblings ...)
  2018-05-03  8:58 ` [MODERATED] Re: [PATCH SSBv11 3/3] seccomp 0 Peter Zijlstra
@ 2018-05-03 12:29 ` Andi Kleen
  2018-05-03 12:45   ` Thomas Gleixner
  4 siblings, 1 reply; 15+ messages in thread
From: Andi Kleen @ 2018-05-03 12:29 UTC (permalink / raw)
  To: speck

On Wed, May 02, 2018 at 05:44:27PM -0700, speck for Kees Cook wrote:
> From: Kees Cook <keescook@chromium.org>
> Subject:  opt-in under seccomp
> 
> As seccomp use overlaps best (though not perfectly) with applications
> most likely to want speculation flaw mitigations enabled, seccomp will
> enable them when seccomp is enabled for a task. Also adds a line to
> /proc/$pid/status for examining the mitigation state of a task.

As I wrote earlier this isn't a good idea. We went through
this earlier. 

It originally seems like a good idea until you start looking at the details.

The big users of seccomp like the web browsers eventually
don't want it once they use site isolation. And it would
unnecessarily slow them down.

And other programs need to be maintained anyways (e.g. for 
spectre variant 1 fixes) so they can as well add it explicitely.

Separate enabling makes more sense. And that is already in the patchkit.

-Andi

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH SSBv11 0/3] seccomp 1
  2018-05-03 12:29 ` [MODERATED] Re: [PATCH SSBv11 0/3] seccomp 1 Andi Kleen
@ 2018-05-03 12:45   ` Thomas Gleixner
  2018-05-03 14:09     ` [MODERATED] " Ingo Molnar
  2018-05-03 14:47     ` Andi Kleen
  0 siblings, 2 replies; 15+ messages in thread
From: Thomas Gleixner @ 2018-05-03 12:45 UTC (permalink / raw)
  To: speck

On Thu, 3 May 2018, speck for Andi Kleen wrote:
> On Wed, May 02, 2018 at 05:44:27PM -0700, speck for Kees Cook wrote:
> > From: Kees Cook <keescook@chromium.org>
> > Subject:  opt-in under seccomp
> > 
> > As seccomp use overlaps best (though not perfectly) with applications
> > most likely to want speculation flaw mitigations enabled, seccomp will
> > enable them when seccomp is enabled for a task. Also adds a line to
> > /proc/$pid/status for examining the mitigation state of a task.
> 
> As I wrote earlier this isn't a good idea. We went through
> this earlier. 
> 
> It originally seems like a good idea until you start looking at the details.
> 
> The big users of seccomp like the web browsers eventually
> don't want it once they use site isolation. And it would
> unnecessarily slow them down.
> 
> And other programs need to be maintained anyways (e.g. for 
> spectre variant 1 fixes) so they can as well add it explicitely.
> 
> Separate enabling makes more sense. And that is already in the patchkit.

You're just ignoring the fact that they are not having it today and it's
about providing the best out of the box protection right now.

Telling people: "We have this shiny new prtcl and your browser will
eventually use it but until then you're on your own." is just bullshit.

Kees certainly knows what he is talking about and being involved in chrome
gives him definitely more weight than your 'eventually don't want' and
'have to be maintained anyways' advisories which have exactly zero
technical content.

The whole thing makes a lot of sense and I've already applied the lot.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [MODERATED] Re: [PATCH SSBv11 0/3] seccomp 1
  2018-05-03 12:45   ` Thomas Gleixner
@ 2018-05-03 14:09     ` Ingo Molnar
  2018-05-03 14:57       ` Andi Kleen
  2018-05-03 17:04       ` Kees Cook
  2018-05-03 14:47     ` Andi Kleen
  1 sibling, 2 replies; 15+ messages in thread
From: Ingo Molnar @ 2018-05-03 14:09 UTC (permalink / raw)
  To: speck


* speck for Thomas Gleixner <speck@linutronix.de> wrote:

> On Thu, 3 May 2018, speck for Andi Kleen wrote:
> > On Wed, May 02, 2018 at 05:44:27PM -0700, speck for Kees Cook wrote:
> > > From: Kees Cook <keescook@chromium.org>
> > > Subject:  opt-in under seccomp
> > > 
> > > As seccomp use overlaps best (though not perfectly) with applications
> > > most likely to want speculation flaw mitigations enabled, seccomp will
> > > enable them when seccomp is enabled for a task. Also adds a line to
> > > /proc/$pid/status for examining the mitigation state of a task.
> > 
> > As I wrote earlier this isn't a good idea. We went through
> > this earlier. 
> > 
> > It originally seems like a good idea until you start looking at the details.
> > 
> > The big users of seccomp like the web browsers eventually
> > don't want it once they use site isolation. And it would
> > unnecessarily slow them down.
> > 
> > And other programs need to be maintained anyways (e.g. for 
> > spectre variant 1 fixes) so they can as well add it explicitely.
> > 
> > Separate enabling makes more sense. And that is already in the patchkit.
> 
> You're just ignoring the fact that they are not having it today and it's
> about providing the best out of the box protection right now.
> 
> Telling people: "We have this shiny new prtcl and your browser will
> eventually use it but until then you're on your own." is just bullshit.
> 
> Kees certainly knows what he is talking about and being involved in chrome
> gives him definitely more weight than your 'eventually don't want' and
> 'have to be maintained anyways' advisories which have exactly zero
> technical content.

The other problem with 'site isolation' is that it doesn't necessarily solve or 
even mitigate the problem: if for example malicious Javascript is injected from an 
ad network, supposedly safely sandboxed, but it can still anomalously read site 
local data via leaky speculation then that's still a dangerous violation of 
sandboxing constraints: it could read pointers to defeat ASLR, it could read local 
keys or other data it's not supposed to read.

Once a browser specifically knows that it has fully mitigated against an attack it 
can turn off any default mitigation early in its init sequence via the prctl, when 
it still has full OS access and no seccomp isolation. All child tasks should 
inherit that.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [MODERATED] Re: [PATCH SSBv11 0/3] seccomp 1
  2018-05-03 12:45   ` Thomas Gleixner
  2018-05-03 14:09     ` [MODERATED] " Ingo Molnar
@ 2018-05-03 14:47     ` Andi Kleen
  1 sibling, 0 replies; 15+ messages in thread
From: Andi Kleen @ 2018-05-03 14:47 UTC (permalink / raw)
  To: speck

> Telling people: "We have this shiny new prtcl and your browser will
> eventually use it but until then you're on your own." is just bullshit.

No the browsers will not need to use the prctl. 

The browser will turn on process site isolation (e.g. in Chrome it is already
a config option) then it doesn't need the prctl because leaking data
inside a process doesn't matter.

And the browsers were actually the main target for this. But if they
don't need it it doesn't make that much sense frankly.

For many other programs it is actually awkward to use because they haven't
been enabled for seccomp yet. For those the plain prctl is better.

So essentially you just slow down the browsers for no gain.

-Andi

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [MODERATED] Re: [PATCH SSBv11 0/3] seccomp 1
  2018-05-03 14:09     ` [MODERATED] " Ingo Molnar
@ 2018-05-03 14:57       ` Andi Kleen
  2018-05-03 17:04       ` Kees Cook
  1 sibling, 0 replies; 15+ messages in thread
From: Andi Kleen @ 2018-05-03 14:57 UTC (permalink / raw)
  To: speck

> The other problem with 'site isolation' is that it doesn't necessarily solve or 
> even mitigate the problem: if for example malicious Javascript is injected from an 
> ad network, supposedly safely sandboxed, but it can still anomalously read site 

If the ad network injects JS on your site it can already read everything
of that site in the JS context. So there's no threat on the JS level data.

But I believe normally ads are running in a different site context anyways,
because they are served from the adservers, not the site's server.

> local data via leaky speculation then that's still a dangerous violation of 
> sandboxing constraints: it could read pointers to defeat ASLR, 

That's true, but then it would still be jailed in the seccomp syscall sandbox.
Also if there's an attack where the pointers help it's likely already
exploitable with standard spraying etc. techniques.

> it could read local keys or other data it's not supposed to read.

Everything sensitive (and especially keys) is supposed to be in other processes.

> Once a browser specifically knows that it has fully mitigated against an attack it 
> can turn off any default mitigation early in its init sequence via the prctl, when 
> it still has full OS access and no seccomp isolation. All child tasks should 
> inherit that.

Ok, so the browser has to then essentially work around that Linux bogosity.
Would be better to not have it in the first place.

-Andi

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [MODERATED] Re: [PATCH SSBv11 3/3] seccomp 0
  2018-05-03  9:21   ` Thomas Gleixner
@ 2018-05-03 16:03     ` Kees Cook
  0 siblings, 0 replies; 15+ messages in thread
From: Kees Cook @ 2018-05-03 16:03 UTC (permalink / raw)
  To: speck

On Thu, May 03, 2018 at 11:21:36AM +0200, speck for Thomas Gleixner wrote:
> On Thu, 3 May 2018, speck for Peter Zijlstra wrote:
> > On Tue, May 01, 2018 at 03:07:31PM -0700, speck for Kees Cook wrote:
> > > @@ -239,6 +254,9 @@ static inline void seccomp_assign_mode(struct task_struct *task,
> > >  	 */
> > >  	smp_mb__before_atomic();
> > >  	set_tsk_thread_flag(task, TIF_SECCOMP);
> > > +
> > > +	/* Assume seccomp processes want speculation flaw mitigation. */
> > > +	spec_mitigate(task, PR_SPEC_STORE_BYPASS);
> > >  }
> > 
> > What about the ordering there? That function appears to explicitly set
> > TIF_SECCOMP last, such that when that is observed, the complete
> > environment is observed.
> > 
> > But now you add something after it. Does this not mean that if you set
> > this on a remote task, this task can execute TIF_SECCOMP thing before it
> > disables SSB.
> > 
> > Is that OK?
> 
> It probably should be the other way round.

I can swap it around. For the seccomp case, it's a thread in the current
thread group, so it's not TOTALLY remote. :) In the case of thread-sync for
seccomp, it is expected to be "correct at next scheduling".

-Kees

-- 
Kees Cook                                            @outflux.net

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [MODERATED] Re: [PATCH SSBv11 0/3] seccomp 1
  2018-05-03 14:09     ` [MODERATED] " Ingo Molnar
  2018-05-03 14:57       ` Andi Kleen
@ 2018-05-03 17:04       ` Kees Cook
  2018-05-03 18:58         ` Andi Kleen
  1 sibling, 1 reply; 15+ messages in thread
From: Kees Cook @ 2018-05-03 17:04 UTC (permalink / raw)
  To: speck

On Thu, May 03, 2018 at 04:09:32PM +0200, speck for Ingo Molnar wrote:
> * speck for Thomas Gleixner <speck@linutronix.de> wrote:
> > On Thu, 3 May 2018, speck for Andi Kleen wrote:
> > > On Wed, May 02, 2018 at 05:44:27PM -0700, speck for Kees Cook wrote:
> > > > From: Kees Cook <keescook@chromium.org>
> > > > Subject:  opt-in under seccomp
> > > > 
> > > > As seccomp use overlaps best (though not perfectly) with applications
> > > > most likely to want speculation flaw mitigations enabled, seccomp will
> > > > enable them when seccomp is enabled for a task. Also adds a line to
> > > > /proc/$pid/status for examining the mitigation state of a task.
> > > 
> > > As I wrote earlier this isn't a good idea. We went through
> > > this earlier. 
> > > 
> > > It originally seems like a good idea until you start looking at the details.
> > > 
> > > The big users of seccomp like the web browsers eventually
> > > don't want it once they use site isolation. And it would
> > > unnecessarily slow them down.
> > > 
> > > And other programs need to be maintained anyways (e.g. for 
> > > spectre variant 1 fixes) so they can as well add it explicitely.
> > > 
> > > Separate enabling makes more sense. And that is already in the patchkit.
> > 
> > You're just ignoring the fact that they are not having it today and it's
> > about providing the best out of the box protection right now.
> > 
> > Telling people: "We have this shiny new prtcl and your browser will
> > eventually use it but until then you're on your own." is just bullshit.
> > 
> > Kees certainly knows what he is talking about and being involved in chrome
> > gives him definitely more weight than your 'eventually don't want' and
> > 'have to be maintained anyways' advisories which have exactly zero
> > technical content.
> 
> The other problem with 'site isolation' is that it doesn't necessarily solve or 
> even mitigate the problem: if for example malicious Javascript is injected from an 
> ad network, supposedly safely sandboxed, but it can still anomalously read site 
> local data via leaky speculation then that's still a dangerous violation of 
> sandboxing constraints: it could read pointers to defeat ASLR, it could read local 
> keys or other data it's not supposed to read.
> 
> Once a browser specifically knows that it has fully mitigated against an attack it 
> can turn off any default mitigation early in its init sequence via the prctl, when 
> it still has full OS access and no seccomp isolation. All child tasks should 
> inherit that.

My goal is providing SOME kind of "by default" coverage that doesn't
require an admin to write new code or wait for a software vendor to provide
an update.

If it is tolerable to wait for a vendor to _enable_ the mitigation, then
it should be tolerable to wait for a vendor to _disable_ the mitigation
as well. i.e. we protect a targeted subset of processes (those using
seccomp) but provide a way to disable it later. To that end, how about
a new prctl or seccomp flag that indicates "do no enable speculation
mitigations under seccomp"? That would give any seccomp users the ability
to opt-out if they wanted to.

-Kees

-- 
Kees Cook                                            @outflux.net

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [MODERATED] Re: [PATCH SSBv11 0/3] seccomp 1
  2018-05-03 17:04       ` Kees Cook
@ 2018-05-03 18:58         ` Andi Kleen
  2018-05-03 23:17           ` Kees Cook
  0 siblings, 1 reply; 15+ messages in thread
From: Andi Kleen @ 2018-05-03 18:58 UTC (permalink / raw)
  To: speck

> My goal is providing SOME kind of "by default" coverage that doesn't
> require an admin to write new code or wait for a software vendor to provide
> an update.

What's your target for this? Is it the web browser or something else?

> 
> If it is tolerable to wait for a vendor to _enable_ the mitigation, then
> it should be tolerable to wait for a vendor to _disable_ the mitigation
> as well. i.e. we protect a targeted subset of processes (those using
> seccomp) but provide a way to disable it later. To that end, how about
> a new prctl or seccomp flag that indicates "do no enable speculation
> mitigations under seccomp"? That would give any seccomp users the ability
> to opt-out if they wanted to.

Yes if you add it to seccomp that would be needed.

It should already work with the existing prctl?  Need to test that.  

The problem with the prctl of course is that it would override external
user choice. But perhaps that is ok.

-Andi

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [MODERATED] Re: [PATCH SSBv11 0/3] seccomp 1
  2018-05-03 18:58         ` Andi Kleen
@ 2018-05-03 23:17           ` Kees Cook
  0 siblings, 0 replies; 15+ messages in thread
From: Kees Cook @ 2018-05-03 23:17 UTC (permalink / raw)
  To: speck

On Thu, May 03, 2018 at 11:58:14AM -0700, speck for Andi Kleen wrote:
> > My goal is providing SOME kind of "by default" coverage that doesn't
> > require an admin to write new code or wait for a software vendor to provide
> > an update.
> 
> What's your target for this? Is it the web browser or something else?

My target is "something unexpected" as we can't know what all the
containers in the world are actually containing. (And, while SpectreV1
is still an issue, it'd be nice to have coverage against SSB for browsers
that are slow with Site Isolation.)

> > If it is tolerable to wait for a vendor to _enable_ the mitigation, then
> > it should be tolerable to wait for a vendor to _disable_ the mitigation
> > as well. i.e. we protect a targeted subset of processes (those using
> > seccomp) but provide a way to disable it later. To that end, how about
> > a new prctl or seccomp flag that indicates "do no enable speculation
> > mitigations under seccomp"? That would give any seccomp users the ability
> > to opt-out if they wanted to.
> 
> Yes if you add it to seccomp that would be needed.
> 
> It should already work with the existing prctl?  Need to test that.  
> 
> The problem with the prctl of course is that it would override external
> user choice. But perhaps that is ok.

I think tglx and I cooked up a workable solution. seccomp users will
now be able to individually opt out of the automatic mitigation (by
adding the new SECCOMP_FILTER_FLAG_SPEC_ALLOW flag), and sysadmins will
be able to globally opt out of the seccomp behavior by booting with
"spec_store_bypass_disable=prctl". (The default is "seccomp" which is
prctl plus seccomp.)

If it turns out immediately that seccomp coverage was a terrible idea,
we can switch the default back to "prctl". And maybe in a few years
once we're happy with the state of software vulnerable to SSB, we can
do the same.

-Kees

-- 
Kees Cook                                            @outflux.net

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2018-05-03 23:17 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-03  0:44 [MODERATED] [PATCH SSBv11 0/3] seccomp 1 Kees Cook
2018-05-01 22:07 ` [MODERATED] [PATCH SSBv11 3/3] seccomp 0 Kees Cook
2018-05-01 22:19 ` [MODERATED] [PATCH SSBv11 1/3] seccomp 2 Kees Cook
2018-05-01 22:31 ` [MODERATED] [PATCH SSBv11 2/3] seccomp 3 Kees Cook
2018-05-03  8:58 ` [MODERATED] Re: [PATCH SSBv11 3/3] seccomp 0 Peter Zijlstra
2018-05-03  9:21   ` Thomas Gleixner
2018-05-03 16:03     ` [MODERATED] " Kees Cook
2018-05-03 12:29 ` [MODERATED] Re: [PATCH SSBv11 0/3] seccomp 1 Andi Kleen
2018-05-03 12:45   ` Thomas Gleixner
2018-05-03 14:09     ` [MODERATED] " Ingo Molnar
2018-05-03 14:57       ` Andi Kleen
2018-05-03 17:04       ` Kees Cook
2018-05-03 18:58         ` Andi Kleen
2018-05-03 23:17           ` Kees Cook
2018-05-03 14:47     ` Andi Kleen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.