linux-kernel - Re: [PATCH 1/7] x86/feature: Detect the x86 feature to control Speculation

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: <20180105134400.GC26807@redhat.com>
Date:   Fri, 5 Jan 2018 14:44:00 +0100
From:   Andrea Arcangeli <aarcange@...hat.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Tim Chen <tim.c.chen@...ux.intel.com>,
        Andy Lutomirski <luto@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Dave Hansen <dave.hansen@...el.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 1/7] x86/feature: Detect the x86 feature to control
 Speculation

On Fri, Jan 05, 2018 at 02:09:43PM +0100, Thomas Gleixner wrote:
> On Thu, 4 Jan 2018, Tim Chen wrote:
> > +#define MSR_IA32_SPEC_CTRL		0x00000048
> > +#define SPEC_CTRL_FEATURE_DISABLE_IBRS	(0 << 0)
> > +#define SPEC_CTRL_FEATURE_ENABLE_IBRS	(1 << 0)
> > +
> > +#define MSR_IA32_PRED_CMD		0x00000049
> > +
> >  #define MSR_IA32_PERFCTR0		0x000000c1
> >  #define MSR_IA32_PERFCTR1		0x000000c2
> >  #define MSR_FSB_FREQ			0x000000cd
> > @@ -439,6 +445,7 @@
> >  #define FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX	(1<<1)
> >  #define FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX	(1<<2)
> >  #define FEATURE_CONTROL_LMCE				(1<<20)
> > +#define FEATURE_SET_IBPB				(1<<0)
> 
> So how is that bit related to the control bits above? This file is
> structured in obvious ways ....

It's not related, FEATURE_SET_IBPB value is specific and only
meaningful to MSR_IA32_PRED_CMD.

The only use that you can ever make of that is:

static inline void __spec_ctrl_ibpb(void)
{
	native_wrmsrl(MSR_IA32_PRED_CMD, FEATURE_SET_IBPB);
}

static inline void spec_ctrl_ibpb(void)
{
	if (static_cpu_has(X86_FEATURE_IBPB_SUPPORT)) {
*obsolete line deleted to be replaced with static key*
			__spec_ctrl_ibpb();
	}
}

You need to set X86_FEATURE_IBPB_SUPPORT by hand if
X86_FEATURE_SPEC_CTRL is present. On some CPU X86_FEATURE_IBPB_SUPPORT
will show up in cpuid, in others only X86_FEATURE_SPEC_CTRL shows up
but that always implies X86_FEATURE_IBPB_SUPPORT.

void spec_ctrl_init(struct cpuinfo_x86 *c)
{
	if (c->x86_vendor != X86_VENDOR_INTEL &&
	    c->x86_vendor != X86_VENDOR_AMD)
		return;

	if (c != &boot_cpu_data) {
		spec_ctrl_cpu_init();
		return;
	}
[..]
	if (cpu_has_spec_ctrl()) {
		setup_force_cpu_cap(X86_FEATURE_IBPB_SUPPORT);
[..]
}

static void identify_cpu(struct cpuinfo_x86 *c)
[..]
	/* Set up SMEP/SMAP */
	setup_smep(c);
	setup_smap(c);

+	spec_ctrl_init(c);
[..]

static inline int cpu_has_spec_ctrl(void)
{
	return static_cpu_has(X86_FEATURE_SPEC_CTRL);
}

Note: if you don't drop all late microcode as discussed yesterday, the
above has to do a if (this/boot_cpu_has()) return 1; rmb() ; return
0;. rmb in the return 0 if there's more than one branch the CPU can
speculate through. Not from where it's called in spec_ctrl_init, but
for all other places that checks cpu_has_spec_ctrl().

Now about the late microcode my preference is not for static_cpu_has
and forcing the early microcode, but my long term preference is to
start with this/boot_cpu_has() and then turn static_cpu_has in a true
static key so that setup_force_cpu_cap shall also flip the static key
for all static_cpu_has(X86_FEATURE_IBPB_SUPPORT) also if run any time
after boot and not only if run before the static_cpu_has alternative
is patched in.