[PATCH] x86/speculation: Allow overriding seccomp speculation disable

[PATCH] x86/speculation: Allow overriding seccomp speculation disable

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

seccomp currently force enables the SSBD and IB mitigations,
which disable certain features in the CPU to avoid speculation
attacks at a performance penalty.

This is a heuristic to detect applications that may run untrusted code
(such as web browsers) and provide mitigation for them.

At least for SSBD the mitigation is really only for side channel
leaks inside processes.

There are two cases when the heuristic has problems:

- The seccomp user has a superior mitigation and doesn't need the
CPU level disables. For example for a Web Browser this is using
site isolation, which separates different sites in different
processes, so side channel leaks inside a process are not
of a concern.

- Another case are seccomp users who don't run untrusted code,
such as sshd, and don't really benefit from SSBD

As currently implemented seccomp force enables the mitigation
so it's not possible for processes to opt-in that they don't
need mitigations (such as when they already use site isolation).

In some cases we're seeing significant performance penalties
of enabling the SSBD mitigation on web workloads.

This patch changes the seccomp code to not force enable,
but merely enable, the SSBD and IB mitigations.

This allows processes to use the PR_SET_SPECULATION prctl
after running seccomp and reenable SSBD and/or IB
if they don't need any extra mitigation.

The effective default has not changed, it just allows
processes to opt-out of the default.

It's not clear to me what the use case for the force
disable is anyways. Certainly if someone controls the process,
and can run prctl(), they can leak data in all kinds of
ways anyways, or just read the whole memory map.

Longer term we probably need to discuss if the seccomp heuristic
is still warranted and should be perhaps changed. It seemed
like a good idea when these vulnerabilities were new, and
no web browsers supported site isolation. But with site isolation
widely deployed -- Chrome has it on by default, and as I understand
it, Firefox is going to enable it by default soon. And other seccomp
users (like sshd or systemd) probably don't really need it.
Given that it's not clear the default heuristic is still a good
idea.

But anyways this patch doesn't change any defaults, just
let's applications override it.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/cpu/bugs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index ed54b3b21c39..f15ae9bfd7ad 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -1215,9 +1215,9 @@ int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long which,
 void arch_seccomp_spec_mitigate(struct task_struct *task)
 {
 	if (ssb_mode == SPEC_STORE_BYPASS_SECCOMP)
-		ssb_prctl_set(task, PR_SPEC_FORCE_DISABLE);
+		ssb_prctl_set(task, PR_SPEC_DISABLE);
 	if (spectre_v2_user == SPECTRE_V2_USER_SECCOMP)
-		ib_prctl_set(task, PR_SPEC_FORCE_DISABLE);
+		ib_prctl_set(task, PR_SPEC_DISABLE);
 }
 #endif
 
-- 
2.24.1

Re: [PATCH] x86/speculation: Allow overriding seccomp speculation disable

[Thomas Gleixner]

Andi Kleen <andi@firstfloor.org> writes:

Cc+: Seccomp maintainers ....

> From: Andi Kleen <ak@linux.intel.com>
>
> seccomp currently force enables the SSBD and IB mitigations,
> which disable certain features in the CPU to avoid speculation
> attacks at a performance penalty.
>
> This is a heuristic to detect applications that may run untrusted code
> (such as web browsers) and provide mitigation for them.
>
> At least for SSBD the mitigation is really only for side channel
> leaks inside processes.
>
> There are two cases when the heuristic has problems:
>
> - The seccomp user has a superior mitigation and doesn't need the
> CPU level disables. For example for a Web Browser this is using
> site isolation, which separates different sites in different
> processes, so side channel leaks inside a process are not
> of a concern.
>
> - Another case are seccomp users who don't run untrusted code,
> such as sshd, and don't really benefit from SSBD
>
> As currently implemented seccomp force enables the mitigation
> so it's not possible for processes to opt-in that they don't
> need mitigations (such as when they already use site isolation).
>
> In some cases we're seeing significant performance penalties
> of enabling the SSBD mitigation on web workloads.
>
> This patch changes the seccomp code to not force enable,

I'm sure I asked you to do

git grep "This patch" Documentation/process/

before.

