arm64: uaccess: Fix omissions from usercopy whitelist

PATCH New Dave Martin 3 participants 6 messages Mar 28, 2018

Subsystems (2)

arm64 ptrace support

lore

Share & tools

lore

Dave Martin

author

2018-03-28 09:50 UTC #0 lore raw

v2 cover arm64: uaccess: Fix omissions from usercopy whitelist

When the hardend usercopy support was added for arm64, it was concluded [1] that all cases of usercopy into and out of thread_struct were statically sized and so didn't require explicit whitelisting of the appropriate fields in thread_struct.

Testing with usercopy hardening enabled has revealed that this is not the case for certain ptrace regset manipulation calls on arm64. This series attempts to fix the usercopy whitelist so that such copies can be policed correctly. See patch 2 for a more detailed explanation of the change.

I wrote patch 1 recently for unrelated reasons, but it turns out to be useful here since it splits the user-copiable cpu field out from struct fpsimd_state. Mixing user and non-user fields in that struct has proven awkard in a variety of situations now, so it is a good opportunity to separate them.

Bug reproduced, and fix tested, on Arm Juno r0, with defconfig + CONFIG_HARDENED_USERCOPY=y CONFIG_HARDENED_USERCOPY_FALLBACK=y

Not tested in many configurations. Edits were done by the equivalent of the following shell commands (plus some rewrapping and a few manual fixups).

	s='\(\.\|->\)\(fpsimd_state\|tp2\?_value\)'
	git grep -l "$s" -- arch/arm64/ | xargs -d'\n' sed -i "s/$s/\1uw.\2/g"

The bug can be reproduced by running up to a breakpoint in gdb (or anything else that does a PTRACE_GETREGSET on NT_PRFPREG or NT_ARM_TLS).

Without the fix, reading an affected regset resulting in a WARN_ON() splat from the usercopy hardening code, similar to the following:

[   37.429560] ------------[ cut here ]------------
[   37.434220] Bad or missing usercopy whitelist? Kernel memory exposure attempt detected from SLUB object 'task_struct' (offset 2664, size 8)!
[   37.446895] WARNING: CPU: 4 PID: 2431 at mm/usercopy.c:81 usercopy_warn+0x7c/0xc8
[   37.454387] Modules linked in:
[   37.457449] CPU: 4 PID: 2431 Comm: gdb Not tainted 4.16.0-rc6 #10
[   37.463550] Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development Platform, BIOS EDK II Dec 15 2016
[   37.474352] pstate: 60000005 (nZCv daif -PAN -UAO)
[   37.479149] pc : usercopy_warn+0x7c/0xc8
[   37.483075] lr : usercopy_warn+0x7c/0xc8
[   37.486998] sp : ffff00000e0fbcb0
[   37.490313] x29: ffff00000e0fbcc0 x28: ffff8009759c0000
[   37.495637] x27: ffff000008a71000 x26: 0000000000000075
[   37.500959] x25: 0000000000000124 x24: 0000000000000015
[   37.506280] x23: ffff7e0025d04880 x22: ffff800974122670
[   37.511601] x21: 0000000000000001 x20: 0000000000000008
[   37.516923] x19: ffff800974122668 x18: 0000000000000006
[   37.522244] x17: 0000ffff8f89ed40 x16: ffff0000080deb58
[   37.527565] x15: ffff0000091864dd x14: 6620646574636574
[   37.532885] x13: 65642074706d6574 x12: 746120657275736f
[   37.538206] x11: ffff0000085c5960 x10: ffff000009029df8
[   37.543527] x9 : 4b203f7473696c65 x8 : 657a6973202c3436
[   37.548848] x7 : 3632207465736666 x6 : 000000000000012f
[   37.554169] x5 : 0000000000000000 x4 : 0000000000000000
[   37.559489] x3 : 0000000000000000 x2 : ffff8009759c0000
[   37.564810] x1 : 73ce9fcbba171400 x0 : 0000000000000000
[   37.570131] Call trace:
[   37.572577]  usercopy_warn+0x7c/0xc8
[   37.576156]  __check_heap_object+0xc4/0x130
[   37.580343]  __check_object_size+0x184/0x1e0
[   37.584618]  tls_get+0x6c/0xd0
[   37.587674]  ptrace_regset+0x11c/0x160
[   37.591426]  ptrace_request+0x590/0x638
[   37.595265]  arch_ptrace+0xc/0x18
[   37.598581]  SyS_ptrace+0xc8/0x120
[   37.601985]  el0_svc_naked+0x30/0x34
[   37.605561] ---[ end trace 28b009f07d48b579 ]---

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-January/554362.html

