linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching
@ 2016-09-02 15:02 Catalin Marinas
  2016-09-02 15:02 ` [PATCH v2 1/7] arm64: Factor out PAN enabling/disabling into separate uaccess_* macros Catalin Marinas
                   ` (9 more replies)
  0 siblings, 10 replies; 30+ messages in thread
From: Catalin Marinas @ 2016-09-02 15:02 UTC (permalink / raw)
  To: linux-arm-kernel

This is the second version of the arm64 PAN emulation by disabling
TTBR0_EL1 accesses. The major change from v1 is the use of a thread_info
member to store the real TTBR0_EL1 value. The advantage is slightly
simpler assembler macros for uaccess_enable with the downside that
switch_mm() must always update the saved ttbr0 even if there is no mm
switch.

Whether we could simplify these patches further to use some TCR_EL1.EPD0
tricks remains to be confirmed with the ARM architects. However, it is
unlikely that they would deem such idea architecturally safe, hence this
series only switches TTBR0_EL1 in accordance with the ARM ARM.

Changes since v1:

- Using thread_info instead of per-CPU variable for the real TTBR0_EL1
  value (mentioned above)

- Factored out the cpu_do_switch_mm errata workaround to a separate
  macro and avoided the "errata" argument to the uaccess_enable asm
  macro

- Fix build error with allnoconfig by moving the uaccess_* asm macros to
  asm/uaccess.h and avoid some cyclic header includes

- _PSR_PAN_BIT moved to the non-uapi ptrace.h

- Use x21 instead of lr as temporary register in entry.S

As per v1, the code requires more testing, especially for combinations
where hardware PAN and/or UAO are present.

The patches are also available on this branch:

  git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux ttbr0-pan

Thanks for reviewing/testing.

Catalin Marinas (7):
  arm64: Factor out PAN enabling/disabling into separate uaccess_* macros
  arm64: Factor out TTBR0_EL1 post-update workaround into a specific asm macro
  arm64: Introduce uaccess_{disable,enable} functionality based on TTBR0_EL1
  arm64: Disable TTBR0_EL1 during normal kernel execution
  arm64: Handle faults caused by inadvertent user access with PAN enabled
  arm64: xen: Enable user access before a privcmd hvc call
  arm64: Enable CONFIG_ARM64_TTBR0_PAN

 arch/arm64/Kconfig                      |   8 ++
 arch/arm64/include/asm/assembler.h      |  37 +++++++++
 arch/arm64/include/asm/cpufeature.h     |   6 ++
 arch/arm64/include/asm/efi.h            |  14 ++++
 arch/arm64/include/asm/futex.h          |  14 ++--
 arch/arm64/include/asm/kernel-pgtable.h |   7 ++
 arch/arm64/include/asm/mmu_context.h    |  32 ++++++--
 arch/arm64/include/asm/ptrace.h         |   2 +
 arch/arm64/include/asm/thread_info.h    |   3 +
 arch/arm64/include/asm/uaccess.h        | 136 ++++++++++++++++++++++++++++++--
 arch/arm64/kernel/armv8_deprecated.c    |  10 +--
 arch/arm64/kernel/asm-offsets.c         |   3 +
 arch/arm64/kernel/cpufeature.c          |   1 +
 arch/arm64/kernel/entry.S               |  71 ++++++++++++++++-
 arch/arm64/kernel/head.S                |   6 +-
 arch/arm64/kernel/setup.c               |   8 ++
 arch/arm64/kernel/vmlinux.lds.S         |   5 ++
 arch/arm64/lib/clear_user.S             |   8 +-
 arch/arm64/lib/copy_from_user.S         |   8 +-
 arch/arm64/lib/copy_in_user.S           |   8 +-
 arch/arm64/lib/copy_to_user.S           |   8 +-
 arch/arm64/mm/context.c                 |   7 +-
 arch/arm64/mm/fault.c                   |  22 ++++--
 arch/arm64/mm/proc.S                    |  12 +--
 arch/arm64/xen/hypercall.S              |  19 +++++
 25 files changed, 381 insertions(+), 74 deletions(-)

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 1/7] arm64: Factor out PAN enabling/disabling into separate uaccess_* macros
  2016-09-02 15:02 [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching Catalin Marinas
@ 2016-09-02 15:02 ` Catalin Marinas
  2016-09-05 15:38   ` Mark Rutland
  2016-09-02 15:02 ` [PATCH v2 2/7] arm64: Factor out TTBR0_EL1 post-update workaround into a specific asm macro Catalin Marinas
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 30+ messages in thread
From: Catalin Marinas @ 2016-09-02 15:02 UTC (permalink / raw)
  To: linux-arm-kernel

This patch moves the directly coded alternatives for turning PAN on/off
into separate uaccess_{enable,disable} macros or functions. The asm
macros take a few arguments which will be used in subsequent patches.

Cc: Will Deacon <will.deacon@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/include/asm/futex.h       | 14 ++++-----
 arch/arm64/include/asm/uaccess.h     | 55 ++++++++++++++++++++++++++++++------
 arch/arm64/kernel/armv8_deprecated.c | 10 +++----
 arch/arm64/lib/clear_user.S          |  8 ++----
 arch/arm64/lib/copy_from_user.S      |  8 ++----
 arch/arm64/lib/copy_in_user.S        |  8 ++----
 arch/arm64/lib/copy_to_user.S        |  8 ++----
 7 files changed, 71 insertions(+), 40 deletions(-)

diff --git a/arch/arm64/include/asm/futex.h b/arch/arm64/include/asm/futex.h
index f2585cdd32c2..7e5f236093be 100644
--- a/arch/arm64/include/asm/futex.h
+++ b/arch/arm64/include/asm/futex.h
@@ -27,9 +27,9 @@
 #include <asm/sysreg.h>
 
 #define __futex_atomic_op(insn, ret, oldval, uaddr, tmp, oparg)		\
+do {									\
+	uaccess_enable(ARM64_HAS_PAN);					\
 	asm volatile(							\
-	ALTERNATIVE("nop", SET_PSTATE_PAN(0), ARM64_HAS_PAN,		\
-		    CONFIG_ARM64_PAN)					\
 "	prfm	pstl1strm, %2\n"					\
 "1:	ldxr	%w1, %2\n"						\
 	insn "\n"							\
@@ -44,11 +44,11 @@
 "	.popsection\n"							\
 	_ASM_EXTABLE(1b, 4b)						\
 	_ASM_EXTABLE(2b, 4b)						\
-	ALTERNATIVE("nop", SET_PSTATE_PAN(1), ARM64_HAS_PAN,		\
-		    CONFIG_ARM64_PAN)					\
 	: "=&r" (ret), "=&r" (oldval), "+Q" (*uaddr), "=&r" (tmp)	\
 	: "r" (oparg), "Ir" (-EFAULT)					\
-	: "memory")
+	: "memory");							\
+	uaccess_disable(ARM64_HAS_PAN);					\
+} while (0)
 
 static inline int
 futex_atomic_op_inuser (int encoded_op, u32 __user *uaddr)
@@ -118,8 +118,8 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
 	if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
 		return -EFAULT;
 
+	uaccess_enable(ARM64_HAS_PAN);
 	asm volatile("// futex_atomic_cmpxchg_inatomic\n"
-ALTERNATIVE("nop", SET_PSTATE_PAN(0), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
 "	prfm	pstl1strm, %2\n"
 "1:	ldxr	%w1, %2\n"
 "	sub	%w3, %w1, %w4\n"
@@ -134,10 +134,10 @@ ALTERNATIVE("nop", SET_PSTATE_PAN(0), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
 "	.popsection\n"
 	_ASM_EXTABLE(1b, 4b)
 	_ASM_EXTABLE(2b, 4b)
-ALTERNATIVE("nop", SET_PSTATE_PAN(1), ARM64_HAS_PAN, CONFIG_ARM64_PAN)
 	: "+r" (ret), "=&r" (val), "+Q" (*uaddr), "=&r" (tmp)
 	: "r" (oldval), "r" (newval), "Ir" (-EFAULT)
 	: "memory");
+	uaccess_disable(ARM64_HAS_PAN);
 
 	*uval = val;
 	return ret;
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index c47257c91b77..fde5f7a13030 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -18,6 +18,8 @@
 #ifndef __ASM_UACCESS_H
 #define __ASM_UACCESS_H
 
+#ifndef __ASSEMBLY__
+
 /*
  * User space memory access functions
  */
@@ -112,6 +114,21 @@ static inline void set_fs(mm_segment_t fs)
 	"	.popsection\n"
 
 /*
+ * User access enabling/disabling.
+ */
+#define uaccess_disable(alt)						\
+do {									\
+	asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1), alt,			\
+			CONFIG_ARM64_PAN));				\
+} while (0)
+
+#define uaccess_enable(alt)						\
+do {									\
+	asm(ALTERNATIVE("nop", SET_PSTATE_PAN(0), alt,			\
+			CONFIG_ARM64_PAN));				\
+} while (0)
+
+/*
  * The "__xxx" versions of the user access functions do not verify the address
  * space - it must have been done previously with a separate "access_ok()"
  * call.
@@ -138,8 +155,7 @@ static inline void set_fs(mm_segment_t fs)
 do {									\
 	unsigned long __gu_val;						\
 	__chk_user_ptr(ptr);						\
-	asm(ALTERNATIVE("nop", SET_PSTATE_PAN(0), ARM64_ALT_PAN_NOT_UAO,\
-			CONFIG_ARM64_PAN));				\
+	uaccess_enable(ARM64_ALT_PAN_NOT_UAO);				\
 	switch (sizeof(*(ptr))) {					\
 	case 1:								\
 		__get_user_asm("ldrb", "ldtrb", "%w", __gu_val, (ptr),  \
@@ -160,9 +176,8 @@ do {									\
 	default:							\
 		BUILD_BUG();						\
 	}								\
+	uaccess_disable(ARM64_ALT_PAN_NOT_UAO);				\
 	(x) = (__force __typeof__(*(ptr)))__gu_val;			\
-	asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1), ARM64_ALT_PAN_NOT_UAO,\
-			CONFIG_ARM64_PAN));				\
 } while (0)
 
 #define __get_user(x, ptr)						\
@@ -207,8 +222,7 @@ do {									\
 do {									\
 	__typeof__(*(ptr)) __pu_val = (x);				\
 	__chk_user_ptr(ptr);						\
-	asm(ALTERNATIVE("nop", SET_PSTATE_PAN(0), ARM64_ALT_PAN_NOT_UAO,\
-			CONFIG_ARM64_PAN));				\
+	uaccess_enable(ARM64_ALT_PAN_NOT_UAO);				\
 	switch (sizeof(*(ptr))) {					\
 	case 1:								\
 		__put_user_asm("strb", "sttrb", "%w", __pu_val, (ptr),	\
@@ -229,8 +243,7 @@ do {									\
 	default:							\
 		BUILD_BUG();						\
 	}								\
-	asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1), ARM64_ALT_PAN_NOT_UAO,\
-			CONFIG_ARM64_PAN));				\
+	uaccess_disable(ARM64_ALT_PAN_NOT_UAO);				\
 } while (0)
 
 #define __put_user(x, ptr)						\
@@ -321,4 +334,30 @@ extern long strncpy_from_user(char *dest, const char __user *src, long count);
 extern __must_check long strlen_user(const char __user *str);
 extern __must_check long strnlen_user(const char __user *str, long n);
 
+#else	/* __ASSEMBLY__ */
+
+#include <asm/alternative.h>
+#include <asm/assembler.h>
+
+/*
+ * User access enabling/disabling macros.
+ */
+	.macro	uaccess_disable, tmp1
+alternative_if_not ARM64_ALT_PAN_NOT_UAO
+	nop
+alternative_else
+	SET_PSTATE_PAN(1)
+alternative_endif
+	.endm
+
+	.macro	uaccess_enable, tmp1, tmp2
+alternative_if_not ARM64_ALT_PAN_NOT_UAO
+	nop
+alternative_else
+	SET_PSTATE_PAN(0)
+alternative_endif
+	.endm
+
+#endif	/* __ASSEMBLY__ */
+
 #endif /* __ASM_UACCESS_H */
diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
index 42ffdb54e162..3aaf2fafbc8a 100644
--- a/arch/arm64/kernel/armv8_deprecated.c
+++ b/arch/arm64/kernel/armv8_deprecated.c
@@ -281,9 +281,9 @@ static void __init register_insn_emulation_sysctl(struct ctl_table *table)
  * Error-checking SWP macros implemented using ldxr{b}/stxr{b}
  */
 #define __user_swpX_asm(data, addr, res, temp, B)		\
+do {								\
+	uaccess_enable(ARM64_HAS_PAN);				\
 	__asm__ __volatile__(					\
-	ALTERNATIVE("nop", SET_PSTATE_PAN(0), ARM64_HAS_PAN,	\
-		    CONFIG_ARM64_PAN)				\
 	"0:	ldxr"B"		%w2, [%3]\n"			\
 	"1:	stxr"B"		%w0, %w1, [%3]\n"		\
 	"	cbz		%w0, 2f\n"			\
@@ -299,11 +299,11 @@ static void __init register_insn_emulation_sysctl(struct ctl_table *table)
 	"	.popsection"					\
 	_ASM_EXTABLE(0b, 4b)					\
 	_ASM_EXTABLE(1b, 4b)					\
-	ALTERNATIVE("nop", SET_PSTATE_PAN(1), ARM64_HAS_PAN,	\
-		CONFIG_ARM64_PAN)				\
 	: "=&r" (res), "+r" (data), "=&r" (temp)		\
 	: "r" (addr), "i" (-EAGAIN), "i" (-EFAULT)		\
-	: "memory")
+	: "memory");						\
+	uaccess_disable(ARM64_HAS_PAN);				\
+} while (0)
 
 #define __user_swp_asm(data, addr, res, temp) \
 	__user_swpX_asm(data, addr, res, temp, "")
diff --git a/arch/arm64/lib/clear_user.S b/arch/arm64/lib/clear_user.S
index 5d1cad3ce6d6..51577e84b0fe 100644
--- a/arch/arm64/lib/clear_user.S
+++ b/arch/arm64/lib/clear_user.S
@@ -17,10 +17,10 @@
  */
 #include <linux/linkage.h>
 
-#include <asm/alternative.h>
 #include <asm/assembler.h>
 #include <asm/cpufeature.h>
 #include <asm/sysreg.h>
+#include <asm/uaccess.h>
 
 	.text
 
@@ -33,8 +33,7 @@
  * Alignment fixed up by hardware.
  */
 ENTRY(__clear_user)
-ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_ALT_PAN_NOT_UAO, \
-	    CONFIG_ARM64_PAN)
+	uaccess_enable x2, x3
 	mov	x2, x1			// save the size for fixup return
 	subs	x1, x1, #8
 	b.mi	2f
@@ -54,8 +53,7 @@ uao_user_alternative 9f, strh, sttrh, wzr, x0, 2
 	b.mi	5f
 uao_user_alternative 9f, strb, sttrb, wzr, x0, 0
 5:	mov	x0, #0
-ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(1)), ARM64_ALT_PAN_NOT_UAO, \
-	    CONFIG_ARM64_PAN)
+	uaccess_disable x2
 	ret
 ENDPROC(__clear_user)
 
diff --git a/arch/arm64/lib/copy_from_user.S b/arch/arm64/lib/copy_from_user.S
index 0b90497d4424..41a614d63410 100644
--- a/arch/arm64/lib/copy_from_user.S
+++ b/arch/arm64/lib/copy_from_user.S
@@ -16,11 +16,11 @@
 
 #include <linux/linkage.h>
 
-#include <asm/alternative.h>
 #include <asm/assembler.h>
 #include <asm/cache.h>
 #include <asm/cpufeature.h>
 #include <asm/sysreg.h>
+#include <asm/uaccess.h>
 
 /*
  * Copy from user space to a kernel buffer (alignment handled by the hardware)
@@ -67,12 +67,10 @@
 
 end	.req	x5
 ENTRY(__arch_copy_from_user)
-ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_ALT_PAN_NOT_UAO, \
-	    CONFIG_ARM64_PAN)
+	uaccess_enable x3, x4
 	add	end, x0, x2
 #include "copy_template.S"
-ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(1)), ARM64_ALT_PAN_NOT_UAO, \
-	    CONFIG_ARM64_PAN)
+	uaccess_disable x3
 	mov	x0, #0				// Nothing to copy
 	ret
 ENDPROC(__arch_copy_from_user)
diff --git a/arch/arm64/lib/copy_in_user.S b/arch/arm64/lib/copy_in_user.S
index f7292dd08c84..5493c427f538 100644
--- a/arch/arm64/lib/copy_in_user.S
+++ b/arch/arm64/lib/copy_in_user.S
@@ -18,11 +18,11 @@
 
 #include <linux/linkage.h>
 
-#include <asm/alternative.h>
 #include <asm/assembler.h>
 #include <asm/cache.h>
 #include <asm/cpufeature.h>
 #include <asm/sysreg.h>
+#include <asm/uaccess.h>
 
 /*
  * Copy from user space to user space (alignment handled by the hardware)
@@ -68,12 +68,10 @@
 
 end	.req	x5
 ENTRY(__copy_in_user)
-ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_ALT_PAN_NOT_UAO, \
-	    CONFIG_ARM64_PAN)
+	uaccess_enable x3, x4
 	add	end, x0, x2
 #include "copy_template.S"
-ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(1)), ARM64_ALT_PAN_NOT_UAO, \
-	    CONFIG_ARM64_PAN)
+	uaccess_disable x3
 	mov	x0, #0
 	ret
 ENDPROC(__copy_in_user)
diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
index 7a7efe255034..d6203c5f84bd 100644
--- a/arch/arm64/lib/copy_to_user.S
+++ b/arch/arm64/lib/copy_to_user.S
@@ -16,11 +16,11 @@
 
 #include <linux/linkage.h>
 
-#include <asm/alternative.h>
 #include <asm/assembler.h>
 #include <asm/cache.h>
 #include <asm/cpufeature.h>
 #include <asm/sysreg.h>
+#include <asm/uaccess.h>
 
 /*
  * Copy to user space from a kernel buffer (alignment handled by the hardware)
@@ -66,12 +66,10 @@
 
 end	.req	x5
 ENTRY(__arch_copy_to_user)
-ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_ALT_PAN_NOT_UAO, \
-	    CONFIG_ARM64_PAN)
+	uaccess_enable x3, x4
 	add	end, x0, x2
 #include "copy_template.S"
-ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(1)), ARM64_ALT_PAN_NOT_UAO, \
-	    CONFIG_ARM64_PAN)
+	uaccess_disable x3
 	mov	x0, #0
 	ret
 ENDPROC(__arch_copy_to_user)

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 2/7] arm64: Factor out TTBR0_EL1 post-update workaround into a specific asm macro
  2016-09-02 15:02 [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching Catalin Marinas
  2016-09-02 15:02 ` [PATCH v2 1/7] arm64: Factor out PAN enabling/disabling into separate uaccess_* macros Catalin Marinas
@ 2016-09-02 15:02 ` Catalin Marinas
  2016-09-05 16:11   ` Mark Rutland
  2016-09-02 15:02 ` [PATCH v2 3/7] arm64: Introduce uaccess_{disable, enable} functionality based on TTBR0_EL1 Catalin Marinas
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 30+ messages in thread
From: Catalin Marinas @ 2016-09-02 15:02 UTC (permalink / raw)
  To: linux-arm-kernel

This patch takes the errata workaround code out of cpu_do_switch_mm into
a dedicated post_ttbr0_update_workaround macro which will be reused in a
subsequent patch.

Cc: Will Deacon <will.deacon@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/include/asm/assembler.h | 21 +++++++++++++++++++++
 arch/arm64/mm/proc.S               | 12 +-----------
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index d5025c69ca81..b16bbf1fb786 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -350,4 +350,25 @@ alternative_endif
 	movk	\reg, :abs_g0_nc:\val
 	.endm
 
+/*
+ * Errata workaround post TTBR0_EL1 update.
+ */
+	.macro	post_ttbr0_update_workaround, ret = 0
+alternative_if_not ARM64_WORKAROUND_CAVIUM_27456
+	.if	\ret
+	ret
+	.endif
+	nop
+	nop
+	nop
+alternative_else
+	ic	iallu
+	dsb	nsh
+	isb
+	.if	\ret
+	ret
+	.endif
+alternative_endif
+	.endm
+
 #endif	/* __ASM_ASSEMBLER_H */
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 5bb61de23201..9359659f2559 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -125,17 +125,7 @@ ENTRY(cpu_do_switch_mm)
 	bfi	x0, x1, #48, #16		// set the ASID
 	msr	ttbr0_el1, x0			// set TTBR0
 	isb
-alternative_if_not ARM64_WORKAROUND_CAVIUM_27456
-	ret
-	nop
-	nop
-	nop
-alternative_else
-	ic	iallu
-	dsb	nsh
-	isb
-	ret
-alternative_endif
+	post_ttbr0_update_workaround ret = 1
 ENDPROC(cpu_do_switch_mm)
 
 	.pushsection ".idmap.text", "ax"

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 3/7] arm64: Introduce uaccess_{disable, enable} functionality based on TTBR0_EL1
  2016-09-02 15:02 [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching Catalin Marinas
  2016-09-02 15:02 ` [PATCH v2 1/7] arm64: Factor out PAN enabling/disabling into separate uaccess_* macros Catalin Marinas
  2016-09-02 15:02 ` [PATCH v2 2/7] arm64: Factor out TTBR0_EL1 post-update workaround into a specific asm macro Catalin Marinas
@ 2016-09-02 15:02 ` Catalin Marinas
  2016-09-05 17:20   ` Mark Rutland
  2016-09-09 17:15   ` Catalin Marinas
  2016-09-02 15:02 ` [PATCH v2 4/7] arm64: Disable TTBR0_EL1 during normal kernel execution Catalin Marinas
                   ` (6 subsequent siblings)
  9 siblings, 2 replies; 30+ messages in thread
From: Catalin Marinas @ 2016-09-02 15:02 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds the uaccess macros/functions to disable access to user
space by setting TTBR0_EL1 to a reserved zeroed page. Since the value
written to TTBR0_EL1 must be a physical address, for simplicity this
patch introduces a reserved_ttbr0 page at a constant offset from
swapper_pg_dir. The uaccess_disable code uses the ttbr1_el1 value
adjusted by the reserved_ttbr0 offset.

Enabling access to user is done by restoring TTBR0_EL1 with the value
from the struct thread_info ttbr0 variable. Interrupts must be disabled
during the uaccess_ttbr0_enable code to ensure the atomicity of the
thread_info.ttbr0 read and TTBR0_EL1 write.

Cc: Will Deacon <will.deacon@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/include/asm/assembler.h      | 16 ++++++
 arch/arm64/include/asm/cpufeature.h     |  6 +++
 arch/arm64/include/asm/kernel-pgtable.h |  7 +++
 arch/arm64/include/asm/thread_info.h    |  3 ++
 arch/arm64/include/asm/uaccess.h        | 89 +++++++++++++++++++++++++++++++--
 arch/arm64/kernel/asm-offsets.c         |  3 ++
 arch/arm64/kernel/cpufeature.c          |  1 +
 arch/arm64/kernel/entry.S               |  4 --
 arch/arm64/kernel/head.S                |  6 +--
 arch/arm64/kernel/vmlinux.lds.S         |  5 ++
 10 files changed, 129 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index b16bbf1fb786..bf1c797052dd 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -41,6 +41,15 @@
 	msr	daifclr, #2
 	.endm
 
+	.macro	save_and_disable_irq, flags
+	mrs	\flags, daif
+	msr	daifset, #2
+	.endm
+
+	.macro	restore_irq, flags
+	msr	daif, \flags
+	.endm
+
 /*
  * Enable and disable debug exceptions.
  */
@@ -351,6 +360,13 @@ alternative_endif
 	.endm
 
 /*
+ * Return the current thread_info.
+ */
+	.macro	get_thread_info, rd
+	mrs	\rd, sp_el0
+	.endm
+
+/*
  * Errata workaround post TTBR0_EL1 update.
  */
 	.macro	post_ttbr0_update_workaround, ret = 0
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index 7099f26e3702..023066d9bf7f 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -216,6 +216,12 @@ static inline bool system_supports_mixed_endian_el0(void)
 	return id_aa64mmfr0_mixed_endian_el0(read_system_reg(SYS_ID_AA64MMFR0_EL1));
 }
 
