linux-kernel - Re: [patch RFC 4/5] x86/cpufeatures: Detect Speculation control feature

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:   Wed, 10 Jan 2018 07:32:11 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...uxfoundation.org>, x86@...nel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Borislav Petkov <bp@...en8.de>,
        David Woodhouse <dwmw@...zon.co.uk>,
        Tim Chen <tim.c.chen@...ux.intel.com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Dave Hansen <dave.hansen@...el.com>,
        Andy Lutomirski <luto@...nel.org>,
        Arjan Van De Ven <arjan.van.de.ven@...el.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [patch RFC 4/5] x86/cpufeatures: Detect Speculation control
 feature


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

> From: Tim Chen <tim.c.chen@...ux.intel.com>
> 
> CPUs can expose a MSR to control speculation. The initial function of this
> MSR is to control Indirect Branch Speculation, which is required to
> mitigate the Spectre_V2 attack on certain CPU generations.

s/a MSR
 /an MSR

> If CPUID(7).RDX[26] is set then MSR_IA32_SPEC_CTRL (0x48) is available and
> bit 0 of that MSR controls whether Indirect Branch Speculation is
> restricted or not. The control bit is named IBRS (Indirect Branch
> Restricted Speculation). The IBSR bit can be unconditionally set to 1
> without clearing it before.

Argh for inverted logic: why was the control bit defined for a _negated_ value, 
i.e. why does '0' mean "don't don't speculate"?

And yes, I know what's behind it: this way 'IBRS' can be called a 'mitigation 
feature' that can be 'enabled', instead of calling it a 'broken CPU feature 
feature' that has to be disabled ...

That's nonsense that causes confusion to no end:

> If IBRS is set, near returns and near indirect jumps/calls will not allow
> their predicted target address to be controlled by code that executed in a
> less privileged prediction mode before the IBRS mode was last written with
> a value of 1 or on another logical processor so long as all Return Stack
> Buffer (RSB) entries from the previous less privileged prediction mode are
> overwritten.
> 
> Thus a near indirect jump/call/return may be affected by code in a less
> privileged prediction mode that executed AFTER IBRS mode was last written
> with a value of 1.
> 
> Code executed by a sibling logical processor cannot control indirect
> jump/call/return predicted target when IBRS is set
> 
> IBRS is not required in order to isolate branch predictions for SMM or SGX
> enclaves.
> 
> Enabling IBRS can cause a measurable and depending on the workload
> significant CPU performance penalty.

This is bound to be really confusing due to the logic negation, in particular:

> +#define SPEC_CTRL_DISABLE_IBRS		(0UL << 0)
> +#define SPEC_CTRL_ENABLE_IBRS		(1UL << 0)

"SPEC_CTRL_ENABLE_IBRS" will _disable_ speculation!

Then that brokenness is propagated into higher code as well by the next patch, 
i.e. "SPECTRE_V2_CMD_IBRS" et al.

This is totally brain-dead and should be inverted to follow natural logic instead. 
The lowest level hardware ABI will obviously stay broken,

Why not define a sane name instead? Something like:

	CTRL_DISABLE_BR_SPECULATION
	CTRL_ENABLE_BR_SPECULATION

and only explain the broken negated Intel naming and flag once in the comments for 
the definition and the lowest level MSR write.

Thanks,

	Ingo