linux-kernel - Re: [PATCH 4/7] x86/idle: Disable IBRS entering idle and enable it on wakeup

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: <0ebb1848-c3ab-f636-f6f9-67716e814915@linux.intel.com>
Date:   Thu, 4 Jan 2018 15:22:09 -0800
From:   Tim Chen <tim.c.chen@...ux.intel.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Andy Lutomirski <luto@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Dave Hansen <dave.hansen@...el.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Arjan Van De Ven <arjan.van.de.ven@...el.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/7] x86/idle: Disable IBRS entering idle and enable it on
 wakeup

On 01/04/2018 02:47 PM, Peter Zijlstra wrote:
> On Thu, Jan 04, 2018 at 09:56:45AM -0800, Tim Chen wrote:
>> @@ -100,15 +101,33 @@ static inline void __sti_mwait(unsigned long eax, unsigned long ecx)
>>  static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx)
>>  {
>>  	if (static_cpu_has_bug(X86_BUG_MONITOR) || !current_set_polling_and_test()) {
>> +		bool can_toggle_ibrs = false;
>>  		if (static_cpu_has_bug(X86_BUG_CLFLUSH_MONITOR)) {
>>  			mb();
>>  			clflush((void *)&current_thread_info()->flags);
>>  			mb();
>>  		}
>>  
>> +		if (irqs_disabled()) {
>> +		       /*
>> +			* CPUs run faster with speculation protection
>> +			* disabled.  All CPU threads in a core must
>> +			* disable speculation protection for it to be
>> +			* disabled.  Disable it while we are idle so the
>> +			* other hyperthread can run fast.
>> +			*
>> +			* nmi uses the save_paranoid model which
>> +			* always enables ibrs on exception entry
>> +			* before any indirect jump can run.
>> +			*/
>> +			can_toggle_ibrs = true;
>> +			unprotected_speculation_begin();
>> +		}
>>  		__monitor((void *)&current_thread_info()->flags, 0, 0);
>>  		if (!need_resched())
>>  			__mwait(eax, ecx);
>> +		if (can_toggle_ibrs)
>> +			unprotected_speculation_end();
>>  	}
>>  	current_clr_polling();
>>  }
> 
> Argh.. no. Who is calling this with IRQs enabled? And why can't we frob
> the MSR with IRQs enabled? That comment doesn't seem to explain
> anything.

No one should be calling this with IRQs enabled.  This check is probably
just paranoid.  I can get rid of it.

We cannot have IRQs enabled here, as if we disable IBRS and an interrupt
comes in, we will be running the interrupt code without IBRS in unsafe
speculation mode.

> 
>> diff --git a/arch/x86/include/asm/spec_ctrl.h b/arch/x86/include/asm/spec_ctrl.h
>> index 16fc4f58..28b0314 100644
>> --- a/arch/x86/include/asm/spec_ctrl.h
>> +++ b/arch/x86/include/asm/spec_ctrl.h
>> @@ -76,5 +76,42 @@
>>  10:
>>  .endm
>>  
>> +#else
>> +#include <asm/microcode.h>
>> +
>> +static inline void __disable_indirect_speculation(void)
>> +{
>> +        native_wrmsrl(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_ENABLE_IBRS);
>> +}
>> +
>> +static inline void __enable_indirect_speculation(void)
>> +{
>> +        native_wrmsrl(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_DISABLE_IBRS);
>> +}
>> +
>> +/*
>> + * Interrupts must be disabled to begin unprotected speculation.
>> + * Otherwise interrupts could be running in unprotected mode.
>> + */
>> +static inline void unprotected_speculation_begin(void)
>> +{
>> +	WARN_ON_ONCE(!irqs_disabled());
> 
> lockdep_assert_irqs_disabled()

OK

> 
>> +	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
>> +		__enable_indirect_speculation();
>> +}
>> +
>> +static inline void unprotected_speculation_end(void)
>> +{
>> +	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
>> +		__disable_indirect_speculation();
>> +	else
>> +		/*
>> +		 * If we intended to disable indirect speculation
>> +		 * but come here due to mis-speculation, we need
>> +		 * to stop the mis-speculation with rmb.
>> +		 */
>> +		rmb();
> 
> Code is lacking {}, also the comment doesn't make sense. If we don't
> have the MSR, why are we doing an LFENCE?

The reason for lfence is because we intend to put up IBRS and disable
speculation.  But if the CPU mis-speculates into the else portion,
we could be speculating without IBRS.  The lfence stop the mis-speculation.

> 
> And why are these boot_cpu_has() and not static_cpu_has() ?
> 

It probably doesn't matter as we will be switching the check
to the spec_ctrl_ibrs a couple of patches later.