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:   Tue, 27 Nov 2018 23:36:32 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Jiri Kosina <jikos@...nel.org>
cc:     Tim Chen <tim.c.chen@...ux.intel.com>,
        Ingo Molnar <mingo@...nel.org>,
        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>,
        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

On Tue, 27 Nov 2018, Jiri Kosina wrote:

> On Tue, 27 Nov 2018, Thomas Gleixner wrote:
> 
> > >  static int ssb_prctl_set(struct task_struct *task, unsigned long ctrl)
> > > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> > > index 3f5e351bdd37..6c4fcef52b19 100644
> > > --- a/arch/x86/kernel/process.c
> > > +++ b/arch/x86/kernel/process.c
> > > @@ -474,6 +474,21 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p)
> > >  
> > >  	tifn = READ_ONCE(task_thread_info(next_p)->flags);
> > >  	tifp = READ_ONCE(task_thread_info(prev_p)->flags);
> > > +
> > > +	/*
> > > +	 * SECCOMP tasks might have had their spec_ctrl flags updated during
> > > +	 * runtime from a different CPU.
> > > +	 *
> > > +	 * When switching to such a task, populate thread flags with the ones
> > > +	 * that have been temporarily saved in spec_flags by task_update_spec_tif()
> > > +	 * in order to make sure MSR value is always kept up to date.
> > > +	 *
> > > +	 * SECCOMP tasks never disable the mitigation for other threads, only enable.
> > > +	 */
> > > +	if (IS_ENABLED(CONFIG_SECCOMP) &&
> > > +			test_and_clear_tsk_thread_flag(next_p, TIF_SPEC_UPDATE))
> > > +		tifp |= READ_ONCE(task_thread_info(next_p)->spec_flags);
> > 
> > And how does that get folded into task_thread_info(next_p)->flags for the
> > next context switch? 
> 
> Does it really have to? 
> 
> We need this special handling only if the next task has TIF_SPEC_UPDATE 
> set, which is one-off event globally (when seccomp marks all its threads 
> so due to seccomp filter change), and once all the TIF_SPEC_UPDATE tasks 
> schedule at least once, we're in a consistent state again and don't need 
> this, as every running task will then have its TIF consistent with MSR 
> value.

And how so? You set the bits is spec_flags. And then you set the TIF_UPDATE
bit which is evaluated once.

Then you OR the bits into tifp which is a local variable and has nothing to
do with the TIF flags of the next task. So on the next context switch this
will evaluate the previous state of the TIF bits and you could have spared
the whole exercise :)

Thanks,

	tglx