[MODERATED] [PATCH v4 03/10] [PATCH v4 3/9] Linux Patch #3

[MODERATED] [PATCH v4 03/10] [PATCH v4 3/9] Linux Patch #3

[konrad.wilk]

The 336996-Speculative-Execution-Side-Channel-Mitigations.pdf
refers to all the other bits as reserved. The Intel SDM glossary
defines reserved as implementation specific - aka unknown.

As such at bootup we should take it into account and apply
proper masking for the bits we use.

xSuggested-by: Jon Masters <jcm@redhat.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v2: New patch
v3: Expose only one global variable
v4: Don't expose global variable.
    Provide x86_set_spec_ctrl and x86_get_spec_ctrl
    Only filter IBRS on x86_set_spec_ctrl
---
 arch/x86/include/asm/nospec-branch.h | 25 +++++++++++++++++++++----
 arch/x86/kernel/cpu/bugs.c           | 30 ++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index f928ad9b143f..53db3010daa3 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -217,6 +217,17 @@ enum spectre_v2_mitigation {
 	SPECTRE_V2_IBRS,
 };
 
+/*
+ * The Intel specification for the SPEC_CTRL MSR requires that we
+ * preserve any already set reserved bits at boot time (e.g. for
+ * future additions that this kernel is not currently aware of).
+ * We then set any additional mitigation bits that we want
+ * ourselves and always use this as the base for SPEC_CTRL.
+ * We also use this when handling guest entry/exit as below.
+ */
+extern void x86_set_spec_ctrl(u64);
+extern u64 x86_get_spec_ctrl(void);
+
 extern char __indirect_thunk_start[];
 extern char __indirect_thunk_end[];
 
@@ -248,12 +259,14 @@ static inline void vmexit_fill_RSB(void)
 				 "movl $0, %%edx\n\t"		\
 				 "wrmsr",			\
 				 _feature)			\
