[PATCH] x86/cpu: Fix migration safety with X86_BUG_NULL_SEL

[PATCH] x86/cpu: Fix migration safety with X86_BUG_NULL_SEL

[Jane Malalane]

Currently, Linux probes for X86_BUG_NULL_SEL unconditionally which
makes it unsafe to migrate in a virtualised environment as the
properties across the migration pool might differ.

Zen3 adds the NullSelectorClearsBase bit to indicate that loading
a NULL segment selector zeroes the base and limit fields, as well as
just attributes. Zen2 also has this behaviour but doesn't have the
NSCB bit.

When virtualised, NSCB might be cleared for migration safety,
therefore we must not probe. Always honour the NSCB bit in this case,
as the hypervisor is expected to synthesize it in the Zen2 case.

Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
---
CC: <x86@kernel.org>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@redhat.com>
CC: Borislav Petkov <bp@alien8.de>
CC: "H. Peter Anvin" <hpa@zytor.com>
CC: Pu Wen <puwen@hygon.cn>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Sean Christopherson <seanjc@google.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Yazen Ghannam <Yazen.Ghannam@amd.com>
CC: Brijesh Singh <brijesh.singh@amd.com>
CC: Huang Rui <ray.huang@amd.com>
CC: Andy Lutomirski <luto@kernel.org>
CC: Kim Phillips <kim.phillips@amd.com>
CC: <stable@vger.kernel.org>
---
 arch/x86/include/asm/cpufeatures.h |  2 +-
 arch/x86/kernel/cpu/amd.c          | 23 +++++++++++++++++++++++
 arch/x86/kernel/cpu/common.c       |  6 ++----
 arch/x86/kernel/cpu/cpu.h          |  1 +
 arch/x86/kernel/cpu/hygon.c        | 23 +++++++++++++++++++++++
 5 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index d0ce5cfd3ac1..f571e4f6fe83 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -96,7 +96,7 @@
 #define X86_FEATURE_SYSCALL32		( 3*32+14) /* "" syscall in IA32 userspace */
 #define X86_FEATURE_SYSENTER32		( 3*32+15) /* "" sysenter in IA32 userspace */
 #define X86_FEATURE_REP_GOOD		( 3*32+16) /* REP microcode works well */
-/* FREE!                                ( 3*32+17) */
+#define X86_FEATURE_NSCB		( 3*32+17) /* Null Selector Clears Base */
 #define X86_FEATURE_LFENCE_RDTSC	( 3*32+18) /* "" LFENCE synchronizes RDTSC */
 #define X86_FEATURE_ACC_POWER		( 3*32+19) /* AMD Accumulated Power Mechanism */
 #define X86_FEATURE_NOPL		( 3*32+20) /* The NOPL (0F 1F) instructions */
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 2131af9f2fa2..73c4863fe0f4 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -650,6 +650,29 @@ static void early_init_amd(struct cpuinfo_x86 *c)
 	if (c->x86_power & BIT(14))
 		set_cpu_cap(c, X86_FEATURE_RAPL);
 
+	/*
+	 * Zen1 and earlier CPUs don't clear segment base/limits when
+	 * loading a NULL selector.  This has been designated
+	 * X86_BUG_NULL_SEG.
+	 *
+	 * Zen3 CPUs advertise Null Selector Clears Base in CPUID.
+	 * Zen2 CPUs also have this behaviour, but no CPUID bit.
+	 *
+	 * A hypervisor may sythesize the bit, but may also hide it
+	 * for migration safety, so we must not probe for model
+	 * specific behaviour when virtualised.
+	 */
+	if (c->extended_cpuid_level >= 0x80000021 &&
+	    cpuid_eax(0x80000021) & BIT(6))
+		set_cpu_cap(c, X86_FEATURE_NSCB);
+
+	if (!cpu_has(c, X86_FEATURE_HYPERVISOR) && !cpu_has(c, X86_FEATURE_NSCB) &&
+	    c->x86 == 0x17)
+		detect_null_seg_behavior(c);
+
+	if (!cpu_has(c, X86_FEATURE_NSCB))
+		set_cpu_bug(c, X86_BUG_NULL_SEG);
+
 #ifdef CONFIG_X86_64
 	set_cpu_cap(c, X86_FEATURE_SYSCALL32);
 #else
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 0f8885949e8c..690337796e61 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1395,7 +1395,7 @@ void __init early_cpu_init(void)
 	early_identify_cpu(&boot_cpu_data);
 }
 