+static inline bool system_supports_ttbr0_pan(void)
+{
+	return IS_ENABLED(CONFIG_ARM64_TTBR0_PAN) &&
+		!cpus_have_cap(ARM64_HAS_PAN);
+}
+
 #endif /* __ASSEMBLY__ */
 
 #endif
diff --git a/arch/arm64/include/asm/kernel-pgtable.h b/arch/arm64/include/asm/kernel-pgtable.h
index 7e51d1b57c0c..f825ffda9c52 100644
--- a/arch/arm64/include/asm/kernel-pgtable.h
+++ b/arch/arm64/include/asm/kernel-pgtable.h
@@ -19,6 +19,7 @@
 #ifndef __ASM_KERNEL_PGTABLE_H
 #define __ASM_KERNEL_PGTABLE_H
 
+#include <asm/pgtable.h>
 #include <asm/sparsemem.h>
 
 /*
@@ -54,6 +55,12 @@
 #define SWAPPER_DIR_SIZE	(SWAPPER_PGTABLE_LEVELS * PAGE_SIZE)
 #define IDMAP_DIR_SIZE		(IDMAP_PGTABLE_LEVELS * PAGE_SIZE)
 
+#ifdef CONFIG_ARM64_TTBR0_PAN
+#define RESERVED_TTBR0_SIZE	(PAGE_SIZE)
+#else
+#define RESERVED_TTBR0_SIZE	(0)
+#endif
+
 /* Initial memory map size */
 #if ARM64_SWAPPER_USES_SECTION_MAPS
 #define SWAPPER_BLOCK_SHIFT	SECTION_SHIFT
diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index abd64bd1f6d9..e4cff7307d28 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -47,6 +47,9 @@ typedef unsigned long mm_segment_t;
 struct thread_info {
 	unsigned long		flags;		/* low level flags */
 	mm_segment_t		addr_limit;	/* address limit */
+#ifdef CONFIG_ARM64_TTBR0_PAN
+	u64			ttbr0;		/* saved TTBR0_EL1 */
+#endif
 	struct task_struct	*task;		/* main task structure */
 	int			preempt_count;	/* 0 => preemptable, <0 => bug */
 	int			cpu;		/* cpu */
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index fde5f7a13030..3b2cc7787d5a 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -29,6 +29,7 @@
 
 #include <asm/alternative.h>
 #include <asm/cpufeature.h>
+#include <asm/kernel-pgtable.h>
 #include <asm/ptrace.h>
 #include <asm/sysreg.h>
 #include <asm/errno.h>
@@ -116,16 +117,56 @@ static inline void set_fs(mm_segment_t fs)
 /*
  * User access enabling/disabling.
  */
+#ifdef CONFIG_ARM64_TTBR0_PAN
+static inline void uaccess_ttbr0_disable(void)
+{
+	unsigned long ttbr;
+
+	/* reserved_ttbr0 placed at the end of swapper_pg_dir */
+	ttbr = read_sysreg(ttbr1_el1) + SWAPPER_DIR_SIZE;
+	write_sysreg(ttbr, ttbr0_el1);
+	isb();
+}
+
+static inline void uaccess_ttbr0_enable(void)
+{
+	unsigned long flags;
+
+	/*
+	 * Disable interrupts to avoid preemption and potential saved
+	 * TTBR0_EL1 updates between reading the variable and the MSR.
+	 */
+	local_irq_save(flags);
+	write_sysreg(current_thread_info()->ttbr0, ttbr0_el1);
+	isb();
+	local_irq_restore(flags);
+}
+#else
+static inline void uaccess_ttbr0_disable(void)
+{
+}
+
+static inline void uaccess_ttbr0_enable(void)
+{
+}
+#endif
+
 #define uaccess_disable(alt)						\
 do {									\
-	asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1), alt,			\
-			CONFIG_ARM64_PAN));				\
+	if (system_supports_ttbr0_pan())				\
+		uaccess_ttbr0_disable();				\
+	else								\
+		asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1), alt,		\
+				CONFIG_ARM64_PAN));			\
 } while (0)
 
 #define uaccess_enable(alt)						\
 do {									\
-	asm(ALTERNATIVE("nop", SET_PSTATE_PAN(0), alt,			\
-			CONFIG_ARM64_PAN));				\
+	if (system_supports_ttbr0_pan())				\
+		uaccess_ttbr0_enable();					\
+	else								\
+		asm(ALTERNATIVE("nop", SET_PSTATE_PAN(0), alt,		\
+				CONFIG_ARM64_PAN));			\
 } while (0)
 
 /*
@@ -338,11 +379,36 @@ extern __must_check long strnlen_user(const char __user *str, long n);
 
 #include <asm/alternative.h>
 #include <asm/assembler.h>
+#include <asm/kernel-pgtable.h>
 
 /*
  * User access enabling/disabling macros.
  */
