[PATCH 1/6] powerpc/64s: Add barrier_nospec

[PATCH 1/6] powerpc/64s: Add barrier_nospec

[Michael Ellerman]

From: Michal Suchanek <msuchanek@suse.de>

A no-op form of ori (or immediate of 0 into r31 and the result stored
in r31) has been re-tasked as a speculation barrier. The instruction
only acts as a barrier on newer machines with appropriate firmware
support. On older CPUs it remains a harmless no-op.

Implement barrier_nospec using this instruction.

mpe: The semantics of the instruction are believed to be that it
prevents execution of subsequent instructions until preceding branches
have been fully resolved and are no longer executing speculatively.
There is no further documentation available at this time.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
mpe: Make it Book3S64 only, update comment & change log, add a
     memory clobber to the asm.
---
 arch/powerpc/include/asm/barrier.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
index c7c63959ba91..e582d2c88092 100644
--- a/arch/powerpc/include/asm/barrier.h
+++ b/arch/powerpc/include/asm/barrier.h
@@ -76,6 +76,21 @@ do {									\
 	___p1;								\
 })
 
+#ifdef CONFIG_PPC_BOOK3S_64
+/*
+ * Prevent execution of subsequent instructions until preceding branches have
+ * been fully resolved and are no longer executing speculatively.
+ */
+#define barrier_nospec_asm ori 31,31,0
+
+// This also acts as a compiler barrier due to the memory clobber.
+#define barrier_nospec() asm (stringify_in_c(barrier_nospec_asm) ::: "memory")
+
+#else /* !CONFIG_PPC_BOOK3S_64 */
+#define barrier_nospec_asm
+#define barrier_nospec()
+#endif
+
 #include <asm-generic/barrier.h>
 
 #endif /* _ASM_POWERPC_BARRIER_H */
-- 
2.14.1

[PATCH 2/6] powerpc/64s: Add support for ori barrier_nospec patching

[Michael Ellerman]

From: Michal Suchanek <msuchanek@suse.de>

Based on the RFI patching. This is required to be able to disable the
speculation barrier.

Only one barrier type is supported and it does nothing when the
firmware does not enable it. Also re-patching modules is not supported
So the only meaningful thing that can be done is patching out the
speculation barrier at boot when the user says it is not wanted.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/barrier.h        |  2 +-
 arch/powerpc/include/asm/feature-fixups.h |  9 +++++++++
 arch/powerpc/include/asm/setup.h          |  1 +
 arch/powerpc/kernel/security.c            |  9 +++++++++
 arch/powerpc/kernel/vmlinux.lds.S         |  7 +++++++
 arch/powerpc/lib/feature-fixups.c         | 27 +++++++++++++++++++++++++++
 6 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/barrier.h b/arch/powerpc/include/asm/barrier.h
index e582d2c88092..f67b3f6e36be 100644
--- a/arch/powerpc/include/asm/barrier.h
+++ b/arch/powerpc/include/asm/barrier.h
@@ -81,7 +81,7 @@ do {									\
  * Prevent execution of subsequent instructions until preceding branches have
  * been fully resolved and are no longer executing speculatively.
  */
-#define barrier_nospec_asm ori 31,31,0
+#define barrier_nospec_asm NOSPEC_BARRIER_FIXUP_SECTION; nop
 
 // This also acts as a compiler barrier due to the memory clobber.
 #define barrier_nospec() asm (stringify_in_c(barrier_nospec_asm) ::: "memory")
diff --git a/arch/powerpc/include/asm/feature-fixups.h b/arch/powerpc/include/asm/feature-fixups.h
index 1e82eb3caabd..86ac59e75f36 100644
--- a/arch/powerpc/include/asm/feature-fixups.h
+++ b/arch/powerpc/include/asm/feature-fixups.h
@@ -195,11 +195,20 @@ label##3:					       	\
 	FTR_ENTRY_OFFSET 951b-952b;			\
 	.popsection;
 
+#define NOSPEC_BARRIER_FIXUP_SECTION			\
+953:							\
+	.pushsection __barrier_nospec_fixup,"a";	\
+	.align 2;					\
+954:							\
+	FTR_ENTRY_OFFSET 953b-954b;			\
+	.popsection;
+
 
 #ifndef __ASSEMBLY__
 #include <linux/types.h>
 
 extern long __start___rfi_flush_fixup, __stop___rfi_flush_fixup;
+extern long __start___barrier_nospec_fixup, __stop___barrier_nospec_fixup;
 
 void apply_feature_fixups(void);
 void setup_feature_keys(void);
diff --git a/arch/powerpc/include/asm/setup.h b/arch/powerpc/include/asm/setup.h
index 27fa52ed6d00..afc7280cce3b 100644
--- a/arch/powerpc/include/asm/setup.h
+++ b/arch/powerpc/include/asm/setup.h
@@ -52,6 +52,7 @@ enum l1d_flush_type {
 
 void setup_rfi_flush(enum l1d_flush_type, bool enable);
 void do_rfi_flush_fixups(enum l1d_flush_type types);
+void do_barrier_nospec_fixups(bool enable);
 
 #endif /* !__ASSEMBLY__ */
 
diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
index bab5a27ea805..b963eae0b0a0 100644
--- a/arch/powerpc/kernel/security.c
+++ b/arch/powerpc/kernel/security.c
@@ -9,10 +9,19 @@
 #include <linux/seq_buf.h>
 
 #include <asm/security_features.h>
+#include <asm/setup.h>
 
 
 unsigned long powerpc_security_features __read_mostly = SEC_FTR_DEFAULT;
 
+static bool barrier_nospec_enabled;
+
+static void enable_barrier_nospec(bool enable)
+{
+	barrier_nospec_enabled = enable;
+	do_barrier_nospec_fixups(enable);
+}
+
 ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	bool thread_priv;
diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
index c8af90ff49f0..ff73f498568c 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -139,6 +139,13 @@ SECTIONS
 		*(__rfi_flush_fixup)
 		__stop___rfi_flush_fixup = .;
 	}
+
+	. = ALIGN(8);
+	__spec_barrier_fixup : AT(ADDR(__spec_barrier_fixup) - LOAD_OFFSET) {
+		__start___barrier_nospec_fixup = .;
+		*(__barrier_nospec_fixup)
+		__stop___barrier_nospec_fixup = .;
+	}
 #endif
 
 	EXCEPTION_TABLE(0)
diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c
index 288fe4f0db4e..093c1d2ea5fd 100644
--- a/arch/powerpc/lib/feature-fixups.c
+++ b/arch/powerpc/lib/feature-fixups.c
@@ -162,6 +162,33 @@ void do_rfi_flush_fixups(enum l1d_flush_type types)
 		(types &  L1D_FLUSH_MTTRIG)     ? "mttrig type"
 						: "unknown");
 }
+
+void do_barrier_nospec_fixups(bool enable)
+{
+	unsigned int instr, *dest;
+	long *start, *end;
+	int i;
+
+	start = PTRRELOC(&__start___barrier_nospec_fixup),
+	end = PTRRELOC(&__stop___barrier_nospec_fixup);
+
+	instr = 0x60000000; /* nop */
+
+	if (enable) {
+		pr_info("barrier-nospec: using ORI speculation barrier\n");
+		instr = 0x63ff0000; /* ori 31,31,0 speculation barrier */
+	}
+
+	for (i = 0; start < end; start++, i++) {
+		dest = (void *)start + *start;
+
+		pr_devel("patching dest %lx\n", (unsigned long)dest);
+		patch_instruction(dest, instr);
+	}
+
+	printk(KERN_DEBUG "barrier-nospec: patched %d locations\n", i);
+}
+
 #endif /* CONFIG_PPC_BOOK3S_64 */
 
 void do_lwsync_fixups(unsigned long value, void *fixup_start, void *fixup_end)
-- 
2.14.1

[PATCH 3/6] powerpc/64s: Patch barrier_nospec in modules

[Michael Ellerman]

From: Michal Suchanek <msuchanek@suse.de>

Note that unlike RFI which is patched only in kernel the nospec state
reflects settings at the time the module was loaded.

Iterating all modules and re-patching every time the settings change
is not implemented.

Based on lwsync patching.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/setup.h  |  6 ++++++
 arch/powerpc/kernel/module.c      |  6 ++++++
 arch/powerpc/lib/feature-fixups.c | 16 +++++++++++++---
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/setup.h b/arch/powerpc/include/asm/setup.h
index afc7280cce3b..4335cddc1cf2 100644
--- a/arch/powerpc/include/asm/setup.h
+++ b/arch/powerpc/include/asm/setup.h
@@ -54,6 +54,12 @@ void setup_rfi_flush(enum l1d_flush_type, bool enable);
 void do_rfi_flush_fixups(enum l1d_flush_type types);
 void do_barrier_nospec_fixups(bool enable);
 
+#ifdef CONFIG_PPC_BOOK3S_64
+void do_barrier_nospec_fixups_range(bool enable, void *start, void *end);
+#else
+static inline void do_barrier_nospec_fixups_range(bool enable, void *start, void *end) { };
+#endif
+
 #endif /* !__ASSEMBLY__ */
 
 #endif	/* _ASM_POWERPC_SETUP_H */
diff --git a/arch/powerpc/kernel/module.c b/arch/powerpc/kernel/module.c
index 3f7ba0f5bf29..a72698cd3dd0 100644
--- a/arch/powerpc/kernel/module.c
+++ b/arch/powerpc/kernel/module.c
@@ -72,6 +72,12 @@ int module_finalize(const Elf_Ehdr *hdr,
 		do_feature_fixups(powerpc_firmware_features,
 				  (void *)sect->sh_addr,
 				  (void *)sect->sh_addr + sect->sh_size);
+
+	sect = find_section(hdr, sechdrs, "__spec_barrier_fixup");
+	if (sect != NULL)
+		do_barrier_nospec_fixups_range(true,
+				  (void *)sect->sh_addr,
+				  (void *)sect->sh_addr + sect->sh_size);
 #endif
 
 	sect = find_section(hdr, sechdrs, "__lwsync_fixup");
diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c
index 093c1d2ea5fd..3b37529f82f8 100644
--- a/arch/powerpc/lib/feature-fixups.c
+++ b/arch/powerpc/lib/feature-fixups.c
@@ -163,14 +163,14 @@ void do_rfi_flush_fixups(enum l1d_flush_type types)
 						: "unknown");
 }
 
-void do_barrier_nospec_fixups(bool enable)
+void do_barrier_nospec_fixups_range(bool enable, void *fixup_start, void *fixup_end)
 {
 	unsigned int instr, *dest;
 	long *start, *end;
 	int i;
 
-	start = PTRRELOC(&__start___barrier_nospec_fixup),
-	end = PTRRELOC(&__stop___barrier_nospec_fixup);
+	start = fixup_start;
+	end = fixup_end;
 
 	instr = 0x60000000; /* nop */
 
@@ -189,6 +189,16 @@ void do_barrier_nospec_fixups(bool enable)
 	printk(KERN_DEBUG "barrier-nospec: patched %d locations\n", i);
 }
 
+void do_barrier_nospec_fixups(bool enable)
+{
+	void *start, *end;
+
+	start = PTRRELOC(&__start___barrier_nospec_fixup),
+	end = PTRRELOC(&__stop___barrier_nospec_fixup);
+
+	do_barrier_nospec_fixups_range(enable, start, end);
+}
+
 #endif /* CONFIG_PPC_BOOK3S_64 */
 
 void do_lwsync_fixups(unsigned long value, void *fixup_start, void *fixup_end)
-- 
2.14.1

[PATCH 4/6] powerpc/64s: Enable barrier_nospec based on firmware settings

[Michael Ellerman]

From: Michal Suchanek <msuchanek@suse.de>

Check what firmware told us and enable/disable the barrier_nospec as
appropriate.

We err on the side of enabling the barrier, as it's no-op on older
systems, see the comment for more detail.

Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/setup.h       |  1 +
 arch/powerpc/kernel/security.c         | 60 ++++++++++++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/setup.c |  1 +
 arch/powerpc/platforms/pseries/setup.c |  1 +
 4 files changed, 63 insertions(+)

diff --git a/arch/powerpc/include/asm/setup.h b/arch/powerpc/include/asm/setup.h
index 4335cddc1cf2..aeb175e8a525 100644
--- a/arch/powerpc/include/asm/setup.h
+++ b/arch/powerpc/include/asm/setup.h
@@ -52,6 +52,7 @@ enum l1d_flush_type {
 
 void setup_rfi_flush(enum l1d_flush_type, bool enable);
 void do_rfi_flush_fixups(enum l1d_flush_type types);
+void setup_barrier_nospec(void);
 void do_barrier_nospec_fixups(bool enable);
 
 #ifdef CONFIG_PPC_BOOK3S_64
diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
index b963eae0b0a0..d1b9639e5e24 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>
 #include <asm/setup.h>
 
@@ -22,6 +23,65 @@ static void enable_barrier_nospec(bool enable)
 	do_barrier_nospec_fixups(enable);
 }
 
+void setup_barrier_nospec(void)
+{
+	bool enable;
+
+	/*
+	 * It would make sense to check SEC_FTR_SPEC_BAR_ORI31 below as well.
+	 * But there's a good reason not to. The two flags we check below are
+	 * both are enabled by default in the kernel, so if the hcall is not
+	 * functional they will be enabled.
+	 * On a system where the host firmware has been updated (so the ori
+	 * functions as a barrier), but on which the hypervisor (KVM/Qemu) has
+	 * not been updated, we would like to enable the barrier. Dropping the
+	 * check for SEC_FTR_SPEC_BAR_ORI31 achieves that. The only downside is
+	 * we potentially enable the barrier on systems where the host firmware
+	 * is not updated, but that's harmless as it's a no-op.
+	 */
+	enable = security_ftr_enabled(SEC_FTR_FAVOUR_SECURITY) &&
+		 security_ftr_enabled(SEC_FTR_BNDS_CHK_SPEC_BAR);
+
+	enable_barrier_nospec(enable);
+}
+
+#ifdef CONFIG_DEBUG_FS
+static int barrier_nospec_set(void *data, u64 val)
+{
+	switch (val) {
+	case 0:
+	case 1:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (!!val == !!barrier_nospec_enabled)
+		return 0;
+
+	enable_barrier_nospec(!!val);
+
+	return 0;
+}
+
+static int barrier_nospec_get(void *data, u64 *val)
+{
+	*val = barrier_nospec_enabled ? 1 : 0;
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(fops_barrier_nospec,
+			barrier_nospec_get, barrier_nospec_set, "%llu\n");
+
+static __init int barrier_nospec_debugfs_init(void)
+{
+	debugfs_create_file("barrier_nospec", 0600, powerpc_debugfs_root, NULL,
+			    &fops_barrier_nospec);
+	return 0;
+}
+device_initcall(barrier_nospec_debugfs_init);
+#endif /* CONFIG_DEBUG_FS */
+
 ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	bool thread_priv;
diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
index ef8c9ce53a61..e2ca5f77a55f 100644
--- a/arch/powerpc/platforms/powernv/setup.c
+++ b/arch/powerpc/platforms/powernv/setup.c
@@ -124,6 +124,7 @@ static void pnv_setup_rfi_flush(void)
 		  security_ftr_enabled(SEC_FTR_L1D_FLUSH_HV));
 
 	setup_rfi_flush(type, enable);
+	setup_barrier_nospec();
 }
 
 static void __init pnv_setup_arch(void)
diff --git a/arch/powerpc/platforms/pseries/setup.c b/arch/powerpc/platforms/pseries/setup.c
index b55ad4286dc7..63b1f0d10ef0 100644
--- a/arch/powerpc/platforms/pseries/setup.c
+++ b/arch/powerpc/platforms/pseries/setup.c
@@ -534,6 +534,7 @@ void pseries_setup_rfi_flush(void)
 		 security_ftr_enabled(SEC_FTR_L1D_FLUSH_PR);
 
 	setup_rfi_flush(types, enable);
