[MODERATED] Re: ***UNCHECKED*** [patch 3/8] [PATCH v1.3.1 3/7] Linux Patch 3

[MODERATED] Re: ***UNCHECKED*** [patch 3/8] [PATCH v1.3.1 3/7] Linux Patch 3

[Borislav Petkov]

On Thu, Apr 12, 2018 at 10:26:52PM -0400, speck for konrad.wilk_at_oracle.com wrote:
> diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> index 561cb228605a..73f76d0f5181 100644
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -72,6 +72,9 @@ void __init check_bugs(void)
>  	 */
>  	if (!direct_gbpages)
>  		set_memory_4k((unsigned long)__va(0), 1);
> +
> +	if (mdd_at_boot())
> +		wrmsrl(MSR_IA32_SPEC_CTRL, SPEC_CTRL_MDD);

You can't do this on the common path as that would explode on !Intel.

Instead, move that check to init_intel().

And add to init_amd_zn() code reading MSR 0xc0011020, setting bit 10 to
1b and then writing it back in the mdd_at_boot() case. We have a define
for that MSR already - MSR_AMD64_LS_CFG.

>  
> @@ -317,7 +320,14 @@ static void __init spectre_v2_select_mitigation(void)
>  #undef pr_fmt
>  #define pr_fmt(fmt)     "MDD: " fmt
>  
> -static enum md_mitigation md_mode = MD_NONE;
> +enum md_mitigation md_mode = MD_NONE;
> +/* When switching from lower privilege level (cpl3) to higher (cpl0). */
> +u64 spec_ctrl_priv;
> +EXPORT_SYMBOL_GPL(spec_ctrl_priv);
> +
> +/* When switching from higher to lower privilege level. */
> +u64 spec_ctrl_unpriv;
> +EXPORT_SYMBOL_GPL(spec_ctrl_unpriv);

I don't like exporting some arbitrary variables to the rest of the
kernel. Do accessors instead pls.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[MODERATED] Re: ***UNCHECKED*** Re: [patch 3/8] [PATCH v1.3.1 3/7] Linux Patch 3

[Borislav Petkov]

On Wed, Apr 18, 2018 at 05:37:13PM +0200, speck for Borislav Petkov wrote:
> And add to init_amd_zn() code reading MSR 0xc0011020, setting bit 10 to
> 1b and then writing it back in the mdd_at_boot() case. We have a define
> for that MSR already - MSR_AMD64_LS_CFG.

... and while you're at it, do the respective thing in:

init_amd_bd() and there set MSR C001_0120, bit 54.

and for F16h, add

	case 0x16: init_amd_jg(c); break;

in the switch statement in init_amd() and in that new function

set MSR C001_1020, bit 33.

All for the mdd_at_boot() case.

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[MODERATED] Re: [patch 3/8] [PATCH v1.3.1 3/7] Linux Patch 3

[Borislav Petkov]

On Wed, Apr 18, 2018 at 06:00:29PM +0200, Borislav Petkov wrote:
> On Wed, Apr 18, 2018 at 05:37:13PM +0200, speck for Borislav Petkov wrote:
> > And add to init_amd_zn() code reading MSR 0xc0011020, setting bit 10 to
> > 1b and then writing it back in the mdd_at_boot() case. We have a define
> > for that MSR already - MSR_AMD64_LS_CFG.
> 
> ... and while you're at it, do the respective thing in:
> 
> init_amd_bd() and there set MSR C001_0120, bit 54.

Correction: F15h should use the same MSR C001_1020, bit 54.

Basically it is the same MSR but different bits for the different
families.

Thx.

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[MODERATED] Re: ***UNCHECKED*** [patch 3/8] [PATCH v1.3.1 3/7] Linux Patch 3

[Konrad Rzeszutek Wilk]

On Wed, Apr 18, 2018 at 05:37:13PM +0200, speck for Borislav Petkov wrote:
> On Thu, Apr 12, 2018 at 10:26:52PM -0400, speck for konrad.wilk_at_oracle.com wrote:
> > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> > index 561cb228605a..73f76d0f5181 100644
> > --- a/arch/x86/kernel/cpu/bugs.c
> > +++ b/arch/x86/kernel/cpu/bugs.c
> > @@ -72,6 +72,9 @@ void __init check_bugs(void)
> >  	 */
> >  	if (!direct_gbpages)
> >  		set_memory_4k((unsigned long)__va(0), 1);
> > +
> > +	if (mdd_at_boot())
> > +		wrmsrl(MSR_IA32_SPEC_CTRL, SPEC_CTRL_MDD);
> 
> You can't do this on the common path as that would explode on !Intel.
> 
> Instead, move that check to init_intel().