+	.macro	uaccess_ttbr0_disable, tmp1
+	mrs	\tmp1, ttbr1_el1		// swapper_pg_dir
+	add	\tmp1, \tmp1, #SWAPPER_DIR_SIZE	// reserved_ttbr0 at the end of swapper_pg_dir
+	msr	ttbr0_el1, \tmp1		// set reserved TTBR0_EL1
+	isb
+	.endm
+
+	.macro	uaccess_ttbr0_enable, tmp1
+	get_thread_info \tmp1
+	ldr	\tmp1, [\tmp1, #TI_TTBR0]	// load saved TTBR0_EL1
+	msr	ttbr0_el1, \tmp1		// set the non-PAN TTBR0_EL1
+	isb
+	.endm
+
 	.macro	uaccess_disable, tmp1
+#ifdef CONFIG_ARM64_TTBR0_PAN
+alternative_if_not ARM64_HAS_PAN
+	uaccess_ttbr0_disable \tmp1
+alternative_else
+	nop
+	nop
+	nop
+	nop
+alternative_endif
+#endif
 alternative_if_not ARM64_ALT_PAN_NOT_UAO
 	nop
 alternative_else
@@ -351,6 +417,21 @@ alternative_endif
 	.endm
 
 	.macro	uaccess_enable, tmp1, tmp2
+#ifdef CONFIG_ARM64_TTBR0_PAN
+alternative_if_not ARM64_HAS_PAN
+	save_and_disable_irq \tmp2		// avoid preemption
+	uaccess_ttbr0_enable \tmp1
+	restore_irq \tmp2
+alternative_else
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+	nop
+alternative_endif
+#endif
 alternative_if_not ARM64_ALT_PAN_NOT_UAO
 	nop
 alternative_else
diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 05070b72fc28..0af4d9a6c10d 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -38,6 +38,9 @@ int main(void)
   DEFINE(TI_FLAGS,		offsetof(struct thread_info, flags));
   DEFINE(TI_PREEMPT,		offsetof(struct thread_info, preempt_count));
   DEFINE(TI_ADDR_LIMIT,		offsetof(struct thread_info, addr_limit));
+#ifdef CONFIG_ARM64_TTBR0_PAN
+  DEFINE(TI_TTBR0,		offsetof(struct thread_info, ttbr0));
+#endif
   DEFINE(TI_TASK,		offsetof(struct thread_info, task));
   DEFINE(TI_CPU,		offsetof(struct thread_info, cpu));
   BLANK();
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 62272eac1352..fd0971afd142 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -45,6 +45,7 @@ unsigned int compat_elf_hwcap2 __read_mostly;
 #endif
 
 DECLARE_BITMAP(cpu_hwcaps, ARM64_NCAPS);
+EXPORT_SYMBOL(cpu_hwcaps);
 
 #define __ARM64_FTR_BITS(SIGNED, STRICT, TYPE, SHIFT, WIDTH, SAFE_VAL) \
 	{						\
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 441420ca7d08..be1e3987c07a 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -190,10 +190,6 @@ alternative_endif
 	eret					// return to kernel
 	.endm
 
-	.macro	get_thread_info, rd
-	mrs	\rd, sp_el0
-	.endm
-
 	.macro	irq_stack_entry
 	mov	x19, sp			// preserve the original sp
 
diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S
index 3e7b050e99dc..d4188396302f 100644
--- a/arch/arm64/kernel/head.S
+++ b/arch/arm64/kernel/head.S
@@ -320,14 +320,14 @@ __create_page_tables:
 	 * dirty cache lines being evicted.
 	 */
 	mov	x0, x25
-	add	x1, x26, #SWAPPER_DIR_SIZE
+	add	x1, x26, #SWAPPER_DIR_SIZE + RESERVED_TTBR0_SIZE
 	bl	__inval_cache_range
 
 	/*
 	 * Clear the idmap and swapper page tables.
 	 */
 	mov	x0, x25
-	add	x6, x26, #SWAPPER_DIR_SIZE
+	add	x6, x26, #SWAPPER_DIR_SIZE + RESERVED_TTBR0_SIZE
 1:	stp	xzr, xzr, [x0], #16
 	stp	xzr, xzr, [x0], #16
 	stp	xzr, xzr, [x0], #16
@@ -406,7 +406,7 @@ __create_page_tables:
 	 * tables again to remove any speculatively loaded cache lines.
 	 */
 	mov	x0, x25
-	add	x1, x26, #SWAPPER_DIR_SIZE
+	add	x1, x26, #SWAPPER_DIR_SIZE + RESERVED_TTBR0_SIZE
 	dmb	sy
 	bl	__inval_cache_range
 
diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
index 659963d40bb4..fe393ccf9352 100644
--- a/arch/arm64/kernel/vmlinux.lds.S
+++ b/arch/arm64/kernel/vmlinux.lds.S
@@ -196,6 +196,11 @@ SECTIONS
 	swapper_pg_dir = .;
 	. += SWAPPER_DIR_SIZE;
 
+#ifdef CONFIG_ARM64_TTBR0_PAN
+	reserved_ttbr0 = .;
+	. += PAGE_SIZE;
+#endif
+
 	_end = .;
 
 	STABS_DEBUG

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 4/7] arm64: Disable TTBR0_EL1 during normal kernel execution
  2016-09-02 15:02 [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching Catalin Marinas
                   ` (2 preceding siblings ...)
  2016-09-02 15:02 ` [PATCH v2 3/7] arm64: Introduce uaccess_{disable, enable} functionality based on TTBR0_EL1 Catalin Marinas
@ 2016-09-02 15:02 ` Catalin Marinas
  2016-09-06 17:31   ` Mark Rutland
  2016-09-02 15:02 ` [PATCH v2 5/7] arm64: Handle faults caused by inadvertent user access with PAN enabled Catalin Marinas
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 30+ messages in thread
From: Catalin Marinas @ 2016-09-02 15:02 UTC (permalink / raw)
  To: linux-arm-kernel

When the TTBR0 PAN feature is enabled, the kernel entry points need to
disable access to TTBR0_EL1. The PAN status of the interrupted context
is stored as part of the saved pstate, reusing the PSR_PAN_BIT (22).
Restoring access to TTBR0_PAN is done on exception return if returning
to user or returning to a context where PAN was disabled.

Context switching via switch_mm() must defer the update of TTBR0_EL1
until a return to user or an explicit uaccess_enable() call.

Special care needs to be taken for two cases where TTBR0_EL1 is set
outside the normal kernel context switch operation: EFI run-time
services (via efi_set_pgd) and CPU suspend (via cpu_(un)install_idmap).
Code has been added to avoid deferred TTBR0_EL1 switching as in
switch_mm() and restore the reserved TTBR0_EL1 when uninstalling the
special TTBR0_EL1.

Cc: Will Deacon <will.deacon@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/include/asm/efi.h         | 14 ++++++++
 arch/arm64/include/asm/mmu_context.h | 32 +++++++++++++----
 arch/arm64/include/asm/ptrace.h      |  2 ++
 arch/arm64/kernel/entry.S            | 67 ++++++++++++++++++++++++++++++++++++
 arch/arm64/kernel/setup.c            |  8 +++++
 arch/arm64/mm/context.c              |  7 +++-
 6 files changed, 122 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index a9e54aad15ef..1d7810b88255 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -1,6 +1,7 @@
 #ifndef _ASM_EFI_H
 #define _ASM_EFI_H
 
+#include <asm/cpufeature.h>
 #include <asm/io.h>
 #include <asm/mmu_context.h>
 #include <asm/neon.h>
@@ -76,6 +77,19 @@ static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt)
 static inline void efi_set_pgd(struct mm_struct *mm)
 {
 	switch_mm(NULL, mm, NULL);
+
+	/*
+	 * Force TTBR0_EL1 setting. If restoring the active_mm pgd, defer the
+	 * switching after uaccess_enable(). This code is calling
+	 * cpu_switch_mm() directly (instead of uaccess_enable()) to force
+	 * potential errata workarounds.
+	 */
+	if (system_supports_ttbr0_pan()) {
+		if (mm != current->active_mm)
+			cpu_switch_mm(mm->pgd, mm);
+		else
+			cpu_set_reserved_ttbr0();
+	}
 }
 
 void efi_virtmap_load(void);
diff --git a/arch/arm64/include/asm/mmu_context.h b/arch/arm64/include/asm/mmu_context.h
index b1892a0dbcb0..cab90250daae 100644
--- a/arch/arm64/include/asm/mmu_context.h
+++ b/arch/arm64/include/asm/mmu_context.h
@@ -23,6 +23,7 @@
 #include <linux/sched.h>
 
 #include <asm/cacheflush.h>
+#include <asm/cpufeature.h>
 #include <asm/proc-fns.h>
 #include <asm-generic/mm_hooks.h>
 #include <asm/cputype.h>
@@ -113,7 +114,7 @@ static inline void cpu_uninstall_idmap(void)
 	local_flush_tlb_all();
 	cpu_set_default_tcr_t0sz();
 
-	if (mm != &init_mm)
+	if (mm != &init_mm && !system_supports_ttbr0_pan())
 		cpu_switch_mm(mm->pgd, mm);
 }
 
@@ -180,14 +181,11 @@ enter_lazy_tlb(struct mm_struct *mm, struct task_struct *tsk)
  * actually changed.
  */
 static inline void
-switch_mm(struct mm_struct *prev, struct mm_struct *next,
-	  struct task_struct *tsk)
+__switch_mm(struct mm_struct *prev, struct mm_struct *next,
+	    struct task_struct *tsk)
 {
 	unsigned int cpu = smp_processor_id();
 
-	if (prev == next)
-		return;
-
 	/*
 	 * init_mm.pgd does not contain any user mappings and it is always
 	 * active for kernel addresses in TTBR1. Just set the reserved TTBR0.
@@ -200,8 +198,28 @@ switch_mm(struct mm_struct *prev, struct mm_struct *next,
 	check_and_switch_context(next, cpu);
 }
 
+static inline void
+switch_mm(struct mm_struct *prev, struct mm_struct *next,
+	  struct task_struct *tsk)
+{
+	if (prev != next)
+		__switch_mm(prev, next, tsk);
+
+#ifdef CONFIG_ARM64_TTBR0_PAN
+	/*
+	 * Update the saved TTBR0_EL1 of the scheduled-in task as the previous
+	 * value may have not been initialised yet (activate_mm caller) or the
+	 * ASID has changed since the last run (following the context switch
+	 * of another thread of the same process).
+	 */
+	if (tsk && system_supports_ttbr0_pan())
+		task_thread_info(tsk)->ttbr0 =
+			virt_to_phys(next->pgd) | ASID(next) << 48;
+#endif
+}
+
 #define deactivate_mm(tsk,mm)	do { } while (0)
-#define activate_mm(prev,next)	switch_mm(prev, next, NULL)
+#define activate_mm(prev,next)	switch_mm(prev, next, current)
 
 void verify_cpu_asid_bits(void);
 
diff --git a/arch/arm64/include/asm/ptrace.h b/arch/arm64/include/asm/ptrace.h
index ada08b5b036d..458773ac5ec9 100644
--- a/arch/arm64/include/asm/ptrace.h
+++ b/arch/arm64/include/asm/ptrace.h
@@ -21,6 +21,8 @@
 
 #include <uapi/asm/ptrace.h>
 
+#define _PSR_PAN_BIT		22
+
 /* Current Exception Level values, as contained in CurrentEL */
 #define CurrentEL_EL1		(1 << 2)
 #define CurrentEL_EL2		(2 << 2)
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index be1e3987c07a..e87cfeda5da5 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -29,7 +29,9 @@
 #include <asm/esr.h>
 #include <asm/irq.h>
 #include <asm/memory.h>
+#include <asm/ptrace.h>
 #include <asm/thread_info.h>
+#include <asm/uaccess.h>
 #include <asm/unistd.h>
 
 /*
@@ -109,6 +111,34 @@
 	mrs	x22, elr_el1
 	mrs	x23, spsr_el1
 	stp	lr, x21, [sp, #S_LR]
+
+#ifdef CONFIG_ARM64_TTBR0_PAN
+	/*
+	 * Set the TTBR0 PAN in SPSR. When the exception is taken from EL0,
+	 * there is no need to check the state of TTBR0_EL1 since accesses are
+	 * always enabled.
+	 * Note that the meaning of this bit differs from the ARMv8.1 PAN
+	 * feature as all TTBR0_EL1 accesses are disabled, not just those to
+	 * user mappings.
+	 */
+alternative_if_not ARM64_HAS_PAN
+	nop
+alternative_else
+	b	1f				// skip TTBR0 PAN
+alternative_endif
+
+	.if	\el != 0
+	mrs	x21, ttbr0_el1
+	tst	x21, #0xffff << 48		// Check for the reserved ASID
+	orr	x23, x23, #PSR_PAN_BIT		// Set the emulated PAN in the saved SPSR
+	b.eq	1f				// TTBR0 access already disabled
+	and	x23, x23, #~PSR_PAN_BIT		// Clear the emulated PAN in the saved SPSR
+	.endif
+
+	uaccess_ttbr0_disable x21
+1:
+#endif
+
 	stp	x22, x23, [sp, #S_PC]
 
 	/*
@@ -147,6 +177,42 @@
 	ldp	x21, x22, [sp, #S_PC]		// load ELR, SPSR
 	.if	\el == 0
 	ct_user_enter
+	.endif
+
+#ifdef CONFIG_ARM64_TTBR0_PAN
+	/*
+	 * Restore access to TTBR0_EL1. If returning to EL0, no need for SPSR
+	 * PAN bit checking.
+	 */
+alternative_if_not ARM64_HAS_PAN
+	nop
+alternative_else
+	b	2f				// skip TTBR0 PAN
+alternative_endif
+
+	.if	\el != 0
+	tbnz	x22, #_PSR_PAN_BIT, 1f		// Skip re-enabling TTBR0 access if previously disabled
+	.endif
+
+	uaccess_ttbr0_enable x0
+
+	.if	\el == 0
+	/*
+	 * Enable errata workarounds only if returning to user. The only
+	 * workaround currently required for TTBR0_EL1 changes are for the
+	 * Cavium erratum 27456 (broadcast TLBI instructions may cause I-cache
+	 * corruption).
+	 */
+	post_ttbr0_update_workaround
+	.endif
+1:
+	.if	\el != 0
+	and	x22, x22, #~PSR_PAN_BIT		// ARMv8.0 CPUs do not understand this bit
+	.endif
+2:
+#endif
+
+	.if	\el == 0
 	ldr	x23, [sp, #S_SP]		// load return stack pointer
 	msr	sp_el0, x23
 #ifdef CONFIG_ARM64_ERRATUM_845719
@@ -168,6 +234,7 @@ alternative_else
 alternative_endif
 #endif
 	.endif
+
 	msr	elr_el1, x21			// set up the return data
 	msr	spsr_el1, x22
 	ldp	x0, x1, [sp, #16 * 0]
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 536dce22fe76..4a4aaa47f869 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -228,6 +228,14 @@ void __init setup_arch(char **cmdline_p)
 {
 	pr_info("Boot CPU: AArch64 Processor [%08x]\n", read_cpuid_id());
 
+#ifdef CONFIG_ARM64_TTBR0_PAN
+	/*
+	 * uaccess_enable() may be called on the init thread, so make sure
+	 * the saved TTBR0_EL1 always generates translation faults.
+	 */
+	init_thread_info.ttbr0 = virt_to_phys(empty_zero_page);
+#endif
+
 	sprintf(init_utsname()->machine, ELF_PLATFORM);
 	init_mm.start_code = (unsigned long) _text;
 	init_mm.end_code   = (unsigned long) _etext;
diff --git a/arch/arm64/mm/context.c b/arch/arm64/mm/context.c
index efcf1f7ef1e4..d120a7911d22 100644
--- a/arch/arm64/mm/context.c
+++ b/arch/arm64/mm/context.c
@@ -221,7 +221,12 @@ void check_and_switch_context(struct mm_struct *mm, unsigned int cpu)
 	raw_spin_unlock_irqrestore(&cpu_asid_lock, flags);
 
 switch_mm_fastpath:
-	cpu_switch_mm(mm->pgd, mm);
+	/*
+	 * Defer TTBR0_EL1 setting for user threads to uaccess_enable() when
+	 * emulating PAN.
+	 */
+	if (!system_supports_ttbr0_pan())
+		cpu_switch_mm(mm->pgd, mm);
 }
 
 static int asids_init(void)

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 5/7] arm64: Handle faults caused by inadvertent user access with PAN enabled
  2016-09-02 15:02 [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching Catalin Marinas
                   ` (3 preceding siblings ...)
  2016-09-02 15:02 ` [PATCH v2 4/7] arm64: Disable TTBR0_EL1 during normal kernel execution Catalin Marinas
@ 2016-09-02 15:02 ` Catalin Marinas
  2016-09-02 15:02 ` [PATCH v2 6/7] arm64: xen: Enable user access before a privcmd hvc call Catalin Marinas
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 30+ messages in thread
From: Catalin Marinas @ 2016-09-02 15:02 UTC (permalink / raw)
  To: linux-arm-kernel

When TTBR0_EL1 is set to the reserved page, an erroneous kernel access
to user space would generate a translation fault. This patch adds the
checks for the software-set PSR_PAN_BIT to emulate a permission fault
and report it accordingly.

This patch also updates the description of the synchronous external
aborts on translation table walks.

Cc: Will Deacon <will.deacon@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/mm/fault.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 05d2bd776c69..bd2af612f0df 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -268,13 +268,19 @@ out:
 	return fault;
 }
 
-static inline bool is_permission_fault(unsigned int esr)
+static inline bool is_permission_fault(unsigned int esr, struct pt_regs *regs)
 {
 	unsigned int ec       = ESR_ELx_EC(esr);
 	unsigned int fsc_type = esr & ESR_ELx_FSC_TYPE;
 
-	return (ec == ESR_ELx_EC_DABT_CUR && fsc_type == ESR_ELx_FSC_PERM) ||
-	       (ec == ESR_ELx_EC_IABT_CUR && fsc_type == ESR_ELx_FSC_PERM);
+	if (ec != ESR_ELx_EC_DABT_CUR && ec != ESR_ELx_EC_IABT_CUR)
+		return false;
+
+	if (system_supports_ttbr0_pan())
+		return fsc_type == ESR_ELx_FSC_FAULT &&
+			(regs->pstate & PSR_PAN_BIT);
+	else
+		return fsc_type == ESR_ELx_FSC_PERM;
 }
 
 static bool is_el0_instruction_abort(unsigned int esr)
@@ -314,7 +320,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
 		mm_flags |= FAULT_FLAG_WRITE;
 	}
 
-	if (is_permission_fault(esr) && (addr < USER_DS)) {
+	if (addr < USER_DS && is_permission_fault(esr, regs)) {
 		/* regs->orig_addr_limit may be 0 if we entered from EL0 */
 		if (regs->orig_addr_limit == KERNEL_DS)
 			die("Accessing user space memory with fs=KERNEL_DS", regs, esr);
@@ -506,10 +512,10 @@ static const struct fault_info {
 	{ do_bad,		SIGBUS,  0,		"unknown 17"			},
 	{ do_bad,		SIGBUS,  0,		"unknown 18"			},
 	{ do_bad,		SIGBUS,  0,		"unknown 19"			},
-	{ do_bad,		SIGBUS,  0,		"synchronous abort (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous abort (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous abort (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous abort (translation table walk)" },
+	{ do_bad,		SIGBUS,  0,		"synchronous external abort (translation table walk)" },
+	{ do_bad,		SIGBUS,  0,		"synchronous external abort (translation table walk)" },
+	{ do_bad,		SIGBUS,  0,		"synchronous external abort (translation table walk)" },
+	{ do_bad,		SIGBUS,  0,		"synchronous external abort (translation table walk)" },
 	{ do_bad,		SIGBUS,  0,		"synchronous parity error"	},
 	{ do_bad,		SIGBUS,  0,		"unknown 25"			},
 	{ do_bad,		SIGBUS,  0,		"unknown 26"			},

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 6/7] arm64: xen: Enable user access before a privcmd hvc call
  2016-09-02 15:02 [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching Catalin Marinas
                   ` (4 preceding siblings ...)
  2016-09-02 15:02 ` [PATCH v2 5/7] arm64: Handle faults caused by inadvertent user access with PAN enabled Catalin Marinas
@ 2016-09-02 15:02 ` Catalin Marinas
  2016-09-02 15:02 ` [PATCH v2 7/7] arm64: Enable CONFIG_ARM64_TTBR0_PAN Catalin Marinas
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 30+ messages in thread
From: Catalin Marinas @ 2016-09-02 15:02 UTC (permalink / raw)
  To: linux-arm-kernel

Privcmd calls are issued by the userspace. The kernel needs to enable
access to TTBR0_EL1 as the hypervisor would issue stage 1 translations
to user memory via AT instructions. Since AT instructions are not
affected by the PAN bit (ARMv8.1), we only need the explicit
uaccess_enable/disable if the TTBR0 PAN option is enabled.

Reviewed-by: Julien Grall <julien.grall@arm.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/xen/hypercall.S | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/arm64/xen/hypercall.S b/arch/arm64/xen/hypercall.S
index 329c8027b0a9..211620dde4de 100644
--- a/arch/arm64/xen/hypercall.S
+++ b/arch/arm64/xen/hypercall.S
@@ -49,6 +49,7 @@
 
 #include <linux/linkage.h>
 #include <asm/assembler.h>
+#include <asm/uaccess.h>
 #include <xen/interface/xen.h>
 
 
@@ -91,6 +92,24 @@ ENTRY(privcmd_call)
 	mov x2, x3
 	mov x3, x4
 	mov x4, x5
+#ifdef CONFIG_ARM64_TTBR0_PAN
+	/*
+	 * Privcmd calls are issued by the userspace. The kernel needs to
+	 * enable access to TTBR0_EL1 as the hypervisor would issue stage 1
+	 * translations to user memory via AT instructions. Since AT
+	 * instructions are not affected by the PAN bit (ARMv8.1), we only
+	 * need the explicit uaccess_enable/disable if the TTBR0 PAN option is
+	 * enabled.
+	 */
+	uaccess_enable x6, x7
+#endif
 	hvc XEN_IMM
+
+#ifdef CONFIG_ARM64_TTBR0_PAN
+	/*
+	 * Disable userspace access from kernel once the hyp call completed.
+	 */
+	uaccess_disable x6
+#endif
 	ret
 ENDPROC(privcmd_call);

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 7/7] arm64: Enable CONFIG_ARM64_TTBR0_PAN
  2016-09-02 15:02 [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching Catalin Marinas
                   ` (5 preceding siblings ...)
  2016-09-02 15:02 ` [PATCH v2 6/7] arm64: xen: Enable user access before a privcmd hvc call Catalin Marinas
@ 2016-09-02 15:02 ` Catalin Marinas
  2016-09-02 15:47   ` Mark Rutland
  2016-09-07 23:20 ` [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching Kees Cook
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 30+ messages in thread
From: Catalin Marinas @ 2016-09-02 15:02 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds the Kconfig option to enable support for TTBR0 PAN. The
option is default off because of a slight performance hit when enabled,
caused by the additional TTBR0_EL1 switching during user access
operations or exception entry/exit code.

Cc: Will Deacon <will.deacon@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/Kconfig | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index bc3f00f586f1..3fb9a6ce464d 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -785,6 +785,14 @@ config SETEND_EMULATION
 	  If unsure, say Y
 endif
 
+config ARM64_TTBR0_PAN
+	bool "Priviledged Access Never using TTBR0_EL1 switching"
+	help
+	  Enabling this option prevents the kernel from accessing
+	  user-space memory directly by pointing TTBR0_EL1 to a reserved
+	  zeroed area and reserved ASID. The user access routines
+	  restore the valid TTBR0_EL1 temporarily.
+
 menu "ARMv8.1 architectural features"
 
 config ARM64_HW_AFDBM

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 7/7] arm64: Enable CONFIG_ARM64_TTBR0_PAN
  2016-09-02 15:02 ` [PATCH v2 7/7] arm64: Enable CONFIG_ARM64_TTBR0_PAN Catalin Marinas
@ 2016-09-02 15:47   ` Mark Rutland
  0 siblings, 0 replies; 30+ messages in thread
From: Mark Rutland @ 2016-09-02 15:47 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Sep 02, 2016 at 04:02:13PM +0100, Catalin Marinas wrote:
> This patch adds the Kconfig option to enable support for TTBR0 PAN. The
> option is default off because of a slight performance hit when enabled,
> caused by the additional TTBR0_EL1 switching during user access
> operations or exception entry/exit code.
> 
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  arch/arm64/Kconfig | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index bc3f00f586f1..3fb9a6ce464d 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -785,6 +785,14 @@ config SETEND_EMULATION
>  	  If unsure, say Y
>  endif
>  
> +config ARM64_TTBR0_PAN
> +	bool "Priviledged Access Never using TTBR0_EL1 switching"

Minor nit/bikeshed, but could we follow the example of arch/arm's
SW_DOMAIN_PAN and call this ARM64_SW_TTRBR0_PAN, prepending "Emulate "
to the dsecription?

That makes it very clear that this is a SW feature, rather than using
the real PAN, but only on TTBR0.

Thanks,
Mark.

> +	help
> +	  Enabling this option prevents the kernel from accessing
> +	  user-space memory directly by pointing TTBR0_EL1 to a reserved
> +	  zeroed area and reserved ASID. The user access routines
> +	  restore the valid TTBR0_EL1 temporarily.
> +
>  menu "ARMv8.1 architectural features"
>  
>  config ARM64_HW_AFDBM
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 1/7] arm64: Factor out PAN enabling/disabling into separate uaccess_* macros
  2016-09-02 15:02 ` [PATCH v2 1/7] arm64: Factor out PAN enabling/disabling into separate uaccess_* macros Catalin Marinas
@ 2016-09-05 15:38   ` Mark Rutland
  2016-09-12 14:52     ` Catalin Marinas
  0 siblings, 1 reply; 30+ messages in thread
From: Mark Rutland @ 2016-09-05 15:38 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Catalin,

On Fri, Sep 02, 2016 at 04:02:07PM +0100, Catalin Marinas wrote:
> This patch moves the directly coded alternatives for turning PAN on/off
> into separate uaccess_{enable,disable} macros or functions. The asm
> macros take a few arguments which will be used in subsequent patches.
> 
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  arch/arm64/include/asm/futex.h       | 14 ++++-----
>  arch/arm64/include/asm/uaccess.h     | 55 ++++++++++++++++++++++++++++++------
>  arch/arm64/kernel/armv8_deprecated.c | 10 +++----
>  arch/arm64/lib/clear_user.S          |  8 ++----
>  arch/arm64/lib/copy_from_user.S      |  8 ++----
>  arch/arm64/lib/copy_in_user.S        |  8 ++----
>  arch/arm64/lib/copy_to_user.S        |  8 ++----
>  7 files changed, 71 insertions(+), 40 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/futex.h b/arch/arm64/include/asm/futex.h
> index f2585cdd32c2..7e5f236093be 100644
> --- a/arch/arm64/include/asm/futex.h
> +++ b/arch/arm64/include/asm/futex.h
> @@ -27,9 +27,9 @@
>  #include <asm/sysreg.h>
>  
>  #define __futex_atomic_op(insn, ret, oldval, uaddr, tmp, oparg)		\
> +do {									\
> +	uaccess_enable(ARM64_HAS_PAN);					\
>  	asm volatile(							\
> -	ALTERNATIVE("nop", SET_PSTATE_PAN(0), ARM64_HAS_PAN,		\
> -		    CONFIG_ARM64_PAN)					\
>  "	prfm	pstl1strm, %2\n"					\
>  "1:	ldxr	%w1, %2\n"						\
>  	insn "\n"							\
> @@ -44,11 +44,11 @@
>  "	.popsection\n"							\
>  	_ASM_EXTABLE(1b, 4b)						\
>  	_ASM_EXTABLE(2b, 4b)						\
> -	ALTERNATIVE("nop", SET_PSTATE_PAN(1), ARM64_HAS_PAN,		\
> -		    CONFIG_ARM64_PAN)					\
>  	: "=&r" (ret), "=&r" (oldval), "+Q" (*uaddr), "=&r" (tmp)	\
>  	: "r" (oparg), "Ir" (-EFAULT)					\
> -	: "memory")
> +	: "memory");							\
> +	uaccess_disable(ARM64_HAS_PAN);					\
> +} while (0)

It might be worth noting in the commit message that this change means
that any memory accesses the compiler decides to spill between uaccess_*
calls and the main asm block are unprotected, but that's unlikely to be
an issue in practice.

[...]

>  /*
> + * User access enabling/disabling.
> + */
> +#define uaccess_disable(alt)						\
> +do {									\
> +	asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1), alt,			\
> +			CONFIG_ARM64_PAN));				\
> +} while (0)
> +
> +#define uaccess_enable(alt)						\
> +do {									\
> +	asm(ALTERNATIVE("nop", SET_PSTATE_PAN(0), alt,			\
> +			CONFIG_ARM64_PAN));				\
> +} while (0)

Passing the alternative down is somewhat confusing. e.g. in the futex
case it looks like we're only doing something when PAN is present,
whereas we'll manipulate TTBR0 in the absence of PAN.

If I've understood correctly, we need this to distinguish regular
load/store uaccess sequences (eg. the futex code) from potentially
patched unprivileged load/store sequences (e.g. {get,put}_user) when
poking PSTATE.PAN.

So perhaps we could ahve something like:

* privileged_uaccess_{enable,disable}()
  Which toggle TTBR0, or PAN (always).
  These would handle cases like the futex/swp code.
 
* (unprivileged_)uaccess_{enable,disable}()
  Which toggle TTBR0, or PAN (in the absence of UAO).
  These would handle cases like the {get,put}_user sequences.

Though perhaps that is just as confusing. ;)

Otherwise, this looks like a nice centralisation of the PSTATE.PAN
manipulation code.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 2/7] arm64: Factor out TTBR0_EL1 post-update workaround into a specific asm macro
  2016-09-02 15:02 ` [PATCH v2 2/7] arm64: Factor out TTBR0_EL1 post-update workaround into a specific asm macro Catalin Marinas
@ 2016-09-05 16:11   ` Mark Rutland
  0 siblings, 0 replies; 30+ messages in thread
From: Mark Rutland @ 2016-09-05 16:11 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Catalin, 

On Fri, Sep 02, 2016 at 04:02:08PM +0100, Catalin Marinas wrote:
> This patch takes the errata workaround code out of cpu_do_switch_mm into
> a dedicated post_ttbr0_update_workaround macro which will be reused in a
> subsequent patch.

> +/*
> + * Errata workaround post TTBR0_EL1 update.
> + */
> +	.macro	post_ttbr0_update_workaround, ret = 0
> +alternative_if_not ARM64_WORKAROUND_CAVIUM_27456
> +	.if	\ret
> +	ret
> +	.endif
> +	nop
> +	nop
> +	nop
> +alternative_else
> +	ic	iallu
> +	dsb	nsh
> +	isb
> +	.if	\ret
> +	ret
> +	.endif
> +alternative_endif
> +	.endm

IMO, the ret parameter makes the callers harder to read. Can we leave
the ret up to the caller and suffer the marginal penalty of a few nops
in a slow(ish) path? 

We can get rid of them in the !CONFIG_CAVIUM_ERRATUM_27456 case with a
simple ifdef:

	.macro post_ttbr0_update_workaround	
#ifdef CONFIG_CAVIUM_ERRATUM_27456
alternative_if_not ARM64_WORKAROUND_CAVIUM_27456
	nop
	nop
	nop
alternative_else
	ic	iallu
	dsb	nsh
	isb
alternative_endif
#endif
	.endm

> +
>  #endif	/* __ASM_ASSEMBLER_H */
> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index 5bb61de23201..9359659f2559 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -125,17 +125,7 @@ ENTRY(cpu_do_switch_mm)
>  	bfi	x0, x1, #48, #16		// set the ASID
>  	msr	ttbr0_el1, x0			// set TTBR0
>  	isb
> -alternative_if_not ARM64_WORKAROUND_CAVIUM_27456
> -	ret
> -	nop
> -	nop
> -	nop
> -alternative_else
> -	ic	iallu
> -	dsb	nsh
> -	isb
> -	ret
> -alternative_endif
> +	post_ttbr0_update_workaround ret = 1

+	ret

Thanks,
Mark,

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 3/7] arm64: Introduce uaccess_{disable, enable} functionality based on TTBR0_EL1
  2016-09-02 15:02 ` [PATCH v2 3/7] arm64: Introduce uaccess_{disable, enable} functionality based on TTBR0_EL1 Catalin Marinas
@ 2016-09-05 17:20   ` Mark Rutland
  2016-09-06 10:27     ` Catalin Marinas
  2016-09-09 17:15   ` Catalin Marinas
  1 sibling, 1 reply; 30+ messages in thread
From: Mark Rutland @ 2016-09-05 17:20 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Catalin,

On Fri, Sep 02, 2016 at 04:02:09PM +0100, Catalin Marinas wrote:
> This patch adds the uaccess macros/functions to disable access to user
> space by setting TTBR0_EL1 to a reserved zeroed page. Since the value
> written to TTBR0_EL1 must be a physical address, for simplicity this
> patch introduces a reserved_ttbr0 page at a constant offset from
> swapper_pg_dir. The uaccess_disable code uses the ttbr1_el1 value
> adjusted by the reserved_ttbr0 offset.
> 
> Enabling access to user is done by restoring TTBR0_EL1 with the value
> from the struct thread_info ttbr0 variable. Interrupts must be disabled
> during the uaccess_ttbr0_enable code to ensure the atomicity of the
> thread_info.ttbr0 read and TTBR0_EL1 write.

[...]

>  /*
> + * Return the current thread_info.
> + */
> +	.macro	get_thread_info, rd
> +	mrs	\rd, sp_el0
> +	.endm

It may be worth noting in the commit message that we had to factor this
out of entry.S, or doing that as a preparatory patch.

> +/*
>   * Errata workaround post TTBR0_EL1 update.
>   */
>  	.macro	post_ttbr0_update_workaround, ret = 0
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 7099f26e3702..023066d9bf7f 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -216,6 +216,12 @@ static inline bool system_supports_mixed_endian_el0(void)
>  	return id_aa64mmfr0_mixed_endian_el0(read_system_reg(SYS_ID_AA64MMFR0_EL1));
>  }
>  
> +static inline bool system_supports_ttbr0_pan(void)
> +{
> +	return IS_ENABLED(CONFIG_ARM64_TTBR0_PAN) &&
> +		!cpus_have_cap(ARM64_HAS_PAN);
> +}
> +

Nit: s/supports/uses/ would be clearer.

[...]

> +#ifdef CONFIG_ARM64_TTBR0_PAN
> +#define RESERVED_TTBR0_SIZE	(PAGE_SIZE)
> +#else
> +#define RESERVED_TTBR0_SIZE	(0)
> +#endif

I was going to suggest that we use the empty_zero_page, which we can
address with an adrp, because I had forgotten that we need to generate
the *physical* address.

It would be good if we could have a description of why we need the new
reserved page somewhere in the code. I'm sure I won't be the only one
tripped up by this.

It would be possible to use the existing empty_zero_page, if we're happy
to have a MOVZ; MOVK; MOVK; MOVK sequence that we patch at boot-time.
That could be faster than an MRS on some implementations.

We don't (yet) have infrastructure for that, though.

[...]

> +static inline void uaccess_ttbr0_enable(void)
> +{
> +	unsigned long flags;
> +
> +	/*
> +	 * Disable interrupts to avoid preemption and potential saved
> +	 * TTBR0_EL1 updates between reading the variable and the MSR.
> +	 */
> +	local_irq_save(flags);
> +	write_sysreg(current_thread_info()->ttbr0, ttbr0_el1);
> +	isb();
> +	local_irq_restore(flags);
> +}

I don't follow what problem this actually protects us against. In the
case of preemption everything should be saved+restored transparently, or
things would go wrong as soon as we enable IRQs anyway.

Is this a hold-over from a percpu approach rather than the
current_thread_info() approach?

What am I missing?

> +#else
> +static inline void uaccess_ttbr0_disable(void)
> +{
> +}
> +
> +static inline void uaccess_ttbr0_enable(void)
> +{
> +}
> +#endif

I think that it's better to drop the ifdef and add:

	if (!IS_ENABLED(CONFIG_ARM64_TTBR0_PAN))
		return;

... at the start of each function. GCC should optimize the entire thing
away when not used, but we'll get compiler coverage regardless, and
therefore less breakage. All the symbols we required should exist
regardless.

[...]

>  	.macro	uaccess_enable, tmp1, tmp2
> +#ifdef CONFIG_ARM64_TTBR0_PAN
> +alternative_if_not ARM64_HAS_PAN
> +	save_and_disable_irq \tmp2		// avoid preemption
> +	uaccess_ttbr0_enable \tmp1
> +	restore_irq \tmp2
> +alternative_else
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +	nop
> +alternative_endif
> +#endif

How about something like:

	.macro alternative_endif_else_nop
	alternative_else
	.rept ((662b-661b) / 4)
	       nop
	.endr
	alternative_endif
	.endm

So for the above we could have:

	alternative_if_not ARM64_HAS_PAN
		save_and_disable_irq \tmp2
		uaccess_ttbr0_enable \tmp1
		restore_irq \tmp2
	alternative_endif_else_nop

I'll see about spinning a patch, or discovering why that happens to be
broken.

[...]

>  	 * tables again to remove any speculatively loaded cache lines.
>  	 */
>  	mov	x0, x25
> -	add	x1, x26, #SWAPPER_DIR_SIZE
> +	add	x1, x26, #SWAPPER_DIR_SIZE + RESERVED_TTBR0_SIZE
>  	dmb	sy
>  	bl	__inval_cache_range
>  
> diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> index 659963d40bb4..fe393ccf9352 100644
> --- a/arch/arm64/kernel/vmlinux.lds.S
> +++ b/arch/arm64/kernel/vmlinux.lds.S
> @@ -196,6 +196,11 @@ SECTIONS
>  	swapper_pg_dir = .;
>  	. += SWAPPER_DIR_SIZE;
>  
> +#ifdef CONFIG_ARM64_TTBR0_PAN
> +	reserved_ttbr0 = .;
> +	. += PAGE_SIZE;
> +#endif

Surely RESERVED_TTBR0_SIZE, as elsewhere?

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 3/7] arm64: Introduce uaccess_{disable, enable} functionality based on TTBR0_EL1
  2016-09-05 17:20   ` Mark Rutland
@ 2016-09-06 10:27     ` Catalin Marinas
  2016-09-06 10:45       ` Mark Rutland
  0 siblings, 1 reply; 30+ messages in thread
From: Catalin Marinas @ 2016-09-06 10:27 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Sep 05, 2016 at 06:20:38PM +0100, Mark Rutland wrote:
> On Fri, Sep 02, 2016 at 04:02:09PM +0100, Catalin Marinas wrote:
> > +#ifdef CONFIG_ARM64_TTBR0_PAN
> > +#define RESERVED_TTBR0_SIZE	(PAGE_SIZE)
> > +#else
> > +#define RESERVED_TTBR0_SIZE	(0)
> > +#endif
> 
> I was going to suggest that we use the empty_zero_page, which we can
> address with an adrp, because I had forgotten that we need to generate
> the *physical* address.
> 
> It would be good if we could have a description of why we need the new
> reserved page somewhere in the code. I'm sure I won't be the only one
> tripped up by this.
> 
> It would be possible to use the existing empty_zero_page, if we're happy
> to have a MOVZ; MOVK; MOVK; MOVK sequence that we patch at boot-time.
> That could be faster than an MRS on some implementations.

I was trying to keep the number of instructions to a minimum in
preference to potentially slightly faster sequence (I haven't done any
benchmarks). On ARMv8.1+ implementations, we just end up with more nops.

We could also do an ldr from a PC-relative address, it's one instruction
and it may not be (significantly) slower than MRS + ADD.

> > +static inline void uaccess_ttbr0_enable(void)
> > +{
> > +	unsigned long flags;
> > +
> > +	/*
> > +	 * Disable interrupts to avoid preemption and potential saved
> > +	 * TTBR0_EL1 updates between reading the variable and the MSR.
> > +	 */
> > +	local_irq_save(flags);
> > +	write_sysreg(current_thread_info()->ttbr0, ttbr0_el1);
> > +	isb();
> > +	local_irq_restore(flags);
> > +}
> 
> I don't follow what problem this actually protects us against. In the
> case of preemption everything should be saved+restored transparently, or
> things would go wrong as soon as we enable IRQs anyway.
> 
> Is this a hold-over from a percpu approach rather than the
> current_thread_info() approach?

If we get preempted between reading current_thread_info()->ttbr0 and
writing TTBR0_EL1, a series of context switches could lead to the update
of the ASID part of ttbr0. The actual MSR would store an old ASID in
TTBR0_EL1.

> > +#else
> > +static inline void uaccess_ttbr0_disable(void)
> > +{
> > +}
> > +
> > +static inline void uaccess_ttbr0_enable(void)
> > +{
> > +}
> > +#endif
> 
> I think that it's better to drop the ifdef and add:
> 
> 	if (!IS_ENABLED(CONFIG_ARM64_TTBR0_PAN))
> 		return;
> 
> ... at the start of each function. GCC should optimize the entire thing
> away when not used, but we'll get compiler coverage regardless, and
> therefore less breakage. All the symbols we required should exist
> regardless.

The reason for this is that thread_info.ttbr0 is conditionally defined.
I don't think the compiler would ignore it.

> >  	.macro	uaccess_enable, tmp1, tmp2
> > +#ifdef CONFIG_ARM64_TTBR0_PAN
> > +alternative_if_not ARM64_HAS_PAN
> > +	save_and_disable_irq \tmp2		// avoid preemption
> > +	uaccess_ttbr0_enable \tmp1
> > +	restore_irq \tmp2
> > +alternative_else
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +	nop
> > +alternative_endif
> > +#endif
> 
> How about something like:
> 
> 	.macro alternative_endif_else_nop
> 	alternative_else
> 	.rept ((662b-661b) / 4)
> 	       nop
> 	.endr
> 	alternative_endif
> 	.endm
> 
> So for the above we could have:
> 
> 	alternative_if_not ARM64_HAS_PAN
> 		save_and_disable_irq \tmp2
> 		uaccess_ttbr0_enable \tmp1
> 		restore_irq \tmp2
> 	alternative_endif_else_nop
> 
> I'll see about spinning a patch, or discovering why that happens to be
> broken.

This looks better. Minor comment, I would actually name the ending
statement alternative_else_nop_endif to match the order in which you'd
normally write them.

> >  	 * tables again to remove any speculatively loaded cache lines.
> >  	 */
> >  	mov	x0, x25
> > -	add	x1, x26, #SWAPPER_DIR_SIZE
> > +	add	x1, x26, #SWAPPER_DIR_SIZE + RESERVED_TTBR0_SIZE
> >  	dmb	sy
> >  	bl	__inval_cache_range
> >  
> > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> > index 659963d40bb4..fe393ccf9352 100644
> > --- a/arch/arm64/kernel/vmlinux.lds.S
> > +++ b/arch/arm64/kernel/vmlinux.lds.S
> > @@ -196,6 +196,11 @@ SECTIONS
> >  	swapper_pg_dir = .;
> >  	. += SWAPPER_DIR_SIZE;
> >  
> > +#ifdef CONFIG_ARM64_TTBR0_PAN
> > +	reserved_ttbr0 = .;
> > +	. += PAGE_SIZE;
> > +#endif
> 
> Surely RESERVED_TTBR0_SIZE, as elsewhere?

I'll try to move it somewhere where it can be included in vmlinux.lds.S
(I can probably include cpufeature.h directly).

-- 
Catalin

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 3/7] arm64: Introduce uaccess_{disable, enable} functionality based on TTBR0_EL1
  2016-09-06 10:27     ` Catalin Marinas
@ 2016-09-06 10:45       ` Mark Rutland
  2016-09-11 13:55         ` [kernel-hardening] " Ard Biesheuvel
  0 siblings, 1 reply; 30+ messages in thread
From: Mark Rutland @ 2016-09-06 10:45 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Sep 06, 2016 at 11:27:42AM +0100, Catalin Marinas wrote:
> On Mon, Sep 05, 2016 at 06:20:38PM +0100, Mark Rutland wrote:
> > On Fri, Sep 02, 2016 at 04:02:09PM +0100, Catalin Marinas wrote:
> > > +static inline void uaccess_ttbr0_enable(void)
> > > +{
> > > +	unsigned long flags;
> > > +
> > > +	/*
> > > +	 * Disable interrupts to avoid preemption and potential saved
> > > +	 * TTBR0_EL1 updates between reading the variable and the MSR.
> > > +	 */
> > > +	local_irq_save(flags);
> > > +	write_sysreg(current_thread_info()->ttbr0, ttbr0_el1);
> > > +	isb();
> > > +	local_irq_restore(flags);
> > > +}
> > 
> > I don't follow what problem this actually protects us against. In the
> > case of preemption everything should be saved+restored transparently, or
> > things would go wrong as soon as we enable IRQs anyway.
> > 
> > Is this a hold-over from a percpu approach rather than the
> > current_thread_info() approach?
> 
> If we get preempted between reading current_thread_info()->ttbr0 and
> writing TTBR0_EL1, a series of context switches could lead to the update
> of the ASID part of ttbr0. The actual MSR would store an old ASID in
> TTBR0_EL1.

Ah! Can you fold something about racing with an ASID update into the
description?

> > > +#else
> > > +static inline void uaccess_ttbr0_disable(void)
> > > +{
> > > +}
> > > +
> > > +static inline void uaccess_ttbr0_enable(void)
> > > +{
> > > +}
> > > +#endif
> > 
> > I think that it's better to drop the ifdef and add:
> > 
> > 	if (!IS_ENABLED(CONFIG_ARM64_TTBR0_PAN))
> > 		return;
> > 
> > ... at the start of each function. GCC should optimize the entire thing
> > away when not used, but we'll get compiler coverage regardless, and
> > therefore less breakage. All the symbols we required should exist
> > regardless.
> 
> The reason for this is that thread_info.ttbr0 is conditionally defined.
> I don't think the compiler would ignore it.

Good point; I missed that.

[...]

> > How about something like:
> > 
> > 	.macro alternative_endif_else_nop
> > 	alternative_else
> > 	.rept ((662b-661b) / 4)
> > 	       nop
> > 	.endr
> > 	alternative_endif
> > 	.endm
> > 
> > So for the above we could have:
> > 
> > 	alternative_if_not ARM64_HAS_PAN
> > 		save_and_disable_irq \tmp2
> > 		uaccess_ttbr0_enable \tmp1
> > 		restore_irq \tmp2
> > 	alternative_endif_else_nop
> > 
> > I'll see about spinning a patch, or discovering why that happens to be
> > broken.
> 
> This looks better. Minor comment, I would actually name the ending
> statement alternative_else_nop_endif to match the order in which you'd
> normally write them.

Completely agreed. I already made this change locally, immediately after
sending the suggestion. :)

> > >  	 * tables again to remove any speculatively loaded cache lines.
> > >  	 */
> > >  	mov	x0, x25
> > > -	add	x1, x26, #SWAPPER_DIR_SIZE
> > > +	add	x1, x26, #SWAPPER_DIR_SIZE + RESERVED_TTBR0_SIZE
> > >  	dmb	sy
> > >  	bl	__inval_cache_range
> > >  
> > > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> > > index 659963d40bb4..fe393ccf9352 100644
> > > --- a/arch/arm64/kernel/vmlinux.lds.S
> > > +++ b/arch/arm64/kernel/vmlinux.lds.S
> > > @@ -196,6 +196,11 @@ SECTIONS
> > >  	swapper_pg_dir = .;
> > >  	. += SWAPPER_DIR_SIZE;
> > >  
> > > +#ifdef CONFIG_ARM64_TTBR0_PAN
> > > +	reserved_ttbr0 = .;
> > > +	. += PAGE_SIZE;
> > > +#endif
> > 
> > Surely RESERVED_TTBR0_SIZE, as elsewhere?
> 
> I'll try to move it somewhere where it can be included in vmlinux.lds.S
> (I can probably include cpufeature.h directly).

Our vmlinux.lds.S already includes <asm/kernel-pagetable.h>, so I think
that should work already.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 4/7] arm64: Disable TTBR0_EL1 during normal kernel execution
  2016-09-02 15:02 ` [PATCH v2 4/7] arm64: Disable TTBR0_EL1 during normal kernel execution Catalin Marinas
