linux-kernel - Re: [Patch v4 17/18] x86/speculation: Update SPEC

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: <alpine.DEB.2.21.1811042042060.10744@nanos.tec.linutronix.de>
Date:   Sun, 4 Nov 2018 20:49:20 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Tim Chen <tim.c.chen@...ux.intel.com>
cc:     Jiri Kosina <jikos@...nel.org>,
        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>,
        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>,
        linux-kernel@...r.kernel.org, x86@...nel.org
Subject: Re: [Patch v4 17/18] x86/speculation: Update SPEC_CTRL MSRs of remote
 CPUs

Tim,

On Tue, 30 Oct 2018, Tim Chen wrote:
>  void arch_set_security(struct task_struct *tsk, unsigned int value)
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 943e90d..048b7f4b 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -426,7 +426,19 @@ static __always_inline void spec_ctrl_update_msr(unsigned long tifn)
>  static __always_inline void __speculation_ctrl_update(unsigned long tifp,
>  						      unsigned long tifn)
>  {
> -	bool updmsr = !!((tifp ^ tifn) & _TIF_STIBP);
> +	/*
> +	 * If TIF_UPDATE_SPEC_CTRL bit is set in tifp, speculation related
> +	 * TIF flags have changed when previous task was running, but
> +	 * SPEC_CTRL MSR has not been synchronized with TIF flag changes.
> +	 * SPEC_CTRL MSR value can be out of date.
> +	 *
> +	 * Need to force update SPEC_CTRL MSR if TIF_UPDATE_SPEC_CTRL
> +	 * bit in tifp is set.
> +	 *
> +	 * The TIF_UPDATE_SPEC_CTRL bit in tifn was cleared before calling
> +	 * this function.
> +	 */
> +	bool updmsr = !!((tifp ^ tifn) & (_TIF_STIBP|_TIF_UPDATE_SPEC_CTRL));
>  
>  	/* If TIF_SSBD is different, select the proper mitigation method */
>  	if ((tifp ^ tifn) & _TIF_SSBD) {
> @@ -482,6 +494,14 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
>  	if ((tifp ^ tifn) & _TIF_NOCPUID)
>  		set_cpuid_faulting(!!(tifn & _TIF_NOCPUID));
>  
> +	if (tifp & _TIF_UPDATE_SPEC_CTRL)
> +		clear_tsk_thread_flag(prev_p, TIF_UPDATE_SPEC_CTRL);
> +
> +	if (tifn & _TIF_UPDATE_SPEC_CTRL) {
> +		clear_tsk_thread_flag(next_p, TIF_UPDATE_SPEC_CTRL);
> +		tifn &= ~_TIF_UPDATE_SPEC_CTRL;
> +	}

I'm really unhappy about adding yet more conditionals into this code
path. We really need to find some better solution for that.

There are basically two options:

 1) Restrict the PRCTL control so it is only possible to modify it at the
    point where the application is still single threaded.

 2) Add _TIF_UPDATE_SPEC_CTRL to the SYSCALL_EXIT_WORK_FLAGS and handle it
    in the slow work path.

    The KVM side can be handled in x86_virt_spec_ctrl().

Thanks,

	tglx