That won't work. The reason is that check_bugs first calls
identify_boot_cpu() which calls identify_cpu() which calls
this_cpu->init. Once identify_boot_cpu() is done _then_ it walks
through the spectre_v2_select_mitigation() and ssb_select_mitigation().

> 
> And add to init_amd_zn() code reading MSR 0xc0011020, setting bit 10 to
> 1b and then writing it back in the mdd_at_boot() case. We have a define
> for that MSR already - MSR_AMD64_LS_CFG.

I am thinking that perhaps we can add a new function:

From 258854941d792de5de4ffdefcce8f52e8984e2e5 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Thu, 19 Apr 2018 16:47:38 -0400
Subject: [PATCH] x86/cpu: Add fix_this_cpu to be called _after_ check_bugs and
 at BSP.

We do a lot of things in the check_bugs() - one of the first
things we do is identify_boot_cpu() which calls identify_cpu()
which calls this_cpu->init.

Once identify_boot_cpu() is done _then_ it walks through the
spectre_v2_select_mitigation() and alternative_assembler().

If there are some CPU fix ups _after_ spectre_v2 is done
we can't activate those on the BSP as we have already
called 'this_cpu->init'. Hence add a new function to
fixup CPUs.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/include/asm/processor.h | 1 +
 arch/x86/kernel/cpu/bugs.c       | 1 +
 arch/x86/kernel/cpu/common.c     | 8 ++++++++
 arch/x86/kernel/cpu/cpu.h        | 1 +
 4 files changed, 11 insertions(+)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index b0ccd4847a58..b22bc9be2385 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -195,6 +195,7 @@ extern void init_amd_cacheinfo(struct cpuinfo_x86 *c);
 
 extern void detect_extended_topology(struct cpuinfo_x86 *c);
 extern void detect_ht(struct cpuinfo_x86 *c);
+extern void apply_cpu_fixes(void);
 
 #ifdef CONFIG_X86_32
 extern int have_cpuid_p(void);
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 4632c26e13e0..9c2987b4f7fa 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -77,6 +77,7 @@ void __init check_bugs(void)
 	if (!direct_gbpages)
 		set_memory_4k((unsigned long)__va(0), 1);
 #endif
+	apply_cpu_fixes();
 }
 
 /* The kernel command line selection */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index f7d80658cced..a59f7902a7fa 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -955,6 +955,12 @@ static void __init cpu_set_bug_bits(struct cpuinfo_x86 *c)
 	setup_force_cpu_bug(X86_BUG_CPU_SSB);
 };
 
+void apply_cpu_fixes(void)
+{
+	if (this_cpu->c_bug_fix)
+		this_cpu->c_bug_fix(&boot_cpu_data);
+}
+
 /*
  * Do minimum CPU detection early.
  * Fields really needed: vendor, cpuid_level, family, model, mask,
@@ -1311,6 +1317,8 @@ static void identify_cpu(struct cpuinfo_x86 *c)
 #ifdef CONFIG_NUMA
 	numa_add_cpu(smp_processor_id());
 #endif
+	if (this_cpu->c_bug_fix)
+		this_cpu->c_bug_fix(c);
 }
 
 /*
diff --git a/arch/x86/kernel/cpu/cpu.h b/arch/x86/kernel/cpu/cpu.h
index e806b11a99af..7081634caac5 100644
--- a/arch/x86/kernel/cpu/cpu.h
+++ b/arch/x86/kernel/cpu/cpu.h
@@ -15,6 +15,7 @@ struct cpu_dev {
 	void		(*c_identify)(struct cpuinfo_x86 *);
 	void		(*c_detect_tlb)(struct cpuinfo_x86 *);
 	void		(*c_bsp_resume)(struct cpuinfo_x86 *);
+	void		(*c_bug_fix)(struct cpuinfo_x86 *);
 	int		c_x86_vendor;
 #ifdef CONFIG_X86_32
 	/* Optional vendor specific routine to obtain the cache size. */
-- 
2.14.3

[MODERATED] Re: [patch 3/8] [PATCH v1.3.1 3/7] Linux Patch 3

[Borislav Petkov]

On Thu, Apr 19, 2018 at 04:51:37PM -0400, speck for Konrad Rzeszutek Wilk wrote:
> That won't work. The reason is that check_bugs first calls
> identify_boot_cpu() which calls identify_cpu() which calls
> this_cpu->init. Once identify_boot_cpu() is done _then_ it walks
> through the spectre_v2_select_mitigation() and ssb_select_mitigation().

So call md_select_mitigation() before identify_boot_cpu() in
check_bugs().

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
-- 

[MODERATED] Re: [patch 3/8] [PATCH v1.3.1 3/7] Linux Patch 3

[Konrad Rzeszutek Wilk]

On Thu, Apr 19, 2018 at 11:10:03PM +0200, speck for Borislav Petkov wrote:
> On Thu, Apr 19, 2018 at 04:51:37PM -0400, speck for Konrad Rzeszutek Wilk wrote:
> > That won't work. The reason is that check_bugs first calls
> > identify_boot_cpu() which calls identify_cpu() which calls
> > this_cpu->init. Once identify_boot_cpu() is done _then_ it walks
> > through the spectre_v2_select_mitigation() and ssb_select_mitigation().
> 
> So call md_select_mitigation() before identify_boot_cpu() in
> check_bugs().

That will mean that the init_speculation_control() won't happen
until _after_ md_select_mitigation().

And that throws a wrench in that as we need to rdmsrl(SPEC_CTRL)
(which is right now done from spectre_v2_select_mitigation):

347         if (boot_cpu_has(X86_FEATURE_IBPB) || boot_cpu_has(X86_FEATURE_IBRS))
348                 rdmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
349 }

And x86_spec_ctrl_base is then ORed in md_select_mitigation().

But
spectre_v2_select_mitigation depends on identify_boot_cpu.
md_select_mitigation depends on spectre_v2_select_mitigation.

If I move the identify_boot_cpu before md_select_mitigation things
get wonky.

Maybe it will be easier to explain when I send out the patches?

[MODERATED] Re: [patch 3/8] [PATCH v1.3.1 3/7] Linux Patch 3

[Konrad Rzeszutek Wilk]

On Thu, Apr 19, 2018 at 08:29:21PM -0400, speck for Konrad Rzeszutek Wilk wrote:
> On Thu, Apr 19, 2018 at 11:10:03PM +0200, speck for Borislav Petkov wrote:
> > On Thu, Apr 19, 2018 at 04:51:37PM -0400, speck for Konrad Rzeszutek Wilk wrote:
> > > That won't work. The reason is that check_bugs first calls
> > > identify_boot_cpu() which calls identify_cpu() which calls
> > > this_cpu->init. Once identify_boot_cpu() is done _then_ it walks
> > > through the spectre_v2_select_mitigation() and ssb_select_mitigation().
> > 
> > So call md_select_mitigation() before identify_boot_cpu() in
> > check_bugs().
> 
> That will mean that the init_speculation_control() won't happen
> until _after_ md_select_mitigation().
> 
> And that throws a wrench in that as we need to rdmsrl(SPEC_CTRL)
> (which is right now done from spectre_v2_select_mitigation):
> 
> 347         if (boot_cpu_has(X86_FEATURE_IBPB) || boot_cpu_has(X86_FEATURE_IBRS))
> 348                 rdmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
> 349 }
> 
> And x86_spec_ctrl_base is then ORed in md_select_mitigation().
> 
> But
> spectre_v2_select_mitigation depends on identify_boot_cpu.
> md_select_mitigation depends on spectre_v2_select_mitigation.
> 
> If I move the identify_boot_cpu before md_select_mitigation things
> get wonky.
> 
> Maybe it will be easier to explain when I send out the patches?

I fixed the circular dependency I mentioned here in the patches.
(See the v2 posting, Patch #3).

But then I realized we still have one more issue - mainly the
X86_FEATURE_MDD. That is you asked in one of the patches review
to guard the md_parse_cmdline with:

	if (!boot_cpu_has(X86_FEATURE_MDD))
		return MD_CMD_NONE;

And on AMD the X86_FEATURE_MDD (with the v2 patch set just sent)
it gets set in c_init as it detects which family can safely
touch the MSR_AMD64_LS_CFG MSR:

       if (!rdmsrl_safe(MSR_AMD64_LS_CFG, &msr_MSR_AMD64_LS_CFG_val))
               set_cpu_cap(c, X86_FEATURE_MDD);

(as the CPUID.7.EDX[31] is not going to be set on AMD I assume).
See the v2 - Patch #9.