linux-kernel - Re: [patch 20/24] x86/speculation: Split out TIF update

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:43:22 +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 20/24] x86/speculation: Split out TIF update


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

> The update of the TIF_SSBD flag and the conditional speculation control MSR
> update is done in the ssb_prctl_set() function directly. The upcoming prctl
> support for controlling indirect branch speculation via STIBP needs the
> same mechanism.
> 
> Split the code out and make it reusable.
> 
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
>  arch/x86/kernel/cpu/bugs.c |   31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -703,10 +703,25 @@ static void ssb_select_mitigation(void)
>  #undef pr_fmt
>  #define pr_fmt(fmt)     "Speculation prctl: " fmt
>  
> -static int ssb_prctl_set(struct task_struct *task, unsigned long ctrl)
> +static void task_update_spec_tif(struct task_struct *tsk, int tifbit, bool on)
>  {
>  	bool update;
>  
> +	if (on)
> +		update = !test_and_set_tsk_thread_flag(tsk, tifbit);
> +	else
> +		update = test_and_clear_tsk_thread_flag(tsk, tifbit);
> +
> +	/*
> +	 * If being set on non-current task, delay setting the CPU
> +	 * mitigation until it is scheduled next.
> +	 */
> +	if (tsk == current && update)
> +		speculation_ctrl_update_current();

Had to read this twice, because the comment and the code are both correct 
but deal with the inverse case. This might have helped:

	/*
	 * Immediately update the speculation MSRs on the current task,
	 * but for non-current tasks delay setting the CPU mitigation 
	 * until it is scheduled next.
	 */
	if (tsk == current && update)
		speculation_ctrl_update_current();

But can the target task ever be non-current here? I don't think so: the 
two callers are prctl and seccomp, and both are passing in the current 
task pointer.

If so then it would be nice to rename all these task variable names from 
'task' to 'curr' or such, to make this property apparent.

Then we can also remove the condition and the comment, and update 
unconditionally, and maybe add:

	WARN_ON_ONCE(curr != current);

... or so?

Thanks,

	Ingo