@ 2016-09-06 17:31   ` Mark Rutland
  0 siblings, 0 replies; 30+ messages in thread
From: Mark Rutland @ 2016-09-06 17:31 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Catalin,

This generally looks fine, and my comments below are mostly nits. :)

On Fri, Sep 02, 2016 at 04:02:10PM +0100, Catalin Marinas wrote:
>  static inline void
> -switch_mm(struct mm_struct *prev, struct mm_struct *next,
> -	  struct task_struct *tsk)
> +__switch_mm(struct mm_struct *prev, struct mm_struct *next,
> +	    struct task_struct *tsk)

It looks like the comment above this function is now out-of-date, and
has been somewhat misleading for a while. While we're making changes
here, can we remove it entirely?

[...]

> @@ -109,6 +111,34 @@
>  	mrs	x22, elr_el1
>  	mrs	x23, spsr_el1
>  	stp	lr, x21, [sp, #S_LR]
> +
> +#ifdef CONFIG_ARM64_TTBR0_PAN
> +	/*
> +	 * Set the TTBR0 PAN in SPSR. When the exception is taken from EL0,
> +	 * there is no need to check the state of TTBR0_EL1 since accesses are
> +	 * always enabled.

Nit: missing 'bit' from the first sentence?

[...]

> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 536dce22fe76..4a4aaa47f869 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -228,6 +228,14 @@ void __init setup_arch(char **cmdline_p)
>  {
>  	pr_info("Boot CPU: AArch64 Processor [%08x]\n", read_cpuid_id());
>  
> +#ifdef CONFIG_ARM64_TTBR0_PAN
> +	/*
> +	 * uaccess_enable() may be called on the init thread, so make sure
> +	 * the saved TTBR0_EL1 always generates translation faults.
> +	 */
> +	init_thread_info.ttbr0 = virt_to_phys(empty_zero_page);
> +#endif

Just to check, does this need to happen so early? e.g. do we need this
to report exceptions safely? Otherwise, it would be nice if we could
group this with the uninstall of the idmap a little later in setup_arch.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching
  2016-09-02 15:02 [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching Catalin Marinas
                   ` (6 preceding siblings ...)
  2016-09-02 15:02 ` [PATCH v2 7/7] arm64: Enable CONFIG_ARM64_TTBR0_PAN Catalin Marinas
@ 2016-09-07 23:20 ` Kees Cook
  2016-09-08 12:51   ` Catalin Marinas
  2016-09-09 23:40 ` [kernel-hardening] " David Brown
  2016-09-10  9:51 ` Catalin Marinas
  9 siblings, 1 reply; 30+ messages in thread
From: Kees Cook @ 2016-09-07 23:20 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Sep 2, 2016 at 8:02 AM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> This is the second version of the arm64 PAN emulation by disabling
> TTBR0_EL1 accesses. The major change from v1 is the use of a thread_info
> member to store the real TTBR0_EL1 value. The advantage is slightly
> simpler assembler macros for uaccess_enable with the downside that
> switch_mm() must always update the saved ttbr0 even if there is no mm
> switch.

Is arm64 thread_info attached to the kernel stack? (i.e. is this
introducing a valuable target for stack-based attacks?)

-Kees

-- 
Kees Cook
Nexus Security

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching
  2016-09-07 23:20 ` [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching Kees Cook
@ 2016-09-08 12:51   ` Catalin Marinas
  2016-09-08 15:50     ` Kees Cook
  2016-09-09 16:31     ` Mark Rutland
  0 siblings, 2 replies; 30+ messages in thread
From: Catalin Marinas @ 2016-09-08 12:51 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Sep 07, 2016 at 04:20:55PM -0700, Kees Cook wrote:
> On Fri, Sep 2, 2016 at 8:02 AM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > This is the second version of the arm64 PAN emulation by disabling
> > TTBR0_EL1 accesses. The major change from v1 is the use of a thread_info
> > member to store the real TTBR0_EL1 value. The advantage is slightly
> > simpler assembler macros for uaccess_enable with the downside that
> > switch_mm() must always update the saved ttbr0 even if there is no mm
> > switch.
> 
> Is arm64 thread_info attached to the kernel stack? (i.e. is this
> introducing a valuable target for stack-based attacks?)

Currently yes, thread_info is on the kernel stack. At some point we'll
decouple it in a similar way to what x86 are doing/planning.

If thread_info on the stack can be corrupted, ttbr0 is not our only
worry but I agree it adds to the existing ones (addr_limit, task_struct
pointer).

That said, I don't have a strong preference for thread_info vs per-CPU
variable for the shadow TTBR0. The latter feels a bit more natural to me
since TTBR0 can be seen as a CPU state (that's what I did in v1).
However, using thread_info saves us couple of instructions in the asm
code for uaccess_enable.

-- 
Catalin

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching
  2016-09-08 12:51   ` Catalin Marinas
@ 2016-09-08 15:50     ` Kees Cook
  2016-09-09 16:31     ` Mark Rutland
  1 sibling, 0 replies; 30+ messages in thread
From: Kees Cook @ 2016-09-08 15:50 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Sep 8, 2016 at 5:51 AM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Wed, Sep 07, 2016 at 04:20:55PM -0700, Kees Cook wrote:
>> On Fri, Sep 2, 2016 at 8:02 AM, Catalin Marinas <catalin.marinas@arm.com> wrote:
>> > This is the second version of the arm64 PAN emulation by disabling
>> > TTBR0_EL1 accesses. The major change from v1 is the use of a thread_info
>> > member to store the real TTBR0_EL1 value. The advantage is slightly
>> > simpler assembler macros for uaccess_enable with the downside that
>> > switch_mm() must always update the saved ttbr0 even if there is no mm
>> > switch.
>>
>> Is arm64 thread_info attached to the kernel stack? (i.e. is this
>> introducing a valuable target for stack-based attacks?)
>
> Currently yes, thread_info is on the kernel stack. At some point we'll
> decouple it in a similar way to what x86 are doing/planning.

Okay, cool. As long as this is on the horizon, that's cool. :)

> If thread_info on the stack can be corrupted, ttbr0 is not our only
> worry but I agree it adds to the existing ones (addr_limit, task_struct
> pointer).
>
> That said, I don't have a strong preference for thread_info vs per-CPU
> variable for the shadow TTBR0. The latter feels a bit more natural to me
> since TTBR0 can be seen as a CPU state (that's what I did in v1).
> However, using thread_info saves us couple of instructions in the asm
> code for uaccess_enable.

I would opt for the safer (in per-CPU area), but I have clear biases.
;) Getting this emulation at all closes a huge exploitation method, so
on balance, the new exposure (which as you say is already not the only
target on the stack) is worth it. :)