-static void detect_null_seg_behavior(struct cpuinfo_x86 *c)
+void detect_null_seg_behavior(struct cpuinfo_x86 *c)
 {
 #ifdef CONFIG_X86_64
 	/*
@@ -1419,7 +1419,7 @@ static void detect_null_seg_behavior(struct cpuinfo_x86 *c)
 	loadsegment(fs, 0);
 	rdmsrl(MSR_FS_BASE, tmp);
 	if (tmp != 0)
-		set_cpu_bug(c, X86_BUG_NULL_SEG);
+		set_cpu_cap(c, X86_FEATURE_NSCB);
 	wrmsrl(MSR_FS_BASE, old_base);
 #endif
 }
@@ -1457,8 +1457,6 @@ static void generic_identify(struct cpuinfo_x86 *c)
 
 	get_model_name(c); /* Default name */
 
-	detect_null_seg_behavior(c);
-
 	/*
 	 * ESPFIX is a strange bug.  All real CPUs have it.  Paravirt
 	 * systems that run Linux at CPL > 0 may or may not have the
diff --git a/arch/x86/kernel/cpu/cpu.h b/arch/x86/kernel/cpu/cpu.h
index 95521302630d..642f46e0dd67 100644
--- a/arch/x86/kernel/cpu/cpu.h
+++ b/arch/x86/kernel/cpu/cpu.h
@@ -75,6 +75,7 @@ extern int detect_extended_topology_early(struct cpuinfo_x86 *c);
 extern int detect_extended_topology(struct cpuinfo_x86 *c);
 extern int detect_ht_early(struct cpuinfo_x86 *c);
 extern void detect_ht(struct cpuinfo_x86 *c);
+extern void detect_null_seg_behavior(struct cpuinfo_x86 *c);
 
 unsigned int aperfmperf_get_khz(int cpu);
 
diff --git a/arch/x86/kernel/cpu/hygon.c b/arch/x86/kernel/cpu/hygon.c
index 6d50136f7ab9..765f1556d964 100644
--- a/arch/x86/kernel/cpu/hygon.c
+++ b/arch/x86/kernel/cpu/hygon.c
@@ -264,6 +264,29 @@ static void early_init_hygon(struct cpuinfo_x86 *c)
 	if (c->x86_power & BIT(14))
 		set_cpu_cap(c, X86_FEATURE_RAPL);
 
+	/*
+	 * Zen1 and earlier CPUs don't clear segment base/limits when
+	 * loading a NULL selector.  This has been designated
+	 * X86_BUG_NULL_SEG.
+	 *
+	 * Zen3 CPUs advertise Null Selector Clears Base in CPUID.
+	 * Zen2 CPUs also have this behaviour, but no CPUID bit.
+	 *
+	 * A hypervisor may sythesize the bit, but may also hide it
+	 * for migration safety, so we must not probe for model
+	 * specific behaviour when virtualised.
+	 */
+	if (c->extended_cpuid_level >= 0x80000021 &&
+	    cpuid_eax(0x80000021) & BIT(6))
+	        set_cpu_cap(c, X86_FEATURE_NSCB);
+
+	if (!cpu_has(c, X86_FEATURE_HYPERVISOR) && !cpu_has(c, X86_FEATURE_NSCB) &&
+	    c->x86 == 0x18)
+                detect_null_seg_behavior(c);
+
+	if (!cpu_has(c, X86_FEATURE_NSCB))
+		set_cpu_bug(c, X86_BUG_NULL_SEG);
+
 #ifdef CONFIG_X86_64
 	set_cpu_cap(c, X86_FEATURE_SYSCALL32);
 #endif
-- 
2.11.0

Re: [PATCH] x86/cpu: Fix migration safety with X86_BUG_NULL_SEL

[Andrew Cooper]

On 01/10/2021 14:33, Jane Malalane wrote:
> Currently, Linux probes for X86_BUG_NULL_SEL unconditionally which
> makes it unsafe to migrate in a virtualised environment as the
> properties across the migration pool might differ.
>
> Zen3 adds the NullSelectorClearsBase bit to indicate that loading
> a NULL segment selector zeroes the base and limit fields, as well as
> just attributes. Zen2 also has this behaviour but doesn't have the
> NSCB bit.
>
> When virtualised, NSCB might be cleared for migration safety,
> therefore we must not probe. Always honour the NSCB bit in this case,
> as the hypervisor is expected to synthesize it in the Zen2 case.
>
> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH] x86/cpu: Fix migration safety with X86_BUG_NULL_SEL