Dave Martin (2):
arm64: fpsimd: Split cpu field out from struct fpsimd_state
arm64: uaccess: Fix omissions from usercopy whitelist

 arch/arm64/include/asm/fpsimd.h    | 29 ++-------------
 arch/arm64/include/asm/processor.h | 40 +++++++++++++--------
 arch/arm64/kernel/fpsimd.c         | 73 ++++++++++++++++++++------------------
 arch/arm64/kernel/process.c        |  6 ++--
 arch/arm64/kernel/ptrace.c         | 30 ++++++++--------
 arch/arm64/kernel/signal.c         |  2 +-
 arch/arm64/kernel/signal32.c       |  2 +-
 arch/arm64/kernel/sys_compat.c     |  2 +-
 8 files changed, 88 insertions(+), 96 deletions(-)

Signature · 1 line

2.1.4

Dave Martin

author

2018-03-28 09:50 UTC #1 lore raw

PATCH v2 1/2 arm64: fpsimd: Split cpu field out from struct fpsimd_state

In preparation for using a common representation of the FPSIMD state for tasks and KVM vcpus, this patch separates out the "cpu" field that is used to track the cpu on which the state was most recently loaded.

This will allow common code to operate on task and vcpu contexts without requiring the cpu field to be stored at the same offset from the FPSIMD register data in both cases. This should avoid the need for messing with the definition of those parts of struct vcpu_arch that are exposed in the KVM user ABI.

The resulting change is also convenient for grouping and defining the set of thread_struct fields that are supposed to be accessible to copy_{to,from}_user(), which includes user_fpsimd_state but should exclude the cpu field. This patch does not amend the usercopy whitelist to match: that will be addressed in a subsequent patch.

Signed-off-by Dave Martin <[email protected]>

---
 arch/arm64/include/asm/fpsimd.h    | 29 ++------------------------
 arch/arm64/include/asm/processor.h |  4 ++--
 arch/arm64/kernel/fpsimd.c         | 42 +++++++++++++++++++++-----------------
 arch/arm64/kernel/ptrace.c         | 10 ++++-----
 arch/arm64/kernel/signal.c         |  3 +--
 arch/arm64/kernel/signal32.c       |  3 +--
 6 files changed, 34 insertions(+), 57 deletions(-)

arch/arm64/include/asm/fpsimd.h +2 ~~-27~~

index 8857a0f..1bfc920 100644
@@ -24,31 +24,6 @@
 #include <linux/cache.h>
 #include <linux/stddef.h>
 
-/*
- * FP/SIMD storage area has:
- *  - FPSR and FPCR
- *  - 32 128-bit data registers
- *
- * Note that user_fpsimd forms a prefix of this structure, which is
- * relied upon in the ptrace FP/SIMD accessors.
- */
-struct fpsimd_state {
-	union {
-		struct user_fpsimd_state user_fpsimd;
-		struct {
-			__uint128_t vregs[32];
-			u32 fpsr;
-			u32 fpcr;
-			/*
-			 * For ptrace compatibility, pad to next 128-bit
-			 * boundary here if extending this struct.
-			 */
-		};
-	};
-	/* the id of the last cpu to have restored this state */
-	unsigned int cpu;
-};
-
 #if defined(__KERNEL__) && defined(CONFIG_COMPAT)
 /* Masks for extracting the FPSR and FPCR from the FPSCR */
 #define VFP_FPSCR_STAT_MASK	0xf800009f