Thanks!

-Kees

-- 
Kees Cook
Nexus Security

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching
  2016-09-08 12:51   ` Catalin Marinas
  2016-09-08 15:50     ` Kees Cook
@ 2016-09-09 16:31     ` Mark Rutland
  2016-09-09 18:24       ` Kees Cook
  1 sibling, 1 reply; 30+ messages in thread
From: Mark Rutland @ 2016-09-09 16:31 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Sep 08, 2016 at 01:51:24PM +0100, Catalin Marinas wrote:
> On Wed, Sep 07, 2016 at 04:20:55PM -0700, Kees Cook wrote:
> > On Fri, Sep 2, 2016 at 8:02 AM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > This is the second version of the arm64 PAN emulation by disabling
> > > TTBR0_EL1 accesses. The major change from v1 is the use of a thread_info
> > > member to store the real TTBR0_EL1 value. The advantage is slightly
> > > simpler assembler macros for uaccess_enable with the downside that
> > > switch_mm() must always update the saved ttbr0 even if there is no mm
> > > switch.
> > 
> > Is arm64 thread_info attached to the kernel stack? (i.e. is this
> > introducing a valuable target for stack-based attacks?)
> 
> Currently yes, thread_info is on the kernel stack. At some point we'll
> decouple it in a similar way to what x86 are doing/planning.

FWIW, I'm currently working on this (atop of Andy's x86 patches). The
IRQ stack work largely removed out dependence on the stack pointer to
find thread_info, and I have a plan for the remaining places.

There's a fair amount of ground work to do first (e.g. reworking headers
to avoid circular dependencies), but hopefully I'll have something that
I can share soon.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 3/7] arm64: Introduce uaccess_{disable, enable} functionality based on TTBR0_EL1
  2016-09-02 15:02 ` [PATCH v2 3/7] arm64: Introduce uaccess_{disable, enable} functionality based on TTBR0_EL1 Catalin Marinas
  2016-09-05 17:20   ` Mark Rutland
@ 2016-09-09 17:15   ` Catalin Marinas
  1 sibling, 0 replies; 30+ messages in thread
From: Catalin Marinas @ 2016-09-09 17:15 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Sep 02, 2016 at 04:02:09PM +0100, Catalin Marinas wrote:
>  /*
>   * User access enabling/disabling.
>   */
> +#ifdef CONFIG_ARM64_TTBR0_PAN
> +static inline void uaccess_ttbr0_disable(void)
> +{
> +	unsigned long ttbr;
> +
> +	/* reserved_ttbr0 placed at the end of swapper_pg_dir */
> +	ttbr = read_sysreg(ttbr1_el1) + SWAPPER_DIR_SIZE;
> +	write_sysreg(ttbr, ttbr0_el1);
> +	isb();
> +}
> +
> +static inline void uaccess_ttbr0_enable(void)
> +{
> +	unsigned long flags;
> +
> +	/*
> +	 * Disable interrupts to avoid preemption and potential saved
> +	 * TTBR0_EL1 updates between reading the variable and the MSR.
> +	 */
> +	local_irq_save(flags);
> +	write_sysreg(current_thread_info()->ttbr0, ttbr0_el1);
> +	isb();
> +	local_irq_restore(flags);
> +}

I followed up with the ARM architects on potential improvements to this
sequence. In summary, changing TCR_EL1.A1 is not guaranteed to have an
effect unless it is followed by TLBI. IOW, we can't use this bit for a
quick switch to the reserved ASID.

Setting TCR_EL1.EPD0 to 1 would work as long as it is followed by an
ASID change to a reserved one with no entries in the TLB. However, the
code sequence above (and the corresponding asm ones) would become even
more complex, so I don't think we gain anything.

Untested, using EPD0 (the assembly version would look sligtly better
than the C version but still a few instructions more than what we
currently have):

static inline void uaccess_ttbr0_disable(void)
{
	unsigned long ttbr;
	unsigned long tcr;

	/* disable TTBR0 page table walks */
	tcr = read_sysreg(tcr_el1);
	tcr |= TCR_ELD0
	write_sysreg(tcr, tcr_el1);
	isb();

	/* mask out the ASID bits (zero is a reserved ASID) */
	ttbr = read_sysreg(ttbr0_el1);
	ttbr &= ~ASID_MASK;
	write_sysreg(ttbr, ttbr0_el1);
	isb();
}

static inline void uaccess_ttbr0_enable(void)
{
	unsigned long flags;

	local_irq_save(flags);

	ttbr = read_sysreg(ttbr0_el1);
	ttbr |= current_thread_info()->asid;
	write_sysreg(ttbr, ttbr0_el1);
	isb();

	/* enable TTBR0 page table walks */
	tcr = read_sysreg(tcr_el1);
	tcr |= TCR_ELD0
	write_sysreg(tcr, tcr_el1);
	isb();

	local_irq_restore(flags);
}

The IRQ disabling for the above sequence is still required since we need
to guarantee the atomicity of the ASID read with the TTBR0_EL1 write.

We may be able to avoid current_thread_info()->asid *if* we find some
other per-CPU place to store the ASID (unused TTBR1_EL1 bits was
suggested, though not sure about the architecture requirements on those
bits being zero when TCR_EL1.A1 is 0). But even with these in place, the
requirement to have to ISBs and the additional TCR_EL1 read/write
doesn't give us anything better.

In conclusion, I propose that we stick to the current TTBR0_EL1 switch
as per these patches.

-- 
Catalin

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching
  2016-09-09 16:31     ` Mark Rutland