> but merely enable, the SSBD and IB mitigations.
>
> This allows processes to use the PR_SET_SPECULATION prctl
> after running seccomp and reenable SSBD and/or IB
> if they don't need any extra mitigation.
>
> The effective default has not changed, it just allows
> processes to opt-out of the default.
>
> It's not clear to me what the use case for the force
> disable is anyways. Certainly if someone controls the process,
> and can run prctl(), they can leak data in all kinds of
> ways anyways, or just read the whole memory map.
>
> Longer term we probably need to discuss if the seccomp heuristic
> is still warranted and should be perhaps changed. It seemed
> like a good idea when these vulnerabilities were new, and
> no web browsers supported site isolation. But with site isolation
> widely deployed -- Chrome has it on by default, and as I understand
> it, Firefox is going to enable it by default soon. And other seccomp
> users (like sshd or systemd) probably don't really need it.
> Given that it's not clear the default heuristic is still a good
> idea.
>
> But anyways this patch doesn't change any defaults, just
> let's applications override it.

It changes the enforcement and I really want the seccomp people to have
a say here.

Thanks,

        tglx

> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/bugs.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index ed54b3b21c39..f15ae9bfd7ad 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -1215,9 +1215,9 @@ int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long which,
>  void arch_seccomp_spec_mitigate(struct task_struct *task)
>  {
>  	if (ssb_mode == SPEC_STORE_BYPASS_SECCOMP)
> -		ssb_prctl_set(task, PR_SPEC_FORCE_DISABLE);
> +		ssb_prctl_set(task, PR_SPEC_DISABLE);
>  	if (spectre_v2_user == SPECTRE_V2_USER_SECCOMP)
> -		ib_prctl_set(task, PR_SPEC_FORCE_DISABLE);
> +		ib_prctl_set(task, PR_SPEC_DISABLE);
>  }
>  #endif
>  
> -- 
> 2.24.1

Re: [PATCH] x86/speculation: Allow overriding seccomp speculation disable

[Kees Cook]

On Sat, Mar 21, 2020 at 03:46:29PM +0100, Thomas Gleixner wrote:
> Cc+: Seccomp maintainers ....

Thanks!

> Andi Kleen <andi@firstfloor.org> writes:
> > [...]
> >
> > Longer term we probably need to discuss if the seccomp heuristic
> > is still warranted and should be perhaps changed. It seemed
> > like a good idea when these vulnerabilities were new, and
> > no web browsers supported site isolation. But with site isolation
> > widely deployed -- Chrome has it on by default, and as I understand
> > it, Firefox is going to enable it by default soon. And other seccomp
> > users (like sshd or systemd) probably don't really need it.
> > Given that it's not clear the default heuristic is still a good
> > idea.
> >
> > But anyways this patch doesn't change any defaults, just
> > let's applications override it.
> 
> It changes the enforcement and I really want the seccomp people to have
> a say here.

None of this commit makes sense to me. :)

The point of the defaults was to grandfather older seccomp users into
speculation mitigations. Newly built seccomp users can choose to disable
this with SECCOMP_FILTER_FLAG_SPEC_ALLOW when applying seccomp filters.
The rationale was that once a process knows how to manage its exposure,
it can choose to leave off the automatic enabling. I don't see any
mention of that method in the commit log, so if there is some reason
it's not workable, that would need to be discussed first.

And the force disable matches the design goals of seccomp: no applied
restrictions can be later relaxed for a process. I'm more in favor of
changing the behavior of SPEC_STORE_BYPASS_CMD_AUTO, but probably not for
another 3 years at least. (To get us to at least 5 years since Meltdown,
which is relatively close to various longer LTS cycles.)

-Kees

-- 
Kees Cook

Re: [PATCH] x86/speculation: Allow overriding seccomp speculation disable

[Andy Lutomirski]

On Sat, Mar 21, 2020 at 7:29 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Sat, Mar 21, 2020 at 03:46:29PM +0100, Thomas Gleixner wrote:
> > Cc+: Seccomp maintainers ....
>
> Thanks!
>
> > Andi Kleen <andi@firstfloor.org> writes:
> > > [...]
> > >
> > > Longer term we probably need to discuss if the seccomp heuristic
> > > is still warranted and should be perhaps changed. It seemed
> > > like a good idea when these vulnerabilities were new, and
> > > no web browsers supported site isolation. But with site isolation
> > > widely deployed -- Chrome has it on by default, and as I understand
> > > it, Firefox is going to enable it by default soon. And other seccomp
> > > users (like sshd or systemd) probably don't really need it.
> > > Given that it's not clear the default heuristic is still a good
> > > idea.
> > >
> > > But anyways this patch doesn't change any defaults, just
> > > let's applications override it.
> >
> > It changes the enforcement and I really want the seccomp people to have
> > a say here.
>
> None of this commit makes sense to me. :)
>
> The point of the defaults was to grandfather older seccomp users into
> speculation mitigations. Newly built seccomp users can choose to disable
> this with SECCOMP_FILTER_FLAG_SPEC_ALLOW when applying seccomp filters.
> The rationale was that once a process knows how to manage its exposure,
> it can choose to leave off the automatic enabling. I don't see any
> mention of that method in the commit log, so if there is some reason
> it's not workable, that would need to be discussed first.

Agreed.


>
> And the force disable matches the design goals of seccomp: no applied
> restrictions can be later relaxed for a process. I'm more in favor of
> changing the behavior of SPEC_STORE_BYPASS_CMD_AUTO, but probably not for
> another 3 years at least. (To get us to at least 5 years since Meltdown,
> which is relatively close to various longer LTS cycles.)
>
> -Kees
>
> --
> Kees Cook

Re: [PATCH] x86/speculation: Allow overriding seccomp speculation disable

[Andi Kleen]

> The point of the defaults was to grandfather older seccomp users into
> speculation mitigations. Newly built seccomp users can choose to disable
> this with SECCOMP_FILTER_FLAG_SPEC_ALLOW when applying seccomp filters.
> The rationale was that once a process knows how to manage its exposure,
> it can choose to leave off the automatic enabling. I don't see any
> mention of that method in the commit log, so if there is some reason
> it's not workable, that would need to be discussed first.

SECCOMP_FILTER_FLAG_SPEC_ALLOW doesn't completely solve the problem because
it enables everything, including cross process defenses, like Spectre.

The motivation of my patch was to only allow to disable SSBD, which
is only relevant for in process attacks, which are completely
mitigated by process isolation, which is now widely deployed.

Completely mitigating Spectre (which could be cross process) is harder,
and it doesn't have have as bad a performance impact as SSBD.

> 
> And the force disable matches the design goals of seccomp: no applied
> restrictions can be later relaxed for a process. I'm more in favor of

It seems to me this design goal is not useful for 
speculation defenses. If an attacker can call prctl they don't
need speculation attacks anymore, they can just read the memory
directly.

> changing the behavior of SPEC_STORE_BYPASS_CMD_AUTO, but probably not for
> another 3 years at least. (To get us to at least 5 years since Meltdown,
> which is relatively close to various longer LTS cycles.)

The seccomp defenses are mainly for web browsers, whose life cycles have
nothing to do with LTS cycles. Web browsers are updated much faster
because of their bazillion other security holes. If someone doesn't
update their web browser they are completely open to other attacks anyways.
So we can assume they do, and the browsers usually enforce it in 
some way.

I'm not aware of anything else that is not a browser that would rely on the
seccomp heuristic. Are you?

Anyways back to the opt-in:

Anyways one way to keep your design goals would be to split the SECCOMP
flags into flags for SSBD and SPECTRE. Then at least the web browser
could reenable it

-Andi

Re: [PATCH] x86/speculation: Allow overriding seccomp speculation disable

[Kees Cook]

On Thu, Mar 26, 2020 at 07:10:47AM -0700, Andi Kleen wrote:
> SECCOMP_FILTER_FLAG_SPEC_ALLOW doesn't completely solve the problem because
> it enables everything, including cross process defenses, like Spectre.

Fair point. It is a much bigger hammer that I was considering.

> I'm not aware of anything else that is not a browser that would rely on the
> seccomp heuristic. Are you?

My memory was that a bunch of container folks were glad to have it for
their workloads. But I'd agree, between browsers and containers, the
lifetime is a bit shorter. (Though what about browsers in cars, hmpf.)

> Anyways back to the opt-in:
> 
> Anyways one way to keep your design goals would be to split the SECCOMP
> flags into flags for SSBD and SPECTRE. Then at least the web browser
> could reenable it

How about relaxing the SSBD side of the "AUTO" setting? I've run out of
time today to go look and see if that's even possible, or if it's bolted
together like SECCOMP_FILTER_FLAG_SPEC_ALLOW is...

-- 
Kees Cook