@@ -62,8 +37,8 @@ struct fpsimd_state {
 
 struct task_struct;
 
-extern void fpsimd_save_state(struct fpsimd_state *state);
-extern void fpsimd_load_state(struct fpsimd_state *state);
+extern void fpsimd_save_state(struct user_fpsimd_state *state);
+extern void fpsimd_load_state(struct user_fpsimd_state *state);
 
 extern void fpsimd_thread_switch(struct task_struct *next);
 extern void fpsimd_flush_thread(void);

arch/arm64/include/asm/processor.h +2 -2

index fce604e..4a04535 100644
@@ -37,7 +37,6 @@
 #include <linux/string.h>
 
 #include <asm/alternative.h>
-#include <asm/fpsimd.h>
 #include <asm/hw_breakpoint.h>
 #include <asm/lse.h>
 #include <asm/pgtable-hwdef.h>

@@ -107,7 +106,8 @@ struct thread_struct {
 #ifdef CONFIG_COMPAT
 	unsigned long		tp2_value;
 #endif
-	struct fpsimd_state	fpsimd_state;
+	struct user_fpsimd_state fpsimd_state;
+	unsigned int		fpsimd_cpu;
 	void			*sve_state;	/* SVE registers, if any */
 	unsigned int		sve_vl;		/* SVE vector length */
 	unsigned int		sve_vl_onexec;	/* SVE vl after next exec */

arch/arm64/kernel/fpsimd.c +23 ~~-19~~

133 lines not on this page · raw message

arch/arm64/kernel/ptrace.c +5 -5

index 9ae31f7..fdeaba0de 100644
@@ -629,7 +629,7 @@ static int __fpr_get(struct task_struct *target,
 
 	sve_sync_to_fpsimd(target);
 
-	uregs = &target->thread.fpsimd_state.user_fpsimd;
+	uregs = &target->thread.fpsimd_state;
 
 	return user_regset_copyout(&pos, &count, &kbuf, &ubuf, uregs,
 				   start_pos, start_pos + sizeof(*uregs));

@@ -660,14 +660,14 @@ static int __fpr_set(struct task_struct *target,
 	 */
 	sve_sync_to_fpsimd(target);
 
-	newstate = target->thread.fpsimd_state.user_fpsimd;
+	newstate = target->thread.fpsimd_state;
 
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &newstate,
 				 start_pos, start_pos + sizeof(newstate));
 	if (ret)
 		return ret;
 
-	target->thread.fpsimd_state.user_fpsimd = newstate;
+	target->thread.fpsimd_state = newstate;
 
 	return ret;
 }

@@ -1169,7 +1169,7 @@ static int compat_vfp_get(struct task_struct *target,
 	compat_ulong_t fpscr;
 	int ret, vregs_end_pos;
 
-	uregs = &target->thread.fpsimd_state.user_fpsimd;
+	uregs = &target->thread.fpsimd_state;
 
 	if (target == current)
 		fpsimd_preserve_current_state();

@@ -1202,7 +1202,7 @@ static int compat_vfp_set(struct task_struct *target,
 	compat_ulong_t fpscr;
 	int ret, vregs_end_pos;
 
-	uregs = &target->thread.fpsimd_state.user_fpsimd;
+	uregs = &target->thread.fpsimd_state;
 
 	vregs_end_pos = VFP_STATE_SIZE - sizeof(compat_ulong_t);
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, uregs, 0,

arch/arm64/kernel/signal.c +1 -2

index f60c052..d026615 100644
@@ -178,8 +178,7 @@ static void __user *apply_user_offset(
 
 static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
 {
-	struct user_fpsimd_state const *fpsimd =
-		&current->thread.fpsimd_state.user_fpsimd;
+	struct user_fpsimd_state const *fpsimd = &current->thread.fpsimd_state;
 	int err;
 
 	/* copy the FP and status/control registers */

arch/arm64/kernel/signal32.c +1 -2

index 79feb86..4ea38d3 100644
@@ -148,8 +148,7 @@ union __fpsimd_vreg {
 
 static int compat_preserve_vfp_context(struct compat_vfp_sigframe __user *frame)
 {
-	struct user_fpsimd_state const *fpsimd =
-		&current->thread.fpsimd_state.user_fpsimd;
+	struct user_fpsimd_state const *fpsimd = &current->thread.fpsimd_state;
 	compat_ulong_t magic = VFP_MAGIC;
 	compat_ulong_t size = VFP_STORAGE_SIZE;
 	compat_ulong_t fpscr, fpexc;

Signature · 1 line

2.1.4

Dave Martin

author

2018-03-28 09:50 UTC #2 lore raw

PATCH v2 2/2 arm64: uaccess: Fix omissions from usercopy whitelist

In reply to Dave Martin

When the hardend usercopy support was added for arm64, it was concluded that all cases of usercopy into and out of thread_struct were statically sized and so didn't require explicit whitelisting of the appropriate fields in thread_struct.

Testing with usercopy hardening enabled has revealed that this is not the case for certain ptrace regset manipulation calls on arm64. This occurs because the sizes of usercopies associated with the regset API are dynamic by construction, and because arm64 does not always stage such copies via the stack: indeed the regset API is designed to avoid the need for that by adding some bounds checking.

This is currently believed to affect only the fpsimd and TLS registers.

Because the whitelisted fields in thread_struct must be contiguous, this patch groups them together in a nested struct. It is also necessary to be able to determine the location and size of that struct, so rather than making the struct anonymous (which would save on edits elsewhere) or adding an anonymous union containing named and unnamed instances of the same struct (gross), this patch gives the struct a name and makes the necessary edits to code that references it (noisy but simple).

Care is needed to ensure that the new struct does not contain padding (which the usercopy hardening would fail to protect).

For this reason, the presence of tp2_value is made unconditional, since a padding field would be needed there in any case. This pads up to the 16-byte alignment required by struct user_fpsimd_state.

Reported-by Mark Rutland <[email protected]>

Fixes 9e8084d3f761 ("arm64: Implement thread_struct whitelist for hardened usercopy")

Signed-off-by Dave Martin <[email protected]>

---

Changes since v1:

* Add a BUILD_BUG_ON() check for padding in the whitelist struct.
* Move to using sizeof_field() for assigning *size; get rid of the dummy pointer that was used previously.
* Delete bogus comment about why no whitelist is (was) needed.

---
 arch/arm64/include/asm/processor.h | 38 +++++++++++++++++++-----------
 arch/arm64/kernel/fpsimd.c         | 47 +++++++++++++++++++-------------------
 arch/arm64/kernel/process.c        |  6 ++---
 arch/arm64/kernel/ptrace.c         | 30 ++++++++++++------------
 arch/arm64/kernel/signal.c         |  3 ++-
 arch/arm64/kernel/signal32.c       |  3 ++-
 arch/arm64/kernel/sys_compat.c     |  2 +-
 7 files changed, 72 insertions(+), 57 deletions(-)

arch/arm64/include/asm/processor.h +25 ~~-13~~

index 4a04535..224af48 100644
@@ -34,6 +34,8 @@
 
 #ifdef __KERNEL__
 
+#include <linux/build_bug.h>
+#include <linux/stddef.h>
 #include <linux/string.h>
 
 #include <asm/alternative.h>

@@ -102,11 +104,18 @@ struct cpu_context {
 
 struct thread_struct {
 	struct cpu_context	cpu_context;	/* cpu context */
-	unsigned long		tp_value;	/* TLS register */
-#ifdef CONFIG_COMPAT
-	unsigned long		tp2_value;
-#endif
-	struct user_fpsimd_state fpsimd_state;
+
+	/*
+	 * Whitelisted fields for hardened usercopy:
+	 * Maintainers must ensure manually that this contains no
+	 * implicit padding.
+	 */
+	struct {
+		unsigned long	tp_value;	/* TLS register */
+		unsigned long	tp2_value;
+		struct user_fpsimd_state fpsimd_state;
+	} uw;
+
 	unsigned int		fpsimd_cpu;
 	void			*sve_state;	/* SVE registers, if any */
 	unsigned int		sve_vl;		/* SVE vector length */

@@ -116,14 +125,17 @@ struct thread_struct {
 	struct debug_info	debug;		/* debugging */
 };
 
-/*
- * Everything usercopied to/from thread_struct is statically-sized, so
- * no hardened usercopy whitelist is needed.
- */
 static inline void arch_thread_struct_whitelist(unsigned long *offset,
 						unsigned long *size)
 {
-	*offset = *size = 0;
+	/* Verify that there is no padding among the whitelisted fields: */
+	BUILD_BUG_ON(sizeof_field(struct thread_struct, uw) !=
+		     sizeof_field(struct thread_struct, uw.tp_value) +
+		     sizeof_field(struct thread_struct, uw.tp2_value) +
+		     sizeof_field(struct thread_struct, uw.fpsimd_state));
+
+	*offset = offsetof(struct thread_struct, uw);
+	*size = sizeof_field(struct thread_struct, uw);
 }
 
 #ifdef CONFIG_COMPAT

@@ -131,13 +143,13 @@ static inline void arch_thread_struct_whitelist(unsigned long *offset,
 ({									\
 	unsigned long *__tls;						\
 	if (is_compat_thread(task_thread_info(t)))			\
-		__tls = &(t)->thread.tp2_value;				\
+		__tls = &(t)->thread.uw.tp2_value;			\
 	else								\
-		__tls = &(t)->thread.tp_value;				\
+		__tls = &(t)->thread.uw.tp_value;			\
 	__tls;								\
  })
 #else
-#define task_user_tls(t)	(&(t)->thread.tp_value)
+#define task_user_tls(t)	(&(t)->thread.uw.tp_value)
 #endif
 
 /* Sync TPIDR_EL0 back to thread_struct for current */

arch/arm64/kernel/fpsimd.c +24 ~~-23~~

157 lines not on this page · raw message

arch/arm64/kernel/process.c +3 -3

index c0da6ef..f08a2ed 100644
@@ -257,7 +257,7 @@ static void tls_thread_flush(void)
 	write_sysreg(0, tpidr_el0);
 
 	if (is_compat_task()) {
-		current->thread.tp_value = 0;
+		current->thread.uw.tp_value = 0;
 
 		/*
 		 * We need to ensure ordering between the shadow state and the

@@ -351,7 +351,7 @@ int copy_thread(unsigned long clone_flags, unsigned long stack_start,
 		 * for the new thread.
 		 */
 		if (clone_flags & CLONE_SETTLS)
-			p->thread.tp_value = childregs->regs[3];
+			p->thread.uw.tp_value = childregs->regs[3];
 	} else {
 		memset(childregs, 0, sizeof(struct pt_regs));
 		childregs->pstate = PSR_MODE_EL1h;

@@ -379,7 +379,7 @@ static void tls_thread_switch(struct task_struct *next)
 	tls_preserve_current_state();
 
 	if (is_compat_thread(task_thread_info(next)))
-		write_sysreg(next->thread.tp_value, tpidrro_el0);
+		write_sysreg(next->thread.uw.tp_value, tpidrro_el0);
 	else if (!arm64_kernel_unmapped_at_el0())
 		write_sysreg(0, tpidrro_el0);

arch/arm64/kernel/ptrace.c +15 ~~-15~~

index fdeaba0de..436a132 100644
@@ -629,7 +629,7 @@ static int __fpr_get(struct task_struct *target,
 
 	sve_sync_to_fpsimd(target);
 
-	uregs = &target->thread.fpsimd_state;
+	uregs = &target->thread.uw.fpsimd_state;
 
 	return user_regset_copyout(&pos, &count, &kbuf, &ubuf, uregs,
 				   start_pos, start_pos + sizeof(*uregs));

@@ -655,19 +655,19 @@ static int __fpr_set(struct task_struct *target,
 	struct user_fpsimd_state newstate;
 
 	/*
-	 * Ensure target->thread.fpsimd_state is up to date, so that a
+	 * Ensure target->thread.uw.fpsimd_state is up to date, so that a
 	 * short copyin can't resurrect stale data.
 	 */
 	sve_sync_to_fpsimd(target);
 
-	newstate = target->thread.fpsimd_state;
+	newstate = target->thread.uw.fpsimd_state;
 
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &newstate,
 				 start_pos, start_pos + sizeof(newstate));
 	if (ret)
 		return ret;
 
-	target->thread.fpsimd_state = newstate;
+	target->thread.uw.fpsimd_state = newstate;
 
 	return ret;
 }

@@ -692,7 +692,7 @@ static int tls_get(struct task_struct *target, const struct user_regset *regset,
 		   unsigned int pos, unsigned int count,
 		   void *kbuf, void __user *ubuf)
 {
-	unsigned long *tls = &target->thread.tp_value;
+	unsigned long *tls = &target->thread.uw.tp_value;
 
 	if (target == current)
 		tls_preserve_current_state();

@@ -705,13 +705,13 @@ static int tls_set(struct task_struct *target, const struct user_regset *regset,
 		   const void *kbuf, const void __user *ubuf)
 {
 	int ret;
-	unsigned long tls = target->thread.tp_value;
+	unsigned long tls = target->thread.uw.tp_value;
 
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &tls, 0, -1);
 	if (ret)
 		return ret;
 
-	target->thread.tp_value = tls;
+	target->thread.uw.tp_value = tls;
 	return ret;
 }

@@ -842,7 +842,7 @@ static int sve_get(struct task_struct *target,
 	start = end;
 	end = SVE_PT_SVE_FPCR_OFFSET(vq) + SVE_PT_SVE_FPCR_SIZE;
 	ret = user_regset_copyout(&pos, &count, &kbuf, &ubuf,
-				  &target->thread.fpsimd_state.fpsr,
+				  &target->thread.uw.fpsimd_state.fpsr,
 				  start, end);
 	if (ret)
 		return ret;

@@ -941,7 +941,7 @@ static int sve_set(struct task_struct *target,
 	start = end;
 	end = SVE_PT_SVE_FPCR_OFFSET(vq) + SVE_PT_SVE_FPCR_SIZE;
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf,
-				 &target->thread.fpsimd_state.fpsr,
+				 &target->thread.uw.fpsimd_state.fpsr,
 				 start, end);
 
 out:

@@ -1169,7 +1169,7 @@ static int compat_vfp_get(struct task_struct *target,
 	compat_ulong_t fpscr;
 	int ret, vregs_end_pos;
 
-	uregs = &target->thread.fpsimd_state;
+	uregs = &target->thread.uw.fpsimd_state;
 
 	if (target == current)
 		fpsimd_preserve_current_state();

@@ -1202,7 +1202,7 @@ static int compat_vfp_set(struct task_struct *target,
 	compat_ulong_t fpscr;
 	int ret, vregs_end_pos;
 
-	uregs = &target->thread.fpsimd_state;
+	uregs = &target->thread.uw.fpsimd_state;
 
 	vregs_end_pos = VFP_STATE_SIZE - sizeof(compat_ulong_t);
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, uregs, 0,

@@ -1225,7 +1225,7 @@ static int compat_tls_get(struct task_struct *target,
 			  const struct user_regset *regset, unsigned int pos,
 			  unsigned int count, void *kbuf, void __user *ubuf)
 {
-	compat_ulong_t tls = (compat_ulong_t)target->thread.tp_value;
+	compat_ulong_t tls = (compat_ulong_t)target->thread.uw.tp_value;
 	return user_regset_copyout(&pos, &count, &kbuf, &ubuf, &tls, 0, -1);
 }

@@ -1235,13 +1235,13 @@ static int compat_tls_set(struct task_struct *target,
 			  const void __user *ubuf)
 {
 	int ret;
-	compat_ulong_t tls = target->thread.tp_value;
+	compat_ulong_t tls = target->thread.uw.tp_value;
 
 	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &tls, 0, -1);
 	if (ret)
 		return ret;
 
-	target->thread.tp_value = tls;
+	target->thread.uw.tp_value = tls;
 	return ret;
 }

@@ -1538,7 +1538,7 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request,
 			break;
 
 		case COMPAT_PTRACE_GET_THREAD_AREA:
-			ret = put_user((compat_ulong_t)child->thread.tp_value,
+			ret = put_user((compat_ulong_t)child->thread.uw.tp_value,
 				       (compat_ulong_t __user *)datap);
 			break;

arch/arm64/kernel/signal.c +2 -1

index d026615..a0c4138 100644
@@ -178,7 +178,8 @@ static void __user *apply_user_offset(
 
 static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
 {
-	struct user_fpsimd_state const *fpsimd = &current->thread.fpsimd_state;
+	struct user_fpsimd_state const *fpsimd =
+		&current->thread.uw.fpsimd_state;
 	int err;
 
 	/* copy the FP and status/control registers */

arch/arm64/kernel/signal32.c +2 -1

index 4ea38d3..884177a 100644
@@ -148,7 +148,8 @@ union __fpsimd_vreg {
 
 static int compat_preserve_vfp_context(struct compat_vfp_sigframe __user *frame)
 {
-	struct user_fpsimd_state const *fpsimd = &current->thread.fpsimd_state;
+	struct user_fpsimd_state const *fpsimd =
+		&current->thread.uw.fpsimd_state;
 	compat_ulong_t magic = VFP_MAGIC;
 	compat_ulong_t size = VFP_STORAGE_SIZE;
 	compat_ulong_t fpscr, fpexc;

arch/arm64/kernel/sys_compat.c +1 -1

index a382b2a..9155989 100644
@@ -88,7 +88,7 @@ long compat_arm_syscall(struct pt_regs *regs)
 		return do_compat_cache_op(regs->regs[0], regs->regs[1], regs->regs[2]);
 
 	case __ARM_NR_compat_set_tls:
-		current->thread.tp_value = regs->regs[0];
+		current->thread.uw.tp_value = regs->regs[0];
 
 		/*
 		 * Protect against register corruption from context switch.

Signature · 1 line

2.1.4

Kees Cook

2018-03-28 12:00 UTC #3 lore raw

PATCH v2 2/2 arm64: uaccess: Fix omissions from usercopy whitelist

Quoted 27 lines · Dave Martin

When the hardend usercopy support was added for arm64, it was concluded that all cases of usercopy into and out of thread_struct were statically sized and so didn't require explicit whitelisting of the appropriate fields in thread_struct.
Testing with usercopy hardening enabled has revealed that this is not the case for certain ptrace regset manipulation calls on arm64. This occurs because the sizes of usercopies associated with the regset API are dynamic by construction, and because arm64 does not always stage such copies via the stack: indeed the regset API is designed to avoid the need for that by adding some bounds checking.
This is currently believed to affect only the fpsimd and TLS registers.
Because the whitelisted fields in thread_struct must be contiguous, this patch groups them together in a nested struct. It is also necessary to be able to determine the location and size of that struct, so rather than making the struct anonymous (which would save on edits elsewhere) or adding an anonymous union containing named and unnamed instances of the same struct (gross), this patch gives the struct a name and makes the necessary edits to code that references it (noisy but simple).
Care is needed to ensure that the new struct does not contain padding (which the usercopy hardening would fail to protect).

Quoted 7 lines

For this reason, the presence of tp2_value is made unconditional, since a padding field would be needed there in any case. This pads up to the 16-byte alignment required by struct user_fpsimd_state.
Reported-by Mark Rutland <[email protected]>
Fixes 9e8084d3f761 ("arm64: Implement thread_struct whitelist for hardened usercopy")
Signed-off-by Dave Martin <[email protected]>

Acked-by Kees Cook <[email protected]>

Awesome! :)

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

Will Deacon

2018-03-28 12:33 UTC #4 lore raw

PATCH v2 2/2 arm64: uaccess: Fix omissions from usercopy whitelist

In reply to Dave Martin

Quoted 76 lines · Dave Martin

When the hardend usercopy support was added for arm64, it was concluded that all cases of usercopy into and out of thread_struct were statically sized and so didn't require explicit whitelisting of the appropriate fields in thread_struct.
Testing with usercopy hardening enabled has revealed that this is not the case for certain ptrace regset manipulation calls on arm64. This occurs because the sizes of usercopies associated with the regset API are dynamic by construction, and because arm64 does not always stage such copies via the stack: indeed the regset API is designed to avoid the need for that by adding some bounds checking.
This is currently believed to affect only the fpsimd and TLS registers.
Because the whitelisted fields in thread_struct must be contiguous, this patch groups them together in a nested struct. It is also necessary to be able to determine the location and size of that struct, so rather than making the struct anonymous (which would save on edits elsewhere) or adding an anonymous union containing named and unnamed instances of the same struct (gross), this patch gives the struct a name and makes the necessary edits to code that references it (noisy but simple).
Care is needed to ensure that the new struct does not contain padding (which the usercopy hardening would fail to protect).
For this reason, the presence of tp2_value is made unconditional, since a padding field would be needed there in any case. This pads up to the 16-byte alignment required by struct user_fpsimd_state.
Reported-by Mark Rutland <[email protected]>
Fixes 9e8084d3f761 ("arm64: Implement thread_struct whitelist for hardened usercopy")
Signed-off-by Dave Martin <[email protected]>
---
Changes since v1:
* Add a BUILD_BUG_ON() check for padding in the whitelist struct.
* Move to using sizeof_field() for assigning *size; get rid of the dummy pointer that was used previously.
* Delete bogus comment about why no whitelist is (was) needed.
---
 arch/arm64/include/asm/processor.h | 38 +++++++++++++++++++-----------
 arch/arm64/kernel/fpsimd.c         | 47 +++++++++++++++++++-------------------
 arch/arm64/kernel/process.c        |  6 ++---
 arch/arm64/kernel/ptrace.c         | 30 ++++++++++++------------
 arch/arm64/kernel/signal.c         |  3 ++-
 arch/arm64/kernel/signal32.c       |  3 ++-
 arch/arm64/kernel/sys_compat.c     |  2 +-
 7 files changed, 72 insertions(+), 57 deletions(-)
arch/arm64/include/asm/processor.h +3 -5
index 4a04535..224af48 100644
@@ -34,6 +34,8 @@
 
 #ifdef __KERNEL__
 
+#include <linux/build_bug.h>
+#include <linux/stddef.h>
 #include <linux/string.h>
 
 #include <asm/alternative.h>
@@ -102,11 +104,18 @@ struct cpu_context {
 
 struct thread_struct {
 	struct cpu_context	cpu_context;	/* cpu context */
-	unsigned long		tp_value;	/* TLS register */
-#ifdef CONFIG_COMPAT
-	unsigned long		tp2_value;
-#endif
-	struct user_fpsimd_state fpsimd_state;
+

arch/arm64/include/asm/processor.h struct cpu_context { · line 117

Quoted 10 lines

arch/arm64/include/asm/processor.h +10 -0

+	/*
+	 * Whitelisted fields for hardened usercopy:
+	 * Maintainers must ensure manually that this contains no
+	 * implicit padding.
+	 */
+	struct {
+		unsigned long	tp_value;	/* TLS register */
+		unsigned long	tp2_value;
+		struct user_fpsimd_state fpsimd_state;
+	} uw;

I was trying to figure out a way to avoid the '.uw.' everywhere we want to access these members. I tried using an anonymous union, but then I couldn't figure out a way to enforce the BUILD_BUG_ON you have later on in the patch so I guess it's fine as-is.

Thanks!

Will

Dave Martin

author

2018-03-28 14:16 UTC #5 lore raw

PATCH v2 2/2 arm64: uaccess: Fix omissions from usercopy whitelist

Quoted 84 lines · Will Deacon

Quoted 83 lines · Dave Martin
When the hardend usercopy support was added for arm64, it was concluded that all cases of usercopy into and out of thread_struct were statically sized and so didn't require explicit whitelisting of the appropriate fields in thread_struct.
Testing with usercopy hardening enabled has revealed that this is not the case for certain ptrace regset manipulation calls on arm64. This occurs because the sizes of usercopies associated with the regset API are dynamic by construction, and because arm64 does not always stage such copies via the stack: indeed the regset API is designed to avoid the need for that by adding some bounds checking.
This is currently believed to affect only the fpsimd and TLS registers.
Because the whitelisted fields in thread_struct must be contiguous, this patch groups them together in a nested struct. It is also necessary to be able to determine the location and size of that struct, so rather than making the struct anonymous (which would save on edits elsewhere) or adding an anonymous union containing named and unnamed instances of the same struct (gross), this patch gives the struct a name and makes the necessary edits to code that references it (noisy but simple).
Care is needed to ensure that the new struct does not contain padding (which the usercopy hardening would fail to protect).
For this reason, the presence of tp2_value is made unconditional, since a padding field would be needed there in any case. This pads up to the 16-byte alignment required by struct user_fpsimd_state.
Reported-by Mark Rutland <[email protected]>
Fixes 9e8084d3f761 ("arm64: Implement thread_struct whitelist for hardened usercopy")
Signed-off-by Dave Martin <[email protected]>
---
Changes since v1:
* Add a BUILD_BUG_ON() check for padding in the whitelist struct.
* Move to using sizeof_field() for assigning *size; get rid of the dummy pointer that was used previously.
* Delete bogus comment about why no whitelist is (was) needed.
---
 arch/arm64/include/asm/processor.h | 38 +++++++++++++++++++-----------
 arch/arm64/kernel/fpsimd.c         | 47 +++++++++++++++++++-------------------
 arch/arm64/kernel/process.c        |  6 ++---
 arch/arm64/kernel/ptrace.c         | 30 ++++++++++++------------
 arch/arm64/kernel/signal.c         |  3 ++-
 arch/arm64/kernel/signal32.c       |  3 ++-
 arch/arm64/kernel/sys_compat.c     |  2 +-
 7 files changed, 72 insertions(+), 57 deletions(-)
arch/arm64/include/asm/processor.h +10 -5
index 4a04535..224af48 100644
@@ -34,6 +34,8 @@
 
 #ifdef __KERNEL__
 
+#include <linux/build_bug.h>
+#include <linux/stddef.h>
 #include <linux/string.h>
 
 #include <asm/alternative.h>
@@ -102,11 +104,18 @@ struct cpu_context {
 
 struct thread_struct {
 	struct cpu_context	cpu_context;	/* cpu context */
-	unsigned long		tp_value;	/* TLS register */
-#ifdef CONFIG_COMPAT
-	unsigned long		tp2_value;
-#endif
-	struct user_fpsimd_state fpsimd_state;
+
+	/*
+	 * Whitelisted fields for hardened usercopy:
+	 * Maintainers must ensure manually that this contains no
+	 * implicit padding.
+	 */
+	struct {
+		unsigned long	tp_value;	/* TLS register */

Quoted 8 lines

arch/arm64/include/asm/processor.h struct cpu_context { · line 117
Quoted 3 lines
arch/arm64/include/asm/processor.h +3 -0
+		unsigned long	tp2_value;
+		struct user_fpsimd_state fpsimd_state;
+	} uw;
I was trying to figure out a way to avoid the '.uw.' everywhere we want to access these members. I tried using an anonymous union, but then I couldn't figure out a way to enforce the BUILD_BUG_ON you have later on in the patch so I guess it's fine as-is.

Agreed, I would have preferred to avoid the explcitly named nested struct, but I couldn't find a good way to do it.

We can revisit this later if someone comes up with a bright idea.

Cheers
---Dave