linux-kernel - RE: [Patch v4 16/18] x86/speculation: Enable STIBP to protect security sensitive tasks

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]

Message-ID: <99FC4B6EFCEFD44486C35F4C281DC6732148DC8C@ORSMSX110.amr.corp.intel.com>
Date:   Tue, 30 Oct 2018 21:07:28 +0000
From:   "Schaufler, Casey" <casey.schaufler@...el.com>
To:     Tim Chen <tim.c.chen@...ux.intel.com>,
        Jiri Kosina <jikos@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>
CC:     Tom Lendacky <thomas.lendacky@....com>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        David Woodhouse <dwmw@...zon.co.uk>,
        Andi Kleen <ak@...ux.intel.com>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "Mallick, Asit K" <asit.k.mallick@...el.com>,
        Arjan van de Ven <arjan@...ux.intel.com>,
        Jon Masters <jcm@...hat.com>,
        Waiman Long <longman9394@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>
Subject: RE: [Patch v4 16/18] x86/speculation: Enable STIBP to protect
 security sensitive tasks

> -----Original Message-----
> From: Tim Chen [mailto:tim.c.chen@...ux.intel.com]
> Sent: Tuesday, October 30, 2018 11:49 AM
> To: Jiri Kosina <jikos@...nel.org>; Thomas Gleixner <tglx@...utronix.de>
> Cc: Tim Chen <tim.c.chen@...ux.intel.com>; Tom Lendacky
> <thomas.lendacky@....com>; Ingo Molnar <mingo@...hat.com>; Peter
> Zijlstra <peterz@...radead.org>; Josh Poimboeuf <jpoimboe@...hat.com>;
> Andrea Arcangeli <aarcange@...hat.com>; David Woodhouse
> <dwmw@...zon.co.uk>; Andi Kleen <ak@...ux.intel.com>; Hansen, Dave
> <dave.hansen@...el.com>; Schaufler, Casey <casey.schaufler@...el.com>;
> Mallick, Asit K <asit.k.mallick@...el.com>; Arjan van de Ven
> <arjan@...ux.intel.com>; Jon Masters <jcm@...hat.com>; Waiman Long
> <longman9394@...il.com>; linux-kernel@...r.kernel.org; x86@...nel.org
> Subject: [Patch v4 16/18] x86/speculation: Enable STIBP to protect security
> sensitive tasks
> 
> Enable STIBP defense on high security tasks.
> 
> For normal tasks, STIBP is unused so they are not impacted by overhead
> from STIBP in lite protection mode.
> 
> Signed-off-by: Tim Chen <tim.c.chen@...ux.intel.com>
> ---
>  arch/x86/kernel/cpu/bugs.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 54f4675..b402b96 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -14,6 +14,8 @@
>  #include <linux/module.h>
>  #include <linux/nospec.h>
>  #include <linux/prctl.h>
> +#include <linux/sched/coredump.h>
> +#include <linux/security.h>
> 
>  #include <asm/spec-ctrl.h>
>  #include <asm/cmdline.h>
> @@ -770,6 +772,37 @@ static int ssb_prctl_set(struct task_struct *task,
> unsigned long ctrl)
>  	return 0;
>  }
> 
> +static void set_task_stibp(struct task_struct *tsk, bool stibp_on)
> +{
> +	bool update = false;
> +
> +	if (!static_branch_unlikely(&spectre_v2_app_lite))
> +		return;
> +
> +	if (stibp_on)
> +		update = !test_and_set_tsk_thread_flag(tsk, TIF_STIBP);
> +	else
> +		update = test_and_clear_tsk_thread_flag(tsk, TIF_STIBP);
> +
> +	if (!update)
> +		return;
> +
> +	if (tsk == current)
> +		speculation_ctrl_update_current();
> +}
> +
> +void arch_set_security(struct task_struct *tsk, unsigned int value)

In this context "security" isn't descriptive. arch_set_stibp_defenses()
would be better.

Since "value" should only ever have one of two values, and those
map directly to "true" or "false" this should be a bool, making the
code trivial:

void arch_set_stibp_defenses(struct task_struct *task, bool stibp)
{
	set_task_stibp(task, stibp);
}

Or perhaps arch_set_security() should go away, and the calling
code would call set_task_stibp() directly. Unless there is some compelling
reason for the abstractions.

> +{
> +	if (value > SECURITY_HIGH)
> +		return;
> +
> +	/* Update STIBP defenses */
> +	if (value == SECURITY_HIGH)
> +		set_task_stibp(tsk, true);
> +	else
> +		set_task_stibp(tsk, false);
> +}
> +
>  int arch_prctl_spec_ctrl_set(struct task_struct *task, unsigned long which,
>  			     unsigned long ctrl)
>  {
> --
> 2.9.4