linux-kernel - Re: [patch 24/24] x86/speculation: Add seccomp Spectre v2 app to app protection mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 22 Nov 2018 08:26:19 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Andy Lutomirski <luto@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Jiri Kosina <jkosina@...e.cz>,
        Tom Lendacky <thomas.lendacky@....com>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        David Woodhouse <dwmw@...zon.co.uk>,
        Andi Kleen <ak@...ux.intel.com>,
        Dave Hansen <dave.hansen@...el.com>,
        Casey Schaufler <casey.schaufler@...el.com>,
        Asit Mallick <asit.k.mallick@...el.com>,
        Arjan van de Ven <arjan@...ux.intel.com>,
        Jon Masters <jcm@...hat.com>,
        Waiman Long <longman9394@...il.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Dave Stewart <david.c.stewart@...el.com>,
        Kees Cook <keescook@...omium.org>
Subject: Re: [patch 24/24] x86/speculation: Add seccomp Spectre v2 app to app
 protection mode


* Thomas Gleixner <tglx@...utronix.de> wrote:

> From: Jiri Kosina <jkosina@...e.cz>
> 
> If 'prctl' mode of app2app protection from spectre v2 is selected on the
> kernel command-line, STIBP and IBPB are applied on tasks which restrict
> their indirect branch speculation via prctl.
> 
> SECCOMP enables the SSBD mitigation for sandboxed tasks already, so it
> makes sense to prevent spectre v2 application to application attacks as
> well.
> 
> The mitigation guide documents how STIPB works:
>     
>    Setting bit 1 (STIBP) of the IA32_SPEC_CTRL MSR on a logical processor
>    prevents the predicted targets of indirect branches on any logical
>    processor of that core from being controlled by software that executes
>    (or executed previously) on another logical processor of the same core.
>     
> Ergo setting STIBP protects the task itself from being attacked from a task
> running on a different hyper-thread and protects the tasks running on
> different hyper-threads from being attacked.
>     
> IBPB is issued when the task switches out, so malicious sandbox code cannot
> mistrain the branch predictor for the next user space task on the same
> logical processor.
> 
> Signed-off-by: Jiri Kosina <jkosina@...e.cz>
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> 
> ---
>  Documentation/admin-guide/kernel-parameters.txt |    7 +++++-
>  arch/x86/include/asm/nospec-branch.h            |    1 
>  arch/x86/kernel/cpu/bugs.c                      |   27 +++++++++++++++++++-----
>  3 files changed, 29 insertions(+), 6 deletions(-)
> 
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4228,10 +4228,15 @@
>  				  by spectre_v2=off
>  			auto    - Kernel selects the mitigation depending on
>  				  the available CPU features and vulnerability.
> -				  Default is prctl.
>  			prctl   - Indirect branch speculation is enabled, but
>  				  mitigation can be enabled via prctl per thread.
>  				  The mitigation control state is inherited on fork.
> +			seccomp - Same as "prctl" above, but all seccomp threads
> +				  will enable the mitigation unless they explicitly
> +				  opt out.
> +
> +			Default mitigation:
> +			If CONFIG_SECCOMP=y "seccomp", otherwise "prctl"
>  
>  			Not specifying this option is equivalent to
>  			spectre_v2_app2app=auto.
> --- a/arch/x86/include/asm/nospec-branch.h
> +++ b/arch/x86/include/asm/nospec-branch.h
> @@ -233,6 +233,7 @@ enum spectre_v2_app2app_mitigation {
>  	SPECTRE_V2_APP2APP_NONE,
>  	SPECTRE_V2_APP2APP_STRICT,
>  	SPECTRE_V2_APP2APP_PRCTL,
> +	SPECTRE_V2_APP2APP_SECCOMP,
>  };
>  
>  /* The Speculative Store Bypass disable variants */
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -256,12 +256,14 @@ enum spectre_v2_app2app_cmd {
>  	SPECTRE_V2_APP2APP_CMD_AUTO,
>  	SPECTRE_V2_APP2APP_CMD_FORCE,
>  	SPECTRE_V2_APP2APP_CMD_PRCTL,
> +	SPECTRE_V2_APP2APP_CMD_SECCOMP,
>  };
>  
>  static const char *spectre_v2_app2app_strings[] = {
>  	[SPECTRE_V2_APP2APP_NONE]	= "App-App Vulnerable",
> -	[SPECTRE_V2_APP2APP_STRICT]	= "App-App Mitigation: STIBP protection",
> -	[SPECTRE_V2_APP2APP_PRCTL]	= "App-App Mitigation: STIBP via prctl",
> +	[SPECTRE_V2_APP2APP_STRICT]	= "App-App Mitigation: forced protection",
> +	[SPECTRE_V2_APP2APP_PRCTL]	= "App-App Mitigation: prctl opt-in",
> +	[SPECTRE_V2_APP2APP_SECCOMP]	= "App-App Mitigation: seccomp and prctl opt-in",

This description is not accurate: it's not a 'seccomp and prctl opt-in', 
the seccomp functionality is opt-out, the prctl is opt-in.

So something like:

> +	[SPECTRE_V2_APP2APP_SECCOMP]	= "App-App Mitigation: seccomp by default and prctl opt-in",

or so?

>  void arch_seccomp_spec_mitigate(struct task_struct *task)
>  {
>  	if (ssb_mode == SPEC_STORE_BYPASS_SECCOMP)
>  		ssb_prctl_set(task, PR_SPEC_FORCE_DISABLE);
> +	if (spectre_v2_app2app == SPECTRE_V2_APP2APP_SECCOMP)
> +		indir_branch_prctl_set(task, PR_SPEC_FORCE_DISABLE);
>  }
>  #endif

Hm, so isn't arch_seccomp_spec_mitigate() called right before untrusted 
seccomp code is executed? So why are we disabling the mitigation here?

> +	case SPECTRE_V2_APP2APP_SECCOMP:
> +		return ", STIBP: seccomp and prctl opt-in";
> +	case SPECTRE_V2_APP2APP_SECCOMP:
> +		return ", IBPB: seccomp and prctl opt-in";

Same feedback wrt. potentially confusing use of 'opt-in' here, while 
seccomp is more like an opt-out mechanism.

Thanks,

	Ingo