* [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] 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 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 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 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 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
* [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