-		     : : [msr] "i" (_msr), [val] "i" (_val)	\
+		     : : [msr] "i" (_msr), [val] "m" (_val)	\
 		     : "eax", "ecx", "edx", "memory")
 
 static inline void indirect_branch_prediction_barrier(void)
 {
-	alternative_msr_write(MSR_IA32_PRED_CMD, PRED_CMD_IBPB,
+	u64 val = PRED_CMD_IBPB;
+
+	alternative_msr_write(MSR_IA32_PRED_CMD, val,
 			      X86_FEATURE_USE_IBPB);
 }
 
@@ -265,14 +278,18 @@ static inline void indirect_branch_prediction_barrier(void)
  */
 #define firmware_restrict_branch_speculation_start()			\
 do {									\
+	u64 val = x86_get_spec_ctrl() | SPEC_CTRL_IBRS;			\
+									\
 	preempt_disable();						\
-	alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,	\
+	alternative_msr_write(MSR_IA32_SPEC_CTRL, val,			\
 			      X86_FEATURE_USE_IBRS_FW);			\
 } while (0)
 
 #define firmware_restrict_branch_speculation_end()			\
 do {									\
-	alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,			\
+	u64 val = x86_get_spec_ctrl();					\
+									\
+	alternative_msr_write(MSR_IA32_SPEC_CTRL, val,			\
 			      X86_FEATURE_USE_IBRS_FW);			\
 	preempt_enable();						\
 } while (0)
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index ad613f73d071..db4ce57598fd 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -27,11 +27,14 @@
 #include <asm/intel-family.h>
 
 static void __init spectre_v2_select_mitigation(void);
+static void __init spec_ctrl_save_msr(void);
 
 void __init check_bugs(void)
 {
 	identify_boot_cpu();
 
+	spec_ctrl_save_msr();
+
 	if (!IS_ENABLED(CONFIG_SMP)) {
 		pr_info("CPU: ");
 		print_cpu_info(&boot_cpu_data);
@@ -95,6 +98,33 @@ static const char *spectre_v2_strings[] = {
 
 static enum spectre_v2_mitigation spectre_v2_enabled = SPECTRE_V2_NONE;
 
+u64 x86_spec_ctrl_base;
+
+static void __init spec_ctrl_save_msr(void)
+{
+	/*
+	 * Read the SPEC_CTRL MSR to account for reserved bits which may have
+	 * unknown values.
+	 */
+	if (boot_cpu_has(X86_FEATURE_IBRS))
+		rdmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
+}
+
+void x86_set_spec_ctrl(u64 val)
+{
+	if (val & ~(SPEC_CTRL_IBRS))
+		WARN_ONCE(1, "SPEC_CTRL MSR value 0x%16llx is unknown.\n", val);
+	else
+		wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base | val);
+}
+EXPORT_SYMBOL_GPL(x86_set_spec_ctrl);
+
+u64 x86_get_spec_ctrl(void)
+{
+	return x86_spec_ctrl_base;
+}
+EXPORT_SYMBOL_GPL(x86_get_spec_ctrl);
+
 #ifdef RETPOLINE
 static bool spectre_v2_bad_module;
 
-- 
2.14.3

[MODERATED] Re: [PATCH v4 03/10] [PATCH v4 3/9] Linux Patch #3

[Borislav Petkov]

On Mon, Apr 23, 2018 at 11:16:04PM -0400, speck for konrad.wilk_at_oracle.com wrote:
> x86/bugs: Read SPEC_CTRL MSR during boot and re-use reserved bits.
> 
> The 336996-Speculative-Execution-Side-Channel-Mitigations.pdf
> refers to all the other bits as reserved. The Intel SDM glossary
> defines reserved as implementation specific - aka unknown.
> 
> As such at bootup we should take it into account and apply
> proper masking for the bits we use.
> 
> xSuggested-by: Jon Masters <jcm@redhat.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> v2: New patch
> v3: Expose only one global variable
> v4: Don't expose global variable.
>     Provide x86_set_spec_ctrl and x86_get_spec_ctrl
>     Only filter IBRS on x86_set_spec_ctrl
> ---
>  arch/x86/include/asm/nospec-branch.h | 25 +++++++++++++++++++++----
>  arch/x86/kernel/cpu/bugs.c           | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+), 4 deletions(-)

An obvious cleanup ontop: no need for function and thus forward
declaration.

Also, x86_spec_ctrl_base should be static now that we have the
accessors.

---
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index db4ce57598fd..91742351f333 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -27,13 +27,19 @@
 #include <asm/intel-family.h>
 
 static void __init spectre_v2_select_mitigation(void);
-static void __init spec_ctrl_save_msr(void);
+
+static u64 x86_spec_ctrl_base;
 
 void __init check_bugs(void)
 {
 	identify_boot_cpu();
 
-	spec_ctrl_save_msr();
+	/*
+	 * Read the SPEC_CTRL MSR to account for reserved bits which may have
+	 * unknown values.
+	 */
+	if (boot_cpu_has(X86_FEATURE_IBRS))
+		rdmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
 
 	if (!IS_ENABLED(CONFIG_SMP)) {
 		pr_info("CPU: ");
@@ -98,18 +104,6 @@ static const char *spectre_v2_strings[] = {
 
 static enum spectre_v2_mitigation spectre_v2_enabled = SPECTRE_V2_NONE;
 
-u64 x86_spec_ctrl_base;
-
-static void __init spec_ctrl_save_msr(void)
-{
-	/*
-	 * Read the SPEC_CTRL MSR to account for reserved bits which may have
-	 * unknown values.
-	 */
-	if (boot_cpu_has(X86_FEATURE_IBRS))
-		rdmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
-}
-
 void x86_set_spec_ctrl(u64 val)
 {
 	if (val & ~(SPEC_CTRL_IBRS))

-- 
Regards/Gruss,
    Boris.

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

[MODERATED] Re: [PATCH v4 03/10] [PATCH v4 3/9] Linux Patch #3

[Jon Masters]

[-- Attachment #1: Type: text/plain, Size: 1541 bytes --]

On 04/23/2018 11:16 PM, speck for konrad.wilk_at_oracle.com wrote:

> +u64 x86_spec_ctrl_base;
> +
> +static void __init spec_ctrl_save_msr(void)
> +{
> +	/*
> +	 * Read the SPEC_CTRL MSR to account for reserved bits which may have
> +	 * unknown values.
> +	 */
> +	if (boot_cpu_has(X86_FEATURE_IBRS))
> +		rdmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
> +}
> +
> +void x86_set_spec_ctrl(u64 val)
> +{
> +	if (val & ~(SPEC_CTRL_IBRS))
> +		WARN_ONCE(1, "SPEC_CTRL MSR value 0x%16llx is unknown.\n", val);
> +	else
> +		wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base | val);
> +}
> +EXPORT_SYMBOL_GPL(x86_set_spec_ctrl);
> +
> +u64 x86_get_spec_ctrl(void)
> +{
> +	return x86_spec_ctrl_base;
> +}
> +EXPORT_SYMBOL_GPL(x86_get_spec_ctrl);

I actually don't like the above (I killed it last time too ;) ) because
the accessors don't quite do what someone might think who hadn't read it
carefully. What we want to do is cache the base MSR and then toggle on
top of it. That's what the above do. But that means the "get" function
doesn't actually give you the current hardware value, only the cached
boot time value and that might differ due to IBRS status, etc.

[ Now, I personally care because in RHEL we've a lot of very different
code for IBRS and it's actually a lot cleaner to leave the direct
export, which is what I am currently doing in our own codebase ]

But I suspect others will fall into traps with the above too.

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

[MODERATED] Re: [PATCH v4 03/10] [PATCH v4 3/9] Linux Patch #3

[Konrad Rzeszutek Wilk]

On Tue, Apr 24, 2018 at 05:43:23PM -0400, speck for Jon Masters wrote:
> On 04/23/2018 11:16 PM, speck for konrad.wilk_at_oracle.com wrote:
> 
> > +u64 x86_spec_ctrl_base;
> > +
> > +static void __init spec_ctrl_save_msr(void)
> > +{
> > +	/*
> > +	 * Read the SPEC_CTRL MSR to account for reserved bits which may have
> > +	 * unknown values.
> > +	 */
> > +	if (boot_cpu_has(X86_FEATURE_IBRS))
> > +		rdmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
> > +}
> > +
> > +void x86_set_spec_ctrl(u64 val)
> > +{
> > +	if (val & ~(SPEC_CTRL_IBRS))
> > +		WARN_ONCE(1, "SPEC_CTRL MSR value 0x%16llx is unknown.\n", val);
> > +	else
> > +		wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base | val);
> > +}
> > +EXPORT_SYMBOL_GPL(x86_set_spec_ctrl);
> > +
> > +u64 x86_get_spec_ctrl(void)
> > +{
> > +	return x86_spec_ctrl_base;
> > +}
> > +EXPORT_SYMBOL_GPL(x86_get_spec_ctrl);
> 
> I actually don't like the above (I killed it last time too ;) ) because
> the accessors don't quite do what someone might think who hadn't read it
> carefully. What we want to do is cache the base MSR and then toggle on
> top of it. That's what the above do. But that means the "get" function
> doesn't actually give you the current hardware value, only the cached
> boot time value and that might differ due to IBRS status, etc.

Let me change it to 'x86_get_default_spec_ctrl' ?
> 
> [ Now, I personally care because in RHEL we've a lot of very different
> code for IBRS and it's actually a lot cleaner to leave the direct
> export, which is what I am currently doing in our own codebase ]
> 
> But I suspect others will fall into traps with the above too.
> 
> Jon.
> 
> -- 
> Computer Architect | Sent from my Fedora powered laptop
> 

[MODERATED] Re: [PATCH v4 03/10] [PATCH v4 3/9] Linux Patch #3

[Jon Masters]

[-- Attachment #1: Type: text/plain, Size: 1646 bytes --]

On 04/25/2018 11:47 AM, speck for Konrad Rzeszutek Wilk wrote:
> On Tue, Apr 24, 2018 at 05:43:23PM -0400, speck for Jon Masters wrote:
>> On 04/23/2018 11:16 PM, speck for konrad.wilk_at_oracle.com wrote:
>>
>>> +u64 x86_spec_ctrl_base;
>>> +
>>> +static void __init spec_ctrl_save_msr(void)
>>> +{
>>> +	/*
>>> +	 * Read the SPEC_CTRL MSR to account for reserved bits which may have
>>> +	 * unknown values.
>>> +	 */
>>> +	if (boot_cpu_has(X86_FEATURE_IBRS))
>>> +		rdmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
>>> +}
>>> +
>>> +void x86_set_spec_ctrl(u64 val)
>>> +{
>>> +	if (val & ~(SPEC_CTRL_IBRS))
>>> +		WARN_ONCE(1, "SPEC_CTRL MSR value 0x%16llx is unknown.\n", val);
>>> +	else
>>> +		wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base | val);
>>> +}
>>> +EXPORT_SYMBOL_GPL(x86_set_spec_ctrl);
>>> +
>>> +u64 x86_get_spec_ctrl(void)
>>> +{
>>> +	return x86_spec_ctrl_base;
>>> +}
>>> +EXPORT_SYMBOL_GPL(x86_get_spec_ctrl);
>>
>> I actually don't like the above (I killed it last time too ;) ) because
>> the accessors don't quite do what someone might think who hadn't read it
>> carefully. What we want to do is cache the base MSR and then toggle on
>> top of it. That's what the above do. But that means the "get" function
>> doesn't actually give you the current hardware value, only the cached
>> boot time value and that might differ due to IBRS status, etc.
> 
> Let me change it to 'x86_get_default_spec_ctrl' ?

Ok ;) That'll be better for upstream. For downstream, it's a giant mess
already and I have a plan ;)

Jon.

-- 
Computer Architect | Sent from my Fedora powered laptop

[MODERATED] Re: [PATCH v4 03/10] [PATCH v4 3/9] Linux Patch #3

[Konrad Rzeszutek Wilk]

..snip..

> @@ -248,12 +259,14 @@ static inline void vmexit_fill_RSB(void)
>  				 "movl $0, %%edx\n\t"		\
>  				 "wrmsr",			\
>  				 _feature)			\
> -		     : : [msr] "i" (_msr), [val] "i" (_val)	\
> +		     : : [msr] "i" (_msr), [val] "m" (_val)	\
>  		     : "eax", "ecx", "edx", "memory")
>  
>  static inline void indirect_branch_prediction_barrier(void)
>  {
> -	alternative_msr_write(MSR_IA32_PRED_CMD, PRED_CMD_IBPB,
> +	u64 val = PRED_CMD_IBPB;
> +
> +	alternative_msr_write(MSR_IA32_PRED_CMD, val,
>  			      X86_FEATURE_USE_IBPB);
>  }
>  
> @@ -265,14 +278,18 @@ static inline void indirect_branch_prediction_barrier(void)
>   */
>  #define firmware_restrict_branch_speculation_start()			\
>  do {									\
> +	u64 val = x86_get_spec_ctrl() | SPEC_CTRL_IBRS;			\
> +									\
>  	preempt_disable();						\
> -	alternative_msr_write(MSR_IA32_SPEC_CTRL, SPEC_CTRL_IBRS,	\
> +	alternative_msr_write(MSR_IA32_SPEC_CTRL, val,			\
>  			      X86_FEATURE_USE_IBRS_FW);			\
>  } while (0)
>  
>  #define firmware_restrict_branch_speculation_end()			\
>  do {									\
> -	alternative_msr_write(MSR_IA32_SPEC_CTRL, 0,			\
> +	u64 val = x86_get_spec_ctrl();					\
> +									\
> +	alternative_msr_write(MSR_IA32_SPEC_CTRL, val,			\
>  			      X86_FEATURE_USE_IBRS_FW);			\
>  	preempt_enable();						\

In v3 this was an extern variable instead of a function. The reason
we had it as an variable is that if you compile with EFI enabled you get
this:

drivers/firmware/efi/runtime-wrappers.o: In function `virt_efi_query_capsule_caps':
runtime-wrappers.c:(.text+0xe8): undefined reference to `x86_get_default_spec_ctrl'
runtime-wrappers.c:(.text+0x151): undefined reference to `x86_get_default_spec_ctrl'
drivers/firmware/efi/runtime-wrappers.o: In function `virt_efi_update_capsule':
runtime-wrappers.c:(.text+0x355): undefined reference to `x86_get_default_spec_ctrl'
runtime-wrappers.c:(.text+0x3b9): undefined reference to `x86_get_default_spec_ctrl'
drivers/firmware/efi/runtime-wrappers.o: In function `virt_efi_query_variable_info_nonblocking.part.3':
runtime-wrappers.c:(.text+0x595): undefined reference to `x86_get_default_spec_ctrl'
drivers/firmware/efi/runtime-wrappers.o:runtime-wrappers.c:(.text+0x602): more undefined references to `x86_get_default_spec_ctrl' follow

Ideas?