[Borislav Petkov]

On Fri, Oct 01, 2021 at 02:33:49PM +0100, Jane Malalane wrote:
> Subject: Re: [PATCH] x86/cpu: Fix migration safety with X86_BUG_NULL_SEL
> ...
> Currently, Linux probes for X86_BUG_NULL_SEL unconditionally which
> makes it unsafe to migrate in a virtualised environment as the
> properties across the migration pool might differ.

Sorry but you need to explain "migration safety" in greater detail -
we're not all virtualizers.

> Zen3 adds the NullSelectorClearsBase bit to indicate that loading
> a NULL segment selector zeroes the base and limit fields, as well as
> just attributes. Zen2 also has this behaviour but doesn't have the
> NSCB bit.

I'm guessing you can detect Zen2 too, without the CPUID bit?

> When virtualised, NSCB might be cleared for migration safety,
> therefore we must not probe. Always honour the NSCB bit in this case,

Who is "we"?

> as the hypervisor is expected to synthesize it in the Zen2 case.
> 
> Signed-off-by: Jane Malalane <jane.malalane@citrix.com>
> ---
> CC: <x86@kernel.org>
> CC: Thomas Gleixner <tglx@linutronix.de>
> CC: Ingo Molnar <mingo@redhat.com>
> CC: Borislav Petkov <bp@alien8.de>
> CC: "H. Peter Anvin" <hpa@zytor.com>
> CC: Pu Wen <puwen@hygon.cn>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Sean Christopherson <seanjc@google.com>
> CC: Peter Zijlstra <peterz@infradead.org>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Yazen Ghannam <Yazen.Ghannam@amd.com>
> CC: Brijesh Singh <brijesh.singh@amd.com>
> CC: Huang Rui <ray.huang@amd.com>
> CC: Andy Lutomirski <luto@kernel.org>
> CC: Kim Phillips <kim.phillips@amd.com>
> CC: <stable@vger.kernel.org>
> ---
>  arch/x86/include/asm/cpufeatures.h |  2 +-
>  arch/x86/kernel/cpu/amd.c          | 23 +++++++++++++++++++++++
>  arch/x86/kernel/cpu/common.c       |  6 ++----
>  arch/x86/kernel/cpu/cpu.h          |  1 +
>  arch/x86/kernel/cpu/hygon.c        | 23 +++++++++++++++++++++++
>  5 files changed, 50 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index d0ce5cfd3ac1..f571e4f6fe83 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -96,7 +96,7 @@
>  #define X86_FEATURE_SYSCALL32		( 3*32+14) /* "" syscall in IA32 userspace */
>  #define X86_FEATURE_SYSENTER32		( 3*32+15) /* "" sysenter in IA32 userspace */
>  #define X86_FEATURE_REP_GOOD		( 3*32+16) /* REP microcode works well */
> -/* FREE!                                ( 3*32+17) */
> +#define X86_FEATURE_NSCB		( 3*32+17) /* Null Selector Clears Base */

You don't really need that bit definition - you check CPUID in the
early_init_amd/hygon() functions and that is enough. IOW, based on that
CPUID bit, you can call detect_null_seg_behavior() and set (or not)
X86_BUG_NULL_SEG.

...

