[PATCH 1/5] powerpc/64s: Kernel Hypervisor Restricted Access Prevention

Kernel Hypervisor Restricted Access Prevention (KHRAP) utilises a feature
of the Radix MMU which disallows read and write access to userspace
addresses.  By utilising this, the kernel is prevented from accessing
user data from outside of trusted paths that perform proper safety checks,
such as copy_{to/from}_user() and friends.

Userspace access is disabled from early boot and is only enabled when:

	- exiting the kernel and entering userspace
	- performing an operation like copy_{to/from}_user()
	- context switching to a process that has access enabled

and similarly, access is disabled again when exiting userspace and entering
the kernel.

This feature has a slight performance impact which I roughly measured to be
4% slower (performing 1GB of 1 byte read()/write() syscalls), and is gated
behind the CONFIG_PPC_RADIX_KHRAP option for performance-critical builds.

This feature can be tested by using the lkdtm driver (CONFIG_LKDTM=y) and
performing the following:

	echo ACCESS_USERSPACE > [debugfs]/provoke-crash/DIRECT

if enabled, this should send SIGSEGV to the thread.

Signed-off-by: Russell Currey <redacted>
---
More detailed benchmarks soon, there's more optimisations here as well.

 arch/powerpc/include/asm/exception-64s.h | 17 +++++++
 arch/powerpc/include/asm/mmu.h           |  7 +++
 arch/powerpc/include/asm/reg.h           |  1 +
 arch/powerpc/include/asm/uaccess.h       | 63 +++++++++++++++++++++---
 arch/powerpc/kernel/dt_cpu_ftrs.c        |  4 ++
 arch/powerpc/kernel/entry_64.S           |  7 +++
 arch/powerpc/mm/fault.c                  |  9 ++++
 arch/powerpc/mm/pgtable-radix.c          |  2 +
 arch/powerpc/mm/pkeys.c                  |  7 ++-
 arch/powerpc/platforms/Kconfig.cputype   | 15 ++++++
 10 files changed, 122 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/include/asm/exception-64s.h b/arch/powerpc/include/asm/exception-64s.h
index 3b4767ed3ec5..3b84a8050bae 100644
--- a/arch/powerpc/include/asm/exception-64s.h
+++ b/arch/powerpc/include/asm/exception-64s.h

@@ -240,6 +240,22 @@ BEGIN_FTR_SECTION_NESTED(941)						\
 	mtspr	SPRN_PPR,ra;						\
 END_FTR_SECTION_NESTED(CPU_FTR_HAS_PPR,CPU_FTR_HAS_PPR,941)
 
+#define LOCK_AMR(reg)							\
+BEGIN_MMU_FTR_SECTION_NESTED(69)						\
+	LOAD_REG_IMMEDIATE(reg,AMR_LOCKED);				\
+	isync;				    				\
+	mtspr	SPRN_AMR,reg;						\
+	isync;								\
+END_MMU_FTR_SECTION_NESTED(MMU_FTR_RADIX_KHRAP,MMU_FTR_RADIX_KHRAP,69)
+
+#define UNLOCK_AMR(reg)							\
+BEGIN_MMU_FTR_SECTION_NESTED(420)						\
+	li	reg,0;							\
+	isync;				    				\
+	mtspr	SPRN_AMR,reg;						\
+	isync;								\
+END_MMU_FTR_SECTION_NESTED(MMU_FTR_RADIX_KHRAP,MMU_FTR_RADIX_KHRAP,420)
+
 /*
  * Get an SPR into a register if the CPU has the given feature
  */

@@ -500,6 +516,7 @@ END_FTR_SECTION_NESTED(ftr,ftr,943)
 	beq	4f;			/* if from kernel mode		*/ \
 	ACCOUNT_CPU_USER_ENTRY(r13, r9, r10);				   \
 	SAVE_PPR(area, r9);						   \
+	LOCK_AMR(r9);							   \
 4:	EXCEPTION_PROLOG_COMMON_2(area)					   \
 	EXCEPTION_PROLOG_COMMON_3(n)					   \
 	ACCOUNT_STOLEN_TIME

diff --git a/arch/powerpc/include/asm/mmu.h b/arch/powerpc/include/asm/mmu.h
index eb20eb3b8fb0..504c8bfa2f9d 100644
--- a/arch/powerpc/include/asm/mmu.h
+++ b/arch/powerpc/include/asm/mmu.h

@@ -107,6 +107,10 @@
  */
 #define MMU_FTR_1T_SEGMENT		ASM_CONST(0x40000000)
 
+/* Supports KHRAP (key 0 controlling userspace addresses) on radix
+ */
+#define MMU_FTR_RADIX_KHRAP		ASM_CONST(0x80000000)
+
 /* MMU feature bit sets for various CPUs */
 #define MMU_FTRS_DEFAULT_HPTE_ARCH_V2	\
 	MMU_FTR_HPTE_TABLE | MMU_FTR_PPCAS_ARCH_V2

@@ -143,6 +147,9 @@ enum {
 		MMU_FTR_KERNEL_RO | MMU_FTR_68_BIT_VA |
 #ifdef CONFIG_PPC_RADIX_MMU
 		MMU_FTR_TYPE_RADIX |
+#endif
+#ifdef CONFIG_PPC_RADIX_KHRAP
+		MMU_FTR_RADIX_KHRAP |
 #endif
 		0,
 };

diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
index 640a4d818772..8aa3540fbedc 100644
--- a/arch/powerpc/include/asm/reg.h
+++ b/arch/powerpc/include/asm/reg.h

@@ -246,6 +246,7 @@
 #define SPRN_DSCR	0x11
 #define SPRN_CFAR	0x1c	/* Come From Address Register */
 #define SPRN_AMR	0x1d	/* Authority Mask Register */
+#define   AMR_LOCKED	0xc000000000000000 /* Read & Write disabled */
 #define SPRN_UAMOR	0x9d	/* User Authority Mask Override Register */
 #define SPRN_AMOR	0x15d	/* Authority Mask Override Register */
 #define SPRN_ACOP	0x1F	/* Available Coprocessor Register */

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 15bea9a0f260..4a39ab000d05 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h

@@ -62,6 +62,31 @@ static inline int __access_ok(unsigned long addr, unsigned long size,
 
 #endif
 
+static inline unsigned long unlock_user_access(void)
+{
+	unsigned long amr;
+
+	if (mmu_has_feature(MMU_FTR_RADIX_KHRAP)) {
+		amr = mfspr(SPRN_AMR);
+
+		isync();
+		mtspr(SPRN_AMR, 0);
+		isync();
+		return amr;
+	}
+
+	return 0;
+}
+
+static inline void lock_user_access(unsigned long amr)
+{
+	if (mmu_has_feature(MMU_FTR_RADIX_KHRAP)) {
+		isync();
+		mtspr(SPRN_AMR, AMR_LOCKED);
+		isync();
+	}
+}
+
 #define access_ok(type, addr, size)		\
 	(__chk_user_ptr(addr),			\
 	 __access_ok((__force unsigned long)(addr), (size), get_fs()))

@@ -140,7 +165,9 @@ extern long __put_user_bad(void);
 
 #define __put_user_size(x, ptr, size, retval)			\
 do {								\
+	unsigned long __amr;					\
 	retval = 0;						\
+	__amr = unlock_user_access();				\
 	switch (size) {						\
 	  case 1: __put_user_asm(x, ptr, retval, "stb"); break;	\
 	  case 2: __put_user_asm(x, ptr, retval, "sth"); break;	\

@@ -148,6 +175,7 @@ do {								\
 	  case 8: __put_user_asm2(x, ptr, retval); break;	\
 	  default: __put_user_bad();				\
 	}							\
+	lock_user_access(__amr);				\
 } while (0)
 
 #define __put_user_nocheck(x, ptr, size)			\

@@ -236,10 +264,12 @@ extern long __get_user_bad(void);
 
 #define __get_user_size(x, ptr, size, retval)			\
 do {								\
+	unsigned long __amr;					\
 	retval = 0;						\
 	__chk_user_ptr(ptr);					\
 	if (size > sizeof(x))					\
 		(x) = __get_user_bad();				\
+	__amr = unlock_user_access();				\
 	switch (size) {						\
 	case 1: __get_user_asm(x, ptr, retval, "lbz"); break;	\
 	case 2: __get_user_asm(x, ptr, retval, "lhz"); break;	\

@@ -247,6 +277,7 @@ do {								\
 	case 8: __get_user_asm2(x, ptr, retval);  break;	\
 	default: (x) = __get_user_bad();			\
 	}							\
+	lock_user_access(__amr);				\
 } while (0)
 
 /*

@@ -306,15 +337,20 @@ extern unsigned long __copy_tofrom_user(void __user *to,
 static inline unsigned long
 raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
 {
-	return __copy_tofrom_user(to, from, n);
+	unsigned long ret, amr;
+	amr = unlock_user_access();				\
+	ret = __copy_tofrom_user(to, from, n);			\
+	lock_user_access(amr);					\
+	return ret;						\
 }
 #endif /* __powerpc64__ */
 
 static inline unsigned long raw_copy_from_user(void *to,
 		const void __user *from, unsigned long n)
 {
+	unsigned long ret, amr;
 	if (__builtin_constant_p(n) && (n <= 8)) {
-		unsigned long ret = 1;
+		ret = 1;
 
 		switch (n) {
 		case 1:

@@ -339,14 +375,18 @@ static inline unsigned long raw_copy_from_user(void *to,
 	}
 
 	barrier_nospec();
-	return __copy_tofrom_user((__force void __user *)to, from, n);
+	amr = unlock_user_access();
+	ret = __copy_tofrom_user((__force void __user *)to, from, n);
+	lock_user_access(amr);
+	return ret;
 }
 
 static inline unsigned long raw_copy_to_user(void __user *to,
 		const void *from, unsigned long n)
 {
+	unsigned long ret, amr;
 	if (__builtin_constant_p(n) && (n <= 8)) {
-		unsigned long ret = 1;
+		ret = 1;
 
 		switch (n) {
 		case 1:

@@ -366,17 +406,24 @@ static inline unsigned long raw_copy_to_user(void __user *to,
 			return 0;
 	}
 
-	return __copy_tofrom_user(to, (__force const void __user *)from, n);
+	amr = unlock_user_access();
+	ret = __copy_tofrom_user(to, (__force const void __user *)from, n);
+	lock_user_access(amr);
+	return ret;
 }
 
 extern unsigned long __clear_user(void __user *addr, unsigned long size);
 
 static inline unsigned long clear_user(void __user *addr, unsigned long size)
 {
+	unsigned long amr, ret = size;
 	might_fault();
-	if (likely(access_ok(VERIFY_WRITE, addr, size)))
-		return __clear_user(addr, size);
-	return size;
+	if (likely(access_ok(VERIFY_WRITE, addr, size))) {
+		amr = unlock_user_access();
+		ret = __clear_user(addr, size);
+		lock_user_access(amr);
+	}
+	return ret;
 }
 
 extern long strncpy_from_user(char *dst, const char __user *src, long count);

diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
index f432054234a4..890fcb690ce7 100644
--- a/arch/powerpc/kernel/dt_cpu_ftrs.c
+++ b/arch/powerpc/kernel/dt_cpu_ftrs.c

@@ -337,6 +337,10 @@ static int __init feat_enable_mmu_radix(struct dt_cpu_feature *f)
 	cur_cpu_spec->mmu_features |= MMU_FTRS_HASH_BASE;
 	cur_cpu_spec->cpu_user_features |= PPC_FEATURE_HAS_MMU;
 
+#ifdef CONFIG_PPC_RADIX_KHRAP
+	cur_cpu_spec->mmu_features |= MMU_FTR_RADIX_KHRAP;
+#endif
+
 	return 1;
 #endif
 	return 0;

diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 7b1693adff2a..090f72cbb02d 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S

@@ -286,6 +286,9 @@ BEGIN_FTR_SECTION
 	HMT_MEDIUM_LOW
 END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 
+	/* headed back to userspace, so unlock the AMR */
+	UNLOCK_AMR(r2)
+
 	ld	r13,GPR13(r1)	/* only restore r13 if returning to usermode */
 	ld	r2,GPR2(r1)
 	ld	r1,GPR1(r1)

@@ -965,6 +968,10 @@ BEGIN_FTR_SECTION
 	ld	r2,_PPR(r1)
 	mtspr	SPRN_PPR,r2
 END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
+
+	/* headed back to userspace, so unlock the AMR */
+	UNLOCK_AMR(r2)
+
 	ACCOUNT_CPU_USER_EXIT(r13, r2, r4)
 	REST_GPR(13, r1)
 

diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index d51cf5f4e45e..697dee2f665b 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c

@@ -462,6 +462,15 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
 		return bad_key_fault_exception(regs, address,
 					       get_mm_addr_key(mm, address));
 
+	if (mmu_has_feature(MMU_FTR_RADIX_KHRAP)) {
+		if (unlikely(!is_user &&
+			     (error_code & DSISR_PROTFAULT) &&
+			     (mfspr(SPRN_AMR) & AMR_LOCKED))) {
+			printk(KERN_CRIT "Kernel attempted to access user data"
+			       " unsafely, possible security vulnerability\n");
+			return bad_area_nosemaphore(regs, address);
+		}
+	}
 	/*
 	 * We want to do this outside mmap_sem, because reading code around nip
 	 * can result in fault, which will cause a deadlock when called with

diff --git a/arch/powerpc/mm/pgtable-radix.c b/arch/powerpc/mm/pgtable-radix.c
index c879979faa73..2e88851916e0 100644
--- a/arch/powerpc/mm/pgtable-radix.c
+++ b/arch/powerpc/mm/pgtable-radix.c

@@ -29,6 +29,7 @@
 #include <asm/powernv.h>
 #include <asm/sections.h>
 #include <asm/trace.h>
+#include <asm/uaccess.h>
 
 #include <trace/events/thp.h>
 

@@ -608,6 +609,7 @@ void __init radix__early_init_mmu(void)
 		mtspr(SPRN_LPCR, lpcr | LPCR_UPRT | LPCR_HR);
 		radix_init_partition_table();
 		radix_init_amor();
+		lock_user_access(0);
 	} else {
 		radix_init_pseries();
 	}

diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index b271b283c785..4d58426af3e3 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c

@@ -7,6 +7,7 @@
 
 #include <asm/mman.h>
 #include <asm/setup.h>
+#include <asm/uaccess.h>
 #include <linux/pkeys.h>
 #include <linux/of_device.h>
 

@@ -266,7 +267,8 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 
 void thread_pkey_regs_save(struct thread_struct *thread)
 {
-	if (static_branch_likely(&pkey_disabled))
+	if (static_branch_likely(&pkey_disabled) &&
+	    !mmu_has_feature(MMU_FTR_RADIX_KHRAP))
 		return;
 
 	/*

@@ -280,7 +282,8 @@ void thread_pkey_regs_save(struct thread_struct *thread)
 void thread_pkey_regs_restore(struct thread_struct *new_thread,
 			      struct thread_struct *old_thread)
 {
-	if (static_branch_likely(&pkey_disabled))
+	if (static_branch_likely(&pkey_disabled) &&
+	    !mmu_has_feature(MMU_FTR_RADIX_KHRAP))
 		return;
 
 	if (old_thread->amr != new_thread->amr)

diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
index f4e2c5729374..6cd32ce6760c 100644
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype

@@ -351,6 +351,21 @@ config PPC_RADIX_MMU_DEFAULT
 
 	  If you're unsure, say Y.
 
+config PPC_RADIX_KHRAP
+	bool "Kernel Hypervisor Restricted Access Prevention on Radix"
+	depends on PPC_RADIX_MMU
+	default y
+	help
+	  Enable support for Kernel Hypervisor Restricted Access Prevention
+	  (KHRAP) when using the Radix MMU.  KHRAP is a security feature
+	  preventing the kernel from directly accessing userspace data
+	  without going through the proper checks.
+
+	  KHRAP has a minor performance impact on context switching and can be
+	  disabled at boot time using the "nosmap" kernel command line option.
+
+	  If you're unsure, say Y.
+
 config ARCH_ENABLE_HUGEPAGE_MIGRATION
 	def_bool y
 	depends on PPC_BOOK3S_64 && HUGETLB_PAGE && MIGRATION

-- 
2.19.1

KHRAP is similar to SMAP on x86 platforms, so implement support for
the same kernel parameter.

Signed-off-by: Russell Currey <redacted>
---
 arch/powerpc/mm/init_64.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
index 7a9886f98b0c..10182ce3b94f 100644
--- a/arch/powerpc/mm/init_64.c
+++ b/arch/powerpc/mm/init_64.c

@@ -312,6 +312,7 @@ void register_page_bootmem_memmap(unsigned long section_nr,
 
 #ifdef CONFIG_PPC_BOOK3S_64
 static bool disable_radix = !IS_ENABLED(CONFIG_PPC_RADIX_MMU_DEFAULT);
+static bool disable_khrap = !IS_ENABLED(CONFIG_PPC_RADIX_KHRAP);
 
 static int __init parse_disable_radix(char *p)
 {

@@ -328,6 +329,18 @@ static int __init parse_disable_radix(char *p)
 }
 early_param("disable_radix", parse_disable_radix);
 
+static int __init parse_nosmap(char *p)
+{
+	/*
+	 * nosmap is an existing option on x86 where it doesn't return -EINVAL
+	 * if the parameter is set to something, so even though it's different
+	 * to disable_radix, don't return an error for compatibility.
+	 */
+	disable_khrap = true;
+	return 0;
+}
+early_param("nosmap", parse_nosmap);
+
 /*
  * If we're running under a hypervisor, we need to check the contents of
  * /chosen/ibm,architecture-vec-5 to see if the hypervisor is willing to do

@@ -381,6 +394,8 @@ void __init mmu_early_init_devtree(void)
 	/* Disable radix mode based on kernel command line. */
 	if (disable_radix)
 		cur_cpu_spec->mmu_features &= ~MMU_FTR_TYPE_RADIX;
+	if (disable_radix || disable_khrap)
+		cur_cpu_spec->mmu_features &= ~MMU_FTR_RADIX_KHRAP;
 
 	/*
 	 * Check /chosen/ibm,architecture-vec-5 if running as a guest.

-- 
2.19.1

Wrap the futex operations in KHRAP locks and unlocks.

Signed-off-by: Russell Currey <redacted>
---
 arch/powerpc/include/asm/futex.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/powerpc/include/asm/futex.h b/arch/powerpc/include/asm/futex.h
index 94542776a62d..e0f4227cfd32 100644
--- a/arch/powerpc/include/asm/futex.h
+++ b/arch/powerpc/include/asm/futex.h

@@ -34,7 +34,9 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
 		u32 __user *uaddr)
 {
 	int oldval = 0, ret;
+	unsigned long amr;
 
+	amr = unlock_user_access();
 	pagefault_disable();
 
 	switch (op) {

@@ -62,6 +64,7 @@ static inline int arch_futex_atomic_op_inuser(int op, int oparg, int *oval,
 	if (!ret)
 		*oval = oldval;
 
+	lock_user_access(amr);
 	return ret;
 }
 

@@ -71,10 +74,12 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
 {
 	int ret = 0;
 	u32 prev;
+	unsigned long amr;
 
 	if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
 		return -EFAULT;
 
+	amr = unlock_user_access();
         __asm__ __volatile__ (
         PPC_ATOMIC_ENTRY_BARRIER
 "1:     lwarx   %1,0,%3         # futex_atomic_cmpxchg_inatomic\n\

@@ -95,6 +100,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
         : "cc", "memory");
 
 	*uval = prev;
+	lock_user_access(amr);
         return ret;
 }
 

-- 
2.19.1

Signed-off-by: Russell Currey <redacted>
---
 Documentation/admin-guide/kernel-parameters.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index a5ad67d5cb16..8f78e75965f0 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt

@@ -2764,7 +2764,7 @@
 			noexec=on: enable non-executable mappings (default)
 			noexec=off: disable non-executable mappings
 
-	nosmap		[X86]
+	nosmap		[X86,PPC]
 			Disable SMAP (Supervisor Mode Access Prevention)
 			even if it is supported by processor.
 

-- 
2.19.1

Wrap the checksumming code in KHRAP locks and unlocks.

Signed-off-by: Russell Currey <redacted>
---
 arch/powerpc/lib/checksum_wrappers.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/lib/checksum_wrappers.c b/arch/powerpc/lib/checksum_wrappers.c
index a0cb63fb76a1..695460a29c9f 100644
--- a/arch/powerpc/lib/checksum_wrappers.c
+++ b/arch/powerpc/lib/checksum_wrappers.c

@@ -26,7 +26,7 @@
 __wsum csum_and_copy_from_user(const void __user *src, void *dst,
 			       int len, __wsum sum, int *err_ptr)
 {
-	unsigned int csum;
+	unsigned int csum, amr = unlock_user_access();
 
 	might_sleep();
 

@@ -60,6 +60,7 @@ __wsum csum_and_copy_from_user(const void __user *src, void *dst,
 	}
 
 out:
+	lock_user_access(amr);
 	return (__force __wsum)csum;
 }
 EXPORT_SYMBOL(csum_and_copy_from_user);

@@ -67,7 +68,7 @@ EXPORT_SYMBOL(csum_and_copy_from_user);
 __wsum csum_and_copy_to_user(const void *src, void __user *dst, int len,
 			     __wsum sum, int *err_ptr)
 {
-	unsigned int csum;
+	unsigned int csum, amr = unlock_user_access();
 
 	might_sleep();
 

@@ -97,6 +98,7 @@ __wsum csum_and_copy_to_user(const void *src, void __user *dst, int len,
 	}
 
 out:
+	lock_user_access(amr);
 	return (__force __wsum)csum;
 }
 EXPORT_SYMBOL(csum_and_copy_to_user);

-- 
2.19.1

Hi Russell,

I love your patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on next-20181016]
[cannot apply to v4.19-rc8]
[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/Russell-Currey/powerpc-64s-Kernel-Hypervisor-Restricted-Access-Prevention/20181017-153543
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-storcenter_defconfig (attached as .config)
compiler: powerpc-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
        GCC_VERSION=7.2.0 make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   In file included from include/linux/uaccess.h:14:0,
                    from net/core/datagram.c:40:
   arch/powerpc/include/asm/uaccess.h: In function 'unlock_user_access':

quoted

arch/powerpc/include/asm/uaccess.h:69:6: error: implicit declaration of function 'mmu_has_feature'; did you mean 'firmware_has_feature'? [-Werror=implicit-function-declaration]

     if (mmu_has_feature(MMU_FTR_RADIX_KHRAP)) {
         ^~~~~~~~~~~~~~~
         firmware_has_feature

quoted

arch/powerpc/include/asm/uaccess.h:69:22: error: 'MMU_FTR_RADIX_KHRAP' undeclared (first use in this function); did you mean 'CPU_FTR_CAN_NAP'?

     if (mmu_has_feature(MMU_FTR_RADIX_KHRAP)) {
                         ^~~~~~~~~~~~~~~~~~~
                         CPU_FTR_CAN_NAP
   arch/powerpc/include/asm/uaccess.h:69:22: note: each undeclared identifier is reported only once for each function it appears in
   arch/powerpc/include/asm/uaccess.h: In function 'lock_user_access':
   arch/powerpc/include/asm/uaccess.h:83:22: error: 'MMU_FTR_RADIX_KHRAP' undeclared (first use in this function); did you mean 'CPU_FTR_CAN_NAP'?
     if (mmu_has_feature(MMU_FTR_RADIX_KHRAP)) {
                         ^~~~~~~~~~~~~~~~~~~
                         CPU_FTR_CAN_NAP
   In file included from include/linux/mm_types.h:18:0,
                    from include/linux/mm.h:17,
                    from net/core/datagram.c:41:
   arch/powerpc/include/asm/mmu.h: At top level:

quoted

arch/powerpc/include/asm/mmu.h:209:20: error: conflicting types for 'mmu_has_feature'

    static inline bool mmu_has_feature(unsigned long feature)
                       ^~~~~~~~~~~~~~~
   In file included from include/linux/uaccess.h:14:0,
                    from net/core/datagram.c:40:
   arch/powerpc/include/asm/uaccess.h:69:6: note: previous implicit declaration of 'mmu_has_feature' was here
     if (mmu_has_feature(MMU_FTR_RADIX_KHRAP)) {
         ^~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors
--
   In file included from include/linux/uaccess.h:14:0,
                    from include/linux/crypto.h:26,
                    from include/crypto/skcipher.h:16,
                    from include/crypto/chacha20.h:9,
                    from lib/chacha20.c:17:
   arch/powerpc/include/asm/uaccess.h: In function 'unlock_user_access':

quoted

arch/powerpc/include/asm/uaccess.h:69:6: error: implicit declaration of function 'mmu_has_feature'; did you mean 'firmware_has_feature'? [-Werror=implicit-function-declaration]

     if (mmu_has_feature(MMU_FTR_RADIX_KHRAP)) {
         ^~~~~~~~~~~~~~~
         firmware_has_feature

quoted

arch/powerpc/include/asm/uaccess.h:69:22: error: 'MMU_FTR_RADIX_KHRAP' undeclared (first use in this function); did you mean 'CPU_FTR_CAN_NAP'?

     if (mmu_has_feature(MMU_FTR_RADIX_KHRAP)) {
                         ^~~~~~~~~~~~~~~~~~~
                         CPU_FTR_CAN_NAP
   arch/powerpc/include/asm/uaccess.h:69:22: note: each undeclared identifier is reported only once for each function it appears in
   arch/powerpc/include/asm/uaccess.h: In function 'lock_user_access':
   arch/powerpc/include/asm/uaccess.h:83:22: error: 'MMU_FTR_RADIX_KHRAP' undeclared (first use in this function); did you mean 'CPU_FTR_CAN_NAP'?
     if (mmu_has_feature(MMU_FTR_RADIX_KHRAP)) {
                         ^~~~~~~~~~~~~~~~~~~
                         CPU_FTR_CAN_NAP
   cc1: some warnings being treated as errors
--
   In file included from include/linux/uaccess.h:14:0,
                    from arch/powerpc/kernel/module.c:25:
   arch/powerpc/include/asm/uaccess.h: In function 'unlock_user_access':

quoted

arch/powerpc/include/asm/uaccess.h:69:6: error: implicit declaration of function 'mmu_has_feature'; did you mean 'firmware_has_feature'? [-Werror=implicit-function-declaration]

     if (mmu_has_feature(MMU_FTR_RADIX_KHRAP)) {
         ^~~~~~~~~~~~~~~
         firmware_has_feature

quoted

arch/powerpc/include/asm/uaccess.h:69:22: error: 'MMU_FTR_RADIX_KHRAP' undeclared (first use in this function); did you mean 'CPU_FTR_CAN_NAP'?

     if (mmu_has_feature(MMU_FTR_RADIX_KHRAP)) {
                         ^~~~~~~~~~~~~~~~~~~
                         CPU_FTR_CAN_NAP
   arch/powerpc/include/asm/uaccess.h:69:22: note: each undeclared identifier is reported only once for each function it appears in
   arch/powerpc/include/asm/uaccess.h: In function 'lock_user_access':
   arch/powerpc/include/asm/uaccess.h:83:22: error: 'MMU_FTR_RADIX_KHRAP' undeclared (first use in this function); did you mean 'CPU_FTR_CAN_NAP'?
     if (mmu_has_feature(MMU_FTR_RADIX_KHRAP)) {
                         ^~~~~~~~~~~~~~~~~~~
                         CPU_FTR_CAN_NAP
   cc1: all warnings being treated as errors

vim +69 arch/powerpc/include/asm/uaccess.h

    64	
    65	static inline unsigned long unlock_user_access(void)
    66	{
    67		unsigned long amr;
    68	
  > 69		if (mmu_has_feature(MMU_FTR_RADIX_KHRAP)) {
    70			amr = mfspr(SPRN_AMR);
    71	
    72			isync();
    73			mtspr(SPRN_AMR, 0);
    74			isync();
    75			return amr;
    76		}
    77	
    78		return 0;
    79	}
    80	

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

Russell Currey [off-list ref] writes:

This one needs an ifdef, or preferable an empty version in a header for
non-book3s 64, otherwise we get:

  arch/powerpc/kernel/entry_64.S: Assembler messages:
  arch/powerpc/kernel/entry_64.S:290: Error: unrecognized opcode: `unlock_amr(%r2)'
  scripts/Makefile.build:405: recipe for target 'arch/powerpc/kernel/entry_64.o' failed

That's a corenet64-ish defconfig.

cheers

On Wed, 17 Oct 2018 17:44:19 +1100
Russell Currey [off-list ref] wrote:

Kernel Hypervisor Restricted Access Prevention (KHRAP) utilises a feature
of the Radix MMU which disallows read and write access to userspace
addresses.  By utilising this, the kernel is prevented from accessing
user data from outside of trusted paths that perform proper safety checks,
such as copy_{to/from}_user() and friends.

Userspace access is disabled from early boot and is only enabled when:

	- exiting the kernel and entering userspace
	- performing an operation like copy_{to/from}_user()
	- context switching to a process that has access enabled

and similarly, access is disabled again when exiting userspace and entering
the kernel.

This feature has a slight performance impact which I roughly measured to be
4% slower (performing 1GB of 1 byte read()/write() syscalls), and is gated
behind the CONFIG_PPC_RADIX_KHRAP option for performance-critical builds.

This feature can be tested by using the lkdtm driver (CONFIG_LKDTM=y) and
performing the following:

	echo ACCESS_USERSPACE > [debugfs]/provoke-crash/DIRECT

if enabled, this should send SIGSEGV to the thread.

Signed-off-by: Russell Currey <redacted>
---
More detailed benchmarks soon, there's more optimisations here as well.

Nice, this turned out to be a lot neater than I feared! Good stuff.

I wonder if you can skip the first isync on the way in and the second
isync on the way out because the interrupt and return should be context
synchronizing. Might not make a difference though.

What do you think about making the name match the C code a bit more.
Like AMR_LOCK_USER_ACCESS()?

Thanks,
Nick

On Wed, 2018-10-17 at 22:59 +1000, Nicholas Piggin wrote:

On Wed, 17 Oct 2018 17:44:19 +1100
Russell Currey [off-list ref] wrote:

quoted

Kernel Hypervisor Restricted Access Prevention (KHRAP) utilises a
feature
of the Radix MMU which disallows read and write access to userspace
addresses.  By utilising this, the kernel is prevented from
accessing
user data from outside of trusted paths that perform proper safety
checks,
such as copy_{to/from}_user() and friends.

Userspace access is disabled from early boot and is only enabled
when:

	- exiting the kernel and entering userspace
	- performing an operation like copy_{to/from}_user()
	- context switching to a process that has access enabled

and similarly, access is disabled again when exiting userspace and
entering
the kernel.

This feature has a slight performance impact which I roughly
measured to be
4% slower (performing 1GB of 1 byte read()/write() syscalls), and
is gated
behind the CONFIG_PPC_RADIX_KHRAP option for performance-critical
builds.

This feature can be tested by using the lkdtm driver
(CONFIG_LKDTM=y) and
performing the following:

	echo ACCESS_USERSPACE > [debugfs]/provoke-crash/DIRECT

if enabled, this should send SIGSEGV to the thread.

Signed-off-by: Russell Currey <redacted>
---
More detailed benchmarks soon, there's more optimisations here as
well.

Nice, this turned out to be a lot neater than I feared! Good stuff.

quoted

@@ -240,6 +240,22 @@ BEGIN_FTR_SECTION_NESTED(941)			

			\
 	mtspr	SPRN_PPR,ra;						
\
 END_FTR_SECTION_NESTED(CPU_FTR_HAS_PPR,CPU_FTR_HAS_PPR,941)
 
+#define LOCK_AMR(reg)						
	\
+BEGIN_MMU_FTR_SECTION_NESTED(69)					
	\
+	LOAD_REG_IMMEDIATE(reg,AMR_LOCKED);				\
+	isync;				    				
\
+	mtspr	SPRN_AMR,reg;						
\
+	isync;								
\
+END_MMU_FTR_SECTION_NESTED(MMU_FTR_RADIX_KHRAP,MMU_FTR_RADIX_KHRAP
,69)
+
+#define UNLOCK_AMR(reg)						
	\
+BEGIN_MMU_FTR_SECTION_NESTED(420)					
	\
+	li	reg,0;							\
+	isync;				    				
\
+	mtspr	SPRN_AMR,reg;						
\
+	isync;								
\
+END_MMU_FTR_SECTION_NESTED(MMU_FTR_RADIX_KHRAP,MMU_FTR_RADIX_KHRAP
,420)

I wonder if you can skip the first isync on the way in and the second
isync on the way out because the interrupt and return should be
context
synchronizing. Might not make a difference though.

Ben thought we wouldn't need at least one of them, but it's
implementation dependent, so there might be some concern with future
chips actually needing both isyncs or something.  There weren't any
consequences to leaving out isyncs, I'll do some quick benchmarking to
see if it's any meaningful speedup to leave one out.

What do you think about making the name match the C code a bit more.
Like AMR_LOCK_USER_ACCESS()?

That is a good idea.

Thanks,
Nick

On Wed, 2018-10-17 at 22:30 +1100, Michael Ellerman wrote:

Russell Currey [off-list ref] writes:

quoted

diff --git a/arch/powerpc/kernel/entry_64.S

b/arch/powerpc/kernel/entry_64.S
index 7b1693adff2a..090f72cbb02d 100644

--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S

@@ -286,6 +286,9 @@ BEGIN_FTR_SECTION
 	HMT_MEDIUM_LOW
 END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 
+	/* headed back to userspace, so unlock the AMR */
+	UNLOCK_AMR(r2)
+

This one needs an ifdef, or preferable an empty version in a header
for
non-book3s 64, otherwise we get:

  arch/powerpc/kernel/entry_64.S: Assembler messages:
  arch/powerpc/kernel/entry_64.S:290: Error: unrecognized opcode:
`unlock_amr(%r2)'
  scripts/Makefile.build:405: recipe for target
'arch/powerpc/kernel/entry_64.o' failed

That's a corenet64-ish defconfig.

Yep, sorry.  I knew it wouldn't build on non-64s but I just wanted to
get the main part out there so people could start looking at it.  Will
fix.

- Russell

cheers