@ 2016-09-09 18:24       ` Kees Cook
  0 siblings, 0 replies; 30+ messages in thread
From: Kees Cook @ 2016-09-09 18:24 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Sep 9, 2016 at 9:31 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Sep 08, 2016 at 01:51:24PM +0100, Catalin Marinas wrote:
>> On Wed, Sep 07, 2016 at 04:20:55PM -0700, Kees Cook wrote:
>> > On Fri, Sep 2, 2016 at 8:02 AM, Catalin Marinas <catalin.marinas@arm.com> wrote:
>> > > This is the second version of the arm64 PAN emulation by disabling
>> > > TTBR0_EL1 accesses. The major change from v1 is the use of a thread_info
>> > > member to store the real TTBR0_EL1 value. The advantage is slightly
>> > > simpler assembler macros for uaccess_enable with the downside that
>> > > switch_mm() must always update the saved ttbr0 even if there is no mm
>> > > switch.
>> >
>> > Is arm64 thread_info attached to the kernel stack? (i.e. is this
>> > introducing a valuable target for stack-based attacks?)
>>
>> Currently yes, thread_info is on the kernel stack. At some point we'll
>> decouple it in a similar way to what x86 are doing/planning.
>
> FWIW, I'm currently working on this (atop of Andy's x86 patches). The
> IRQ stack work largely removed out dependence on the stack pointer to
> find thread_info, and I have a plan for the remaining places.
>
> There's a fair amount of ground work to do first (e.g. reworking headers
> to avoid circular dependencies), but hopefully I'll have something that
> I can share soon.

Fantastic! Thanks for the heads-up. :)

-Kees

-- 
Kees Cook
Nexus Security

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [kernel-hardening] [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching
  2016-09-02 15:02 [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching Catalin Marinas
                   ` (7 preceding siblings ...)
  2016-09-07 23:20 ` [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching Kees Cook
@ 2016-09-09 23:40 ` David Brown
  2016-09-10  9:51 ` Catalin Marinas
  9 siblings, 0 replies; 30+ messages in thread
From: David Brown @ 2016-09-09 23:40 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Sep 02, 2016 at 04:02:06PM +0100, Catalin Marinas wrote:
>This is the second version of the arm64 PAN emulation by disabling
>TTBR0_EL1 accesses. The major change from v1 is the use of a thread_info
>member to store the real TTBR0_EL1 value. The advantage is slightly
>simpler assembler macros for uaccess_enable with the downside that
>switch_mm() must always update the saved ttbr0 even if there is no mm
>switch.

I seem to have a lot better results with this series than I was having
with v1.  I've tested this both within qemu, and on the HiKey board
and with OP-TEE (to make sure it didn't break anything), and
everything seems to be working.

Tested-by: David Brown <david.brown@linaro.org>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching
  2016-09-02 15:02 [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching Catalin Marinas
                   ` (8 preceding siblings ...)
  2016-09-09 23:40 ` [kernel-hardening] " David Brown
@ 2016-09-10  9:51 ` Catalin Marinas
  2016-09-10 10:56   ` Ard Biesheuvel
  9 siblings, 1 reply; 30+ messages in thread
From: Catalin Marinas @ 2016-09-10  9:51 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Sep 02, 2016 at 04:02:06PM +0100, Catalin Marinas wrote:
> This is the second version of the arm64 PAN emulation by disabling
> TTBR0_EL1 accesses. The major change from v1 is the use of a thread_info
> member to store the real TTBR0_EL1 value. The advantage is slightly
> simpler assembler macros for uaccess_enable with the downside that
> switch_mm() must always update the saved ttbr0 even if there is no mm
> switch.

FYI, I got the Juno board rebooting in a loop with defconfig +
ARM64_TTBR0_PAN enabled. It takes about 20-40 reboots to get the panic
below on the EFI run-time services. I'll look into it on Monday (and
cc'ing Ard who I forgot to add originally). Including the full log
below:

EFI stub: Booting Linux Kernel...
EFI stub: Using DTB from configuration table
EFI stub: Exiting boot services and installing virtual address map...
[    0.000000] Booting Linux on physical CPU 0x100
[    0.000000] Linux version 4.8.0-rc4-00007-g07a1ca6cb00d (cmarinas at e104818-lin) (gcc version 5.3.1 20160309 (fsf-5.264) ) #1 SMP PREEMPT Fri Sep 9 18:38:47 BST 2016
[    0.000000] Boot CPU: AArch64 Processor [410fd030]
[    0.000000] earlycon: pl11 at MMIO 0x000000007ff80000 (options '')
[    0.000000] bootconsole [pl11] enabled
[    0.000000] efi: Getting EFI parameters from FDT:
[    0.000000] efi: EFI v2.40 by ARM Juno EFI Oct  7 2014 15:05:42
[    0.000000] efi:  ACPI=0xfebdc000  ACPI 2.0=0xfebdc014 
[    0.000000] cma: Reserved 16 MiB at 0x00000000fd800000
[    0.000000] psci: probing for conduit method from DT.
[    0.000000] psci: PSCIv0.2 detected in firmware.
[    0.000000] psci: Using standard PSCI v0.2 function IDs
[    0.000000] psci: Trusted OS migration not required
[    0.000000] percpu: Embedded 21 pages/cpu @ffff80097fea6000 s47488 r8192 d30336 u86016
[    0.000000] Detected VIPT I-cache on CPU0
[    0.000000] CPU features: enabling workaround for ARM errata 826319, 827319, 824069
[    0.000000] CPU features: enabling workaround for ARM erratum 845719
[    0.000000] Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 2060048
[    0.000000] Kernel command line: console=ttyAMA0,115200 earlycon=pl011,0x7ff80000 ip=dhcp root=/dev/nfs rw nfsroot=10.1.206.48:/srv/nfs/debian-arm64,tcp
[    0.000000] log_buf_len individual max cpu contribution: 4096 bytes
[    0.000000] log_buf_len total cpu_extra contributions: 20480 bytes
[    0.000000] log_buf_len min size: 16384 bytes
[    0.000000] log_buf_len: 65536 bytes
[    0.000000] early log buf free: 14320(87%)
[    0.000000] PID hash table entries: 4096 (order: 3, 32768 bytes)
[    0.000000] Dentry cache hash table entries: 1048576 (order: 11, 8388608 bytes)
[    0.000000] Inode-cache hash table entries: 524288 (order: 10, 4194304 bytes)
[    0.000000] software IO TLB [mem 0xf9800000-0xfd800000] (64MB) mapped at [ffff800079800000-ffff80007d7fffff]
[    0.000000] Memory: 8129516K/8371264K available (8636K kernel code, 796K rwdata, 3532K rodata, 960K init, 275K bss, 225364K reserved, 16384K cma-reserved)
[    0.000000] Virtual kernel memory layout:
[    0.000000]     modules : 0xffff000000000000 - 0xffff000008000000   (   128 MB)
[    0.000000]     vmalloc : 0xffff000008000000 - 0xffff7dffbfff0000   (129022 GB)
[    0.000000]       .text : 0xffff000008080000 - 0xffff0000088f0000   (  8640 KB)
[    0.000000]     .rodata : 0xffff0000088f0000 - 0xffff000008c70000   (  3584 KB)
[    0.000000]       .init : 0xffff000008c70000 - 0xffff000008d60000   (   960 KB)
[    0.000000]       .data : 0xffff000008d60000 - 0xffff000008e27200   (   797 KB)
[    0.000000]        .bss : 0xffff000008e27200 - 0xffff000008e6c138   (   276 KB)
[    0.000000]     fixed   : 0xffff7dfffe7fd000 - 0xffff7dfffec00000   (  4108 KB)
[    0.000000]     PCI I/O : 0xffff7dfffee00000 - 0xffff7dffffe00000   (    16 MB)
[    0.000000]     vmemmap : 0xffff7e0000000000 - 0xffff800000000000   (  2048 GB maximum)
[    0.000000]               0xffff7e0000000000 - 0xffff7e0026000000   (   608 MB actual)
[    0.000000]     memory  : 0xffff800000000000 - 0xffff800980000000   ( 38912 MB)
[    0.000000] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=6, Nodes=1
[    0.000000] Preemptible hierarchical RCU implementation.
[    0.000000] 	Build-time adjustment of leaf fanout to 64.
[    0.000000] 	RCU restricting CPUs from NR_CPUS=64 to nr_cpu_ids=6.
[    0.000000] RCU: Adjusting geometry for rcu_fanout_leaf=64, nr_cpu_ids=6
[    0.000000] NR_IRQS:64 nr_irqs:64 0
[    0.000000] GIC: Using split EOI/Deactivate mode
[    0.000000] GICv2m: range[mem 0x2c1c0000-0x2c1c0fff], SPI[224:255]
[    0.000000] arm_arch_timer: Architected cp15 timer(s) running at 50.00MHz (phys).
[    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff max_cycles: 0xb8812736b, max_idle_ns: 440795202655 ns
[    0.000005] sched_clock: 56 bits at 50MHz, resolution 20ns, wraps every 4398046511100ns
[    0.008439] clocksource: arm,sp804: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 1911260446275 ns
[    0.018257] Failed to initialize '/smb/motherboard/iofpga at 3,00000000/timer at 120000': -22
[    0.026339] Console: colour dummy device 80x25
[    0.031047] Calibrating delay loop (skipped), value calculated using timer frequency.. 100.00 BogoMIPS (lpj=200000)
[    0.041642] pid_max: default: 32768 minimum: 301
[    0.046406] Security Framework initialized
[    0.050607] Mount-cache hash table entries: 16384 (order: 5, 131072 bytes)
[    0.057587] Mountpoint-cache hash table entries: 16384 (order: 5, 131072 bytes)
[    0.066233] ASID allocator initialised with 65536 entries
[    0.096367] Remapping and enabling EFI services.
[    0.101091]   EFI remap 0x0000000008000000 => 0000000020000000
[    0.107016]   EFI remap 0x000000001c170000 => 0000000024000000
[    0.112941]   EFI remap 0x00000009faf6f000 => 000000002401f000
[    0.118865]   EFI remap 0x00000009fff6e000 => 000000002406e000
[    0.124789]   EFI remap 0x00000009fffaf000 => 00000000240af000
[    0.155087] Detected PIPT I-cache on CPU1
[    0.155092] CPU features: enabling workaround for ARM erratum 832075
[    0.155094] CPU features: enabling workaround for ARM erratum 834220
[    0.155129] CPU1: Booted secondary processor [410fd070]
[    0.171085] Detected PIPT I-cache on CPU2
[    0.171110] CPU2: Booted secondary processor [410fd070]
[    0.187086] Detected VIPT I-cache on CPU3
[    0.187127] CPU3: Booted secondary processor [410fd030]
[    0.203127] Detected VIPT I-cache on CPU4
[    0.203157] CPU4: Booted secondary processor [410fd030]
[    0.219170] Detected VIPT I-cache on CPU5
[    0.219200] CPU5: Booted secondary processor [410fd030]
[    0.219264] Brought up 6 CPUs
[    0.281960] SMP: Total of 6 processors activated.
[    0.286735] CPU features: detected feature: 32-bit EL0 Support
[    0.292670] CPU: All CPU(s) started at EL2
[    0.296856] alternatives: patching kernel code
[    0.307334] devtmpfs: initialized
[    0.314498] DMI not present or invalid.
[    0.318621] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 7645041785100000 ns
[    0.329272] pinctrl core: initialized pinctrl subsystem
[    0.335640] NET: Registered protocol family 16
[    0.364205] cpuidle: using governor menu
[    0.368387] vdso: 2 pages (1 code @ ffff0000088f6000, 1 data @ ffff000008d64000)
[    0.375906] hw-breakpoint: found 6 breakpoint and 4 watchpoint registers.
[    0.383492] DMA: preallocated 256 KiB pool for atomic allocations
[    0.389926] Serial: AMBA PL011 UART driver
[    0.396607] 7ff80000.uart: ttyAMA0 at MMIO 0x7ff80000 (irq = 24, base_baud = 0) is a PL011 rev3
[    0.405455] console [ttyAMA0] enabled
[    0.405455] console [ttyAMA0] enabled
[    0.412747] bootconsole [pl11] disabled
[    0.412747] bootconsole [pl11] disabled
[    0.448879] HugeTLB registered 2 MB page size, pre-allocated 0 pages
[    0.456038] ACPI: Interpreter disabled.
[    0.460736] vgaarb: loaded
[    0.463676] SCSI subsystem initialized
[    0.467861] usbcore: registered new interface driver usbfs
[    0.473364] usbcore: registered new interface driver hub
[    0.478725] usbcore: registered new device driver usb
[    0.484678] pps_core: LinuxPPS API ver. 1 registered
[    0.489606] pps_core: Software ver. 5.3.6 - Copyright 2005-2007 Rodolfo Giometti <giometti@linux.it>
[    0.498694] PTP clock support registered
[    0.502752] dmi: Firmware registration failed.
[    0.507965] Advanced Linux Sound Architecture Driver Initialized.
[    0.514808] clocksource: Switched to clocksource arch_sys_counter
[    0.520983] VFS: Disk quotas dquot_6.6.0
[    0.524930] VFS: Dquot-cache hash table entries: 512 (order 0, 4096 bytes)
[    0.531982] pnp: PnP ACPI: disabled
[    0.543951] NET: Registered protocol family 2
[    0.548779] TCP established hash table entries: 65536 (order: 7, 524288 bytes)
[    0.556814] TCP bind hash table entries: 65536 (order: 8, 1048576 bytes)
[    0.565070] TCP: Hash tables configured (established 65536 bind 65536)
[    0.571619] UDP hash table entries: 4096 (order: 5, 131072 bytes)
[    0.577902] UDP-Lite hash table entries: 4096 (order: 5, 131072 bytes)
[    0.584771] NET: Registered protocol family 1
[    0.589467] RPC: Registered named UNIX socket transport module.
[    0.595355] RPC: Registered udp transport module.
[    0.600021] RPC: Registered tcp transport module.
[    0.604685] RPC: Registered tcp NFSv4.1 backchannel transport module.
[    0.611740] hw perfevents: enabled with armv8_cortex_a57 PMU driver, 7 counters available
[    0.619970] hw perfevents: enabled with armv8_cortex_a53 PMU driver, 7 counters available
[    0.628262] kvm [1]: IDMAP page: 808d8000
[    0.632250] kvm [1]: HYP VA range: 800000000000:ffffffffffff
[    0.638698] kvm [1]: 8-bit VMID
[    0.641816] kvm [1]: Hyp mode initialized successfully
[    0.646947] kvm [1]: vgic-v2 at 2c04f000
[    0.650721] kvm [1]: vgic interrupt IRQ1
[    0.654720] kvm [1]: virtual timer IRQ4
[    0.661194] futex hash table entries: 2048 (order: 6, 262144 bytes)
[    0.667506] audit: initializing netlink subsys (disabled)
[    0.672915] audit: type=2000 audit(0.536:1): initialized
[    0.678965] workingset: timestamp_bits=46 max_order=21 bucket_order=0
[    0.696868] squashfs: version 4.0 (2009/01/31) Phillip Lougher
[    0.703578] NFS: Registering the id_resolver key type
[    0.708612] Key type id_resolver registered
[    0.712766] Key type id_legacy registered
[    0.716752] nfs4filelayout_init: NFSv4 File Layout Driver Registering...
[    0.723546] SGI XFS with security attributes, no debug enabled
[    0.730934] 9p: Installing v9fs 9p2000 file system support
[    0.738565] io scheduler noop registered
[    0.742480] io scheduler cfq registered (default)
[    0.747923] libphy: mdio_driver_register: phy-bcm-ns2-pci
[    0.754326] pl061_gpio 1c1d0000.gpio: PL061 GPIO chip @0x000000001c1d0000 registered
[    0.763984] dma-pl330 7ff00000.dma: Loaded driver for PL330 DMAC-341330
[    0.770560] dma-pl330 7ff00000.dma: 	DBUFF-1024x16bytes Num_Chans-8 Num_Peri-8 Num_Events-8
[    0.779809] xenfs: not registering filesystem on non-xen platform
[    0.788999] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
[    0.796505] SuperH (H)SCI(F) driver initialized
[    0.801247] msm_serial: driver initialized
[    0.811254] loop: module loaded
[    0.815840] libphy: Fixed MDIO Bus: probed
[    0.820328] tun: Universal TUN/TAP device driver, 1.6
[    0.825338] tun: (C) 1999-2004 Max Krasnyansky <maxk@qualcomm.com>
[    0.831905] e1000e: Intel(R) PRO/1000 Network Driver - 3.2.6-k
[    0.837692] e1000e: Copyright(c) 1999 - 2015 Intel Corporation.
[    0.843605] igb: Intel(R) Gigabit Ethernet Network Driver - version 5.3.0-k
[    0.850507] igb: Copyright (c) 2007-2014 Intel Corporation.
[    0.856072] igbvf: Intel(R) Gigabit Virtual Function Network Driver - version 2.0.2-k
[    0.863833] igbvf: Copyright (c) 2009 - 2012 Intel Corporation.
[    0.869799] sky2: driver version 1.30
[    0.895685] libphy: smsc911x-mdio: probed
[    0.970934] Generic PHY 18000000.etherne:01: attached PHY driver [Generic PHY] (mii_bus:phy_addr=18000000.etherne:01, irq=-1)
[    0.982316] smsc911x 18000000.ethernet eth0: MAC Address: 00:02:f7:00:58:bb
[    0.989404] VFIO - User Level meta-driver version: 0.3
[    0.995517] ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver
[    1.001993] ehci-pci: EHCI PCI platform driver
[    1.006432] ehci-platform: EHCI generic platform driver
[    1.011718] ehci-platform 7ffc0000.ehci: EHCI Host Controller
[    1.017428] ehci-platform 7ffc0000.ehci: new USB bus registered, assigned bus number 1
[    1.025419] ehci-platform 7ffc0000.ehci: irq 27, io mem 0x7ffc0000
[    1.046826] ehci-platform 7ffc0000.ehci: USB 2.0 started, EHCI 1.00
[    1.053541] hub 1-0:1.0: USB hub found
[    1.057279] hub 1-0:1.0: 1 port detected
[    1.061448] ehci-exynos: EHCI EXYNOS driver
[    1.065666] ehci-msm: Qualcomm On-Chip EHCI Host Controller
[    1.071254] ohci_hcd: USB 1.1 'Open' Host Controller (OHCI) Driver
[    1.077401] ohci-pci: OHCI PCI platform driver
[    1.081845] ohci-platform: OHCI generic platform driver
[    1.087113] ohci-platform 7ffb0000.ohci: Generic Platform OHCI controller
[    1.093855] ohci-platform 7ffb0000.ohci: new USB bus registered, assigned bus number 2
[    1.101791] ohci-platform 7ffb0000.ohci: irq 26, io mem 0x7ffb0000
[    1.187263] hub 2-0:1.0: USB hub found
[    1.191003] hub 2-0:1.0: 1 port detected
[    1.195133] ohci-exynos: OHCI EXYNOS driver
[    1.199619] usbcore: registered new interface driver usb-storage
[    1.206575] mousedev: PS/2 mouse device common for all mice
[    1.212778] rtc-efi rtc-efi: rtc core: registered rtc-efi as rtc0
[    1.219087] rtc-pl031 1c170000.rtc: rtc core: registered pl031 as rtc1
[    1.225912] i2c /dev entries driver
[    1.231598] mmci-pl18x 1c050000.mmci: mmc0: PL180 manf 41 rev0 at 0x1c050000 irq 32,0 (pio)
[    1.239880] mmci-pl18x 1c050000.mmci: DMA channels RX none, TX none
[    1.283106] sdhci: Secure Digital Host Controller Interface driver
[    1.289269] sdhci: Copyright(c) Pierre Ossman
[    1.293646] Synopsys Designware Multimedia Card Interface Driver
[    1.300985] sdhci-pltfm: SDHCI platform and OF driver helper
[    1.309245] leds-syscon 1c010000.apbregs:led at 08.0: registered LED vexpress:0
[    1.316358] leds-syscon 1c010000.apbregs:led at 08.1: registered LED vexpress:1
[    1.323550] leds-syscon 1c010000.apbregs:led at 08.2: registered LED vexpress:2
[    1.330953] leds-syscon 1c010000.apbregs:led at 08.3: registered LED vexpress:3
[    1.338043] leds-syscon 1c010000.apbregs:led at 08.4: registered LED vexpress:4
[    1.345139] leds-syscon 1c010000.apbregs:led at 08.5: registered LED vexpress:5
[    1.352224] leds-syscon 1c010000.apbregs:led at 08.6: registered LED vexpress:6
[    1.359553] leds-syscon 1c010000.apbregs:led at 08.7: registered LED vexpress:7
[    1.368522] ledtrig-cpu: registered to indicate activity on CPUs
[    1.375694] usbcore: registered new interface driver usbhid
[    1.381223] usbhid: USB HID core driver
[    1.385921] mhu 2b1f0000.mhu: ARM MHU Mailbox registered
[    1.392777] NET: Registered protocol family 17
[    1.394826] usb 1-1: new high-speed USB device number 2 using ehci-platform
[    1.404224] 9pnet: Installing 9P2000 support
[    1.408545] Key type dns_resolver registered
[    1.413706] registered taskstats version 1
[    1.455390] scpi_protocol scpi: incorrect or no SCP firmware found
[    1.461595] scpi_protocol: probe of scpi failed with error -110
[    1.468487] input: smb:motherboard:gpio_keys as /devices/platform/smb/smb:motherboard/smb:motherboard:gpio_keys/input/input1
[    1.480518] Unable to handle kernel paging request at virtual address 240ab2f8
[    1.487693] pgd = ffff000008e70000
[    1.491084] [240ab2f8] *pgd=00000009fff6d003, *pud=00000009fff6c003, *pmd=0000000000000000
[    1.499336] Internal error: Oops: 86000004 [#1] PREEMPT SMP
[    1.504849] Modules linked in:
[    1.507877] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.8.0-rc4-00007-g07a1ca6cb00d #1
[    1.515711] Hardware name: ARM Juno development board (r0) (DT)
[    1.521569] task: ffff8009768a8000 task.stack: ffff8009768b0000
[    1.527427] PC is at 0x240ab2f8
[    1.530531] LR is at 0x240aaac0
[    1.533637] pc : [<00000000240ab2f8>] lr : [<00000000240aaac0>] pstate: 60400145
[    1.540954] sp : ffff8009768b3c50
[    1.544230] x29: ffff8009768b3ca0 x28: 0000000000000000 
[    1.549492] x27: ffff000008d279c0 x26: ffff000008c70470 
[    1.554754] x25: ffff000008cbf230 x24: ffff000008c5fd70 
[    1.560016] x23: ffff000008e2c000 x22: ffff8009768b3cf0 
[    1.565278] x21: ffff8009768b3cf0 x20: ffff8009768b3d00 
[    1.570540] x19: 8000000000000003 x18: ffffffffffffffff 
[    1.575801] x17: 0000000000000000 x16: 0000000000000000 
[    1.581063] x15: ffff80097612f16b x14: 0000000000000000 
[    1.586323] x13: 0000000000000000 x12: 0000000000000038 
[    1.591584] x11: 0000000000000020 x10: 0101010101010101 
[    1.596846] x9 : 0000000000000000 x8 : ffffff7f7f7f7f7f 
[    1.602107] x7 : fefe7eff2f627371 x6 : 000000008080ffff 
[    1.607368] x5 : ffff8009768b3c98 x4 : 0000000000010001 
[    1.612629] x3 : 00000000ff000001 x2 : 00000000240aa3e4 
[    1.617890] x1 : ffff8009768b3c98 x0 : 0000000057d752e6 
[    1.623150] 
[    1.624622] Process swapper/0 (pid: 1, stack limit = 0xffff8009768b0028)
[    1.631253] Stack: (0xffff8009768b3c50 to 0xffff8009768b4000)
[    1.636941] 3c40:                                   ffff000008e5dcd8 ffff8009768b3d00
[    1.644694] 3c60: 0000000000000140 ffff8009768b3cf0 ffff000008e2c000 ffff000008c5fd70
[    1.652447] 3c80: ffff00000876236c ffff8009768b3d00 ffff8009768b3da8 ffff00000855d1b4
[    1.660201] 3ca0: ffff8009768b3cd0 ffff000008703de8 ffff8009768b3da8 ffff000008dd9ff0
[    1.667954] 3cc0: ffff8009768b3da8 0000000000000007 ffff8009768b3d10 ffff000008700964
[    1.675707] 3ce0: ffff8009768b3da8 ffff800976a752e8 ffff8009768b3d40 ffff0000087006a8
[    1.683461] 3d00: ffff000008bbe958 ffff000008ca081c ffff8009768b3d30 ffff0000087009c8
[    1.691214] 3d20: ffff800976a75000 ffff0000087006b8 ffff8009768b3d70 ffff000008ca0878
[    1.698974] 3d40: ffff000008bbe958 ffff800976a75000 0000000000000000 0000000000000000
[    1.706727] 3d60: ffff000008dd94b0 ffff0000083f4820 ffff8009768b3dd0 ffff000008083b40
[    1.714480] 3d80: ffff8009768b0000 ffff000008ca081c ffff8009768b3db0 0000000000000000
[    1.722232] 3da0: 000000001dcd6500 0000000000000000 0000000000000000 0000000000000000
[    1.729984] 3dc0: 0000000000000000 ffff000000000000 ffff8009768b3e40 ffff000008c70d14
[    1.737737] 3de0: 0000000000000110 ffff000008e2c000 ffff000008cbf2c0 0000000000000007
[    1.745490] 3e00: ffff000008d27900 0000000000000000 ffff000008e2c000 ffff000008b48ac8
[    1.753242] 3e20: 0000000700000007 0000000000000000 0000000000000000 ffff000008c5fd70
[    1.760995] 3e40: ffff8009768b3ea0 ffff0000088d15d0 ffff0000088d15c0 0000000000000000
[    1.768747] 3e60: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[    1.776499] 3e80: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[    1.784252] 3ea0: 0000000000000000 ffff0000080830a0 ffff0000088d15c0 0000000000000000
[    1.792004] 3ec0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[    1.799756] 3ee0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[    1.807508] 3f00: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[    1.815260] 3f20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[    1.823023] 3f40: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[    1.830775] 3f60: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[    1.838527] 3f80: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[    1.846279] 3fa0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[    1.854031] 3fc0: 0000000000000000 0000000000000005 0000000000000000 0000000000000000
[    1.861783] 3fe0: 0000000000000000 0000000000000000 035b4d0700426040 000f000200440000
[    1.869531] Call trace:
[    1.871950] Exception stack(0xffff8009768b3a80 to 0xffff8009768b3bb0)
[    1.878327] 3a80: 8000000000000003 0001000000000000 0000000080e2b000 00000000240ab2f8
[    1.886080] 3aa0: ffff8009768b3b00 ffff00000818bb68 ffff800976015f80 00000000024000c0
[    1.893832] 3ac0: ffff00000818bbe4 000000000000001a ffff80097608dc10 ffff800976bd1218
[    1.901585] 3ae0: ffff8009768b3af0 ffff0000083f2670 ffff8009768b3b70 ffff0000083f2b40
[    1.909338] 3b00: 0000000000000000 ffff800976811b00 0000000000000000 ffff800976811a90
[    1.917091] 3b20: 0000000057d752e6 ffff8009768b3c98 00000000240aa3e4 00000000ff000001
[    1.924844] 3b40: 0000000000010001 ffff8009768b3c98 000000008080ffff fefe7eff2f627371
[    1.932596] 3b60: ffffff7f7f7f7f7f 0000000000000000 0101010101010101 0000000000000020
[    1.940349] 3b80: 0000000000000038 0000000000000000 0000000000000000 ffff80097612f16b
[    1.948098] 3ba0: 0000000000000000 0000000000000000
[    1.952925] [<00000000240ab2f8>] 0x240ab2f8
[    1.957069] [<ffff000008703de8>] efi_read_time+0x28/0x78
[    1.962329] [<ffff000008700964>] __rtc_read_time.isra.1+0x44/0x70
[    1.968360] [<ffff0000087009c8>] rtc_read_time+0x38/0x58
[    1.973620] [<ffff000008ca0878>] rtc_hctosys+0x5c/0xe4
[    1.978708] [<ffff000008083b40>] do_one_initcall+0x38/0x128
[    1.984227] [<ffff000008c70d14>] kernel_init_freeable+0x1ac/0x250
[    1.990261] [<ffff0000088d15d0>] kernel_init+0x10/0x100
[    1.995433] [<ffff0000080830a0>] ret_from_fork+0x10/0x30
[    2.000691] Code: bad PC value

-- 
Catalin

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching
  2016-09-10  9:51 ` Catalin Marinas
@ 2016-09-10 10:56   ` Ard Biesheuvel
  2016-09-11 12:16     ` Catalin Marinas
  0 siblings, 1 reply; 30+ messages in thread
From: Ard Biesheuvel @ 2016-09-10 10:56 UTC (permalink / raw)
  To: linux-arm-kernel

On 10 September 2016 at 10:51, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Fri, Sep 02, 2016 at 04:02:06PM +0100, Catalin Marinas wrote:
>> This is the second version of the arm64 PAN emulation by disabling
>> TTBR0_EL1 accesses. The major change from v1 is the use of a thread_info
>> member to store the real TTBR0_EL1 value. The advantage is slightly
>> simpler assembler macros for uaccess_enable with the downside that
>> switch_mm() must always update the saved ttbr0 even if there is no mm
>> switch.
>
> FYI, I got the Juno board rebooting in a loop with defconfig +
> ARM64_TTBR0_PAN enabled. It takes about 20-40 reboots to get the panic
> below on the EFI run-time services. I'll look into it on Monday (and
> cc'ing Ard who I forgot to add originally). Including the full log
> below:
>

Hi David,

Could you please try to reproduce it again, but this time with
'efi=debug' on the kernel command line? Thanks

-- 
Ard.


> EFI stub: Booting Linux Kernel...
> EFI stub: Using DTB from configuration table
> EFI stub: Exiting boot services and installing virtual address map...
> [    0.000000] Booting Linux on physical CPU 0x100
> [    0.000000] Linux version 4.8.0-rc4-00007-g07a1ca6cb00d (cmarinas at e104818-lin) (gcc version 5.3.1 20160309 (fsf-5.264) ) #1 SMP PREEMPT Fri Sep 9 18:38:47 BST 2016
> [    0.000000] Boot CPU: AArch64 Processor [410fd030]
> [    0.000000] earlycon: pl11 at MMIO 0x000000007ff80000 (options '')
> [    0.000000] bootconsole [pl11] enabled
> [    0.000000] efi: Getting EFI parameters from FDT:
> [    0.000000] efi: EFI v2.40 by ARM Juno EFI Oct  7 2014 15:05:42
> [    0.000000] efi:  ACPI=0xfebdc000  ACPI 2.0=0xfebdc014
> [    0.000000] cma: Reserved 16 MiB at 0x00000000fd800000
> [    0.000000] psci: probing for conduit method from DT.
> [    0.000000] psci: PSCIv0.2 detected in firmware.
> [    0.000000] psci: Using standard PSCI v0.2 function IDs
> [    0.000000] psci: Trusted OS migration not required
> [    0.000000] percpu: Embedded 21 pages/cpu @ffff80097fea6000 s47488 r8192 d30336 u86016
> [    0.000000] Detected VIPT I-cache on CPU0
> [    0.000000] CPU features: enabling workaround for ARM errata 826319, 827319, 824069
> [    0.000000] CPU features: enabling workaround for ARM erratum 845719
> [    0.000000] Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 2060048
> [    0.000000] Kernel command line: console=ttyAMA0,115200 earlycon=pl011,0x7ff80000 ip=dhcp root=/dev/nfs rw nfsroot=10.1.206.48:/srv/nfs/debian-arm64,tcp
> [    0.000000] log_buf_len individual max cpu contribution: 4096 bytes
> [    0.000000] log_buf_len total cpu_extra contributions: 20480 bytes
> [    0.000000] log_buf_len min size: 16384 bytes
> [    0.000000] log_buf_len: 65536 bytes
> [    0.000000] early log buf free: 14320(87%)
> [    0.000000] PID hash table entries: 4096 (order: 3, 32768 bytes)
> [    0.000000] Dentry cache hash table entries: 1048576 (order: 11, 8388608 bytes)
> [    0.000000] Inode-cache hash table entries: 524288 (order: 10, 4194304 bytes)
> [    0.000000] software IO TLB [mem 0xf9800000-0xfd800000] (64MB) mapped at [ffff800079800000-ffff80007d7fffff]
> [    0.000000] Memory: 8129516K/8371264K available (8636K kernel code, 796K rwdata, 3532K rodata, 960K init, 275K bss, 225364K reserved, 16384K cma-reserved)
> [    0.000000] Virtual kernel memory layout:
> [    0.000000]     modules : 0xffff000000000000 - 0xffff000008000000   (   128 MB)
> [    0.000000]     vmalloc : 0xffff000008000000 - 0xffff7dffbfff0000   (129022 GB)
> [    0.000000]       .text : 0xffff000008080000 - 0xffff0000088f0000   (  8640 KB)
> [    0.000000]     .rodata : 0xffff0000088f0000 - 0xffff000008c70000   (  3584 KB)
> [    0.000000]       .init : 0xffff000008c70000 - 0xffff000008d60000   (   960 KB)
> [    0.000000]       .data : 0xffff000008d60000 - 0xffff000008e27200   (   797 KB)
> [    0.000000]        .bss : 0xffff000008e27200 - 0xffff000008e6c138   (   276 KB)
> [    0.000000]     fixed   : 0xffff7dfffe7fd000 - 0xffff7dfffec00000   (  4108 KB)
> [    0.000000]     PCI I/O : 0xffff7dfffee00000 - 0xffff7dffffe00000   (    16 MB)
> [    0.000000]     vmemmap : 0xffff7e0000000000 - 0xffff800000000000   (  2048 GB maximum)
> [    0.000000]               0xffff7e0000000000 - 0xffff7e0026000000   (   608 MB actual)
> [    0.000000]     memory  : 0xffff800000000000 - 0xffff800980000000   ( 38912 MB)
> [    0.000000] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=6, Nodes=1
> [    0.000000] Preemptible hierarchical RCU implementation.
> [    0.000000]  Build-time adjustment of leaf fanout to 64.
> [    0.000000]  RCU restricting CPUs from NR_CPUS=64 to nr_cpu_ids=6.
> [    0.000000] RCU: Adjusting geometry for rcu_fanout_leaf=64, nr_cpu_ids=6
> [    0.000000] NR_IRQS:64 nr_irqs:64 0
> [    0.000000] GIC: Using split EOI/Deactivate mode
> [    0.000000] GICv2m: range[mem 0x2c1c0000-0x2c1c0fff], SPI[224:255]
> [    0.000000] arm_arch_timer: Architected cp15 timer(s) running at 50.00MHz (phys).
> [    0.000000] clocksource: arch_sys_counter: mask: 0xffffffffffffff max_cycles: 0xb8812736b, max_idle_ns: 440795202655 ns
> [    0.000005] sched_clock: 56 bits at 50MHz, resolution 20ns, wraps every 4398046511100ns
> [    0.008439] clocksource: arm,sp804: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 1911260446275 ns
> [    0.018257] Failed to initialize '/smb/motherboard/iofpga at 3,00000000/timer at 120000': -22
> [    0.026339] Console: colour dummy device 80x25
> [    0.031047] Calibrating delay loop (skipped), value calculated using timer frequency.. 100.00 BogoMIPS (lpj=200000)
> [    0.041642] pid_max: default: 32768 minimum: 301
> [    0.046406] Security Framework initialized
> [    0.050607] Mount-cache hash table entries: 16384 (order: 5, 131072 bytes)
> [    0.057587] Mountpoint-cache hash table entries: 16384 (order: 5, 131072 bytes)
> [    0.066233] ASID allocator initialised with 65536 entries
> [    0.096367] Remapping and enabling EFI services.
> [    0.101091]   EFI remap 0x0000000008000000 => 0000000020000000
> [    0.107016]   EFI remap 0x000000001c170000 => 0000000024000000
> [    0.112941]   EFI remap 0x00000009faf6f000 => 000000002401f000
> [    0.118865]   EFI remap 0x00000009fff6e000 => 000000002406e000
> [    0.124789]   EFI remap 0x00000009fffaf000 => 00000000240af000
> [    0.155087] Detected PIPT I-cache on CPU1
> [    0.155092] CPU features: enabling workaround for ARM erratum 832075
> [    0.155094] CPU features: enabling workaround for ARM erratum 834220
> [    0.155129] CPU1: Booted secondary processor [410fd070]
> [    0.171085] Detected PIPT I-cache on CPU2
> [    0.171110] CPU2: Booted secondary processor [410fd070]
> [    0.187086] Detected VIPT I-cache on CPU3
> [    0.187127] CPU3: Booted secondary processor [410fd030]
> [    0.203127] Detected VIPT I-cache on CPU4
> [    0.203157] CPU4: Booted secondary processor [410fd030]
> [    0.219170] Detected VIPT I-cache on CPU5
> [    0.219200] CPU5: Booted secondary processor [410fd030]
> [    0.219264] Brought up 6 CPUs
> [    0.281960] SMP: Total of 6 processors activated.
> [    0.286735] CPU features: detected feature: 32-bit EL0 Support
> [    0.292670] CPU: All CPU(s) started at EL2
> [    0.296856] alternatives: patching kernel code
> [    0.307334] devtmpfs: initialized
> [    0.314498] DMI not present or invalid.
> [    0.318621] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 7645041785100000 ns
> [    0.329272] pinctrl core: initialized pinctrl subsystem
> [    0.335640] NET: Registered protocol family 16
> [    0.364205] cpuidle: using governor menu
> [    0.368387] vdso: 2 pages (1 code @ ffff0000088f6000, 1 data @ ffff000008d64000)
> [    0.375906] hw-breakpoint: found 6 breakpoint and 4 watchpoint registers.
> [    0.383492] DMA: preallocated 256 KiB pool for atomic allocations
> [    0.389926] Serial: AMBA PL011 UART driver
> [    0.396607] 7ff80000.uart: ttyAMA0 at MMIO 0x7ff80000 (irq = 24, base_baud = 0) is a PL011 rev3
> [    0.405455] console [ttyAMA0] enabled
> [    0.405455] console [ttyAMA0] enabled
> [    0.412747] bootconsole [pl11] disabled
> [    0.412747] bootconsole [pl11] disabled
> [    0.448879] HugeTLB registered 2 MB page size, pre-allocated 0 pages
> [    0.456038] ACPI: Interpreter disabled.
> [    0.460736] vgaarb: loaded
> [    0.463676] SCSI subsystem initialized
> [    0.467861] usbcore: registered new interface driver usbfs
> [    0.473364] usbcore: registered new interface driver hub
> [    0.478725] usbcore: registered new device driver usb
> [    0.484678] pps_core: LinuxPPS API ver. 1 registered
> [    0.489606] pps_core: Software ver. 5.3.6 - Copyright 2005-2007 Rodolfo Giometti <giometti@linux.it>
> [ 0.498694] PTP clock support registered
> [ 0.502752] dmi: Firmware registration failed.
> [ 0.507965] Advanced Linux Sound Architecture Driver Initialized.
> [ 0.514808] clocksource: Switched to clocksource arch_sys_counter
> [    0.520983] VFS: Disk quotas dquot_6.6.0
> [ 0.524930] VFS: Dquot-cache hash table entries: 512 (order 0, 4096 bytes)
> [    0.531982] pnp: PnP ACPI: disabled
> [    0.543951] NET: Registered protocol family 2
> [    0.548779] TCP established hash table entries: 65536 (order: 7, 524288 bytes)
> [    0.556814] TCP bind hash table entries: 65536 (order: 8, 1048576 bytes)
> [    0.565070] TCP: Hash tables configured (established 65536 bind 65536)
> [    0.571619] UDP hash table entries: 4096 (order: 5, 131072 bytes)
> [    0.577902] UDP-Lite hash table entries: 4096 (order: 5, 131072 bytes)
> [    0.584771] NET: Registered protocol family 1
> [    0.589467] RPC: Registered named UNIX socket transport module.
> [    0.595355] RPC: Registered udp transport module.
> [    0.600021] RPC: Registered tcp transport module.
> [    0.604685] RPC: Registered tcp NFSv4.1 backchannel transport module.
> [    0.611740] hw perfevents: enabled with armv8_cortex_a57 PMU driver, 7 counters available
> [    0.619970] hw perfevents: enabled with armv8_cortex_a53 PMU driver, 7 counters available
> [    0.628262] kvm [1]: IDMAP page: 808d8000
> [    0.632250] kvm [1]: HYP VA range: 800000000000:ffffffffffff
> [    0.638698] kvm [1]: 8-bit VMID
> [    0.641816] kvm [1]: Hyp mode initialized successfully
> [    0.646947] kvm [1]: vgic-v2 at 2c04f000
> [ 0.650721] kvm [1]: vgic interrupt IRQ1
> [ 0.654720] kvm [1]: virtual timer IRQ4
> [ 0.661194] futex hash table entries: 2048 (order: 6, 262144 bytes)
> [ 0.667506] audit: initializing netlink subsys (disabled)
> [    0.672915] audit: type=2000 audit(0.536:1): initialized
> [    0.678965] workingset: timestamp_bits=46 max_order=21 bucket_order=0
> [    0.696868] squashfs: version 4.0 (2009/01/31) Phillip Lougher
> [    0.703578] NFS: Registering the id_resolver key type
> [    0.708612] Key type id_resolver registered
> [    0.712766] Key type id_legacy registered
> [    0.716752] nfs4filelayout_init: NFSv4 File Layout Driver Registering...
> [    0.723546] SGI XFS with security attributes, no debug enabled
> [    0.730934] 9p: Installing v9fs 9p2000 file system support
> [    0.738565] io scheduler noop registered
> [    0.742480] io scheduler cfq registered (default)
> [    0.747923] libphy: mdio_driver_register: phy-bcm-ns2-pci
> [    0.754326] pl061_gpio 1c1d0000.gpio: PL061 GPIO chip @0x000000001c1d0000 registered
> [    0.763984] dma-pl330 7ff00000.dma: Loaded driver for PL330 DMAC-341330
> [    0.770560] dma-pl330 7ff00000.dma:  DBUFF-1024x16bytes Num_Chans-8 Num_Peri-8 Num_Events-8
> [    0.779809] xenfs: not registering filesystem on non-xen platform
> [    0.788999] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> [    0.796505] SuperH (H)SCI(F) driver initialized
> [    0.801247] msm_serial: driver initialized
> [    0.811254] loop: module loaded
> [    0.815840] libphy: Fixed MDIO Bus: probed
> [    0.820328] tun: Universal TUN/TAP device driver, 1.6
> [    0.825338] tun: (C) 1999-2004 Max Krasnyansky <maxk@qualcomm.com>
> [    0.831905] e1000e: Intel(R) PRO/1000 Network Driver - 3.2.6-k
> [    0.837692] e1000e: Copyright(c) 1999 - 2015 Intel Corporation.
> [    0.843605] igb: Intel(R) Gigabit Ethernet Network Driver - version 5.3.0-k
> [    0.850507] igb: Copyright (c) 2007-2014 Intel Corporation.
> [    0.856072] igbvf: Intel(R) Gigabit Virtual Function Network Driver - version 2.0.2-k
> [    0.863833] igbvf: Copyright (c) 2009 - 2012 Intel Corporation.
> [    0.869799] sky2: driver version 1.30
> [    0.895685] libphy: smsc911x-mdio: probed
> [    0.970934] Generic PHY 18000000.etherne:01: attached PHY driver [Generic PHY] (mii_bus:phy_addr=18000000.etherne:01, irq=-1)
> [    0.982316] smsc911x 18000000.ethernet eth0: MAC Address: 00:02:f7:00:58:bb
> [    0.989404] VFIO - User Level meta-driver version: 0.3
> [    0.995517] ehci_hcd: USB 2.0 'Enhanced' Host Controller (EHCI) Driver
> [    1.001993] ehci-pci: EHCI PCI platform driver
> [    1.006432] ehci-platform: EHCI generic platform driver
> [    1.011718] ehci-platform 7ffc0000.ehci: EHCI Host Controller
> [    1.017428] ehci-platform 7ffc0000.ehci: new USB bus registered, assigned bus number 1
> [    1.025419] ehci-platform 7ffc0000.ehci: irq 27, io mem 0x7ffc0000
> [    1.046826] ehci-platform 7ffc0000.ehci: USB 2.0 started, EHCI 1.00
> [    1.053541] hub 1-0:1.0: USB hub found
> [    1.057279] hub 1-0:1.0: 1 port detected
> [    1.061448] ehci-exynos: EHCI EXYNOS driver
> [    1.065666] ehci-msm: Qualcomm On-Chip EHCI Host Controller
> [    1.071254] ohci_hcd: USB 1.1 'Open' Host Controller (OHCI) Driver
> [    1.077401] ohci-pci: OHCI PCI platform driver
> [    1.081845] ohci-platform: OHCI generic platform driver
> [    1.087113] ohci-platform 7ffb0000.ohci: Generic Platform OHCI controller
> [    1.093855] ohci-platform 7ffb0000.ohci: new USB bus registered, assigned bus number 2
> [    1.101791] ohci-platform 7ffb0000.ohci: irq 26, io mem 0x7ffb0000
> [    1.187263] hub 2-0:1.0: USB hub found
> [    1.191003] hub 2-0:1.0: 1 port detected
> [    1.195133] ohci-exynos: OHCI EXYNOS driver
> [    1.199619] usbcore: registered new interface driver usb-storage
> [    1.206575] mousedev: PS/2 mouse device common for all mice
> [    1.212778] rtc-efi rtc-efi: rtc core: registered rtc-efi as rtc0
> [    1.219087] rtc-pl031 1c170000.rtc: rtc core: registered pl031 as rtc1
> [    1.225912] i2c /dev entries driver
> [    1.231598] mmci-pl18x 1c050000.mmci: mmc0: PL180 manf 41 rev0 at 0x1c050000 irq 32,0 (pio)
> [    1.239880] mmci-pl18x 1c050000.mmci: DMA channels RX none, TX none
> [    1.283106] sdhci: Secure Digital Host Controller Interface driver
> [    1.289269] sdhci: Copyright(c) Pierre Ossman
> [    1.293646] Synopsys Designware Multimedia Card Interface Driver
> [    1.300985] sdhci-pltfm: SDHCI platform and OF driver helper
> [    1.309245] leds-syscon 1c010000.apbregs:led at 08.0: registered LED vexpress:0
> [    1.316358] leds-syscon 1c010000.apbregs:led at 08.1: registered LED vexpress:1
> [    1.323550] leds-syscon 1c010000.apbregs:led at 08.2: registered LED vexpress:2
> [    1.330953] leds-syscon 1c010000.apbregs:led at 08.3: registered LED vexpress:3
> [    1.338043] leds-syscon 1c010000.apbregs:led at 08.4: registered LED vexpress:4
> [    1.345139] leds-syscon 1c010000.apbregs:led at 08.5: registered LED vexpress:5
> [    1.352224] leds-syscon 1c010000.apbregs:led at 08.6: registered LED vexpress:6
> [    1.359553] leds-syscon 1c010000.apbregs:led at 08.7: registered LED vexpress:7
> [    1.368522] ledtrig-cpu: registered to indicate activity on CPUs
> [    1.375694] usbcore: registered new interface driver usbhid
> [    1.381223] usbhid: USB HID core driver
> [    1.385921] mhu 2b1f0000.mhu: ARM MHU Mailbox registered
> [    1.392777] NET: Registered protocol family 17
> [    1.394826] usb 1-1: new high-speed USB device number 2 using ehci-platform
> [    1.404224] 9pnet: Installing 9P2000 support
> [    1.408545] Key type dns_resolver registered
> [    1.413706] registered taskstats version 1
> [    1.455390] scpi_protocol scpi: incorrect or no SCP firmware found
> [    1.461595] scpi_protocol: probe of scpi failed with error -110
> [    1.468487] input: smb:motherboard:gpio_keys as /devices/platform/smb/smb:motherboard/smb:motherboard:gpio_keys/input/input1
> [    1.480518] Unable to handle kernel paging request at virtual address 240ab2f8
> [    1.487693] pgd = ffff000008e70000
> [    1.491084] [240ab2f8] *pgd=00000009fff6d003, *pud=00000009fff6c003, *pmd=0000000000000000
> [    1.499336] Internal error: Oops: 86000004 [#1] PREEMPT SMP
> [    1.504849] Modules linked in:
> [    1.507877] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.8.0-rc4-00007-g07a1ca6cb00d #1
> [    1.515711] Hardware name: ARM Juno development board (r0) (DT)
> [    1.521569] task: ffff8009768a8000 task.stack: ffff8009768b0000
> [    1.527427] PC is at 0x240ab2f8
> [    1.530531] LR is at 0x240aaac0
> [    1.533637] pc : [<00000000240ab2f8>] lr : [<00000000240aaac0>] pstate: 60400145
> [    1.540954] sp : ffff8009768b3c50
> [    1.544230] x29: ffff8009768b3ca0 x28: 0000000000000000
> [    1.549492] x27: ffff000008d279c0 x26: ffff000008c70470
> [    1.554754] x25: ffff000008cbf230 x24: ffff000008c5fd70
> [    1.560016] x23: ffff000008e2c000 x22: ffff8009768b3cf0
> [    1.565278] x21: ffff8009768b3cf0 x20: ffff8009768b3d00
> [    1.570540] x19: 8000000000000003 x18: ffffffffffffffff
> [    1.575801] x17: 0000000000000000 x16: 0000000000000000
> [    1.581063] x15: ffff80097612f16b x14: 0000000000000000
> [    1.586323] x13: 0000000000000000 x12: 0000000000000038
> [    1.591584] x11: 0000000000000020 x10: 0101010101010101
> [    1.596846] x9 : 0000000000000000 x8 : ffffff7f7f7f7f7f
> [    1.602107] x7 : fefe7eff2f627371 x6 : 000000008080ffff
> [    1.607368] x5 : ffff8009768b3c98 x4 : 0000000000010001
> [    1.612629] x3 : 00000000ff000001 x2 : 00000000240aa3e4
> [    1.617890] x1 : ffff8009768b3c98 x0 : 0000000057d752e6
> [    1.623150]
> [    1.624622] Process swapper/0 (pid: 1, stack limit = 0xffff8009768b0028)
> [    1.631253] Stack: (0xffff8009768b3c50 to 0xffff8009768b4000)
> [    1.636941] 3c40:                                   ffff000008e5dcd8 ffff8009768b3d00
> [    1.644694] 3c60: 0000000000000140 ffff8009768b3cf0 ffff000008e2c000 ffff000008c5fd70
> [    1.652447] 3c80: ffff00000876236c ffff8009768b3d00 ffff8009768b3da8 ffff00000855d1b4
> [    1.660201] 3ca0: ffff8009768b3cd0 ffff000008703de8 ffff8009768b3da8 ffff000008dd9ff0
> [    1.667954] 3cc0: ffff8009768b3da8 0000000000000007 ffff8009768b3d10 ffff000008700964
> [    1.675707] 3ce0: ffff8009768b3da8 ffff800976a752e8 ffff8009768b3d40 ffff0000087006a8
> [    1.683461] 3d00: ffff000008bbe958 ffff000008ca081c ffff8009768b3d30 ffff0000087009c8
> [    1.691214] 3d20: ffff800976a75000 ffff0000087006b8 ffff8009768b3d70 ffff000008ca0878
> [    1.698974] 3d40: ffff000008bbe958 ffff800976a75000 0000000000000000 0000000000000000
> [    1.706727] 3d60: ffff000008dd94b0 ffff0000083f4820 ffff8009768b3dd0 ffff000008083b40
> [    1.714480] 3d80: ffff8009768b0000 ffff000008ca081c ffff8009768b3db0 0000000000000000
> [    1.722232] 3da0: 000000001dcd6500 0000000000000000 0000000000000000 0000000000000000
> [    1.729984] 3dc0: 0000000000000000 ffff000000000000 ffff8009768b3e40 ffff000008c70d14
> [    1.737737] 3de0: 0000000000000110 ffff000008e2c000 ffff000008cbf2c0 0000000000000007
> [    1.745490] 3e00: ffff000008d27900 0000000000000000 ffff000008e2c000 ffff000008b48ac8
> [    1.753242] 3e20: 0000000700000007 0000000000000000 0000000000000000 ffff000008c5fd70
> [    1.760995] 3e40: ffff8009768b3ea0 ffff0000088d15d0 ffff0000088d15c0 0000000000000000
> [    1.768747] 3e60: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [    1.776499] 3e80: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [    1.784252] 3ea0: 0000000000000000 ffff0000080830a0 ffff0000088d15c0 0000000000000000
> [    1.792004] 3ec0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [    1.799756] 3ee0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [    1.807508] 3f00: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [    1.815260] 3f20: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [    1.823023] 3f40: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [    1.830775] 3f60: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [    1.838527] 3f80: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [    1.846279] 3fa0: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [    1.854031] 3fc0: 0000000000000000 0000000000000005 0000000000000000 0000000000000000
> [    1.861783] 3fe0: 0000000000000000 0000000000000000 035b4d0700426040 000f000200440000
> [    1.869531] Call trace:
> [    1.871950] Exception stack(0xffff8009768b3a80 to 0xffff8009768b3bb0)
> [    1.878327] 3a80: 8000000000000003 0001000000000000 0000000080e2b000 00000000240ab2f8
> [    1.886080] 3aa0: ffff8009768b3b00 ffff00000818bb68 ffff800976015f80 00000000024000c0
> [    1.893832] 3ac0: ffff00000818bbe4 000000000000001a ffff80097608dc10 ffff800976bd1218
> [    1.901585] 3ae0: ffff8009768b3af0 ffff0000083f2670 ffff8009768b3b70 ffff0000083f2b40
> [    1.909338] 3b00: 0000000000000000 ffff800976811b00 0000000000000000 ffff800976811a90
> [    1.917091] 3b20: 0000000057d752e6 ffff8009768b3c98 00000000240aa3e4 00000000ff000001
> [    1.924844] 3b40: 0000000000010001 ffff8009768b3c98 000000008080ffff fefe7eff2f627371
> [    1.932596] 3b60: ffffff7f7f7f7f7f 0000000000000000 0101010101010101 0000000000000020
> [    1.940349] 3b80: 0000000000000038 0000000000000000 0000000000000000 ffff80097612f16b
> [    1.948098] 3ba0: 0000000000000000 0000000000000000
> [    1.952925] [<00000000240ab2f8>] 0x240ab2f8
> [    1.957069] [<ffff000008703de8>] efi_read_time+0x28/0x78
> [    1.962329] [<ffff000008700964>] __rtc_read_time.isra.1+0x44/0x70
> [    1.968360] [<ffff0000087009c8>] rtc_read_time+0x38/0x58
> [    1.973620] [<ffff000008ca0878>] rtc_hctosys+0x5c/0xe4
> [    1.978708] [<ffff000008083b40>] do_one_initcall+0x38/0x128
> [    1.984227] [<ffff000008c70d14>] kernel_init_freeable+0x1ac/0x250
> [    1.990261] [<ffff0000088d15d0>] kernel_init+0x10/0x100
> [    1.995433] [<ffff0000080830a0>] ret_from_fork+0x10/0x30
> [    2.000691] Code: bad PC value
>
> --
> Catalin

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching
  2016-09-10 10:56   ` Ard Biesheuvel
@ 2016-09-11 12:16     ` Catalin Marinas
  0 siblings, 0 replies; 30+ messages in thread
From: Catalin Marinas @ 2016-09-11 12:16 UTC (permalink / raw)
  To: linux-arm-kernel

On Sat, Sep 10, 2016 at 11:56:14AM +0100, Ard Biesheuvel wrote:
> On 10 September 2016 at 10:51, Catalin Marinas <catalin.marinas@arm.com> wrote:
> > On Fri, Sep 02, 2016 at 04:02:06PM +0100, Catalin Marinas wrote:
> >> This is the second version of the arm64 PAN emulation by disabling
> >> TTBR0_EL1 accesses. The major change from v1 is the use of a thread_info
> >> member to store the real TTBR0_EL1 value. The advantage is slightly
> >> simpler assembler macros for uaccess_enable with the downside that
> >> switch_mm() must always update the saved ttbr0 even if there is no mm
> >> switch.
> >
> > FYI, I got the Juno board rebooting in a loop with defconfig +
> > ARM64_TTBR0_PAN enabled. It takes about 20-40 reboots to get the panic
> > below on the EFI run-time services. I'll look into it on Monday (and
> > cc'ing Ard who I forgot to add originally). Including the full log
> > below:
> 
> Could you please try to reproduce it again, but this time with
> 'efi=debug' on the kernel command line? Thanks

Just for the record, following our private email exchanges: when
executing an EFI runtime service, an interrupt comes in and Linux
disables TTBR0 accesses. When returning from interrupt, the kernel
restores TTBR0_EL1 with the value in current thread_info which is
different from the efi_mm one.

Two potential solutions:

1. Temporarily update the current thread_info ttbr0 during EFI runtime
   services (preemption is disabled). Suggested by Ard

2. Go back to per-CPU saving of TTBR0_EL1 as in v1

I'll implement 1 on Monday and give it a try.

-- 
Catalin

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [kernel-hardening] Re: [PATCH v2 3/7] arm64: Introduce uaccess_{disable, enable} functionality based on TTBR0_EL1
  2016-09-06 10:45       ` Mark Rutland
@ 2016-09-11 13:55         ` Ard Biesheuvel
  2016-09-12  9:32           ` Catalin Marinas
  0 siblings, 1 reply; 30+ messages in thread
From: Ard Biesheuvel @ 2016-09-11 13:55 UTC (permalink / raw)
  To: linux-arm-kernel

On 6 September 2016 at 11:45, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Sep 06, 2016 at 11:27:42AM +0100, Catalin Marinas wrote:
>> On Mon, Sep 05, 2016 at 06:20:38PM +0100, Mark Rutland wrote:
>> > On Fri, Sep 02, 2016 at 04:02:09PM +0100, Catalin Marinas wrote:
>> > > +static inline void uaccess_ttbr0_enable(void)
>> > > +{
>> > > + unsigned long flags;
>> > > +
>> > > + /*
>> > > +  * Disable interrupts to avoid preemption and potential saved
>> > > +  * TTBR0_EL1 updates between reading the variable and the MSR.
>> > > +  */
>> > > + local_irq_save(flags);
>> > > + write_sysreg(current_thread_info()->ttbr0, ttbr0_el1);
>> > > + isb();
>> > > + local_irq_restore(flags);
>> > > +}
>> >
>> > I don't follow what problem this actually protects us against. In the
>> > case of preemption everything should be saved+restored transparently, or
>> > things would go wrong as soon as we enable IRQs anyway.
>> >
>> > Is this a hold-over from a percpu approach rather than the
>> > current_thread_info() approach?
>>
>> If we get preempted between reading current_thread_info()->ttbr0 and
>> writing TTBR0_EL1, a series of context switches could lead to the update
>> of the ASID part of ttbr0. The actual MSR would store an old ASID in
>> TTBR0_EL1.
>
> Ah! Can you fold something about racing with an ASID update into the
> description?
>
>> > > +#else
>> > > +static inline void uaccess_ttbr0_disable(void)
>> > > +{
>> > > +}
>> > > +
>> > > +static inline void uaccess_ttbr0_enable(void)
>> > > +{
>> > > +}
>> > > +#endif
>> >
>> > I think that it's better to drop the ifdef and add:
>> >
>> >     if (!IS_ENABLED(CONFIG_ARM64_TTBR0_PAN))
>> >             return;
>> >
>> > ... at the start of each function. GCC should optimize the entire thing
>> > away when not used, but we'll get compiler coverage regardless, and
>> > therefore less breakage. All the symbols we required should exist
>> > regardless.
>>
>> The reason for this is that thread_info.ttbr0 is conditionally defined.
>> I don't think the compiler would ignore it.
>
> Good point; I missed that.
>
> [...]
>
>> > How about something like:
>> >
>> >     .macro alternative_endif_else_nop
>> >     alternative_else
>> >     .rept ((662b-661b) / 4)
>> >            nop
>> >     .endr
>> >     alternative_endif
>> >     .endm
>> >
>> > So for the above we could have:
>> >
>> >     alternative_if_not ARM64_HAS_PAN
>> >             save_and_disable_irq \tmp2
>> >             uaccess_ttbr0_enable \tmp1
>> >             restore_irq \tmp2
>> >     alternative_endif_else_nop
>> >
>> > I'll see about spinning a patch, or discovering why that happens to be
>> > broken.
>>
>> This looks better. Minor comment, I would actually name the ending
>> statement alternative_else_nop_endif to match the order in which you'd
>> normally write them.
>
> Completely agreed. I already made this change locally, immediately after
> sending the suggestion. :)
>
>> > >    * tables again to remove any speculatively loaded cache lines.
>> > >    */
>> > >   mov     x0, x25
>> > > - add     x1, x26, #SWAPPER_DIR_SIZE
>> > > + add     x1, x26, #SWAPPER_DIR_SIZE + RESERVED_TTBR0_SIZE
>> > >   dmb     sy
>> > >   bl      __inval_cache_range
>> > >
>> > > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
>> > > index 659963d40bb4..fe393ccf9352 100644
>> > > --- a/arch/arm64/kernel/vmlinux.lds.S
>> > > +++ b/arch/arm64/kernel/vmlinux.lds.S
>> > > @@ -196,6 +196,11 @@ SECTIONS
>> > >   swapper_pg_dir = .;
>> > >   . += SWAPPER_DIR_SIZE;
>> > >
>> > > +#ifdef CONFIG_ARM64_TTBR0_PAN
>> > > + reserved_ttbr0 = .;
>> > > + . += PAGE_SIZE;
>> > > +#endif
>> >
>> > Surely RESERVED_TTBR0_SIZE, as elsewhere?
>>
>> I'll try to move it somewhere where it can be included in vmlinux.lds.S
>> (I can probably include cpufeature.h directly).
>

Do we really need another zero page? The ordinary zero page is already
statically allocated these days, so we could simply move it between
idmap_pg_dir[] and swapper_pg_dir[], and get all the changes in the
early boot code for free (given that it covers the range between the
start of idmap_pg_dir[] and the end of swapper_pg_dir[])

That way, we could refer to __pa(empty_zero_page) anywhere by reading
ttbr1_el1 and subtracting PAGE_SIZE

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [kernel-hardening] Re: [PATCH v2 3/7] arm64: Introduce uaccess_{disable, enable} functionality based on TTBR0_EL1
  2016-09-11 13:55         ` [kernel-hardening] " Ard Biesheuvel
@ 2016-09-12  9:32           ` Catalin Marinas
  0 siblings, 0 replies; 30+ messages in thread
From: Catalin Marinas @ 2016-09-12  9:32 UTC (permalink / raw)
  To: linux-arm-kernel

On Sun, Sep 11, 2016 at 02:55:12PM +0100, Ard Biesheuvel wrote:
> On 6 September 2016 at 11:45, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Tue, Sep 06, 2016 at 11:27:42AM +0100, Catalin Marinas wrote:
> >> On Mon, Sep 05, 2016 at 06:20:38PM +0100, Mark Rutland wrote:
> >> > On Fri, Sep 02, 2016 at 04:02:09PM +0100, Catalin Marinas wrote:
> >> > >    * tables again to remove any speculatively loaded cache lines.
> >> > >    */
> >> > >   mov     x0, x25
> >> > > - add     x1, x26, #SWAPPER_DIR_SIZE
> >> > > + add     x1, x26, #SWAPPER_DIR_SIZE + RESERVED_TTBR0_SIZE
> >> > >   dmb     sy
> >> > >   bl      __inval_cache_range
> >> > >
> >> > > diff --git a/arch/arm64/kernel/vmlinux.lds.S b/arch/arm64/kernel/vmlinux.lds.S
> >> > > index 659963d40bb4..fe393ccf9352 100644
> >> > > --- a/arch/arm64/kernel/vmlinux.lds.S
> >> > > +++ b/arch/arm64/kernel/vmlinux.lds.S
> >> > > @@ -196,6 +196,11 @@ SECTIONS
> >> > >   swapper_pg_dir = .;
> >> > >   . += SWAPPER_DIR_SIZE;
> >> > >
> >> > > +#ifdef CONFIG_ARM64_TTBR0_PAN
> >> > > + reserved_ttbr0 = .;
> >> > > + . += PAGE_SIZE;
> >> > > +#endif
> >> >
> >> > Surely RESERVED_TTBR0_SIZE, as elsewhere?
> >>
> >> I'll try to move it somewhere where it can be included in vmlinux.lds.S
> >> (I can probably include cpufeature.h directly).
> 
> Do we really need another zero page? The ordinary zero page is already
> statically allocated these days, so we could simply move it between
> idmap_pg_dir[] and swapper_pg_dir[], and get all the changes in the
> early boot code for free (given that it covers the range between the
> start of idmap_pg_dir[] and the end of swapper_pg_dir[])
> 
> That way, we could refer to __pa(empty_zero_page) anywhere by reading
> ttbr1_el1 and subtracting PAGE_SIZE

That's fine by me. I'll cherry-pick your patch and rebase this series on
top.

-- 
Catalin

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 1/7] arm64: Factor out PAN enabling/disabling into separate uaccess_* macros
  2016-09-05 15:38   ` Mark Rutland
@ 2016-09-12 14:52     ` Catalin Marinas
  2016-09-12 15:09       ` Mark Rutland
  0 siblings, 1 reply; 30+ messages in thread
From: Catalin Marinas @ 2016-09-12 14:52 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Sep 05, 2016 at 04:38:28PM +0100, Mark Rutland wrote:
> On Fri, Sep 02, 2016 at 04:02:07PM +0100, Catalin Marinas wrote:
> >  /*
> > + * User access enabling/disabling.
> > + */
> > +#define uaccess_disable(alt)						\
> > +do {									\
> > +	asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1), alt,			\
> > +			CONFIG_ARM64_PAN));				\
> > +} while (0)
> > +
> > +#define uaccess_enable(alt)						\
> > +do {									\
> > +	asm(ALTERNATIVE("nop", SET_PSTATE_PAN(0), alt,			\
> > +			CONFIG_ARM64_PAN));				\
> > +} while (0)
> 
> Passing the alternative down is somewhat confusing. e.g. in the futex
> case it looks like we're only doing something when PAN is present,
> whereas we'll manipulate TTBR0 in the absence of PAN.