> diff --git a/arch/x86/kernel/cpu/hygon.c b/arch/x86/kernel/cpu/hygon.c
> index 6d50136f7ab9..765f1556d964 100644
> --- a/arch/x86/kernel/cpu/hygon.c
> +++ b/arch/x86/kernel/cpu/hygon.c
> @@ -264,6 +264,29 @@ static void early_init_hygon(struct cpuinfo_x86 *c)
>  	if (c->x86_power & BIT(14))
>  		set_cpu_cap(c, X86_FEATURE_RAPL);
>  
> +	/*
> +	 * Zen1 and earlier CPUs don't clear segment base/limits when
> +	 * loading a NULL selector.  This has been designated
> +	 * X86_BUG_NULL_SEG.
> +	 *
> +	 * Zen3 CPUs advertise Null Selector Clears Base in CPUID.
> +	 * Zen2 CPUs also have this behaviour, but no CPUID bit.
> +	 *
> +	 * A hypervisor may sythesize the bit, but may also hide it
> +	 * for migration safety, so we must not probe for model
> +	 * specific behaviour when virtualised.
> +	 */
> +	if (c->extended_cpuid_level >= 0x80000021 &&
> +	    cpuid_eax(0x80000021) & BIT(6))
> +	        set_cpu_cap(c, X86_FEATURE_NSCB);
> +
> +	if (!cpu_has(c, X86_FEATURE_HYPERVISOR) && !cpu_has(c, X86_FEATURE_NSCB) &&
> +	    c->x86 == 0x18)
> +                detect_null_seg_behavior(c);
> +
> +	if (!cpu_has(c, X86_FEATURE_NSCB))
> +		set_cpu_bug(c, X86_BUG_NULL_SEG);

Please integrate scripts/checkpatch.pl into your patch creation
workflow. Some of the warnings/errors *actually* make sense.

I'll show you that there's whitespace damage here:

ERROR: code indent should use tabs where possible
#238: FILE: arch/x86/kernel/cpu/hygon.c:281:
+^I        set_cpu_cap(c, X86_FEATURE_NSCB);$

ERROR: code indent should use tabs where possible
#242: FILE: arch/x86/kernel/cpu/hygon.c:285:
+                detect_null_seg_behavior(c);$

WARNING: please, no spaces at the start of a line
#242: FILE: arch/x86/kernel/cpu/hygon.c:285:
+                detect_null_seg_behavior(c);$

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH] x86/cpu: Fix migration safety with X86_BUG_NULL_SEL

[Andy Lutomirski]

On Fri, Oct 1, 2021, at 6:33 AM, Jane Malalane wrote:

> -static void detect_null_seg_behavior(struct cpuinfo_x86 *c)
> +void detect_null_seg_behavior(struct cpuinfo_x86 *c)

__init, please 

Re: [PATCH] x86/cpu: Fix migration safety with X86_BUG_NULL_SEL

[Andrew Cooper]

On 01/10/2021 15:19, Borislav Petkov wrote:
> On Fri, Oct 01, 2021 at 02:33:49PM +0100, Jane Malalane wrote:
>> Subject: Re: [PATCH] x86/cpu: Fix migration safety with X86_BUG_NULL_SEL
>> ...
>> Currently, Linux probes for X86_BUG_NULL_SEL unconditionally which
>> makes it unsafe to migrate in a virtualised environment as the
>> properties across the migration pool might differ.
> Sorry but you need to explain "migration safety" in greater detail -
> we're not all virtualizers.

The case which goes wrong is this:

1. Zen1 (or earlier) and Zen2 (or later) in a migration pool
2. Linux boots on Zen2, probes and finds the absence of X86_BUG_NULL_SEL
3. Linux is then migrated to Zen1

Linux is now running on a X86_BUG_NULL_SEL-impacted CPU while believing
that the bug is fixed.

The only way to address the problem is to fully trust the "no longer
affected" CPUID bit when virtualised, because in the above case it would
be clear deliberately to indicate the fact "you might migrate to
somewhere which really is affected".

~Andrew

Re: [PATCH] x86/cpu: Fix migration safety with X86_BUG_NULL_SEL

[Borislav Petkov]

On Wed, Oct 06, 2021 at 03:15:51PM +0100, Andrew Cooper wrote:
> The case which goes wrong is this:
> 
> 1. Zen1 (or earlier) and Zen2 (or later) in a migration pool
> 2. Linux boots on Zen2, probes and finds the absence of X86_BUG_NULL_SEL
> 3. Linux is then migrated to Zen1
> 
> Linux is now running on a X86_BUG_NULL_SEL-impacted CPU while believing
> that the bug is fixed.
> 
> The only way to address the problem is to fully trust the "no longer
> affected" CPUID bit when virtualised, because in the above case it would
> be clear deliberately to indicate the fact "you might migrate to
> somewhere which really is affected".

Yap, makes sense.

Thanks for taking the time - that's what I was looking for.

Please add to the commit message of the next version.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette