ACK/Cmnt: [SRU][Bionic][PATCH 0/2] Fixes for LP:1773162

Message ID b7a40c6d-f890-fc06-5c90-0e95884ed2ed@canonical.com
State New
Headers show
Series
  • ACK/Cmnt: [SRU][Bionic][PATCH 0/2] Fixes for LP:1773162
Related show

Commit Message

Stefan Bader June 6, 2018, 6:50 p.m.
On 25.05.2018 15:39, Joseph Salisbury wrote:
> BugLink: http://bugs.launchpad.net/bugs/1773162
> 
> == SRU Justification ==
> IBM reports that 4.15.0-22-generic fails to boot on IBM S822LC (POWER8 
> (raw), altivec supported).  The system then gets stuck in a reboot
> cycle.
> 
> It was found that Bionic commit 06f7e3d39f2f was the cause of this
> regression.  IBM believes the backport of this commit for CVE-2018-3639 (powerpc) was
> done incorrectly.  
> 
> I reverted the backport of commit 06f7e3d39f2f.  I then performed a new
> backport of mainline commit a048a07d7f45.  I built a test kernel for IBM
> who tested this kernel.  It was reported this new backport resolves the
> bug.
> 
> == Fixes ==
> Revert "powerpc/64s: Add support for a store forwarding barrier at kernel entry/exit"
> a048a07d7f45 ("powerpc/64s: Add support for a store forwarding barrier at kernel entry/exit")
> 
> == Regression Potential ==
> Low.  Fixes a regression and limited to powerpc.
> 
> == Test Case ==
> A test kernel was built with the revert and second backport. IBM tested
> this new patch and stated the test kernel resolved the bug.
> 
> Joseph Salisbury (1):
>   Revert "powerpc/64s: Add support for a store forwarding barrier at
>     kernel entry/exit"
> 
> Nicholas Piggin (1):
>   powerpc/64s: Add support for a store forwarding barrier at kernel
>     entry/exit
> 
>  arch/powerpc/include/asm/exception-64s.h     |   6 +-
>  arch/powerpc/include/asm/security_features.h |  11 ++
>  arch/powerpc/include/asm/setup.h             |  12 +--
>  arch/powerpc/kernel/security.c               | 149 +++++++++++++++++++++++++++
>  arch/powerpc/kernel/setup_64.c               | 119 +--------------------
>  arch/powerpc/lib/feature-fixups.c            |  16 ++-
>  arch/powerpc/platforms/powernv/setup.c       |   2 +-
>  7 files changed, 183 insertions(+), 132 deletions(-)
> 
Looking at the complete delta (revert + new patch applied) it rather feels like
the patch we had was some older variant of the current one. But given this got
successful testing:

Acked-by: Stefan Bader <stefan.bader@canonical.com>

Patch

diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
index d89411a..48732b1 100644
--- a/arch/powerpc/include/asm/exception-64s.h
+++ b/arch/powerpc/include/asm/exception-64s.h
@@ -76,9 +76,9 @@ 
 
 #define STF_ENTRY_BARRIER_SLOT						\
 	STF_ENTRY_BARRIER_FIXUP_SECTION;				\
-	mflr	r10;							\
-	bl	stf_barrier_fallback;					\
-	mtlr	r10
+	nop;								\
+	nop;								\
+	nop
 
 #define STF_EXIT_BARRIER_SLOT						\
 	STF_EXIT_BARRIER_FIXUP_SECTION;					\
diff --git a/arch/powerpc/include/asm/security_features.h b/arch/powerpc/include/asm/security_features.h
index 400a905..33c5d1f 100644
--- a/arch/powerpc/include/asm/security_features.h
+++ b/arch/powerpc/include/asm/security_features.h
@@ -12,6 +12,17 @@ 
 extern unsigned long powerpc_security_features;
 extern bool rfi_flush;
 
+/* These are bit flags */
+enum stf_barrier_type {
+	STF_BARRIER_NONE	= 0x1,
+	STF_BARRIER_FALLBACK	= 0x2,
+	STF_BARRIER_EIEIO	= 0x4,
+	STF_BARRIER_SYNC_ORI	= 0x8,
+};
+
+void setup_stf_barrier(void);
+void do_stf_barrier_fixups(enum stf_barrier_type types);
+
 static inline void security_ftr_set(unsigned long feature)
 {
 	powerpc_security_features |= feature;
diff --git a/arch/powerpc/include/asm/setup.h b/arch/powerpc/include/asm/setup.h
index 39baa94..bbcdf929 100644
--- a/arch/powerpc/include/asm/setup.h
+++ b/arch/powerpc/include/asm/setup.h
@@ -39,17 +39,9 @@  static inline void pseries_big_endian_exceptions(void) {}
 static inline void pseries_little_endian_exceptions(void) {}
 #endif /* CONFIG_PPC_PSERIES */
 
-/* These are bit flags */
-enum stf_barrier_type {
-	STF_BARRIER_NONE	= 0x1,
-	STF_BARRIER_FALLBACK	= 0x2,
-	STF_BARRIER_EIEIO	= 0x4,
-	STF_BARRIER_SYNC_ORI	= 0x8,
-};
-
-void setup_stf_barrier(void);
-void do_stf_barrier_fixups(enum stf_barrier_type types);
+void rfi_flush_enable(bool enable);
 
+/* These are bit flags */
 enum l1d_flush_type {
 	L1D_FLUSH_NONE		= 0x1,
 	L1D_FLUSH_FALLBACK	= 0x2,
diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
index 2cee3dc..60402d7 100644
--- a/arch/powerpc/kernel/security.c
+++ b/arch/powerpc/kernel/security.c
@@ -8,6 +8,7 @@ 
 #include <linux/device.h>
 #include <linux/seq_buf.h>
 
+#include <asm/debugfs.h>
 #include <asm/security_features.h>
 
 
@@ -91,3 +92,151 @@  ssize_t cpu_show_spectre_v2(struct device *dev, struct device_attribute *attr, c
 
 	return s.len;
 }
+
+/*
+ * Store-forwarding barrier support.
+ */
+
+static enum stf_barrier_type stf_enabled_flush_types;
+static bool no_stf_barrier;
+bool stf_barrier;
+
+static int __init handle_no_stf_barrier(char *p)
+{
+	pr_info("stf-barrier: disabled on command line.");
+	no_stf_barrier = true;
+	return 0;
+}
+
+early_param("no_stf_barrier", handle_no_stf_barrier);
+
+/* This is the generic flag used by other architectures */
+static int __init handle_ssbd(char *p)
+{
+	if (!p || strncmp(p, "auto", 5) == 0 || strncmp(p, "on", 2) == 0 ) {
+		/* Until firmware tells us, we have the barrier with auto */
+		return 0;
+	} else if (strncmp(p, "off", 3) == 0) {
+		handle_no_stf_barrier(NULL);
+		return 0;
+	} else
+		return 1;
+
+	return 0;
+}
+early_param("spec_store_bypass_disable", handle_ssbd);
+
+/* This is the generic flag used by other architectures */
+static int __init handle_no_ssbd(char *p)
+{
+	handle_no_stf_barrier(NULL);
+	return 0;
+}
+early_param("nospec_store_bypass_disable", handle_no_ssbd);
+
+static void stf_barrier_enable(bool enable)
+{
+	if (enable)
+		do_stf_barrier_fixups(stf_enabled_flush_types);
+	else
+		do_stf_barrier_fixups(STF_BARRIER_NONE);
+
+	stf_barrier = enable;
+}
+
+void setup_stf_barrier(void)
+{
+	enum stf_barrier_type type;
+	bool enable, hv;
+
+	hv = cpu_has_feature(CPU_FTR_HVMODE);
+
+	/* Default to fallback in case fw-features are not available */
+	if (cpu_has_feature(CPU_FTR_ARCH_300))
+		type = STF_BARRIER_EIEIO;
+	else if (cpu_has_feature(CPU_FTR_ARCH_207S))
+		type = STF_BARRIER_SYNC_ORI;
+	else if (cpu_has_feature(CPU_FTR_ARCH_206))
+		type = STF_BARRIER_FALLBACK;
+	else
+		type = STF_BARRIER_NONE;
+
+	enable = security_ftr_enabled(SEC_FTR_FAVOUR_SECURITY) &&
+		(security_ftr_enabled(SEC_FTR_L1D_FLUSH_PR) ||
+		 (security_ftr_enabled(SEC_FTR_L1D_FLUSH_HV) && hv));
+
+	if (type == STF_BARRIER_FALLBACK) {
+		pr_info("stf-barrier: fallback barrier available\n");
+	} else if (type == STF_BARRIER_SYNC_ORI) {
+		pr_info("stf-barrier: hwsync barrier available\n");
+	} else if (type == STF_BARRIER_EIEIO) {
+		pr_info("stf-barrier: eieio barrier available\n");
+	}
+
+	stf_enabled_flush_types = type;
+
+	if (!no_stf_barrier)
+		stf_barrier_enable(enable);
+}
+
+ssize_t cpu_show_spec_store_bypass(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	if (stf_barrier && stf_enabled_flush_types != STF_BARRIER_NONE) {
+		const char *type;
+		switch (stf_enabled_flush_types) {
+		case STF_BARRIER_EIEIO:
+			type = "eieio";
+			break;
+		case STF_BARRIER_SYNC_ORI:
+			type = "hwsync";
+			break;
+		case STF_BARRIER_FALLBACK:
+			type = "fallback";
+			break;
+		default:
+			type = "unknown";
+		}
+		return sprintf(buf, "Mitigation: Kernel entry/exit barrier (%s)\n", type);
+	}
+
+	if (!security_ftr_enabled(SEC_FTR_L1D_FLUSH_HV) &&
+	    !security_ftr_enabled(SEC_FTR_L1D_FLUSH_PR))
+		return sprintf(buf, "Not affected\n");
+
+	return sprintf(buf, "Vulnerable\n");
+}
+
+#ifdef CONFIG_DEBUG_FS
+static int stf_barrier_set(void *data, u64 val)
+{
+	bool enable;
+
+	if (val == 1)
+		enable = true;
+	else if (val == 0)
+		enable = false;
+	else
+		return -EINVAL;
+
+	/* Only do anything if we're changing state */
+	if (enable != stf_barrier)
+		stf_barrier_enable(enable);
+
+	return 0;
+}
+
+static int stf_barrier_get(void *data, u64 *val)
+{
+	*val = stf_barrier ? 1 : 0;
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(fops_stf_barrier, stf_barrier_get, stf_barrier_set, "%llu\n");
+
+static __init int stf_barrier_debugfs_init(void)
+{
+	debugfs_create_file("stf_barrier", 0600, powerpc_debugfs_root, NULL, &fops_stf_barrier);
+	return 0;
+}
+device_initcall(stf_barrier_debugfs_init);
+#endif /* CONFIG_DEBUG_FS */
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 288389e..b8e2b5c 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -69,7 +69,6 @@ 
 #include <asm/livepatch.h>
 #include <asm/opal.h>
 #include <asm/cputhreads.h>
-#include <asm/security_features.h>
 
 #include "setup.h"
 
@@ -805,48 +804,11 @@  static int __init disable_hardlockup_detector(void)
 early_initcall(disable_hardlockup_detector);
 
 #ifdef CONFIG_PPC_BOOK3S_64
-static enum stf_barrier_type stf_enabled_flush_types;
-static bool no_stf_barrier;
-bool stf_barrier;
-
-static enum l1d_flush_type rfi_enabled_flush_types;
+static enum l1d_flush_type enabled_flush_types;
 static void *l1d_flush_fallback_area;
 static bool no_rfi_flush;
 bool rfi_flush;
 
-static int __init handle_no_stf_barrier(char *p)
-{
-	pr_info("stf-barrier: disabled on command line.");
-	no_stf_barrier = true;
-	return 0;
-}
-
-early_param("no_stf_barrier", handle_no_stf_barrier);
-
-/* This is the generic flag used by other architectures */
-static int __init handle_ssbd(char *p)
-{
-	if (!p || strncmp(p, "off", 3) == 0) {
-		handle_no_stf_barrier(NULL);
-		return 0;
-	} else if (strncmp(p, "auto", 5) == 0 || strncmp(p, "on", 2) == 0 )
-		/* Until firmware tells us, we have the barrier with auto */
-		return 0;
-	else
-		return 1;
-
-	return 0;
-}
-early_param("spec_store_bypass_disable", handle_ssbd);
-
-/* This is the generic flag used by other architectures */
-static int __init handle_no_ssbd(char *p)
-{
-	handle_no_stf_barrier(NULL);
-	return 0;
-}
-early_param("nospec_store_bypass_disable", handle_no_ssbd);
-
 static int __init handle_no_rfi_flush(char *p)
 {
 	pr_info("rfi-flush: disabled on command line.");
@@ -875,21 +837,10 @@  static void do_nothing(void *unused)
 	 */
 }
 
-static void stf_barrier_enable(bool enable)
-{
-	if (enable) {
-		do_stf_barrier_fixups(stf_enabled_flush_types);
-		on_each_cpu(do_nothing, NULL, 1);
-	} else
-		do_stf_barrier_fixups(STF_BARRIER_NONE);
-
-	stf_barrier = enable;
-}
-
-static void rfi_flush_enable(bool enable)
+void rfi_flush_enable(bool enable)
 {
 	if (enable) {
-		do_rfi_flush_fixups(rfi_enabled_flush_types);
+		do_rfi_flush_fixups(enabled_flush_types);
 		on_each_cpu(do_nothing, NULL, 1);
 	} else
 		do_rfi_flush_fixups(L1D_FLUSH_NONE);
@@ -897,41 +848,6 @@  static void rfi_flush_enable(bool enable)
 	rfi_flush = enable;
 }
 
-void setup_stf_barrier(void)
-{
-	enum stf_barrier_type type;
-	bool enable, hv;
-
-	hv = cpu_has_feature(CPU_FTR_HVMODE);
-
-	/* Default to fallback in case fw-features are not available */
-	if (cpu_has_feature(CPU_FTR_ARCH_300))
-		type = STF_BARRIER_EIEIO;
-	else if (cpu_has_feature(CPU_FTR_ARCH_207S))
-		type = STF_BARRIER_SYNC_ORI;
-	else if (cpu_has_feature(CPU_FTR_ARCH_206))
-		type = STF_BARRIER_FALLBACK;
-	else
-		type = STF_BARRIER_NONE;
-
-	enable = security_ftr_enabled(SEC_FTR_FAVOUR_SECURITY) &&
-		(security_ftr_enabled(SEC_FTR_L1D_FLUSH_PR) ||
-		 (security_ftr_enabled(SEC_FTR_L1D_FLUSH_HV) && hv));
-
-	if (type == STF_BARRIER_FALLBACK) {
-		pr_info("stf-barrier: fallback barrier available\n");
-	} else if (type == STF_BARRIER_SYNC_ORI) {
-		pr_info("stf-barrier: hwsync barrier available\n");
-	} else if (type == STF_BARRIER_EIEIO) {
-		pr_info("stf-barrier: eieio barrier available\n");
-	}
-
-	stf_enabled_flush_types = type;
-
-	if (!no_stf_barrier)
-		stf_barrier_enable(enable);
-}
-
 static void init_fallback_flush(void)
 {
 	u64 l1d_size, limit;
@@ -982,39 +898,13 @@  void setup_rfi_flush(enum l1d_flush_type types, bool enable)
 	if (types & L1D_FLUSH_MTTRIG)
 		pr_info("rfi-flush: mttrig type flush available\n");
 
-	rfi_enabled_flush_types = types;
+	enabled_flush_types = types;
 
 	if (!no_rfi_flush)
 		rfi_flush_enable(enable);
 }
 
 #ifdef CONFIG_DEBUG_FS
-static int stf_barrier_set(void *data, u64 val)
-{
-	bool enable;
-
-	if (val == 1)
-		enable = true;
-	else if (val == 0)
-		enable = false;
-	else
-		return -EINVAL;
-
-	/* Only do anything if we're changing state */
-	if (enable != stf_barrier)
-		stf_barrier_enable(enable);
-
-	return 0;
-}
-
-static int stf_barrier_get(void *data, u64 *val)
-{
-	*val = stf_barrier ? 1 : 0;
-	return 0;
-}
-
-DEFINE_SIMPLE_ATTRIBUTE(fops_stf_barrier, stf_barrier_get, stf_barrier_set, "%llu\n");
-
 static int rfi_flush_set(void *data, u64 val)
 {
 	bool enable;
@@ -1044,7 +934,6 @@  DEFINE_SIMPLE_ATTRIBUTE(fops_rfi_flush, rfi_flush_get, rfi_flush_set, "%llu\n");
 static __init int rfi_flush_debugfs_init(void)
 {
 	debugfs_create_file("rfi_flush", 0600, powerpc_debugfs_root, NULL, &fops_rfi_flush);
-	debugfs_create_file("stf_barrier", 0600, powerpc_debugfs_root, NULL, &fops_stf_barrier);
 	return 0;
 }
 device_initcall(rfi_flush_debugfs_init);
diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c
index 47373c6..762a899 100644
--- a/arch/powerpc/lib/feature-fixups.c
+++ b/arch/powerpc/lib/feature-fixups.c
@@ -23,6 +23,7 @@ 
 #include <asm/page.h>
 #include <asm/sections.h>
 #include <asm/setup.h>
+#include <asm/security_features.h>
 #include <asm/firmware.h>
 
 struct fixup_entry {
@@ -185,12 +186,21 @@  void do_stf_exit_barrier_fixups(enum stf_barrier_type types)
 
 	i = 0;
 	if (types & STF_BARRIER_FALLBACK || types & STF_BARRIER_SYNC_ORI) {
-		instrs[i++] = 0x7db243a6; /* mtsprg 2,r13	*/
-		instrs[i++] = 0x7db142a6; /* mfsprg r13,1	*/
+		if (cpu_has_feature(CPU_FTR_HVMODE)) {
+			instrs[i++] = 0x7db14ba6; /* mtspr 0x131, r13 (HSPRG1) */
+			instrs[i++] = 0x7db04aa6; /* mfspr r13, 0x130 (HSPRG0) */
+		} else {
+			instrs[i++] = 0x7db243a6; /* mtsprg 2,r13	*/
+			instrs[i++] = 0x7db142a6; /* mfsprg r13,1    */
+	        }
 		instrs[i++] = 0x7c0004ac; /* hwsync		*/
 		instrs[i++] = 0xe9ad0000; /* ld r13,0(r13)	*/
 		instrs[i++] = 0x63ff0000; /* ori 31,31,0 speculation barrier */
-		instrs[i++] = 0x7db242a6; /* mfsprg r13,2	*/
+		if (cpu_has_feature(CPU_FTR_HVMODE)) {
+			instrs[i++] = 0x7db14aa6; /* mfspr r13, 0x131 (HSPRG1) */
+		} else {
+			instrs[i++] = 0x7db242a6; /* mfsprg r13,2 */
+		}
 	} else if (types & STF_BARRIER_EIEIO) {
 		instrs[i++] = 0x7e0006ac; /* eieio + bit 6 hint */
 	}
diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
index 62ecc66..fc0412d 100644
--- a/arch/powerpc/platforms/powernv/setup.c
+++ b/arch/powerpc/platforms/powernv/setup.c
@@ -130,8 +130,8 @@  static void __init pnv_setup_arch(void)
 {
 	set_arch_panic_timeout(10, ARCH_PANIC_TIMEOUT);
 
-	setup_stf_barrier();
 	pnv_setup_rfi_flush();
+	setup_stf_barrier();
 
 	/* Initialize SMP */
 	pnv_smp_init();