I agree it's confusing (I got it wrong first time as well and used the
wrong alternative for futex).

> If I've understood correctly, we need this to distinguish regular
> load/store uaccess sequences (eg. the futex code) from potentially
> patched unprivileged load/store sequences (e.g. {get,put}_user) when
> poking PSTATE.PAN.
> 
> So perhaps we could ahve something like:
> 
> * privileged_uaccess_{enable,disable}()
>   Which toggle TTBR0, or PAN (always).
>   These would handle cases like the futex/swp code.
>  
> * (unprivileged_)uaccess_{enable,disable}()
>   Which toggle TTBR0, or PAN (in the absence of UAO).
>   These would handle cases like the {get,put}_user sequences.
> 
> Though perhaps that is just as confusing. ;)

I find it more confusing. In the non-UAO case, get_user etc. would
normally have to use privileged_uaccess_enable() since ldr is not
replaced with ldtr. Maybe uaccess_enable_for_exclusives() but it doesn't
look any better. I think adding some comments to the code
(uaccess_enable macro) would work better, clarifying what the
alternative is for.

-- 
Catalin

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 1/7] arm64: Factor out PAN enabling/disabling into separate uaccess_* macros
  2016-09-12 14:52     ` Catalin Marinas
@ 2016-09-12 15:09       ` Mark Rutland
  2016-09-12 16:26         ` Catalin Marinas
  0 siblings, 1 reply; 30+ messages in thread
From: Mark Rutland @ 2016-09-12 15:09 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Sep 12, 2016 at 03:52:19PM +0100, Catalin Marinas wrote:
> On Mon, Sep 05, 2016 at 04:38:28PM +0100, Mark Rutland wrote:
> > On Fri, Sep 02, 2016 at 04:02:07PM +0100, Catalin Marinas wrote:
> > >  /*
> > > + * User access enabling/disabling.
> > > + */
> > > +#define uaccess_disable(alt)						\
> > > +do {									\
> > > +	asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1), alt,			\
> > > +			CONFIG_ARM64_PAN));				\
> > > +} while (0)
> > > +
> > > +#define uaccess_enable(alt)						\
> > > +do {									\
> > > +	asm(ALTERNATIVE("nop", SET_PSTATE_PAN(0), alt,			\
> > > +			CONFIG_ARM64_PAN));				\
> > > +} while (0)
> > 
> > Passing the alternative down is somewhat confusing. e.g. in the futex
> > case it looks like we're only doing something when PAN is present,
> > whereas we'll manipulate TTBR0 in the absence of PAN.
> 
> I agree it's confusing (I got it wrong first time as well and used the
> wrong alternative for futex).
> 
> > If I've understood correctly, we need this to distinguish regular
> > load/store uaccess sequences (eg. the futex code) from potentially
> > patched unprivileged load/store sequences (e.g. {get,put}_user) when
> > poking PSTATE.PAN.
> > 
> > So perhaps we could ahve something like:
> > 
> > * privileged_uaccess_{enable,disable}()
> >   Which toggle TTBR0, or PAN (always).
> >   These would handle cases like the futex/swp code.
> >  
> > * (unprivileged_)uaccess_{enable,disable}()
> >   Which toggle TTBR0, or PAN (in the absence of UAO).
> >   These would handle cases like the {get,put}_user sequences.
> > 
> > Though perhaps that is just as confusing. ;)
> 
> I find it more confusing. 

Fair enough. :)

> In the non-UAO case, get_user etc. would
> normally have to use privileged_uaccess_enable() since ldr is not
> replaced with ldtr. Maybe uaccess_enable_for_exclusives() but it doesn't
> look any better. 

I strongly prefer uaccess_enable_exclusives(), or something of that sort
to both of the above. ;)

> I think adding some comments to the code (uaccess_enable macro) would
> work better, clarifying what the alternative is for.

That will make things smoewhat clearer, though only after one reads the
comments. In contrast, uaccess_enable_exclusives() would be
self-documenting w.r.t. the intented use-case.

Do we ever want to use the 8.1 atomics for futexes? If so, perhaps
uaccess_enable_atomics()?

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 1/7] arm64: Factor out PAN enabling/disabling into separate uaccess_* macros
  2016-09-12 15:09       ` Mark Rutland
@ 2016-09-12 16:26         ` Catalin Marinas
  0 siblings, 0 replies; 30+ messages in thread
From: Catalin Marinas @ 2016-09-12 16:26 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Sep 12, 2016 at 04:09:59PM +0100, Mark Rutland wrote:
> On Mon, Sep 12, 2016 at 03:52:19PM +0100, Catalin Marinas wrote:
> > On Mon, Sep 05, 2016 at 04:38:28PM +0100, Mark Rutland wrote:
> > > On Fri, Sep 02, 2016 at 04:02:07PM +0100, Catalin Marinas wrote:
> > > >  /*
> > > > + * User access enabling/disabling.
> > > > + */
> > > > +#define uaccess_disable(alt)						\
> > > > +do {									\
> > > > +	asm(ALTERNATIVE("nop", SET_PSTATE_PAN(1), alt,			\
> > > > +			CONFIG_ARM64_PAN));				\
> > > > +} while (0)
> > > > +
> > > > +#define uaccess_enable(alt)						\
> > > > +do {									\
> > > > +	asm(ALTERNATIVE("nop", SET_PSTATE_PAN(0), alt,			\
> > > > +			CONFIG_ARM64_PAN));				\
> > > > +} while (0)
> > > 
> > > Passing the alternative down is somewhat confusing. e.g. in the futex
> > > case it looks like we're only doing something when PAN is present,
> > > whereas we'll manipulate TTBR0 in the absence of PAN.
> > 
> > I agree it's confusing (I got it wrong first time as well and used the
> > wrong alternative for futex).
> > 
> > > If I've understood correctly, we need this to distinguish regular
> > > load/store uaccess sequences (eg. the futex code) from potentially
> > > patched unprivileged load/store sequences (e.g. {get,put}_user) when
> > > poking PSTATE.PAN.
> > > 
> > > So perhaps we could ahve something like:
> > > 
> > > * privileged_uaccess_{enable,disable}()
> > >   Which toggle TTBR0, or PAN (always).
> > >   These would handle cases like the futex/swp code.
> > >  
> > > * (unprivileged_)uaccess_{enable,disable}()
> > >   Which toggle TTBR0, or PAN (in the absence of UAO).
> > >   These would handle cases like the {get,put}_user sequences.
> > > 
> > > Though perhaps that is just as confusing. ;)
> > 
> > I find it more confusing. 
> 
> Fair enough. :)
> 
> > In the non-UAO case, get_user etc. would
> > normally have to use privileged_uaccess_enable() since ldr is not
> > replaced with ldtr. Maybe uaccess_enable_for_exclusives() but it doesn't
> > look any better. 
> 
> I strongly prefer uaccess_enable_exclusives(), or something of that sort
> to both of the above. ;)

I think we would need a few more uaccess_enable_* variants (cache
maintenance, Xen) which makes this impractical.

We can consider the PAN_NOT_UAO the special case and if we assume that
UAO also implies PAN (ARMv8.2), we can define uaccess_enable_not_uao()
for the get_user etc. cases. We would use uaccess_enable() for the rest.

-- 
Catalin

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2016-09-12 16:26 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-02 15:02 [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching Catalin Marinas
2016-09-02 15:02 ` [PATCH v2 1/7] arm64: Factor out PAN enabling/disabling into separate uaccess_* macros Catalin Marinas
2016-09-05 15:38   ` Mark Rutland
2016-09-12 14:52     ` Catalin Marinas
2016-09-12 15:09       ` Mark Rutland
2016-09-12 16:26         ` Catalin Marinas
2016-09-02 15:02 ` [PATCH v2 2/7] arm64: Factor out TTBR0_EL1 post-update workaround into a specific asm macro Catalin Marinas
2016-09-05 16:11   ` Mark Rutland
2016-09-02 15:02 ` [PATCH v2 3/7] arm64: Introduce uaccess_{disable, enable} functionality based on TTBR0_EL1 Catalin Marinas
2016-09-05 17:20   ` Mark Rutland
2016-09-06 10:27     ` Catalin Marinas
2016-09-06 10:45       ` Mark Rutland
2016-09-11 13:55         ` [kernel-hardening] " Ard Biesheuvel
2016-09-12  9:32           ` Catalin Marinas
2016-09-09 17:15   ` Catalin Marinas
2016-09-02 15:02 ` [PATCH v2 4/7] arm64: Disable TTBR0_EL1 during normal kernel execution Catalin Marinas
2016-09-06 17:31   ` Mark Rutland
2016-09-02 15:02 ` [PATCH v2 5/7] arm64: Handle faults caused by inadvertent user access with PAN enabled Catalin Marinas
2016-09-02 15:02 ` [PATCH v2 6/7] arm64: xen: Enable user access before a privcmd hvc call Catalin Marinas
2016-09-02 15:02 ` [PATCH v2 7/7] arm64: Enable CONFIG_ARM64_TTBR0_PAN Catalin Marinas
2016-09-02 15:47   ` Mark Rutland
2016-09-07 23:20 ` [PATCH v2 0/7] arm64: Privileged Access Never using TTBR0_EL1 switching Kees Cook
2016-09-08 12:51   ` Catalin Marinas
2016-09-08 15:50     ` Kees Cook
2016-09-09 16:31     ` Mark Rutland
2016-09-09 18:24       ` Kees Cook
2016-09-09 23:40 ` [kernel-hardening] " David Brown
2016-09-10  9:51 ` Catalin Marinas
2016-09-10 10:56   ` Ard Biesheuvel
2016-09-11 12:16     ` Catalin Marinas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).