+	setup_barrier_nospec();
 }
 
 #ifdef CONFIG_PCI_IOV
-- 
2.14.1

[PATCH 5/6] powerpc: Use barrier_nospec in copy_from_user()

[Michael Ellerman]

Based on the x86 commit doing the same.

See commit 304ec1b05031 ("x86/uaccess: Use __uaccess_begin_nospec()
and uaccess_try_nospec") and b3bbfb3fb5d2 ("x86: Introduce
__uaccess_begin_nospec() and uaccess_try_nospec") for more detail.

In all cases we are ordering the load from the potentially
user-controlled pointer vs a previous branch based on an access_ok()
check or similar.

Base on a patch from Michal Suchanek.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/uaccess.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index a62ee663b2c8..6dc3d2eeea4a 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -252,6 +252,7 @@ do {								\
 	__chk_user_ptr(ptr);					\
 	if (!is_kernel_addr((unsigned long)__gu_addr))		\
 		might_fault();					\
+	barrier_nospec();					\
 	__get_user_size(__gu_val, __gu_addr, (size), __gu_err);	\
 	(x) = (__typeof__(*(ptr)))__gu_val;			\
 	__gu_err;						\
@@ -263,8 +264,10 @@ do {								\
 	unsigned long  __gu_val = 0;					\
 	const __typeof__(*(ptr)) __user *__gu_addr = (ptr);		\
 	might_fault();							\
-	if (access_ok(VERIFY_READ, __gu_addr, (size)))			\
+	if (access_ok(VERIFY_READ, __gu_addr, (size))) {		\
+		barrier_nospec();					\
 		__get_user_size(__gu_val, __gu_addr, (size), __gu_err);	\
+	}								\
 	(x) = (__force __typeof__(*(ptr)))__gu_val;				\
 	__gu_err;							\
 })
@@ -275,6 +278,7 @@ do {								\
 	unsigned long __gu_val;					\
 	const __typeof__(*(ptr)) __user *__gu_addr = (ptr);	\
 	__chk_user_ptr(ptr);					\
+	barrier_nospec();					\
 	__get_user_size(__gu_val, __gu_addr, (size), __gu_err);	\
 	(x) = (__force __typeof__(*(ptr)))__gu_val;			\
 	__gu_err;						\
@@ -302,15 +306,19 @@ static inline unsigned long raw_copy_from_user(void *to,
 
 		switch (n) {
 		case 1:
+			barrier_nospec();
 			__get_user_size(*(u8 *)to, from, 1, ret);
 			break;
 		case 2:
+			barrier_nospec();
 			__get_user_size(*(u16 *)to, from, 2, ret);
 			break;
 		case 4:
+			barrier_nospec();
 			__get_user_size(*(u32 *)to, from, 4, ret);
 			break;
 		case 8:
+			barrier_nospec();
 			__get_user_size(*(u64 *)to, from, 8, ret);
 			break;
 		}
@@ -318,6 +326,7 @@ static inline unsigned long raw_copy_from_user(void *to,
 			return 0;
 	}
 
+	barrier_nospec();
 	return __copy_tofrom_user((__force void __user *)to, from, n);
 }
 
-- 
2.14.1

[PATCH 6/6] powerpc/64: Use barrier_nospec in syscall entry

[Michael Ellerman]

Our syscall entry is done in assembly so patch in an explicit
barrier_nospec.

Based on a patch by Michal Suchanek.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
mpe: Move the barrier to immediately prior to the vulnerable load,
     and add a comment trying to explain why. Drop the barrier from
     syscall_dotrace, because that syscall number comes from the
     kernel.
---
 arch/powerpc/kernel/entry_64.S | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 51695608c68b..de30f9a34c0c 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -36,6 +36,7 @@
 #include <asm/context_tracking.h>
 #include <asm/tm.h>
 #include <asm/ppc-opcode.h>
+#include <asm/barrier.h>
 #include <asm/export.h>
 #ifdef CONFIG_PPC_BOOK3S
 #include <asm/exception-64s.h>
@@ -178,6 +179,15 @@ system_call:			/* label this so stack traces look sane */
 	clrldi	r8,r8,32
 15:
 	slwi	r0,r0,4
+
+	barrier_nospec_asm
+	/*
+	 * Prevent the load of the handler below (based on the user-passed
+	 * system call number) being speculatively executed until the test
+	 * against NR_syscalls and branch to .Lsyscall_enosys above has
+	 * committed.
+	 */
+
 	ldx	r12,r11,r0	/* Fetch system call handler [ptr] */
 	mtctr   r12
 	bctrl			/* Call handler */
-- 
2.14.1

Re: [PATCH 1/6] powerpc/64s: Add barrier_nospec

[Nicholas Piggin]

On Tue, 24 Apr 2018 14:15:54 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> From: Michal Suchanek <msuchanek@suse.de>
> 
> A no-op form of ori (or immediate of 0 into r31 and the result stored
> in r31) has been re-tasked as a speculation barrier. The instruction
> only acts as a barrier on newer machines with appropriate firmware
> support. On older CPUs it remains a harmless no-op.
> 
> Implement barrier_nospec using this instruction.
> 
> mpe: The semantics of the instruction are believed to be that it
> prevents execution of subsequent instructions until preceding branches
> have been fully resolved and are no longer executing speculatively.
> There is no further documentation available at this time.
> 
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> mpe: Make it Book3S64 only, update comment & change log, add a
>      memory clobber to the asm.

These all seem good to me. Thanks Michal.

We should (eventually) work on the module patching problem too.

Thanks,
Nick

Re: [PATCH 4/6] powerpc/64s: Enable barrier_nospec based on firmware settings

[Michal Suchánek]

Hello,

On Tue, 24 Apr 2018 14:15:57 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> From: Michal Suchanek <msuchanek@suse.de>
> 
> Check what firmware told us and enable/disable the barrier_nospec as
> appropriate.
> 
> We err on the side of enabling the barrier, as it's no-op on older
> systems, see the comment for more detail.
> 
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/include/asm/setup.h       |  1 +
>  arch/powerpc/kernel/security.c         | 60
> ++++++++++++++++++++++++++++++++++
> arch/powerpc/platforms/powernv/setup.c |  1 +
> arch/powerpc/platforms/pseries/setup.c |  1 + 4 files changed, 63
> insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/setup.h
> b/arch/powerpc/include/asm/setup.h index 4335cddc1cf2..aeb175e8a525
> 100644 --- a/arch/powerpc/include/asm/setup.h
> +++ b/arch/powerpc/include/asm/setup.h
> @@ -52,6 +52,7 @@ enum l1d_flush_type {
>  
>  void setup_rfi_flush(enum l1d_flush_type, bool enable);
>  void do_rfi_flush_fixups(enum l1d_flush_type types);
> +void setup_barrier_nospec(void);
>  void do_barrier_nospec_fixups(bool enable);
>  
>  #ifdef CONFIG_PPC_BOOK3S_64
> diff --git a/arch/powerpc/kernel/security.c
> b/arch/powerpc/kernel/security.c index b963eae0b0a0..d1b9639e5e24
> 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>
>  #include <asm/setup.h>
>  
> @@ -22,6 +23,65 @@ static void enable_barrier_nospec(bool enable)
>  	do_barrier_nospec_fixups(enable);
>  }
>  
> +void setup_barrier_nospec(void)
> +{
> +	bool enable;
> +
> +	/*
> +	 * It would make sense to check SEC_FTR_SPEC_BAR_ORI31 below
> as well.
> +	 * But there's a good reason not to. The two flags we check
> below are
> +	 * both are enabled by default in the kernel, so if the
> hcall is not
> +	 * functional they will be enabled.
> +	 * On a system where the host firmware has been updated (so
> the ori
> +	 * functions as a barrier), but on which the hypervisor
> (KVM/Qemu) has
> +	 * not been updated, we would like to enable the barrier.
> Dropping the
> +	 * check for SEC_FTR_SPEC_BAR_ORI31 achieves that. The only
> downside is
> +	 * we potentially enable the barrier on systems where the
> host firmware
> +	 * is not updated, but that's harmless as it's a no-op.
> +	 */
> +	enable = security_ftr_enabled(SEC_FTR_FAVOUR_SECURITY) &&
> +		 security_ftr_enabled(SEC_FTR_BNDS_CHK_SPEC_BAR);
> +
> +	enable_barrier_nospec(enable);
> +}

I am missing the option for the barrier to be disabled by a kernel
commandline argument here.

It does make sense to add a kernel parameter that is checked on boot to
be compatible with other platforms that implement one.

Thanks

Michal

Re: [PATCH 2/6] powerpc/64s: Add support for ori barrier_nospec patching

[Michal Suchánek]

Hello,

On Tue, 24 Apr 2018 14:15:55 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> From: Michal Suchanek <msuchanek@suse.de>
> 
> Based on the RFI patching. This is required to be able to disable the
> speculation barrier.

why do you not patch the nospec barrier which is included as part of
the RFI flush code?

I think when debugging the code it would make more sense if RFI is
patched by RFI patcher and nospec by nospec patcher.

A separate question is if the RFI flush would break without the nospec
barrier.

Thanks

Michal


> 
> Only one barrier type is supported and it does nothing when the
> firmware does not enable it. Also re-patching modules is not supported
> So the only meaningful thing that can be done is patching out the
> speculation barrier at boot when the user says it is not wanted.
> 
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/include/asm/barrier.h        |  2 +-
>  arch/powerpc/include/asm/feature-fixups.h |  9 +++++++++
>  arch/powerpc/include/asm/setup.h          |  1 +
>  arch/powerpc/kernel/security.c            |  9 +++++++++
>  arch/powerpc/kernel/vmlinux.lds.S         |  7 +++++++
>  arch/powerpc/lib/feature-fixups.c         | 27
> +++++++++++++++++++++++++++ 6 files changed, 54 insertions(+), 1
> deletion(-)
> 
> diff --git a/arch/powerpc/include/asm/barrier.h
> b/arch/powerpc/include/asm/barrier.h index e582d2c88092..f67b3f6e36be
> 100644 --- a/arch/powerpc/include/asm/barrier.h
> +++ b/arch/powerpc/include/asm/barrier.h
> @@ -81,7 +81,7 @@ do
> {
> \
>   * Prevent execution of subsequent instructions until preceding
> branches have
>   * been fully resolved and are no longer executing speculatively.
>   */
> -#define barrier_nospec_asm ori 31,31,0
> +#define barrier_nospec_asm NOSPEC_BARRIER_FIXUP_SECTION; nop
>  
>  // This also acts as a compiler barrier due to the memory clobber.
>  #define barrier_nospec() asm (stringify_in_c(barrier_nospec_asm) :::
> "memory") diff --git a/arch/powerpc/include/asm/feature-fixups.h
> b/arch/powerpc/include/asm/feature-fixups.h index
> 1e82eb3caabd..86ac59e75f36 100644 ---
> a/arch/powerpc/include/asm/feature-fixups.h +++
> b/arch/powerpc/include/asm/feature-fixups.h @@ -195,11 +195,20 @@
> label##3:					       	\
> FTR_ENTRY_OFFSET 951b-952b;			\ .popsection;
>  
> +#define NOSPEC_BARRIER_FIXUP_SECTION			\
> +953:							\
> +	.pushsection __barrier_nospec_fixup,"a";	\
> +	.align 2;					\
> +954:							\
> +	FTR_ENTRY_OFFSET 953b-954b;			\
> +	.popsection;
> +
>  
>  #ifndef __ASSEMBLY__
>  #include <linux/types.h>
>  
>  extern long __start___rfi_flush_fixup, __stop___rfi_flush_fixup;
> +extern long __start___barrier_nospec_fixup,
> __stop___barrier_nospec_fixup; 
>  void apply_feature_fixups(void);
>  void setup_feature_keys(void);
> diff --git a/arch/powerpc/include/asm/setup.h
> b/arch/powerpc/include/asm/setup.h index 27fa52ed6d00..afc7280cce3b
> 100644 --- a/arch/powerpc/include/asm/setup.h
> +++ b/arch/powerpc/include/asm/setup.h
> @@ -52,6 +52,7 @@ enum l1d_flush_type {
>  
>  void setup_rfi_flush(enum l1d_flush_type, bool enable);
>  void do_rfi_flush_fixups(enum l1d_flush_type types);
> +void do_barrier_nospec_fixups(bool enable);
>  
>  #endif /* !__ASSEMBLY__ */
>  
> diff --git a/arch/powerpc/kernel/security.c
> b/arch/powerpc/kernel/security.c index bab5a27ea805..b963eae0b0a0
> 100644 --- a/arch/powerpc/kernel/security.c
> +++ b/arch/powerpc/kernel/security.c
> @@ -9,10 +9,19 @@
>  #include <linux/seq_buf.h>
>  
>  #include <asm/security_features.h>
> +#include <asm/setup.h>
>  
>  
>  unsigned long powerpc_security_features __read_mostly =
> SEC_FTR_DEFAULT; 
> +static bool barrier_nospec_enabled;
> +
> +static void enable_barrier_nospec(bool enable)
> +{
> +	barrier_nospec_enabled = enable;
> +	do_barrier_nospec_fixups(enable);
> +}
> +
>  ssize_t cpu_show_meltdown(struct device *dev, struct
> device_attribute *attr, char *buf) {
>  	bool thread_priv;
> diff --git a/arch/powerpc/kernel/vmlinux.lds.S
> b/arch/powerpc/kernel/vmlinux.lds.S index c8af90ff49f0..ff73f498568c
> 100644 --- a/arch/powerpc/kernel/vmlinux.lds.S
> +++ b/arch/powerpc/kernel/vmlinux.lds.S
> @@ -139,6 +139,13 @@ SECTIONS
>  		*(__rfi_flush_fixup)
>  		__stop___rfi_flush_fixup = .;
>  	}
> +
> +	. = ALIGN(8);
> +	__spec_barrier_fixup : AT(ADDR(__spec_barrier_fixup) -
> LOAD_OFFSET) {
> +		__start___barrier_nospec_fixup = .;
> +		*(__barrier_nospec_fixup)
> +		__stop___barrier_nospec_fixup = .;
> +	}
>  #endif
>  
>  	EXCEPTION_TABLE(0)
> diff --git a/arch/powerpc/lib/feature-fixups.c
> b/arch/powerpc/lib/feature-fixups.c index 288fe4f0db4e..093c1d2ea5fd
> 100644 --- a/arch/powerpc/lib/feature-fixups.c
> +++ b/arch/powerpc/lib/feature-fixups.c
> @@ -162,6 +162,33 @@ void do_rfi_flush_fixups(enum l1d_flush_type
> types) (types &  L1D_FLUSH_MTTRIG)     ? "mttrig type"
>  						: "unknown");
>  }
> +
> +void do_barrier_nospec_fixups(bool enable)
> +{
> +	unsigned int instr, *dest;
> +	long *start, *end;
> +	int i;
> +
> +	start = PTRRELOC(&__start___barrier_nospec_fixup),
> +	end = PTRRELOC(&__stop___barrier_nospec_fixup);
> +
> +	instr = 0x60000000; /* nop */
> +
> +	if (enable) {
> +		pr_info("barrier-nospec: using ORI speculation
> barrier\n");
> +		instr = 0x63ff0000; /* ori 31,31,0 speculation
> barrier */
> +	}
> +
> +	for (i = 0; start < end; start++, i++) {
> +		dest = (void *)start + *start;
> +
> +		pr_devel("patching dest %lx\n", (unsigned long)dest);
> +		patch_instruction(dest, instr);
> +	}
> +
> +	printk(KERN_DEBUG "barrier-nospec: patched %d locations\n",
> i); +}
> +
>  #endif /* CONFIG_PPC_BOOK3S_64 */
>  
>  void do_lwsync_fixups(unsigned long value, void *fixup_start, void
> *fixup_end)

Re: [PATCH 4/6] powerpc/64s: Enable barrier_nospec based on firmware settings

[Michael Ellerman]

Michal Suchánek <msuchanek@suse.de> writes:
> Hello,
>
> On Tue, 24 Apr 2018 14:15:57 +1000
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>
>> From: Michal Suchanek <msuchanek@suse.de>
>> 
>> Check what firmware told us and enable/disable the barrier_nospec as
>> appropriate.
>> 
>> We err on the side of enabling the barrier, as it's no-op on older
>> systems, see the comment for more detail.
>> 
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
...
>
> I am missing the option for the barrier to be disabled by a kernel
> commandline argument here.
>
> It does make sense to add a kernel parameter that is checked on boot to
> be compatible with other platforms that implement one.

No other platforms have an option to disable variant 1 mitigations, so
there isn't an existing parameter we can use.

Which is not to say we can't add one, but I wasn't sure if it was really
worth it.

But I guess we should add one, I might do it as a follow-up patch.

cheers

Re: [PATCH 4/6] powerpc/64s: Enable barrier_nospec based on firmware settings

[Michael Ellerman]

Michal Such=C3=A1nek <msuchanek@suse.de> writes:
> Hello,
>
> On Tue, 24 Apr 2018 14:15:57 +1000
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>
>> From: Michal Suchanek <msuchanek@suse.de>
>>=20
>> Check what firmware told us and enable/disable the barrier_nospec as
>> appropriate.
>>=20
>> We err on the side of enabling the barrier, as it's no-op on older
>> systems, see the comment for more detail.
>>=20
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
...
>
> I am missing the option for the barrier to be disabled by a kernel
> commandline argument here.
>
> It does make sense to add a kernel parameter that is checked on boot to
> be compatible with other platforms that implement one.

No other platforms have an option to disable variant 1 mitigations, so
there isn't an existing parameter we can use.

Which is not to say we can't add one, but I wasn't sure if it was really
worth it.

But I guess we should add one, I might do it as a follow-up patch.

cheers

Re: [PATCH 2/6] powerpc/64s: Add support for ori barrier_nospec patching

[Michael Ellerman]

Michal Suchánek <msuchanek@suse.de> writes:
> On Tue, 24 Apr 2018 14:15:55 +1000
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>
>> From: Michal Suchanek <msuchanek@suse.de>
>> 
>> Based on the RFI patching. This is required to be able to disable the
>> speculation barrier.
>
> why do you not patch the nospec barrier which is included as part of
> the RFI flush code?

We didn't want to put it in the asm like you had, because not all RFI
flush types need the spec barrier.

So it's more complicated than just patching in the spec barrier, we'd
need to only patch it in if the configured RFI flush also needed it.

> I think when debugging the code it would make more sense if RFI is
> patched by RFI patcher and nospec by nospec patcher.

True. I think what I'm saying is that the spec barrier in the RFI
flush is not a nospec barrier, it's part of that RFI flush type.

> A separate question is if the RFI flush would break without the nospec
> barrier.

The ORI flush requires it, but the others don't.

cheers

Re: [PATCH 2/6] powerpc/64s: Add support for ori barrier_nospec patching

[Michael Ellerman]

Michal Such=C3=A1nek <msuchanek@suse.de> writes:
> On Tue, 24 Apr 2018 14:15:55 +1000
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>
>> From: Michal Suchanek <msuchanek@suse.de>
>>=20
>> Based on the RFI patching. This is required to be able to disable the
>> speculation barrier.
>
> why do you not patch the nospec barrier which is included as part of
> the RFI flush code?

We didn't want to put it in the asm like you had, because not all RFI
flush types need the spec barrier.

So it's more complicated than just patching in the spec barrier, we'd
need to only patch it in if the configured RFI flush also needed it.

> I think when debugging the code it would make more sense if RFI is
> patched by RFI patcher and nospec by nospec patcher.

True. I think what I'm saying is that the spec barrier in the RFI
flush is not a nospec barrier, it's part of that RFI flush type.

> A separate question is if the RFI flush would break without the nospec
> barrier.

The ORI flush requires it, but the others don't.

cheers

Re: [PATCH 4/6] powerpc/64s: Enable barrier_nospec based on firmware settings

[Michal Suchánek]

On Tue, 01 May 2018 21:11:06 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Michal Suchánek <msuchanek@suse.de> writes:
> > Hello,
> >
> > On Tue, 24 Apr 2018 14:15:57 +1000
> > Michael Ellerman <mpe@ellerman.id.au> wrote:
> >  
> >> From: Michal Suchanek <msuchanek@suse.de>
> >> 
> >> Check what firmware told us and enable/disable the barrier_nospec
> >> as appropriate.
> >> 
> >> We err on the side of enabling the barrier, as it's no-op on older
> >> systems, see the comment for more detail.
> >> 
> >> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>  
> ...
> >
> > I am missing the option for the barrier to be disabled by a kernel
> > commandline argument here.
> >
> > It does make sense to add a kernel parameter that is checked on
> > boot to be compatible with other platforms that implement one.  
> 
> No other platforms have an option to disable variant 1 mitigations, so
> there isn't an existing parameter we can use.

Right, I was looking at an older implementation which turned off both
v1 and v2 with same parameter. In current kernel the v1 mitigation is
not turned off at all.
> 
> Which is not to say we can't add one, but I wasn't sure if it was
> really worth it.

The current thinking is that most performance relevant cases are
covered with array_nospec which has little overhead. The less code we
have for this the better ;-)

Thanks

Michal

Re: [PATCH 4/6] powerpc/64s: Enable barrier_nospec based on firmware settings

[Michal Suchánek]

On Tue, 01 May 2018 21:11:06 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Michal Such=C3=A1nek <msuchanek@suse.de> writes:
> > Hello,
> >
> > On Tue, 24 Apr 2018 14:15:57 +1000
> > Michael Ellerman <mpe@ellerman.id.au> wrote:
> > =20
> >> From: Michal Suchanek <msuchanek@suse.de>
> >>=20
> >> Check what firmware told us and enable/disable the barrier_nospec
> >> as appropriate.
> >>=20
> >> We err on the side of enabling the barrier, as it's no-op on older
> >> systems, see the comment for more detail.
> >>=20
> >> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> =20
> ...
> >
> > I am missing the option for the barrier to be disabled by a kernel
> > commandline argument here.
> >
> > It does make sense to add a kernel parameter that is checked on
> > boot to be compatible with other platforms that implement one. =20
>=20
> No other platforms have an option to disable variant 1 mitigations, so
> there isn't an existing parameter we can use.

Right, I was looking at an older implementation which turned off both
v1 and v2 with same parameter. In current kernel the v1 mitigation is
not turned off at all.
>=20
> Which is not to say we can't add one, but I wasn't sure if it was
> really worth it.

The current thinking is that most performance relevant cases are
covered with array_nospec which has little overhead. The less code we
have for this the better ;-)

Thanks

Michal

Re: [PATCH 3/6] powerpc/64s: Patch barrier_nospec in modules

[Michal Suchánek]

On Tue, 24 Apr 2018 14:15:56 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> From: Michal Suchanek <msuchanek@suse.de>
> 
> Note that unlike RFI which is patched only in kernel the nospec state
> reflects settings at the time the module was loaded.
> 
> Iterating all modules and re-patching every time the settings change
> is not implemented.
> 
> Based on lwsync patching.
> 
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
>  arch/powerpc/include/asm/setup.h  |  6 ++++++
>  arch/powerpc/kernel/module.c      |  6 ++++++
>  arch/powerpc/lib/feature-fixups.c | 16 +++++++++++++---
>  3 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/setup.h
> b/arch/powerpc/include/asm/setup.h index afc7280cce3b..4335cddc1cf2
> 100644 --- a/arch/powerpc/include/asm/setup.h
> +++ b/arch/powerpc/include/asm/setup.h
> @@ -54,6 +54,12 @@ void setup_rfi_flush(enum l1d_flush_type, bool
> enable); void do_rfi_flush_fixups(enum l1d_flush_type types);
>  void do_barrier_nospec_fixups(bool enable);
>  
> +#ifdef CONFIG_PPC_BOOK3S_64
> +void do_barrier_nospec_fixups_range(bool enable, void *start, void
> *end); +#else
> +static inline void do_barrier_nospec_fixups_range(bool enable, void
> *start, void *end) { }; +#endif
> +
>  #endif /* !__ASSEMBLY__ */
>  
>  #endif	/* _ASM_POWERPC_SETUP_H */
> diff --git a/arch/powerpc/kernel/module.c
> b/arch/powerpc/kernel/module.c index 3f7ba0f5bf29..a72698cd3dd0 100644
> --- a/arch/powerpc/kernel/module.c
> +++ b/arch/powerpc/kernel/module.c
> @@ -72,6 +72,12 @@ int module_finalize(const Elf_Ehdr *hdr,
>  		do_feature_fixups(powerpc_firmware_features,
>  				  (void *)sect->sh_addr,
>  				  (void *)sect->sh_addr +
> sect->sh_size); +
> +	sect = find_section(hdr, sechdrs, "__spec_barrier_fixup");
> +	if (sect != NULL)
> +		do_barrier_nospec_fixups_range(true,

This saves a global but always patches modules with the ORI
flush. The patching also produces a log message which can be confusing
to the user. Even when barrier is disabled "barrier-nospec: using ORI
speculation barrier" is logged when modules are loaded.

For consistency when debugging the settings I would suggest to keep the
export.

It is also no longer consistent with the commit message.

Thanks

Michal

> +				  (void *)sect->sh_addr,
> +				  (void *)sect->sh_addr +
> sect->sh_size); #endif
>  
>  	sect = find_section(hdr, sechdrs, "__lwsync_fixup");
> diff --git a/arch/powerpc/lib/feature-fixups.c
> b/arch/powerpc/lib/feature-fixups.c index 093c1d2ea5fd..3b37529f82f8
> 100644 --- a/arch/powerpc/lib/feature-fixups.c
> +++ b/arch/powerpc/lib/feature-fixups.c
> @@ -163,14 +163,14 @@ void do_rfi_flush_fixups(enum l1d_flush_type
> types) : "unknown");
>  }
>  
> -void do_barrier_nospec_fixups(bool enable)
> +void do_barrier_nospec_fixups_range(bool enable, void *fixup_start,
> void *fixup_end) {
>  	unsigned int instr, *dest;
>  	long *start, *end;
>  	int i;
>  
> -	start = PTRRELOC(&__start___barrier_nospec_fixup),
> -	end = PTRRELOC(&__stop___barrier_nospec_fixup);
> +	start = fixup_start;
> +	end = fixup_end;
>  
>  	instr = 0x60000000; /* nop */
>  
> @@ -189,6 +189,16 @@ void do_barrier_nospec_fixups(bool enable)
>  	printk(KERN_DEBUG "barrier-nospec: patched %d locations\n",
> i); }
>  
> +void do_barrier_nospec_fixups(bool enable)
> +{
> +	void *start, *end;
> +
> +	start = PTRRELOC(&__start___barrier_nospec_fixup),
> +	end = PTRRELOC(&__stop___barrier_nospec_fixup);
> +
> +	do_barrier_nospec_fixups_range(enable, start, end);
> +}
> +
>  #endif /* CONFIG_PPC_BOOK3S_64 */
>  
>  void do_lwsync_fixups(unsigned long value, void *fixup_start, void
> *fixup_end)

Re: [PATCH 4/6] powerpc/64s: Enable barrier_nospec based on firmware settings

[Michael Ellerman]

Michal Suchánek <msuchanek@suse.de> writes:
> On Tue, 01 May 2018 21:11:06 +1000
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Michal Suchánek <msuchanek@suse.de> writes:
>> > On Tue, 24 Apr 2018 14:15:57 +1000
>> > Michael Ellerman <mpe@ellerman.id.au> wrote:
>> >  
>> >> From: Michal Suchanek <msuchanek@suse.de>
>> >> 
>> >> Check what firmware told us and enable/disable the barrier_nospec
>> >> as appropriate.
>> >> 
>> >> We err on the side of enabling the barrier, as it's no-op on older
>> >> systems, see the comment for more detail.
>> >> 
>> >> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>  
>> ...
>> >
>> > I am missing the option for the barrier to be disabled by a kernel
>> > commandline argument here.
>> >
>> > It does make sense to add a kernel parameter that is checked on
>> > boot to be compatible with other platforms that implement one.  
>> 
>> No other platforms have an option to disable variant 1 mitigations, so
>> there isn't an existing parameter we can use.
>
> Right, I was looking at an older implementation which turned off both
> v1 and v2 with same parameter. In current kernel the v1 mitigation is
> not turned off at all.

Ah OK.

>> Which is not to say we can't add one, but I wasn't sure if it was
>> really worth it.
>
> The current thinking is that most performance relevant cases are
> covered with array_nospec which has little overhead. The less code we
> have for this the better ;-)

Amen to that :)

cheers

Re: [PATCH 4/6] powerpc/64s: Enable barrier_nospec based on firmware settings

[Michael Ellerman]

Michal Such=C3=A1nek <msuchanek@suse.de> writes:
> On Tue, 01 May 2018 21:11:06 +1000
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Michal Such=C3=A1nek <msuchanek@suse.de> writes:
>> > On Tue, 24 Apr 2018 14:15:57 +1000
>> > Michael Ellerman <mpe@ellerman.id.au> wrote:
>> >=20=20
>> >> From: Michal Suchanek <msuchanek@suse.de>
>> >>=20
>> >> Check what firmware told us and enable/disable the barrier_nospec
>> >> as appropriate.
>> >>=20
>> >> We err on the side of enabling the barrier, as it's no-op on older
>> >> systems, see the comment for more detail.
>> >>=20
>> >> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>=20=20
>> ...
>> >
>> > I am missing the option for the barrier to be disabled by a kernel
>> > commandline argument here.
>> >
>> > It does make sense to add a kernel parameter that is checked on
>> > boot to be compatible with other platforms that implement one.=20=20
>>=20
>> No other platforms have an option to disable variant 1 mitigations, so
>> there isn't an existing parameter we can use.
>
> Right, I was looking at an older implementation which turned off both
> v1 and v2 with same parameter. In current kernel the v1 mitigation is
> not turned off at all.

Ah OK.

>> Which is not to say we can't add one, but I wasn't sure if it was
>> really worth it.
>
> The current thinking is that most performance relevant cases are
> covered with array_nospec which has little overhead. The less code we
> have for this the better ;-)

Amen to that :)

cheers

[PATCH] powerpc/64s: Enhance the information in cpu_show_spectre_v1()

[Michal Suchanek]

We now have barrier_nospec as mitigation so print it in
cpu_show_spectre_v1 when enabled.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 arch/powerpc/kernel/security.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
index 0239383c7e4d..a0c32d53980b 100644
--- a/arch/powerpc/kernel/security.c
+++ b/arch/powerpc/kernel/security.c
@@ -120,7 +120,10 @@ ssize_t cpu_show_spectre_v1(struct device *dev, struct device_attribute *attr, c
 	if (!security_ftr_enabled(SEC_FTR_BNDS_CHK_SPEC_BAR))
 		return sprintf(buf, "Not affected\n");
 
-	return sprintf(buf, "Vulnerable\n");
+	if (barrier_nospec_enabled)
+		return sprintf(buf, "Mitigation: __user pointer sanitization\n");
+	else
+		return sprintf(buf, "Vulnerable\n");
 }
 
 ssize_t cpu_show_spectre_v2(struct device *dev, struct device_attribute *attr, char *buf)
-- 
2.13.6

Re: [PATCH] powerpc/64s: Enhance the information in cpu_show_spectre_v1()

[kbuild test robot]

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

Hi Michal,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.17-rc7 next-20180529]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Michal-Suchanek/powerpc-64s-Enhance-the-information-in-cpu_show_spectre_v1/20180529-181036
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   arch/powerpc/kernel/security.c: In function 'cpu_show_spectre_v1':
>> arch/powerpc/kernel/security.c:54:6: error: 'barrier_nospec_enabled' undeclared (first use in this function); did you mean 'kernfs_ns_enabled'?
     if (barrier_nospec_enabled)
         ^~~~~~~~~~~~~~~~~~~~~~
         kernfs_ns_enabled
   arch/powerpc/kernel/security.c:54:6: note: each undeclared identifier is reported only once for each function it appears in
   arch/powerpc/kernel/security.c:58:1: error: control reaches end of non-void function [-Werror=return-type]
    }
    ^
   cc1: all warnings being treated as errors

vim +54 arch/powerpc/kernel/security.c

    48	
    49	ssize_t cpu_show_spectre_v1(struct device *dev, struct device_attribute *attr, char *buf)
    50	{
    51		if (!security_ftr_enabled(SEC_FTR_BNDS_CHK_SPEC_BAR))
    52			return sprintf(buf, "Not affected\n");
    53	
  > 54		if (barrier_nospec_enabled)
    55			return sprintf(buf, "Mitigation: __user pointer sanitization\n");
    56		else
    57			return sprintf(buf, "Vulnerable\n");
    58	}
    59	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 23378 bytes --]

Re: [PATCH] powerpc/64s: Enhance the information in cpu_show_spectre_v1()

[Christophe LEROY]

Le 28/05/2018 à 15:19, Michal Suchanek a écrit :
> We now have barrier_nospec as mitigation so print it in
> cpu_show_spectre_v1 when enabled.
> 
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
>   arch/powerpc/kernel/security.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/security.c b/arch/powerpc/kernel/security.c
> index 0239383c7e4d..a0c32d53980b 100644
> --- a/arch/powerpc/kernel/security.c
> +++ b/arch/powerpc/kernel/security.c
> @@ -120,7 +120,10 @@ ssize_t cpu_show_spectre_v1(struct device *dev, struct device_attribute *attr, c
>   	if (!security_ftr_enabled(SEC_FTR_BNDS_CHK_SPEC_BAR))
>   		return sprintf(buf, "Not affected\n");
>   
> -	return sprintf(buf, "Vulnerable\n");
> +	if (barrier_nospec_enabled)

> +		return sprintf(buf, "Mitigation: __user pointer sanitization\n");
> +	else
> +		return sprintf(buf, "Vulnerable\n");

Checkpatch would tell you that an else is unneeded after a return. So 
just leave it as it was before.

Christophe

>   }
>   
>   ssize_t cpu_show_spectre_v2(struct device *dev, struct device_attribute *attr, char *buf)
> 

Re: [PATCH] powerpc/64s: Enhance the information in cpu_show_spectre_v1()

[Michal Suchánek]

On Tue, 29 May 2018 16:13:49 +0200
Christophe LEROY <christophe.leroy@c-s.fr> wrote:

> Le 28/05/2018 à 15:19, Michal Suchanek a écrit :
> > We now have barrier_nospec as mitigation so print it in
> > cpu_show_spectre_v1 when enabled.
> > 
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> >   arch/powerpc/kernel/security.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/kernel/security.c
> > b/arch/powerpc/kernel/security.c index 0239383c7e4d..a0c32d53980b
> > 100644 --- a/arch/powerpc/kernel/security.c
> > +++ b/arch/powerpc/kernel/security.c
> > @@ -120,7 +120,10 @@ ssize_t cpu_show_spectre_v1(struct device
> > *dev, struct device_attribute *attr, c if
> > (!security_ftr_enabled(SEC_FTR_BNDS_CHK_SPEC_BAR)) return
> > sprintf(buf, "Not affected\n"); 
> > -	return sprintf(buf, "Vulnerable\n");
> > +	if (barrier_nospec_enabled)  
> 
> > +		return sprintf(buf, "Mitigation: __user pointer
> > sanitization\n");
> > +	else
> > +		return sprintf(buf, "Vulnerable\n");  
> 
> Checkpatch would tell you that an else is unneeded after a return. So 
> just leave it as it was before.

Where did you get your copy of checkpatch? The one in Linux tree does
not do that.

Thanks

Michal

Re: [PATCH] powerpc/64s: Enhance the information in cpu_show_spectre_v1()

[Michal Suchánek]

On Tue, 29 May 2018 16:13:49 +0200
Christophe LEROY <christophe.leroy@c-s.fr> wrote:

> Le 28/05/2018 =C3=A0 15:19, Michal Suchanek a =C3=A9crit=C2=A0:
> > We now have barrier_nospec as mitigation so print it in
> > cpu_show_spectre_v1 when enabled.
> >=20
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> >   arch/powerpc/kernel/security.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> >=20
> > diff --git a/arch/powerpc/kernel/security.c
> > b/arch/powerpc/kernel/security.c index 0239383c7e4d..a0c32d53980b
> > 100644 --- a/arch/powerpc/kernel/security.c
> > +++ b/arch/powerpc/kernel/security.c
> > @@ -120,7 +120,10 @@ ssize_t cpu_show_spectre_v1(struct device
> > *dev, struct device_attribute *attr, c if
> > (!security_ftr_enabled(SEC_FTR_BNDS_CHK_SPEC_BAR)) return
> > sprintf(buf, "Not affected\n");=20
> > -	return sprintf(buf, "Vulnerable\n");
> > +	if (barrier_nospec_enabled) =20
>=20
> > +		return sprintf(buf, "Mitigation: __user pointer
> > sanitization\n");
> > +	else
> > +		return sprintf(buf, "Vulnerable\n"); =20
>=20
> Checkpatch would tell you that an else is unneeded after a return. So=20
> just leave it as it was before.

Where did you get your copy of checkpatch? The one in Linux tree does
not do that.

Thanks

Michal

Re: [PATCH] powerpc/64s: Enhance the information in cpu_show_spectre_v1()

[Christophe Leroy]

On 05/29/2018 02:46 PM, Michal Suchánek wrote:
> On Tue, 29 May 2018 16:13:49 +0200
> Christophe LEROY <christophe.leroy@c-s.fr> wrote:
> 
>> Le 28/05/2018 à 15:19, Michal Suchanek a écrit :
>>> We now have barrier_nospec as mitigation so print it in
>>> cpu_show_spectre_v1 when enabled.
>>>
>>> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
>>> ---
>>>    arch/powerpc/kernel/security.c | 5 ++++-
>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/powerpc/kernel/security.c
>>> b/arch/powerpc/kernel/security.c index 0239383c7e4d..a0c32d53980b
>>> 100644 --- a/arch/powerpc/kernel/security.c
>>> +++ b/arch/powerpc/kernel/security.c
>>> @@ -120,7 +120,10 @@ ssize_t cpu_show_spectre_v1(struct device
>>> *dev, struct device_attribute *attr, c if
>>> (!security_ftr_enabled(SEC_FTR_BNDS_CHK_SPEC_BAR)) return
>>> sprintf(buf, "Not affected\n");
>>> -	return sprintf(buf, "Vulnerable\n");
>>> +	if (barrier_nospec_enabled)
>>
>>> +		return sprintf(buf, "Mitigation: __user pointer
>>> sanitization\n");
>>> +	else
>>> +		return sprintf(buf, "Vulnerable\n");
>>
>> Checkpatch would tell you that an else is unneeded after a return. So
>> just leave it as it was before.
> 
> Where did you get your copy of checkpatch? The one in Linux tree does
> not do that.

Strange, it should, as checkpatch.pl includes the following code:

# check indentation of any line with a bare else
# (but not if it is a multiple line "if (foo) return bar; else return baz;")
# if the previous line is a break or return and is indented 1 tab more...
		if ($sline =~ /^\+([\t]+)(?:}[ \t]*)?else(?:[ \t]*{)?\s*$/) {
			my $tabs = length($1) + 1;
			if ($prevline =~ /^\+\t{$tabs,$tabs}break\b/ ||
			    ($prevline =~ /^\+\t{$tabs,$tabs}return\b/ &&
			     defined $lines[$linenr] &&
			     $lines[$linenr] !~ /^[ \+]\t{$tabs,$tabs}return/)) {
				WARN("UNNECESSARY_ELSE",
				     "else is not generally useful after a break or return\n" . 
$hereprev);
			}
		}


Anyway, you should remove that 'else' in your patch.
And the other sprintf line is over 80 characters.

Christophe

> 
> Thanks
> 
> Michal
> 

Re: [PATCH] powerpc/64s: Enhance the information in cpu_show_spectre_v1()

[Joe Perches]

On Tue, 2018-05-29 at 15:24 +0000, Christophe Leroy wrote:
> On 05/29/2018 02:46 PM, Michal Suchánek wrote:
> > On Tue, 29 May 2018 16:13:49 +0200 Christophe LEROY <christophe.leroy@c-s.fr> wrote:
[]
> > diff --git a/arch/powerpc/kernel/security.c
> > > > b/arch/powerpc/kernel/security.c index 0239383c7e4d..a0c32d53980b
> > > > 100644 --- a/arch/powerpc/kernel/security.c
> > > > +++ b/arch/powerpc/kernel/security.c
> > > > @@ -120,7 +120,10 @@ ssize_t cpu_show_spectre_v1(struct device
> > > > *dev, struct device_attribute *attr, c if
> > > > (!security_ftr_enabled(SEC_FTR_BNDS_CHK_SPEC_BAR)) return
> > > > sprintf(buf, "Not affected\n");
> > > > -	return sprintf(buf, "Vulnerable\n");
> > > > +	if (barrier_nospec_enabled)
> > > > +		return sprintf(buf, "Mitigation: __user pointer
> > > > sanitization\n");
> > > > +	else
> > > > +		return sprintf(buf, "Vulnerable\n");
> > > 
> > > Checkpatch would tell you that an else is unneeded after a return. So
> > > just leave it as it was before.
> > 
> > Where did you get your copy of checkpatch? The one in Linux tree does
> > not do that.

Correct as this particular style is a maintainer preference.

> Strange, it should, as checkpatch.pl includes the following code:
> 
> # check indentation of any line with a bare else
> # (but not if it is a multiple line "if (foo) return bar; else return baz;")

Note this comment and also that this case is

	if (foo)
		return bar;
	else
		return baz;

so no warning is generated.

> # if the previous line is a break or return and is indented 1 tab more...
> 		if ($sline =~ /^\+([\t]+)(?:}[ \t]*)?else(?:[ \t]*{)?\s*$/) {
> 			my $tabs = length($1) + 1;
> 			if ($prevline =~ /^\+\t{$tabs,$tabs}break\b/ ||
> 			    ($prevline =~ /^\+\t{$tabs,$tabs}return\b/ &&
> 			     defined $lines[$linenr] &&
> 			     $lines[$linenr] !~ /^[ \+]\t{$tabs,$tabs}return/)) {
> 				WARN("UNNECESSARY_ELSE",
> 				     "else is not generally useful after a break or return\n" . 
> $hereprev);
> 			}
> 		}
> 
> 
> Anyway, you should remove that 'else' in your patch.
> And the other sprintf line is over 80 characters.
> 
> Christophe
> 
> > 
> > Thanks
> > 
> > Michal
> > 

Re: [PATCH] powerpc/64s: Enhance the information in cpu_show_spectre_v1()

[Joe Perches]

On Tue, 2018-05-29 at 15:24 +0000, Christophe Leroy wrote:
> On 05/29/2018 02:46 PM, Michal Suchánek wrote:
> > On Tue, 29 May 2018 16:13:49 +0200 Christophe LEROY <christophe.leroy@c-s.fr> wrote:
[]
> > diff --git a/arch/powerpc/kernel/security.c
> > > > b/arch/powerpc/kernel/security.c index 0239383c7e4d..a0c32d53980b
> > > > 100644 --- a/arch/powerpc/kernel/security.c
> > > > +++ b/arch/powerpc/kernel/security.c
> > > > @@ -120,7 +120,10 @@ ssize_t cpu_show_spectre_v1(struct device
> > > > *dev, struct device_attribute *attr, c if
> > > > (!security_ftr_enabled(SEC_FTR_BNDS_CHK_SPEC_BAR)) return
> > > > sprintf(buf, "Not affected\n");
> > > > -	return sprintf(buf, "Vulnerable\n");
> > > > +	if (barrier_nospec_enabled)
> > > > +		return sprintf(buf, "Mitigation: __user pointer
> > > > sanitization\n");
> > > > +	else
> > > > +		return sprintf(buf, "Vulnerable\n");
> > > 
> > > Checkpatch would tell you that an else is unneeded after a return. So
> > > just leave it as it was before.
> > 
> > Where did you get your copy of checkpatch? The one in Linux tree does
> > not do that.

Correct as this particular style is a maintainer preference.

> Strange, it should, as checkpatch.pl includes the following code:
> 
> # check indentation of any line with a bare else
> # (but not if it is a multiple line "if (foo) return bar; else return baz;")

Note this comment and also that this case is

	if (foo)
		return bar;
	else
		return baz;

so no warning is generated.

> # if the previous line is a break or return and is indented 1 tab more...
> 		if ($sline =~ /^\+([\t]+)(?:}[ \t]*)?else(?:[ \t]*{)?\s*$/) {
> 			my $tabs = length($1) + 1;
> 			if ($prevline =~ /^\+\t{$tabs,$tabs}break\b/ ||
> 			    ($prevline =~ /^\+\t{$tabs,$tabs}return\b/ &&
> 			     defined $lines[$linenr] &&
> 			     $lines[$linenr] !~ /^[ \+]\t{$tabs,$tabs}return/)) {
> 				WARN("UNNECESSARY_ELSE",
> 				     "else is not generally useful after a break or return\n" . 
> $hereprev);
> 			}
> 		}
> 
> 
> Anyway, you should remove that 'else' in your patch.
> And the other sprintf line is over 80 characters.
> 
> Christophe
> 
> > 
> > Thanks
> > 
> > Michal
> > 

Re: [1/6] powerpc/64s: Add barrier_nospec

[Michael Ellerman]

On Tue, 2018-04-24 at 04:15:54 UTC, Michael Ellerman wrote:
> From: Michal Suchanek <msuchanek@suse.de>
> 
> A no-op form of ori (or immediate of 0 into r31 and the result stored
> in r31) has been re-tasked as a speculation barrier. The instruction
> only acts as a barrier on newer machines with appropriate firmware
> support. On older CPUs it remains a harmless no-op.
> 
> Implement barrier_nospec using this instruction.
> 
> mpe: The semantics of the instruction are believed to be that it
> prevents execution of subsequent instructions until preceding branches
> have been fully resolved and are no longer executing speculatively.
> There is no further documentation available at this time.
> 
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Series applied to powerpc next.

https://git.kernel.org/powerpc/c/a6b3964ad71a61bb7c61d80a60bea7

cheers

Re: powerpc/64s: Enhance the information in cpu_show_spectre_v1()

[Michael Ellerman]

On Mon, 2018-05-28 at 13:19:14 UTC, Michal Suchanek wrote:
> We now have barrier_nospec as mitigation so print it in
> cpu_show_spectre_v1 when enabled.
> 
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/a377514519b9a20fa1ea9adddbb412

cheers