[PATCH 0/9] mm: Hardened usercopy

[PATCH 0/9] mm: Hardened usercopy

[Kees Cook]

Hi,

This is a start of the mainline port of PAX_USERCOPY[1]. After I started
writing tests (now in lkdtm in -next) for Casey's earlier port[2], I
kept tweaking things further and further until I ended up with a whole
new patch series. To that end, I took Rik's feedback and made a number
of other changes and clean-ups as well.

Based on my understanding, PAX_USERCOPY was designed to catch a few
classes of flaws around the use of copy_to_user()/copy_from_user(). These
changes don't touch get_user() and put_user(), since these operate on
constant sized lengths, and tend to be much less vulnerable. There
are effectively three distinct protections in the whole series,
each of which I've given a separate CONFIG, though this patch set is
only the first of the three intended protections. (Generally speaking,
PAX_USERCOPY covers what I'm calling CONFIG_HARDENED_USERCOPY (this) and
CONFIG_HARDENED_USERCOPY_WHITELIST (future), and PAX_USERCOPY_SLABS covers
CONFIG_HARDENED_USERCOPY_SPLIT_KMALLOC (future).)

This series, which adds CONFIG_HARDENED_USERCOPY, checks that objects
being copied to/from userspace meet certain criteria:
- if address is a heap object, the size must not exceed the object's
  allocated size. (This will catch all kinds of heap overflow flaws.)
- if address range is in the current process stack, it must be within the
  current stack frame (if such checking is possible) or at least entirely
  within the current process's stack. (This could catch large lengths that
  would have extended beyond the current process stack, or overflows if
  their length extends back into the original stack.)
- if the address range is part of kernel data, rodata, or bss, allow it.
- if address range is page-allocated, that it doesn't span multiple
  allocations.
- if address is within the kernel text, reject it.
- everything else is accepted

The patches in the series are:
- The core copy_to/from_user() checks, without the slab object checks:
	1- mm: Hardened usercopy
- Per-arch enablement of the protection:
	2- x86/uaccess: Enable hardened usercopy
	3- ARM: uaccess: Enable hardened usercopy
	4- arm64/uaccess: Enable hardened usercopy
	5- ia64/uaccess: Enable hardened usercopy
	6- powerpc/uaccess: Enable hardened usercopy
	7- sparc/uaccess: Enable hardened usercopy
- The heap allocator implementation of object size checking:
	8- mm: SLAB hardened usercopy support
	9- mm: SLUB hardened usercopy support

Some notes:

- This is expected to apply on top of -next which contains fixes for the
  position of _etext on both arm and arm64.

- I couldn't detect a measurable performance change with these features
  enabled. Kernel build times were unchanged, hackbench was unchanged,
  etc. I think we could flip this to "on by default" at some point.

- The SLOB support extracted from grsecurity seems entirely broken. I
  have no idea what's going on there, I spent my time testing SLAB and
  SLUB. Having someone else look at SLOB would be nice, but this series
  doesn't depend on it.

Additional features that would be nice, but aren't blocking this series:

- Needs more architecture support for stack frame checking (only x86 now).


Thanks!

-Kees

[1] https://grsecurity.net/download.php "grsecurity - test kernel patch"
[2] http://www.openwall.com/lists/kernel-hardening/2016/05/19/5

[PATCH 1/9] mm: Hardened usercopy

[Kees Cook]

This is the start of porting PAX_USERCOPY into the mainline kernel. This
is the first set of features, controlled by CONFIG_HARDENED_USERCOPY. The
work is based on code by PaX Team and Brad Spengler, and an earlier port
from Casey Schaufler. Additional non-slab page tests are from Rik van Riel.

This patch contains the logic for validating several conditions when
performing copy_to_user() and copy_from_user() on the kernel object
being copied to/from:
- address range doesn't wrap around
- address range isn't NULL or zero-allocated (with a non-zero copy size)
- if on the slab allocator:
  - object size must be less than or equal to copy size (when check is
    implemented in the allocator, which appear in subsequent patches)
- otherwise, object must not span page allocations
- if on the stack
  - object must not extend before/after the current process task
  - object must be contained by the current stack frame (when there is
    arch/build support for identifying stack frames)
- object must not overlap with kernel text

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/Kconfig                |   7 ++
 include/linux/slab.h        |  12 +++
 include/linux/thread_info.h |  15 +++
 mm/Makefile                 |   4 +
 mm/usercopy.c               | 239 ++++++++++++++++++++++++++++++++++++++++++++
 security/Kconfig            |  27 +++++
 6 files changed, 304 insertions(+)
 create mode 100644 mm/usercopy.c

diff --git a/arch/Kconfig b/arch/Kconfig
index d794384a0404..3ea04d8dcf62 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -424,6 +424,13 @@ config CC_STACKPROTECTOR_STRONG
 
 endchoice
 
+config HAVE_ARCH_LINEAR_KERNEL_MAPPING
+	bool
+	help
+	  An architecture should select this if it has a secondary linear
+	  mapping of the kernel text. This is used to verify that kernel
+	  text exposures are not visible under CONFIG_HARDENED_USERCOPY.
+
 config HAVE_CONTEXT_TRACKING
 	bool
 	help
diff --git a/include/linux/slab.h b/include/linux/slab.h
index aeb3e6d00a66..96a16a3fb7cb 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -155,6 +155,18 @@ void kfree(const void *);
 void kzfree(const void *);
 size_t ksize(const void *);
 
+#ifdef CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR
+const char *__check_heap_object(const void *ptr, unsigned long n,
+				struct page *page);
+#else
+static inline const char *__check_heap_object(const void *ptr,
+					      unsigned long n,
+					      struct page *page)
+{
+	return NULL;
+}
+#endif
+
 /*
  * Some archs want to perform DMA into kmalloc caches and need a guaranteed
  * alignment larger than the alignment of a 64-bit integer.
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index b4c2a485b28a..a02200db9c33 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -146,6 +146,21 @@ static inline bool test_and_clear_restore_sigmask(void)
 #error "no set_restore_sigmask() provided and default one won't work"
 #endif
 
+#ifdef CONFIG_HARDENED_USERCOPY
+extern void __check_object_size(const void *ptr, unsigned long n,
+					bool to_user);
+
+static inline void check_object_size(const void *ptr, unsigned long n,
+				     bool to_user)
+{
+	__check_object_size(ptr, n, to_user);
+}
+#else
+static inline void check_object_size(const void *ptr, unsigned long n,
+				     bool to_user)
+{ }
+#endif /* CONFIG_HARDENED_USERCOPY */
+
 #endif	/* __KERNEL__ */
 
 #endif /* _LINUX_THREAD_INFO_H */
diff --git a/mm/Makefile b/mm/Makefile
index 78c6f7dedb83..32d37247c7e5 100644
--- a/mm/Makefile
+++ b/mm/Makefile
@@ -21,6 +21,9 @@ KCOV_INSTRUMENT_memcontrol.o := n
 KCOV_INSTRUMENT_mmzone.o := n
 KCOV_INSTRUMENT_vmstat.o := n
 
+# Since __builtin_frame_address does work as used, disable the warning.
+CFLAGS_usercopy.o += $(call cc-disable-warning, frame-address)
+
 mmu-y			:= nommu.o
 mmu-$(CONFIG_MMU)	:= gup.o highmem.o memory.o mincore.o \
 			   mlock.o mmap.o mprotect.o mremap.o msync.o rmap.o \
@@ -99,3 +102,4 @@ obj-$(CONFIG_USERFAULTFD) += userfaultfd.o
 obj-$(CONFIG_IDLE_PAGE_TRACKING) += page_idle.o
 obj-$(CONFIG_FRAME_VECTOR) += frame_vector.o
 obj-$(CONFIG_DEBUG_PAGE_REF) += debug_page_ref.o
+obj-$(CONFIG_HARDENED_USERCOPY) += usercopy.o
diff --git a/mm/usercopy.c b/mm/usercopy.c
new file mode 100644
index 000000000000..ad2765dd6dc4
--- /dev/null
+++ b/mm/usercopy.c
@@ -0,0 +1,239 @@
+/*
+ * This implements the various checks for CONFIG_HARDENED_USERCOPY*,
+ * which are designed to protect kernel memory from needless exposure
+ * and overwrite under many unintended conditions. This code is based
+ * on PAX_USERCOPY, which is:
+ *
+ * Copyright (C) 2001-2016 PaX Team, Bradley Spengler, Open Source
+ * Security Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/mm.h>
+#include <linux/slab.h>
+#include <asm/sections.h>
+
+/*
+ * Checks if a given pointer and length is contained by the current
+ * stack frame (if possible).
+ *
+ *	0: not at all on the stack
+ *	1: fully on the stack (when can't do frame-checking)
+ *	2: fully inside the current stack frame
+ *	-1: error condition (invalid stack position or bad stack frame)
+ */
+static noinline int check_stack_object(const void *obj, unsigned long len)
+{
+	const void * const stack = task_stack_page(current);
+	const void * const stackend = stack + THREAD_SIZE;
+
+#if defined(CONFIG_FRAME_POINTER) && defined(CONFIG_X86)
+	const void *frame = NULL;
+	const void *oldframe;
+#endif
+
+	/* Object is not on the stack at all. */
+	if (obj + len <= stack || stackend <= obj)
+		return 0;
+
+	/*
+	 * Reject: object partially overlaps the stack (passing the
+	 * the check above means@least one end is within the stack,
+	 * so if this check fails, the other end is outside the stack).
+	 */
+	if (obj < stack || stackend < obj + len)
+		return -1;
+
+#if defined(CONFIG_FRAME_POINTER) && defined(CONFIG_X86)
+	oldframe = __builtin_frame_address(1);
+	if (oldframe)
+		frame = __builtin_frame_address(2);
+	/*
+	 * low ----------------------------------------------> high
+	 * [saved bp][saved ip][args][local vars][saved bp][saved ip]
+	 *		     ^----------------^
+	 *             allow copies only within here
+	 */
+	while (stack <= frame && frame < stackend) {
+		/*
+		 * If obj + len extends past the last frame, this
+		 * check won't pass and the next frame will be 0,
+		 * causing us to bail out and correctly report
+		 * the copy as invalid.
+		 */
+		if (obj + len <= frame)
+			return obj >= oldframe + 2 * sizeof(void *) ? 2 : -1;
+		oldframe = frame;
+		frame = *(const void * const *)frame;
+	}
+	return -1;
+#else
+	return 1;
+#endif
+}
+
+static void report_usercopy(const void *ptr, unsigned long len,
+			    bool to_user, const char *type)
+{
+	pr_emerg("kernel memory %s attempt detected %s %p (%s) (%lu bytes)\n",
+		to_user ? "exposure" : "overwrite",
+		to_user ? "from" : "to", ptr, type ? : "unknown", len);
+	dump_stack();
+	do_group_exit(SIGKILL);
+}
+
+/* Returns true if any portion of [ptr,ptr+n) over laps with [low,high). */
+static bool overlaps(const void *ptr, unsigned long n, unsigned long low,
+		     unsigned long high)
+{
+	unsigned long check_low = (uintptr_t)ptr;
+	unsigned long check_high = check_low + n;
+
+	/* Does not overlap if entirely above or entirely below. */
+	if (check_low >= high || check_high < low)
+		return false;
+
+	return true;
+}
+
+/* Is this address range in the kernel text area? */
+static inline const char *check_kernel_text_object(const void *ptr,
+						   unsigned long n)
+{
+	unsigned long textlow = (unsigned long)_stext;
+	unsigned long texthigh = (unsigned long)_etext;
+
+	if (overlaps(ptr, n, textlow, texthigh))
+		return "<kernel text>";
+
+#ifdef HAVE_ARCH_LINEAR_KERNEL_MAPPING
+	/* Check against linear mapping as well. */
+	if (overlaps(ptr, n, (unsigned long)__va(__pa(textlow)),
+		     (unsigned long)__va(__pa(texthigh))))
+		return "<linear kernel text>";
+#endif
+
+	return NULL;
+}
+
+static inline const char *check_bogus_address(const void *ptr, unsigned long n)
+{
+	/* Reject if object wraps past end of memory. */
+	if (ptr + n < ptr)
+		return "<wrapped address>";
+
+	/* Reject if NULL or ZERO-allocation. */
+	if (ZERO_OR_NULL_PTR(ptr))
+		return "<null>";
+
+	return NULL;
+}
+
+static inline const char *check_heap_object(const void *ptr, unsigned long n)
+{
+	struct page *page, *endpage;
+	const void *end = ptr + n - 1;
+
+	if (!virt_addr_valid(ptr))
+		return NULL;
+
+	page = virt_to_head_page(ptr);
+
+	/* Check slab allocator for flags and size. */
+	if (PageSlab(page))
+		return __check_heap_object(ptr, n, page);
+
+	/* Is the object wholly within one base page? */
+	if (likely(((unsigned long)ptr & (unsigned long)PAGE_MASK) ==
+		   ((unsigned long)end & (unsigned long)PAGE_MASK)))
+		return NULL;
+
+	/* Allow if start and end are inside the same compound page. */
+	endpage = virt_to_head_page(end);
+	if (likely(endpage == page))
+		return NULL;
+
+	/* Allow special areas, device memory, and sometimes kernel data. */
+	if (PageReserved(page) && PageReserved(endpage))
+		return NULL;
+
+	/*
+	 * Sometimes the kernel data regions are not marked Reserved. And
+	 * sometimes [_sdata,_edata) does not cover rodata and/or bss,
+	 * so check each range explicitly.
+	 */
+
+	/* Allow kernel data region (if not marked as Reserved). */
+	if (ptr >= (const void *)_sdata && end <= (const void *)_edata)
+		return NULL;
+
+	/* Allow kernel rodata region (if not marked as Reserved). */
+	if (ptr >= (const void *)__start_rodata &&
+	    end <= (const void *)__end_rodata)
+		return NULL;
+
+	/* Allow kernel bss region (if not marked as Reserved). */
+	if (ptr >= (const void *)__bss_start &&
+	    end <= (const void *)__bss_stop)
+		return NULL;
+
+	/* Uh oh. The "object" spans several independently allocated pages. */
+	return "<spans multiple pages>";
+}
+
+/*
+ * Validates that the given object is one of:
+ * - known safe heap object
+ * - known safe stack object
+ * - not in kernel text
+ */
+void __check_object_size(const void *ptr, unsigned long n, bool to_user)
+{
+	const char *err;
+
+	/* Skip all tests if size is zero. */
+	if (!n)
+		return;
+
+	/* Check for invalid addresses. */
+	err = check_bogus_address(ptr, n);
+	if (err)
+		goto report;
+
+	/* Check for bad heap object. */
+	err = check_heap_object(ptr, n);
+	if (err)
+		goto report;
+
+	/* Check for bad stack object. */
+	switch (check_stack_object(ptr, n)) {
+	case 0:
+		/* Object is not touching the current process stack. */
+		break;
+	case 1:
+	case 2:
+		/*
+		 * Object is either in the correct frame (when it
+		 * is possible to check) or just generally on the
+		 * process stack (when frame checking not available).
+		 */
+		return;
+	default:
+		err = "<process stack>";
+		goto report;
+	}
+
+	/* Check for object in kernel to avoid text exposure. */
+	err = check_kernel_text_object(ptr, n);
+	if (!err)
+		return;
+
+report:
+	report_usercopy(ptr, n, to_user, err);
+}
+EXPORT_SYMBOL(__check_object_size);
diff --git a/security/Kconfig b/security/Kconfig
index 176758cdfa57..63340ad0b9f9 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -118,6 +118,33 @@ config LSM_MMAP_MIN_ADDR
 	  this low address space will need the permission specific to the
 	  systems running LSM.
 
+config HAVE_HARDENED_USERCOPY_ALLOCATOR
+	bool
+	help
+	  The heap allocator implements __check_heap_object() for
+	  validating memory ranges against heap object sizes in
+	  support of CONFIG_HARDENED_USERCOPY.
+
+config HAVE_ARCH_HARDENED_USERCOPY
+	bool
+	help
+	  The architecture supports CONFIG_HARDENED_USERCOPY by
+	  calling check_object_size() just before performing the
+	  userspace copies in the low level implementation of
+	  copy_to_user() and copy_from_user().
+
+config HARDENED_USERCOPY
+	bool "Harden memory copies between kernel and userspace"
+	depends on HAVE_ARCH_HARDENED_USERCOPY
+	help
+	  This option checks for obviously wrong memory regions when
+	  copying memory to/from the kernel (via copy_to_user() and
+	  copy_from_user() functions) by rejecting memory ranges that
+	  are larger than the specified heap object, span multiple
+	  separately allocates pages, are not on the process stack,
+	  or are part of the kernel text. This kills entire classes
+	  of heap overflow exploits and similar kernel memory exposures.
+
 source security/selinux/Kconfig
 source security/smack/Kconfig
 source security/tomoyo/Kconfig
-- 
2.7.4

[PATCH 2/9] x86/uaccess: Enable hardened usercopy

[Kees Cook]

Enables CONFIG_HARDENED_USERCOPY checks on x86. This is done both in
copy_*_user() and __copy_*_user() because copy_*_user() actually calls
down to _copy_*_user() and not __copy_*_user().

Based on code from PaX and grsecurity.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/Kconfig                  |  2 ++
 arch/x86/include/asm/uaccess.h    | 10 ++++++----
 arch/x86/include/asm/uaccess_32.h |  2 ++
 arch/x86/include/asm/uaccess_64.h |  2 ++
 4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 0a7b885964ba..2a66b73a996d 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -80,11 +80,13 @@ config X86
 	select HAVE_ALIGNED_STRUCT_PAGE		if SLUB
 	select HAVE_AOUT			if X86_32
 	select HAVE_ARCH_AUDITSYSCALL
+	select HAVE_ARCH_HARDENED_USERCOPY
 	select HAVE_ARCH_HUGE_VMAP		if X86_64 || X86_PAE
 	select HAVE_ARCH_JUMP_LABEL
 	select HAVE_ARCH_KASAN			if X86_64 && SPARSEMEM_VMEMMAP
 	select HAVE_ARCH_KGDB
 	select HAVE_ARCH_KMEMCHECK
+	select HAVE_ARCH_LINEAR_KERNEL_MAPPING	if X86_64
 	select HAVE_ARCH_MMAP_RND_BITS		if MMU
 	select HAVE_ARCH_MMAP_RND_COMPAT_BITS	if MMU && COMPAT
 	select HAVE_ARCH_SECCOMP_FILTER
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 2982387ba817..aa9cc58409c6 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -742,9 +742,10 @@ copy_from_user(void *to, const void __user *from, unsigned long n)
 	 * case, and do only runtime checking for non-constant sizes.
 	 */
 
-	if (likely(sz < 0 || sz >= n))
+	if (likely(sz < 0 || sz >= n)) {
+		check_object_size(to, n, false);
 		n = _copy_from_user(to, from, n);
-	else if(__builtin_constant_p(n))
+	} else if(__builtin_constant_p(n))
 		copy_from_user_overflow();
 	else
 		__copy_from_user_overflow(sz, n);
@@ -762,9 +763,10 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
 	might_fault();
 
 	/* See the comment in copy_from_user() above. */
-	if (likely(sz < 0 || sz >= n))
+	if (likely(sz < 0 || sz >= n)) {
+		check_object_size(from, n, true);
 		n = _copy_to_user(to, from, n);
-	else if(__builtin_constant_p(n))
+	} else if(__builtin_constant_p(n))
 		copy_to_user_overflow();
 	else
 		__copy_to_user_overflow(sz, n);
diff --git a/arch/x86/include/asm/uaccess_32.h b/arch/x86/include/asm/uaccess_32.h
index 4b32da24faaf..7d3bdd1ed697 100644
--- a/arch/x86/include/asm/uaccess_32.h
+++ b/arch/x86/include/asm/uaccess_32.h
@@ -37,6 +37,7 @@ unsigned long __must_check __copy_from_user_ll_nocache_nozero
 static __always_inline unsigned long __must_check
 __copy_to_user_inatomic(void __user *to, const void *from, unsigned long n)
 {
+	check_object_size(from, n, true);
 	return __copy_to_user_ll(to, from, n);
 }
 
@@ -95,6 +96,7 @@ static __always_inline unsigned long
 __copy_from_user(void *to, const void __user *from, unsigned long n)
 {
 	might_fault();
+	check_object_size(to, n, false);
 	if (__builtin_constant_p(n)) {
 		unsigned long ret;
 
diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index 2eac2aa3e37f..673059a109fe 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -54,6 +54,7 @@ int __copy_from_user_nocheck(void *dst, const void __user *src, unsigned size)
 {
 	int ret = 0;
 
+	check_object_size(dst, size, false);
 	if (!__builtin_constant_p(size))
 		return copy_user_generic(dst, (__force void *)src, size);
 	switch (size) {
@@ -119,6 +120,7 @@ int __copy_to_user_nocheck(void __user *dst, const void *src, unsigned size)
 {
 	int ret = 0;
 
+	check_object_size(src, size, true);
 	if (!__builtin_constant_p(size))
 		return copy_user_generic((__force void *)dst, src, size);
 	switch (size) {
-- 
2.7.4

[PATCH 3/9] ARM: uaccess: Enable hardened usercopy

[Kees Cook]

Enables CONFIG_HARDENED_USERCOPY checks on arm.

Based on code from PaX and grsecurity.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arm/Kconfig               |  1 +
 arch/arm/include/asm/uaccess.h | 11 +++++++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 90542db1220d..f56b29b3f57e 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -35,6 +35,7 @@ config ARM
 	select HARDIRQS_SW_RESEND
 	select HAVE_ARCH_AUDITSYSCALL if (AEABI && !OABI_COMPAT)
 	select HAVE_ARCH_BITREVERSE if (CPU_32v7M || CPU_32v7) && !CPU_32v6
+	select HAVE_ARCH_HARDENED_USERCOPY
 	select HAVE_ARCH_JUMP_LABEL if !XIP_KERNEL && !CPU_ENDIAN_BE32 && MMU
 	select HAVE_ARCH_KGDB if !CPU_ENDIAN_BE32 && MMU
 	select HAVE_ARCH_MMAP_RND_BITS if MMU
diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index 35c9db857ebe..7fb59199c6bb 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -496,7 +496,10 @@ arm_copy_from_user(void *to, const void __user *from, unsigned long n);
 static inline unsigned long __must_check
 __copy_from_user(void *to, const void __user *from, unsigned long n)
 {
-	unsigned int __ua_flags = uaccess_save_and_enable();
+	unsigned int __ua_flags;
+
+	check_object_size(to, n, false);
+	__ua_flags = uaccess_save_and_enable();
 	n = arm_copy_from_user(to, from, n);
 	uaccess_restore(__ua_flags);
 	return n;
@@ -511,11 +514,15 @@ static inline unsigned long __must_check
 __copy_to_user(void __user *to, const void *from, unsigned long n)
 {
 #ifndef CONFIG_UACCESS_WITH_MEMCPY
-	unsigned int __ua_flags = uaccess_save_and_enable();
+	unsigned int __ua_flags;
+
+	check_object_size(from, n, true);
+	__ua_flags = uaccess_save_and_enable();
 	n = arm_copy_to_user(to, from, n);
 	uaccess_restore(__ua_flags);
 	return n;
 #else
+	check_object_size(from, n, true);
 	return arm_copy_to_user(to, from, n);
 #endif
 }
-- 
2.7.4

[PATCH 4/9] arm64/uaccess: Enable hardened usercopy

[Kees Cook]

Enables CONFIG_HARDENED_USERCOPY checks on arm64. As done by KASAN in -next,
renames the low-level functions to __arch_copy_*_user() so a static inline
can do additional work before the copy.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arm64/Kconfig               |  2 ++
 arch/arm64/include/asm/uaccess.h | 18 ++++++++++++++++--
 arch/arm64/kernel/arm64ksyms.c   |  4 ++--
 arch/arm64/lib/copy_from_user.S  |  4 ++--
 arch/arm64/lib/copy_to_user.S    |  4 ++--
 5 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 5a0a691d4220..b771cd97f74b 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -51,10 +51,12 @@ config ARM64
 	select HAVE_ALIGNED_STRUCT_PAGE if SLUB
 	select HAVE_ARCH_AUDITSYSCALL
 	select HAVE_ARCH_BITREVERSE
+	select HAVE_ARCH_HARDENED_USERCOPY
 	select HAVE_ARCH_HUGE_VMAP
 	select HAVE_ARCH_JUMP_LABEL
 	select HAVE_ARCH_KASAN if SPARSEMEM_VMEMMAP && !(ARM64_16K_PAGES && ARM64_VA_BITS_48)
 	select HAVE_ARCH_KGDB
+	select HAVE_ARCH_LINEAR_KERNEL_MAPPING
 	select HAVE_ARCH_MMAP_RND_BITS
 	select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
 	select HAVE_ARCH_SECCOMP_FILTER
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 9e397a542756..6d0f86300936 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -256,11 +256,25 @@ do {									\
 		-EFAULT;						\
 })
 
-extern unsigned long __must_check __copy_from_user(void *to, const void __user *from, unsigned long n);
-extern unsigned long __must_check __copy_to_user(void __user *to, const void *from, unsigned long n);
+extern unsigned long __must_check __arch_copy_from_user(void *to, const void __user *from, unsigned long n);
+extern unsigned long __must_check __arch_copy_to_user(void __user *to, const void *from, unsigned long n);
 extern unsigned long __must_check __copy_in_user(void __user *to, const void __user *from, unsigned long n);
 extern unsigned long __must_check __clear_user(void __user *addr, unsigned long n);
 
+static inline unsigned long __must_check
+__copy_from_user(void *to, const void __user *from, unsigned long n)
+{
+	check_object_size(to, n, false);
+	return __arch_copy_from_user(to, from, n);
+}
+
+static inline unsigned long __must_check
+__copy_to_user(void __user *to, const void *from, unsigned long n)
+{
+	check_object_size(from, n, true);
+	return __arch_copy_to_user(to, from, n);
+}
+
 static inline unsigned long __must_check copy_from_user(void *to, const void __user *from, unsigned long n)
 {
 	if (access_ok(VERIFY_READ, from, n))
diff --git a/arch/arm64/kernel/arm64ksyms.c b/arch/arm64/kernel/arm64ksyms.c
index 678f30b05a45..2dc44406a7ad 100644
--- a/arch/arm64/kernel/arm64ksyms.c
+++ b/arch/arm64/kernel/arm64ksyms.c
@@ -34,8 +34,8 @@ EXPORT_SYMBOL(copy_page);
 EXPORT_SYMBOL(clear_page);
 
 	/* user mem (segment) */
-EXPORT_SYMBOL(__copy_from_user);
-EXPORT_SYMBOL(__copy_to_user);
+EXPORT_SYMBOL(__arch_copy_from_user);
+EXPORT_SYMBOL(__arch_copy_to_user);
 EXPORT_SYMBOL(__clear_user);
 EXPORT_SYMBOL(__copy_in_user);
 
diff --git a/arch/arm64/lib/copy_from_user.S b/arch/arm64/lib/copy_from_user.S
index 17e8306dca29..0b90497d4424 100644
--- a/arch/arm64/lib/copy_from_user.S
+++ b/arch/arm64/lib/copy_from_user.S
@@ -66,7 +66,7 @@
 	.endm
 
 end	.req	x5
-ENTRY(__copy_from_user)
+ENTRY(__arch_copy_from_user)
 ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_ALT_PAN_NOT_UAO, \
 	    CONFIG_ARM64_PAN)
 	add	end, x0, x2
@@ -75,7 +75,7 @@ ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(1)), ARM64_ALT_PAN_NOT_UAO, \
 	    CONFIG_ARM64_PAN)
 	mov	x0, #0				// Nothing to copy
 	ret
-ENDPROC(__copy_from_user)
+ENDPROC(__arch_copy_from_user)
 
 	.section .fixup,"ax"
 	.align	2
diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
index 21faae60f988..7a7efe255034 100644
--- a/arch/arm64/lib/copy_to_user.S
+++ b/arch/arm64/lib/copy_to_user.S
@@ -65,7 +65,7 @@
 	.endm
 
 end	.req	x5
-ENTRY(__copy_to_user)
+ENTRY(__arch_copy_to_user)
 ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_ALT_PAN_NOT_UAO, \
 	    CONFIG_ARM64_PAN)
 	add	end, x0, x2
@@ -74,7 +74,7 @@ ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(1)), ARM64_ALT_PAN_NOT_UAO, \
 	    CONFIG_ARM64_PAN)
 	mov	x0, #0
 	ret
-ENDPROC(__copy_to_user)
+ENDPROC(__arch_copy_to_user)
 
 	.section .fixup,"ax"
 	.align	2
-- 
2.7.4

[PATCH 5/9] ia64/uaccess: Enable hardened usercopy

[Kees Cook]

Enables CONFIG_HARDENED_USERCOPY checks on ia64.

Based on code from PaX and grsecurity.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/ia64/Kconfig               |  1 +
 arch/ia64/include/asm/uaccess.h | 18 +++++++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index f80758cb7157..32a87ef516a0 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -53,6 +53,7 @@ config IA64
 	select MODULES_USE_ELF_RELA
 	select ARCH_USE_CMPXCHG_LOCKREF
 	select HAVE_ARCH_AUDITSYSCALL
+	select HAVE_ARCH_HARDENED_USERCOPY
 	default y
 	help
 	  The Itanium Processor Family is Intel's 64-bit successor to
diff --git a/arch/ia64/include/asm/uaccess.h b/arch/ia64/include/asm/uaccess.h
index 2189d5ddc1ee..465c70982f40 100644
--- a/arch/ia64/include/asm/uaccess.h
+++ b/arch/ia64/include/asm/uaccess.h
@@ -241,12 +241,18 @@ extern unsigned long __must_check __copy_user (void __user *to, const void __use
 static inline unsigned long
 __copy_to_user (void __user *to, const void *from, unsigned long count)
 {
+	if (!__builtin_constant_p(count))
+		check_object_size(from, count, true);
+
 	return __copy_user(to, (__force void __user *) from, count);
 }
 
 static inline unsigned long
 __copy_from_user (void *to, const void __user *from, unsigned long count)
 {
+	if (!__builtin_constant_p(count))
+		check_object_size(to, count, false);
+
 	return __copy_user((__force void __user *) to, from, count);
 }
 
@@ -258,8 +264,11 @@ __copy_from_user (void *to, const void __user *from, unsigned long count)
 	const void *__cu_from = (from);							\
 	long __cu_len = (n);								\
 											\
-	if (__access_ok(__cu_to, __cu_len, get_fs()))					\
-		__cu_len = __copy_user(__cu_to, (__force void __user *) __cu_from, __cu_len);	\
+	if (__access_ok(__cu_to, __cu_len, get_fs())) {					\
+		if (!__builtin_constant_p(n))						\
+			check_object_size(__cu_from, __cu_len, true);			\
+		__cu_len = __copy_user(__cu_to, (__force void __user *)  __cu_from, __cu_len);	\
+	}										\
 	__cu_len;									\
 })
 
@@ -270,8 +279,11 @@ __copy_from_user (void *to, const void __user *from, unsigned long count)
 	long __cu_len = (n);								\
 											\
 	__chk_user_ptr(__cu_from);							\
-	if (__access_ok(__cu_from, __cu_len, get_fs()))					\
+	if (__access_ok(__cu_from, __cu_len, get_fs())) {				\
+		if (!__builtin_constant_p(n))						\
+			check_object_size(__cu_to, __cu_len, false);			\
 		__cu_len = __copy_user((__force void __user *) __cu_to, __cu_from, __cu_len);	\
+	}										\
 	__cu_len;									\
 })
 
-- 
2.7.4

[PATCH 6/9] powerpc/uaccess: Enable hardened usercopy

[Kees Cook]

Enables CONFIG_HARDENED_USERCOPY checks on powerpc.

Based on code from PaX and grsecurity.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/powerpc/Kconfig               |  1 +
 arch/powerpc/include/asm/uaccess.h | 21 +++++++++++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 01f7464d9fea..b7a18b2604be 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -164,6 +164,7 @@ config PPC
 	select ARCH_HAS_UBSAN_SANITIZE_ALL
 	select ARCH_SUPPORTS_DEFERRED_STRUCT_PAGE_INIT
 	select HAVE_LIVEPATCH if HAVE_DYNAMIC_FTRACE_WITH_REGS
+	select HAVE_ARCH_HARDENED_USERCOPY
 
 config GENERIC_CSUM
 	def_bool CPU_LITTLE_ENDIAN
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index b7c20f0b8fbe..c1dc6c14deb8 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -310,10 +310,15 @@ static inline unsigned long copy_from_user(void *to,
 {
 	unsigned long over;
 
-	if (access_ok(VERIFY_READ, from, n))
+	if (access_ok(VERIFY_READ, from, n)) {
+		if (!__builtin_constant_p(n))
+			check_object_size(to, n, false);
 		return __copy_tofrom_user((__force void __user *)to, from, n);
+	}
 	if ((unsigned long)from < TASK_SIZE) {
 		over = (unsigned long)from + n - TASK_SIZE;
+		if (!__builtin_constant_p(n - over))
+			check_object_size(to, n - over, false);
 		return __copy_tofrom_user((__force void __user *)to, from,
 				n - over) + over;
 	}
@@ -325,10 +330,15 @@ static inline unsigned long copy_to_user(void __user *to,
 {
 	unsigned long over;
 
-	if (access_ok(VERIFY_WRITE, to, n))
+	if (access_ok(VERIFY_WRITE, to, n)) {
+		if (!__builtin_constant_p(n))
+			check_object_size(from, n, true);
 		return __copy_tofrom_user(to, (__force void __user *)from, n);
+	}
 	if ((unsigned long)to < TASK_SIZE) {
 		over = (unsigned long)to + n - TASK_SIZE;
+		if (!__builtin_constant_p(n))
+			check_object_size(from, n - over, true);
 		return __copy_tofrom_user(to, (__force void __user *)from,
 				n - over) + over;
 	}
@@ -372,6 +382,10 @@ static inline unsigned long __copy_from_user_inatomic(void *to,
 		if (ret == 0)
 			return 0;
 	}
+
+	if (!__builtin_constant_p(n))
+		check_object_size(to, n, false);
+
 	return __copy_tofrom_user((__force void __user *)to, from, n);
 }
 
@@ -398,6 +412,9 @@ static inline unsigned long __copy_to_user_inatomic(void __user *to,
 		if (ret == 0)
 			return 0;
 	}
+	if (!__builtin_constant_p(n))
+		check_object_size(from, n, true);
+
 	return __copy_tofrom_user(to, (__force const void __user *)from, n);
 }
 
-- 
2.7.4

[PATCH 7/9] sparc/uaccess: Enable hardened usercopy

[Kees Cook]

Enables CONFIG_HARDENED_USERCOPY checks on sparc.

Based on code from PaX and grsecurity.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/sparc/Kconfig                  |  1 +
 arch/sparc/include/asm/uaccess_32.h | 14 ++++++++++----
 arch/sparc/include/asm/uaccess_64.h | 11 +++++++++--
 3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index 546293d9e6c5..59b09600dd32 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -43,6 +43,7 @@ config SPARC
 	select OLD_SIGSUSPEND
 	select ARCH_HAS_SG_CHAIN
 	select CPU_NO_EFFICIENT_FFS
+	select HAVE_ARCH_HARDENED_USERCOPY
 
 config SPARC32
 	def_bool !64BIT
diff --git a/arch/sparc/include/asm/uaccess_32.h b/arch/sparc/include/asm/uaccess_32.h
index 57aca2792d29..341a5a133f48 100644
--- a/arch/sparc/include/asm/uaccess_32.h
+++ b/arch/sparc/include/asm/uaccess_32.h
@@ -248,22 +248,28 @@ unsigned long __copy_user(void __user *to, const void __user *from, unsigned lon
 
 static inline unsigned long copy_to_user(void __user *to, const void *from, unsigned long n)
 {
-	if (n && __access_ok((unsigned long) to, n))
+	if (n && __access_ok((unsigned long) to, n)) {
+		if (!__builtin_constant_p(n))
+			check_object_size(from, n, true);
 		return __copy_user(to, (__force void __user *) from, n);
-	else
+	} else
 		return n;
 }
 
 static inline unsigned long __copy_to_user(void __user *to, const void *from, unsigned long n)
 {
+	if (!__builtin_constant_p(n))
+		check_object_size(from, n, true);
 	return __copy_user(to, (__force void __user *) from, n);
 }
 
 static inline unsigned long copy_from_user(void *to, const void __user *from, unsigned long n)
 {
-	if (n && __access_ok((unsigned long) from, n))
+	if (n && __access_ok((unsigned long) from, n)) {
+		if (!__builtin_constant_p(n))
+			check_object_size(to, n, false);
 		return __copy_user((__force void __user *) to, from, n);
-	else
+	} else
 		return n;
 }
 
diff --git a/arch/sparc/include/asm/uaccess_64.h b/arch/sparc/include/asm/uaccess_64.h
index e9a51d64974d..8bda94fab8e8 100644
--- a/arch/sparc/include/asm/uaccess_64.h
+++ b/arch/sparc/include/asm/uaccess_64.h
@@ -210,8 +210,12 @@ unsigned long copy_from_user_fixup(void *to, const void __user *from,
 static inline unsigned long __must_check
 copy_from_user(void *to, const void __user *from, unsigned long size)
 {
-	unsigned long ret = ___copy_from_user(to, from, size);
+	unsigned long ret;
 
+	if (!__builtin_constant_p(size))
+		check_object_size(to, size, false);
+
+	ret = ___copy_from_user(to, from, size);
 	if (unlikely(ret))
 		ret = copy_from_user_fixup(to, from, size);
 
@@ -227,8 +231,11 @@ unsigned long copy_to_user_fixup(void __user *to, const void *from,
 static inline unsigned long __must_check
 copy_to_user(void __user *to, const void *from, unsigned long size)
 {
-	unsigned long ret = ___copy_to_user(to, from, size);
+	unsigned long ret;
 
+	if (!__builtin_constant_p(size))
+		check_object_size(from, size, true);
+	ret = ___copy_to_user(to, from, size);
 	if (unlikely(ret))
 		ret = copy_to_user_fixup(to, from, size);
 	return ret;
-- 
2.7.4

[PATCH 8/9] mm: SLAB hardened usercopy support

[Kees Cook]

Under CONFIG_HARDENED_USERCOPY, this adds object size checking to the
SLAB allocator to catch any copies that may span objects.

Based on code from PaX and grsecurity.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 init/Kconfig |  1 +
 mm/slab.c    | 30 ++++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/init/Kconfig b/init/Kconfig
index f755a602d4a1..798c2020ee7c 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1757,6 +1757,7 @@ choice
 
 config SLAB
 	bool "SLAB"
+	select HAVE_HARDENED_USERCOPY_ALLOCATOR
 	help
 	  The regular slab allocator that is established and known to work
 	  well in all environments. It organizes cache hot objects in
diff --git a/mm/slab.c b/mm/slab.c
index cc8bbc1e6bc9..5e2d5f349aca 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -4477,6 +4477,36 @@ static int __init slab_proc_init(void)
 module_init(slab_proc_init);
 #endif
 
+#ifdef CONFIG_HARDENED_USERCOPY
+/*
+ * Rejects objects that are incorrectly sized.
+ *
+ * Returns NULL if check passes, otherwise const char * to name of cache
+ * to indicate an error.
+ */
+const char *__check_heap_object(const void *ptr, unsigned long n,
+				struct page *page)
+{
+	struct kmem_cache *cachep;
+	unsigned int objnr;
+	unsigned long offset;
+
+	/* Find and validate object. */
+	cachep = page->slab_cache;
+	objnr = obj_to_index(cachep, page, (void *)ptr);
+	BUG_ON(objnr >= cachep->num);
+
+	/* Find offset within object. */
+	offset = ptr - index_to_obj(cachep, page, objnr) - obj_offset(cachep);
+
+	/* Allow address range falling entirely within object size. */
+	if (offset <= cachep->object_size && n <= cachep->object_size - offset)
+		return NULL;
+
+	return cachep->name;
+}
+#endif /* CONFIG_HARDENED_USERCOPY */
+
 /**
  * ksize - get the actual amount of memory allocated for a given object
  * @objp: Pointer to the object
-- 
2.7.4

[PATCH 9/9] mm: SLUB hardened usercopy support

[Kees Cook]

Under CONFIG_HARDENED_USERCOPY, this adds object size checking to the
SLUB allocator to catch any copies that may span objects.

Based on code from PaX and grsecurity.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 init/Kconfig |  1 +
 mm/slub.c    | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/init/Kconfig b/init/Kconfig
index 798c2020ee7c..1c4711819dfd 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1765,6 +1765,7 @@ config SLAB
 
 config SLUB
 	bool "SLUB (Unqueued Allocator)"
+	select HAVE_HARDENED_USERCOPY_ALLOCATOR
 	help
 	   SLUB is a slab allocator that minimizes cache line usage
 	   instead of managing queues of cached objects (SLAB approach).
diff --git a/mm/slub.c b/mm/slub.c
index 825ff4505336..0c8ace04f075 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3614,6 +3614,33 @@ void *__kmalloc_node(size_t size, gfp_t flags, int node)
 EXPORT_SYMBOL(__kmalloc_node);
 #endif
 
+#ifdef CONFIG_HARDENED_USERCOPY
+/*
+ * Rejects objects that are incorrectly sized.
+ *
+ * Returns NULL if check passes, otherwise const char * to name of cache
+ * to indicate an error.
+ */
+const char *__check_heap_object(const void *ptr, unsigned long n,
+				struct page *page)
+{
+	struct kmem_cache *s;
+	unsigned long offset;
+
+	/* Find object. */
+	s = page->slab_cache;
+
+	/* Find offset within object. */
+	offset = (ptr - page_address(page)) % s->size;
+
+	/* Allow address range falling entirely within object size. */
+	if (offset <= s->object_size && n <= s->object_size - offset)
+		return NULL;
+
+	return s->name;
+}
+#endif /* CONFIG_HARDENED_USERCOPY */
+
 static size_t __ksize(const void *object)
 {
 	struct page *page;
-- 
2.7.4

[PATCH 1/9] mm: Hardened usercopy

[Baruch Siach]

Hi Kees,

On Wed, Jul 06, 2016 at 03:25:20PM -0700, Kees Cook wrote:
> +#ifdef CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR

Should be CONFIG_HARDENED_USERCOPY to match the slab/slub implementation 
condition.

> +const char *__check_heap_object(const void *ptr, unsigned long n,
> +				struct page *page);
> +#else
> +static inline const char *__check_heap_object(const void *ptr,
> +					      unsigned long n,
> +					      struct page *page)
> +{
> +	return NULL;
> +}
> +#endif

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

[PATCH 0/9] mm: Hardened usercopy

[Christian Borntraeger]

On 07/07/2016 12:25 AM, Kees Cook wrote:
> Hi,
> 
> This is a start of the mainline port of PAX_USERCOPY[1]. After I started
> writing tests (now in lkdtm in -next) for Casey's earlier port[2], I
> kept tweaking things further and further until I ended up with a whole
> new patch series. To that end, I took Rik's feedback and made a number
> of other changes and clean-ups as well.
> 
> Based on my understanding, PAX_USERCOPY was designed to catch a few
> classes of flaws around the use of copy_to_user()/copy_from_user(). These
> changes don't touch get_user() and put_user(), since these operate on
> constant sized lengths, and tend to be much less vulnerable. There
> are effectively three distinct protections in the whole series,
> each of which I've given a separate CONFIG, though this patch set is
> only the first of the three intended protections. (Generally speaking,
> PAX_USERCOPY covers what I'm calling CONFIG_HARDENED_USERCOPY (this) and
> CONFIG_HARDENED_USERCOPY_WHITELIST (future), and PAX_USERCOPY_SLABS covers
> CONFIG_HARDENED_USERCOPY_SPLIT_KMALLOC (future).)
> 
> This series, which adds CONFIG_HARDENED_USERCOPY, checks that objects
> being copied to/from userspace meet certain criteria:
> - if address is a heap object, the size must not exceed the object's
>   allocated size. (This will catch all kinds of heap overflow flaws.)
> - if address range is in the current process stack, it must be within the
>   current stack frame (if such checking is possible) or at least entirely
>   within the current process's stack. (This could catch large lengths that
>   would have extended beyond the current process stack, or overflows if
>   their length extends back into the original stack.)
> - if the address range is part of kernel data, rodata, or bss, allow it.
> - if address range is page-allocated, that it doesn't span multiple
>   allocations.
> - if address is within the kernel text, reject it.
> - everything else is accepted
> 
> The patches in the series are:
> - The core copy_to/from_user() checks, without the slab object checks:
> 	1- mm: Hardened usercopy
> - Per-arch enablement of the protection:
> 	2- x86/uaccess: Enable hardened usercopy
> 	3- ARM: uaccess: Enable hardened usercopy
> 	4- arm64/uaccess: Enable hardened usercopy
> 	5- ia64/uaccess: Enable hardened usercopy
> 	6- powerpc/uaccess: Enable hardened usercopy
> 	7- sparc/uaccess: Enable hardened usercopy

Was there a reason why you did not change s390?

[PATCH 1/9] mm: Hardened usercopy

[Thomas Gleixner]

On Wed, 6 Jul 2016, Kees Cook wrote:
> +
> +#if defined(CONFIG_FRAME_POINTER) && defined(CONFIG_X86)
> +	const void *frame = NULL;
> +	const void *oldframe;
> +#endif

That's ugly

> +
> +	/* Object is not on the stack at all. */
> +	if (obj + len <= stack || stackend <= obj)
> +		return 0;
> +
> +	/*
> +	 * Reject: object partially overlaps the stack (passing the
> +	 * the check above means at least one end is within the stack,
> +	 * so if this check fails, the other end is outside the stack).
> +	 */
> +	if (obj < stack || stackend < obj + len)
> +		return -1;
> +
> +#if defined(CONFIG_FRAME_POINTER) && defined(CONFIG_X86)
> +	oldframe = __builtin_frame_address(1);
> +	if (oldframe)
> +		frame = __builtin_frame_address(2);
> +	/*
> +	 * low ----------------------------------------------> high
> +	 * [saved bp][saved ip][args][local vars][saved bp][saved ip]
> +	 *		     ^----------------^
> +	 *             allow copies only within here
> +	 */
> +	while (stack <= frame && frame < stackend) {
> +		/*
> +		 * If obj + len extends past the last frame, this
> +		 * check won't pass and the next frame will be 0,
> +		 * causing us to bail out and correctly report
> +		 * the copy as invalid.
> +		 */
> +		if (obj + len <= frame)
> +			return obj >= oldframe + 2 * sizeof(void *) ? 2 : -1;
> +		oldframe = frame;
> +		frame = *(const void * const *)frame;
> +	}
> +	return -1;
> +#else
> +	return 1;
> +#endif

I'd rather make that a weak function returning 1 which can be replaced by
x86 for CONFIG_FRAME_POINTER=y. That also allows other architectures to
implement their specific frame checks.

Thanks,

	tglx

[PATCH 1/9] mm: Hardened usercopy

[Arnd Bergmann]

On Wednesday, July 6, 2016 3:25:20 PM CEST Kees Cook wrote:
> This is the start of porting PAX_USERCOPY into the mainline kernel. This
> is the first set of features, controlled by CONFIG_HARDENED_USERCOPY. The
> work is based on code by PaX Team and Brad Spengler, and an earlier port
> from Casey Schaufler. Additional non-slab page tests are from Rik van Riel.
> 
> This patch contains the logic for validating several conditions when
> performing copy_to_user() and copy_from_user() on the kernel object
> being copied to/from:
> - address range doesn't wrap around
> - address range isn't NULL or zero-allocated (with a non-zero copy size)
> - if on the slab allocator:
>   - object size must be less than or equal to copy size (when check is
>     implemented in the allocator, which appear in subsequent patches)
> - otherwise, object must not span page allocations
> - if on the stack
>   - object must not extend before/after the current process task
>   - object must be contained by the current stack frame (when there is
>     arch/build support for identifying stack frames)
> - object must not overlap with kernel text
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

Nice!

I have a few further thoughts, most of which have probably been
considered before:

> +static inline const char *check_bogus_address(const void *ptr, unsigned long n)
> +{
> +	/* Reject if object wraps past end of memory. */
> +	if (ptr + n < ptr)
> +		return "<wrapped address>";
> +
> +	/* Reject if NULL or ZERO-allocation. */
> +	if (ZERO_OR_NULL_PTR(ptr))
> +		return "<null>";
> +
> +	return NULL;
> +}

This checks against address (void*)16, but I guess on most architectures the
lowest possible kernel address is much higher. While there may not be much
that to exploit if the expected kernel address points to userland, forbidding
any obviously incorrect address that is outside of the kernel may be easier.

Even on architectures like s390 that start the kernel memory at (void *)0x0,
the lowest address to which we may want to do a copy_to_user would be much
higher than (void*)0x16.

> +
> +	/* Allow kernel rodata region (if not marked as Reserved). */
> +	if (ptr >= (const void *)__start_rodata &&
> +	    end <= (const void *)__end_rodata)
> +		return NULL;

Should we explicitly forbid writing to rodata, or is it enough to
rely on page protection here?

> +	/* Allow kernel bss region (if not marked as Reserved). */
> +	if (ptr >= (const void *)__bss_start &&
> +	    end <= (const void *)__bss_stop)
> +		return NULL;

accesses to .data/.rodata/.bss are probably not performance critical,
so we could go further here and check the kallsyms table to ensure
that we are not spanning multiple symbols here.

For stuff that is performance critical, should there be a way to
opt out of the checks, or do we assume it already uses functions
that avoid the checks? I looked at the file and network I/O path
briefly and they seem to use kmap_atomic() to get to the user pages
at least in some of the common cases (but I may well be missing
important ones).

	Arnd

[PATCH 4/9] arm64/uaccess: Enable hardened usercopy

[Mark Rutland]

Hi,

On Wed, Jul 06, 2016 at 03:25:23PM -0700, Kees Cook wrote:
> Enables CONFIG_HARDENED_USERCOPY checks on arm64. As done by KASAN in -next,
> renames the low-level functions to __arch_copy_*_user() so a static inline
> can do additional work before the copy.

The checks themselves look fine, but as with the KASAN checks, it seems
a shame that this logic is duplicated per arch, integrated in subtly
different ways.

Can we not __arch prefix all the arch uaccess helpers, and place
kasan_check_*() and check_object_size() calls in generic wrappers?

If we're going to update all the arch uaccess helpers anyway, doing that
would make it easier to fix things up, or to add new checks in future.

Thanks,
Mark.

> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  arch/arm64/Kconfig               |  2 ++
>  arch/arm64/include/asm/uaccess.h | 18 ++++++++++++++++--
>  arch/arm64/kernel/arm64ksyms.c   |  4 ++--
>  arch/arm64/lib/copy_from_user.S  |  4 ++--
>  arch/arm64/lib/copy_to_user.S    |  4 ++--
>  5 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 5a0a691d4220..b771cd97f74b 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -51,10 +51,12 @@ config ARM64
>  	select HAVE_ALIGNED_STRUCT_PAGE if SLUB
>  	select HAVE_ARCH_AUDITSYSCALL
>  	select HAVE_ARCH_BITREVERSE
> +	select HAVE_ARCH_HARDENED_USERCOPY
>  	select HAVE_ARCH_HUGE_VMAP
>  	select HAVE_ARCH_JUMP_LABEL
>  	select HAVE_ARCH_KASAN if SPARSEMEM_VMEMMAP && !(ARM64_16K_PAGES && ARM64_VA_BITS_48)
>  	select HAVE_ARCH_KGDB
> +	select HAVE_ARCH_LINEAR_KERNEL_MAPPING
>  	select HAVE_ARCH_MMAP_RND_BITS
>  	select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPAT
>  	select HAVE_ARCH_SECCOMP_FILTER
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 9e397a542756..6d0f86300936 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -256,11 +256,25 @@ do {									\
>  		-EFAULT;						\
>  })
>  
> -extern unsigned long __must_check __copy_from_user(void *to, const void __user *from, unsigned long n);
> -extern unsigned long __must_check __copy_to_user(void __user *to, const void *from, unsigned long n);
> +extern unsigned long __must_check __arch_copy_from_user(void *to, const void __user *from, unsigned long n);
> +extern unsigned long __must_check __arch_copy_to_user(void __user *to, const void *from, unsigned long n);
>  extern unsigned long __must_check __copy_in_user(void __user *to, const void __user *from, unsigned long n);
>  extern unsigned long __must_check __clear_user(void __user *addr, unsigned long n);
>  
> +static inline unsigned long __must_check
> +__copy_from_user(void *to, const void __user *from, unsigned long n)
> +{
> +	check_object_size(to, n, false);
> +	return __arch_copy_from_user(to, from, n);
> +}
> +
> +static inline unsigned long __must_check
> +__copy_to_user(void __user *to, const void *from, unsigned long n)
> +{
> +	check_object_size(from, n, true);
> +	return __arch_copy_to_user(to, from, n);
> +}
> +
>  static inline unsigned long __must_check copy_from_user(void *to, const void __user *from, unsigned long n)
>  {
>  	if (access_ok(VERIFY_READ, from, n))
> diff --git a/arch/arm64/kernel/arm64ksyms.c b/arch/arm64/kernel/arm64ksyms.c
> index 678f30b05a45..2dc44406a7ad 100644
> --- a/arch/arm64/kernel/arm64ksyms.c
> +++ b/arch/arm64/kernel/arm64ksyms.c
> @@ -34,8 +34,8 @@ EXPORT_SYMBOL(copy_page);
>  EXPORT_SYMBOL(clear_page);
>  
>  	/* user mem (segment) */
> -EXPORT_SYMBOL(__copy_from_user);
> -EXPORT_SYMBOL(__copy_to_user);
> +EXPORT_SYMBOL(__arch_copy_from_user);
> +EXPORT_SYMBOL(__arch_copy_to_user);
>  EXPORT_SYMBOL(__clear_user);
>  EXPORT_SYMBOL(__copy_in_user);
>  
> diff --git a/arch/arm64/lib/copy_from_user.S b/arch/arm64/lib/copy_from_user.S
> index 17e8306dca29..0b90497d4424 100644
> --- a/arch/arm64/lib/copy_from_user.S
> +++ b/arch/arm64/lib/copy_from_user.S
> @@ -66,7 +66,7 @@
>  	.endm
>  
>  end	.req	x5
> -ENTRY(__copy_from_user)
> +ENTRY(__arch_copy_from_user)
>  ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_ALT_PAN_NOT_UAO, \
>  	    CONFIG_ARM64_PAN)
>  	add	end, x0, x2
> @@ -75,7 +75,7 @@ ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(1)), ARM64_ALT_PAN_NOT_UAO, \
>  	    CONFIG_ARM64_PAN)
>  	mov	x0, #0				// Nothing to copy
>  	ret
> -ENDPROC(__copy_from_user)
> +ENDPROC(__arch_copy_from_user)
>  
>  	.section .fixup,"ax"
>  	.align	2
> diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S
> index 21faae60f988..7a7efe255034 100644
> --- a/arch/arm64/lib/copy_to_user.S
> +++ b/arch/arm64/lib/copy_to_user.S
> @@ -65,7 +65,7 @@
>  	.endm
>  
>  end	.req	x5
> -ENTRY(__copy_to_user)
> +ENTRY(__arch_copy_to_user)
>  ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(0)), ARM64_ALT_PAN_NOT_UAO, \
>  	    CONFIG_ARM64_PAN)
>  	add	end, x0, x2
> @@ -74,7 +74,7 @@ ALTERNATIVE("nop", __stringify(SET_PSTATE_PAN(1)), ARM64_ALT_PAN_NOT_UAO, \
>  	    CONFIG_ARM64_PAN)
>  	mov	x0, #0
>  	ret
> -ENDPROC(__copy_to_user)
> +ENDPROC(__arch_copy_to_user)
>  
>  	.section .fixup,"ax"
>  	.align	2
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

[PATCH 1/9] mm: Hardened usercopy

[Rik van Riel]

On Wed, 2016-07-06 at 15:25 -0700, Kees Cook wrote:
> This is the start of porting PAX_USERCOPY into the mainline kernel.
> This
> is the first set of features, controlled by CONFIG_HARDENED_USERCOPY.
> The
> work is based on code by PaX Team and Brad Spengler, and an earlier
> port
> from Casey Schaufler. Additional non-slab page tests are from Rik van
> Riel.

Feel free to add my S-O-B for the code I wrote. The rest
looks good, too.

There may be some room for optimization later on, by putting
the most likely branches first, annotating with likely/unlikely,
etc, but I suspect the less likely checks are already towards
the ends of the functions.

Signed-off-by: Rik van Riel <riel@redhat.com>

> This patch contains the logic for validating several conditions when
> performing copy_to_user() and copy_from_user() on the kernel object
> being copied to/from:
> - address range doesn't wrap around
> - address range isn't NULL or zero-allocated (with a non-zero copy
> size)
> - if on the slab allocator:
> ? - object size must be less than or equal to copy size (when check
> is
> ????implemented in the allocator, which appear in subsequent patches)
> - otherwise, object must not span page allocations
> - if on the stack
> ? - object must not extend before/after the current process task
> ? - object must be contained by the current stack frame (when there
> is
> ????arch/build support for identifying stack frames)
> - object must not overlap with kernel text
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> ?arch/Kconfig????????????????|???7 ++
> ?include/linux/slab.h????????|??12 +++
> ?include/linux/thread_info.h |??15 +++
> ?mm/Makefile?????????????????|???4 +
> ?mm/usercopy.c???????????????| 239
> ++++++++++++++++++++++++++++++++++++++++++++
> ?security/Kconfig????????????|??27 +++++
> ?6 files changed, 304 insertions(+)
> ?create mode 100644 mm/usercopy.c
> 
> diff --git a/arch/Kconfig b/arch/Kconfig
> index d794384a0404..3ea04d8dcf62 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -424,6 +424,13 @@ config CC_STACKPROTECTOR_STRONG
> ?
> ?endchoice
> ?
> +config HAVE_ARCH_LINEAR_KERNEL_MAPPING
> +	bool
> +	help
> +	??An architecture should select this if it has a secondary
> linear
> +	??mapping of the kernel text. This is used to verify that
> kernel
> +	??text exposures are not visible under
> CONFIG_HARDENED_USERCOPY.
> +
> ?config HAVE_CONTEXT_TRACKING
> ?	bool
> ?	help
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index aeb3e6d00a66..96a16a3fb7cb 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -155,6 +155,18 @@ void kfree(const void *);
> ?void kzfree(const void *);
> ?size_t ksize(const void *);
> ?
> +#ifdef CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR
> +const char *__check_heap_object(const void *ptr, unsigned long n,
> +				struct page *page);
> +#else
> +static inline const char *__check_heap_object(const void *ptr,
> +					??????unsigned long n,
> +					??????struct page *page)
> +{
> +	return NULL;
> +}
> +#endif
> +
> ?/*
> ? * Some archs want to perform DMA into kmalloc caches and need a
> guaranteed
> ? * alignment larger than the alignment of a 64-bit integer.
> diff --git a/include/linux/thread_info.h
> b/include/linux/thread_info.h
> index b4c2a485b28a..a02200db9c33 100644
> --- a/include/linux/thread_info.h
> +++ b/include/linux/thread_info.h
> @@ -146,6 +146,21 @@ static inline bool
> test_and_clear_restore_sigmask(void)
> ?#error "no set_restore_sigmask() provided and default one won't
> work"
> ?#endif
> ?
> +#ifdef CONFIG_HARDENED_USERCOPY
> +extern void __check_object_size(const void *ptr, unsigned long n,
> +					bool to_user);
> +
> +static inline void check_object_size(const void *ptr, unsigned long
> n,
> +				?????bool to_user)
> +{
> +	__check_object_size(ptr, n, to_user);
> +}
> +#else
> +static inline void check_object_size(const void *ptr, unsigned long
> n,
> +				?????bool to_user)
> +{ }
> +#endif /* CONFIG_HARDENED_USERCOPY */
> +
> ?#endif	/* __KERNEL__ */
> ?
> ?#endif /* _LINUX_THREAD_INFO_H */
> diff --git a/mm/Makefile b/mm/Makefile
> index 78c6f7dedb83..32d37247c7e5 100644
> --- a/mm/Makefile
> +++ b/mm/Makefile
> @@ -21,6 +21,9 @@ KCOV_INSTRUMENT_memcontrol.o := n
> ?KCOV_INSTRUMENT_mmzone.o := n
> ?KCOV_INSTRUMENT_vmstat.o := n
> ?
> +# Since __builtin_frame_address does work as used, disable the
> warning.
> +CFLAGS_usercopy.o += $(call cc-disable-warning, frame-address)
> +
> ?mmu-y			:= nommu.o
> ?mmu-$(CONFIG_MMU)	:= gup.o highmem.o memory.o mincore.o \
> ?			???mlock.o mmap.o mprotect.o mremap.o
> msync.o rmap.o \
> @@ -99,3 +102,4 @@ obj-$(CONFIG_USERFAULTFD) += userfaultfd.o
> ?obj-$(CONFIG_IDLE_PAGE_TRACKING) += page_idle.o
> ?obj-$(CONFIG_FRAME_VECTOR) += frame_vector.o
> ?obj-$(CONFIG_DEBUG_PAGE_REF) += debug_page_ref.o
> +obj-$(CONFIG_HARDENED_USERCOPY) += usercopy.o
> diff --git a/mm/usercopy.c b/mm/usercopy.c
> new file mode 100644
> index 000000000000..ad2765dd6dc4
> --- /dev/null
> +++ b/mm/usercopy.c
> @@ -0,0 +1,239 @@
> +/*
> + * This implements the various checks for CONFIG_HARDENED_USERCOPY*,
> + * which are designed to protect kernel memory from needless
> exposure
> + * and overwrite under many unintended conditions. This code is
> based
> + * on PAX_USERCOPY, which is:
> + *
> + * Copyright (C) 2001-2016 PaX Team, Bradley Spengler, Open Source
> + * Security Inc.
> + *
> + * This program is free software; you can redistribute it and/or
> modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/mm.h>
> +#include <linux/slab.h>
> +#include <asm/sections.h>
> +
> +/*
> + * Checks if a given pointer and length is contained by the current
> + * stack frame (if possible).
> + *
> + *	0: not at all on the stack
> + *	1: fully on the stack (when can't do frame-checking)
> + *	2: fully inside the current stack frame
> + *	-1: error condition (invalid stack position or bad stack
> frame)
> + */
> +static noinline int check_stack_object(const void *obj, unsigned
> long len)
> +{
> +	const void * const stack = task_stack_page(current);
> +	const void * const stackend = stack + THREAD_SIZE;
> +
> +#if defined(CONFIG_FRAME_POINTER) && defined(CONFIG_X86)
> +	const void *frame = NULL;
> +	const void *oldframe;
> +#endif
> +
> +	/* Object is not on the stack at all. */
> +	if (obj + len <= stack || stackend <= obj)
> +		return 0;
> +
> +	/*
> +	?* Reject: object partially overlaps the stack (passing the
> +	?* the check above means at least one end is within the
> stack,
> +	?* so if this check fails, the other end is outside the
> stack).
> +	?*/
> +	if (obj < stack || stackend < obj + len)
> +		return -1;
> +
> +#if defined(CONFIG_FRAME_POINTER) && defined(CONFIG_X86)
> +	oldframe = __builtin_frame_address(1);
> +	if (oldframe)
> +		frame = __builtin_frame_address(2);
> +	/*
> +	?* low ----------------------------------------------> high
> +	?* [saved bp][saved ip][args][local vars][saved bp][saved
> ip]
> +	?*		?????^----------------^
> +	?*?????????????allow copies only within here
> +	?*/
> +	while (stack <= frame && frame < stackend) {
> +		/*
> +		?* If obj + len extends past the last frame, this
> +		?* check won't pass and the next frame will be 0,
> +		?* causing us to bail out and correctly report
> +		?* the copy as invalid.
> +		?*/
> +		if (obj + len <= frame)
> +			return obj >= oldframe + 2 * sizeof(void *)
> ? 2 : -1;
> +		oldframe = frame;
> +		frame = *(const void * const *)frame;
> +	}
> +	return -1;
> +#else
> +	return 1;
> +#endif
> +}
> +
> +static void report_usercopy(const void *ptr, unsigned long len,
> +			????bool to_user, const char *type)
> +{
> +	pr_emerg("kernel memory %s attempt detected %s %p (%s) (%lu
> bytes)\n",
> +		to_user ? "exposure" : "overwrite",
> +		to_user ? "from" : "to", ptr, type ? : "unknown",
> len);
> +	dump_stack();
> +	do_group_exit(SIGKILL);
> +}
> +
> +/* Returns true if any portion of [ptr,ptr+n) over laps with
> [low,high). */
> +static bool overlaps(const void *ptr, unsigned long n, unsigned long
> low,
> +		?????unsigned long high)
> +{
> +	unsigned long check_low = (uintptr_t)ptr;
> +	unsigned long check_high = check_low + n;
> +
> +	/* Does not overlap if entirely above or entirely below. */
> +	if (check_low >= high || check_high < low)
> +		return false;
> +
> +	return true;
> +}
> +
> +/* Is this address range in the kernel text area? */
> +static inline const char *check_kernel_text_object(const void *ptr,
> +						???unsigned long n)
> +{
> +	unsigned long textlow = (unsigned long)_stext;
> +	unsigned long texthigh = (unsigned long)_etext;
> +
> +	if (overlaps(ptr, n, textlow, texthigh))
> +		return "<kernel text>";
> +
> +#ifdef HAVE_ARCH_LINEAR_KERNEL_MAPPING
> +	/* Check against linear mapping as well. */
> +	if (overlaps(ptr, n, (unsigned long)__va(__pa(textlow)),
> +		?????(unsigned long)__va(__pa(texthigh))))
> +		return "<linear kernel text>";
> +#endif
> +
> +	return NULL;
> +}
> +
> +static inline const char *check_bogus_address(const void *ptr,
> unsigned long n)
> +{
> +	/* Reject if object wraps past end of memory. */
> +	if (ptr + n < ptr)
> +		return "<wrapped address>";
> +
> +	/* Reject if NULL or ZERO-allocation. */
> +	if (ZERO_OR_NULL_PTR(ptr))
> +		return "<null>";
> +
> +	return NULL;
> +}
> +
> +static inline const char *check_heap_object(const void *ptr,
> unsigned long n)
> +{
> +	struct page *page, *endpage;
> +	const void *end = ptr + n - 1;
> +
> +	if (!virt_addr_valid(ptr))
> +		return NULL;
> +
> +	page = virt_to_head_page(ptr);
> +
> +	/* Check slab allocator for flags and size. */
> +	if (PageSlab(page))
> +		return __check_heap_object(ptr, n, page);
> +
> +	/* Is the object wholly within one base page? */
> +	if (likely(((unsigned long)ptr & (unsigned long)PAGE_MASK)
> ==
> +		???((unsigned long)end & (unsigned long)PAGE_MASK)))
> +		return NULL;
> +
> +	/* Allow if start and end are inside the same compound page.
> */
> +	endpage = virt_to_head_page(end);
> +	if (likely(endpage == page))
> +		return NULL;
> +
> +	/* Allow special areas, device memory, and sometimes kernel
> data. */
> +	if (PageReserved(page) && PageReserved(endpage))
> +		return NULL;
> +
> +	/*
> +	?* Sometimes the kernel data regions are not marked
> Reserved. And
> +	?* sometimes [_sdata,_edata) does not cover rodata and/or
> bss,
> +	?* so check each range explicitly.
> +	?*/
> +
> +	/* Allow kernel data region (if not marked as Reserved). */
> +	if (ptr >= (const void *)_sdata && end <= (const void
> *)_edata)
> +		return NULL;
> +
> +	/* Allow kernel rodata region (if not marked as Reserved).
> */
> +	if (ptr >= (const void *)__start_rodata &&
> +	????end <= (const void *)__end_rodata)
> +		return NULL;
> +
> +	/* Allow kernel bss region (if not marked as Reserved). */
> +	if (ptr >= (const void *)__bss_start &&
> +	????end <= (const void *)__bss_stop)
> +		return NULL;
> +
> +	/* Uh oh. The "object" spans several independently allocated
> pages. */
> +	return "<spans multiple pages>";
> +}
> +
> +/*
> + * Validates that the given object is one of:
> + * - known safe heap object
> + * - known safe stack object
> + * - not in kernel text
> + */
> +void __check_object_size(const void *ptr, unsigned long n, bool
> to_user)
> +{
> +	const char *err;
> +
> +	/* Skip all tests if size is zero. */
> +	if (!n)
> +		return;
> +
> +	/* Check for invalid addresses. */
> +	err = check_bogus_address(ptr, n);
> +	if (err)
> +		goto report;
> +
> +	/* Check for bad heap object. */
> +	err = check_heap_object(ptr, n);
> +	if (err)
> +		goto report;
> +
> +	/* Check for bad stack object. */
> +	switch (check_stack_object(ptr, n)) {
> +	case 0:
> +		/* Object is not touching the current process stack.
> */
> +		break;
> +	case 1:
> +	case 2:
> +		/*
> +		?* Object is either in the correct frame (when it
> +		?* is possible to check) or just generally on the
> +		?* process stack (when frame checking not
> available).
> +		?*/
> +		return;
> +	default:
> +		err = "<process stack>";
> +		goto report;
> +	}
> +
> +	/* Check for object in kernel to avoid text exposure. */
> +	err = check_kernel_text_object(ptr, n);
> +	if (!err)
> +		return;
> +
> +report:
> +	report_usercopy(ptr, n, to_user, err);
> +}
> +EXPORT_SYMBOL(__check_object_size);
> diff --git a/security/Kconfig b/security/Kconfig
> index 176758cdfa57..63340ad0b9f9 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -118,6 +118,33 @@ config LSM_MMAP_MIN_ADDR
> ?	??this low address space will need the permission specific
> to the
> ?	??systems running LSM.
> ?
> +config HAVE_HARDENED_USERCOPY_ALLOCATOR
> +	bool
> +	help
> +	??The heap allocator implements __check_heap_object() for
> +	??validating memory ranges against heap object sizes in
> +	??support of CONFIG_HARDENED_USERCOPY.
> +
> +config HAVE_ARCH_HARDENED_USERCOPY
> +	bool
> +	help
> +	??The architecture supports CONFIG_HARDENED_USERCOPY by
> +	??calling check_object_size() just before performing the
> +	??userspace copies in the low level implementation of
> +	??copy_to_user() and copy_from_user().
> +
> +config HARDENED_USERCOPY
> +	bool "Harden memory copies between kernel and userspace"
> +	depends on HAVE_ARCH_HARDENED_USERCOPY
> +	help
> +	??This option checks for obviously wrong memory regions when
> +	??copying memory to/from the kernel (via copy_to_user() and
> +	??copy_from_user() functions) by rejecting memory ranges
> that
> +	??are larger than the specified heap object, span multiple
> +	??separately allocates pages, are not on the process stack,
> +	??or are part of the kernel text. This kills entire classes
> +	??of heap overflow exploits and similar kernel memory
> exposures.
> +
> ?source security/selinux/Kconfig
> ?source security/smack/Kconfig
> ?source security/tomoyo/Kconfig
-- 

All Rights Reversed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160707/29b69556/attachment-0001.sig>

[PATCH 1/9] mm: Hardened usercopy

[Rik van Riel]

On Wed, 2016-07-06 at 15:25 -0700, Kees Cook wrote:
>?
> +	/* Allow kernel rodata region (if not marked as Reserved).
> */
> +	if (ptr >= (const void *)__start_rodata &&
> +	????end <= (const void *)__end_rodata)
> +		return NULL;
> 
One comment here.

__check_object_size gets "to_user" as an argument.

It may make sense to pass that to check_heap_object, and
only allow copy_to_user from rodata, never copy_from_user,
since that section should be read only.

> +void __check_object_size(const void *ptr, unsigned long n, bool
> to_user)
> +{
>?

-- 

All Rights Reversed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160707/f13757cc/attachment.sig>

[PATCH 4/9] arm64/uaccess: Enable hardened usercopy

[Kees Cook]

On Thu, Jul 7, 2016 at 6:07 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi,
>
> On Wed, Jul 06, 2016 at 03:25:23PM -0700, Kees Cook wrote:
>> Enables CONFIG_HARDENED_USERCOPY checks on arm64. As done by KASAN in -next,
>> renames the low-level functions to __arch_copy_*_user() so a static inline
>> can do additional work before the copy.
>
> The checks themselves look fine, but as with the KASAN checks, it seems
> a shame that this logic is duplicated per arch, integrated in subtly
> different ways.
>
> Can we not __arch prefix all the arch uaccess helpers, and place
> kasan_check_*() and check_object_size() calls in generic wrappers?
>
> If we're going to update all the arch uaccess helpers anyway, doing that
> would make it easier to fix things up, or to add new checks in future.

Yeah, I totally agree, and my work on the next step of this hardening
will require something like this to separate the "check" logic from
the "copy" logic, as I want to introduce a set of constant-sized
copy_*_user helpers.

Though currently x86 poses a weird problem in this regard (they have
separate code paths for copy_* and __copy*, but I think it's actually
a harmless(?) mistake.

For now, I'd like to leave this as-is, and then do the copy_* cleanup,
then do step 2 (slab whitelisting).

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[PATCH 1/9] mm: Hardened usercopy

[Kees Cook]

On Thu, Jul 7, 2016 at 1:37 AM, Baruch Siach <baruch@tkos.co.il> wrote:
> Hi Kees,
>
> On Wed, Jul 06, 2016 at 03:25:20PM -0700, Kees Cook wrote:
>> +#ifdef CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR
>
> Should be CONFIG_HARDENED_USERCOPY to match the slab/slub implementation
> condition.
>
>> +const char *__check_heap_object(const void *ptr, unsigned long n,
>> +                             struct page *page);
>> +#else
>> +static inline const char *__check_heap_object(const void *ptr,
>> +                                           unsigned long n,
>> +                                           struct page *page)
>> +{
>> +     return NULL;
>> +}
>> +#endif

Hmm, I think what I have is correct: if the allocator supports the
heap object checking, it defines __check_heap_object as existing via
CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR. If usercopy checking is done
at all is controlled by CONFIG_HARDENED_USERCOPY.

I.e. you can have the other usercopy checks even if your allocator
doesn't support object size checking.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[PATCH 0/9] mm: Hardened usercopy

[Kees Cook]

On Thu, Jul 7, 2016 at 3:30 AM, Christian Borntraeger
<borntraeger@de.ibm.com> wrote:
> On 07/07/2016 12:25 AM, Kees Cook wrote:
>> Hi,
>>
>> This is a start of the mainline port of PAX_USERCOPY[1]. After I started
>> writing tests (now in lkdtm in -next) for Casey's earlier port[2], I
>> kept tweaking things further and further until I ended up with a whole
>> new patch series. To that end, I took Rik's feedback and made a number
>> of other changes and clean-ups as well.
>>
>> Based on my understanding, PAX_USERCOPY was designed to catch a few
>> classes of flaws around the use of copy_to_user()/copy_from_user(). These
>> changes don't touch get_user() and put_user(), since these operate on
>> constant sized lengths, and tend to be much less vulnerable. There
>> are effectively three distinct protections in the whole series,
>> each of which I've given a separate CONFIG, though this patch set is
>> only the first of the three intended protections. (Generally speaking,
>> PAX_USERCOPY covers what I'm calling CONFIG_HARDENED_USERCOPY (this) and
>> CONFIG_HARDENED_USERCOPY_WHITELIST (future), and PAX_USERCOPY_SLABS covers
>> CONFIG_HARDENED_USERCOPY_SPLIT_KMALLOC (future).)
>>
>> This series, which adds CONFIG_HARDENED_USERCOPY, checks that objects
>> being copied to/from userspace meet certain criteria:
>> - if address is a heap object, the size must not exceed the object's
>>   allocated size. (This will catch all kinds of heap overflow flaws.)
>> - if address range is in the current process stack, it must be within the
>>   current stack frame (if such checking is possible) or at least entirely
>>   within the current process's stack. (This could catch large lengths that
>>   would have extended beyond the current process stack, or overflows if
>>   their length extends back into the original stack.)
>> - if the address range is part of kernel data, rodata, or bss, allow it.
>> - if address range is page-allocated, that it doesn't span multiple
>>   allocations.
>> - if address is within the kernel text, reject it.
>> - everything else is accepted
>>
>> The patches in the series are:
>> - The core copy_to/from_user() checks, without the slab object checks:
>>       1- mm: Hardened usercopy
>> - Per-arch enablement of the protection:
>>       2- x86/uaccess: Enable hardened usercopy
>>       3- ARM: uaccess: Enable hardened usercopy
>>       4- arm64/uaccess: Enable hardened usercopy
>>       5- ia64/uaccess: Enable hardened usercopy
>>       6- powerpc/uaccess: Enable hardened usercopy
>>       7- sparc/uaccess: Enable hardened usercopy
>
> Was there a reason why you did not change s390?

No reason -- just didn't have a good build setup for testing it.
(Everything but arm64 was already in grsecurity, and I was able to
build-test arm64 when I added it there.) I would love to include s390
too!

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[PATCH 1/9] mm: Hardened usercopy

[Kees Cook]

On Thu, Jul 7, 2016 at 3:42 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Wed, 6 Jul 2016, Kees Cook wrote:
>> +
>> +#if defined(CONFIG_FRAME_POINTER) && defined(CONFIG_X86)
>> +     const void *frame = NULL;
>> +     const void *oldframe;
>> +#endif
>
> That's ugly

Yeah, I'd like to have this be controlled by a specific CONFIG, like I
invented for the linear mapping, but I wasn't sure what was the best
approach.

>
>> +
>> +     /* Object is not on the stack at all. */
>> +     if (obj + len <= stack || stackend <= obj)
>> +             return 0;
>> +
>> +     /*
>> +      * Reject: object partially overlaps the stack (passing the
>> +      * the check above means at least one end is within the stack,
>> +      * so if this check fails, the other end is outside the stack).
>> +      */
>> +     if (obj < stack || stackend < obj + len)
>> +             return -1;
>> +
>> +#if defined(CONFIG_FRAME_POINTER) && defined(CONFIG_X86)
>> +     oldframe = __builtin_frame_address(1);
>> +     if (oldframe)
>> +             frame = __builtin_frame_address(2);
>> +     /*
>> +      * low ----------------------------------------------> high
>> +      * [saved bp][saved ip][args][local vars][saved bp][saved ip]
>> +      *                   ^----------------^
>> +      *             allow copies only within here
>> +      */
>> +     while (stack <= frame && frame < stackend) {
>> +             /*
>> +              * If obj + len extends past the last frame, this
>> +              * check won't pass and the next frame will be 0,
>> +              * causing us to bail out and correctly report
>> +              * the copy as invalid.
>> +              */
>> +             if (obj + len <= frame)
>> +                     return obj >= oldframe + 2 * sizeof(void *) ? 2 : -1;
>> +             oldframe = frame;
>> +             frame = *(const void * const *)frame;
>> +     }
>> +     return -1;
>> +#else
>> +     return 1;
>> +#endif
>
> I'd rather make that a weak function returning 1 which can be replaced by
> x86 for CONFIG_FRAME_POINTER=y. That also allows other architectures to
> implement their specific frame checks.

Yeah, though I prefer CONFIG-controlled stuff over weak functions, but
I agree, something like arch_check_stack_frame(...) or similar. I'll
build something for this on the next revision.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[PATCH 1/9] mm: Hardened usercopy

[Kees Cook]

On Thu, Jul 7, 2016 at 4:01 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday, July 6, 2016 3:25:20 PM CEST Kees Cook wrote:
>> This is the start of porting PAX_USERCOPY into the mainline kernel. This
>> is the first set of features, controlled by CONFIG_HARDENED_USERCOPY. The
>> work is based on code by PaX Team and Brad Spengler, and an earlier port
>> from Casey Schaufler. Additional non-slab page tests are from Rik van Riel.
>>
>> This patch contains the logic for validating several conditions when
>> performing copy_to_user() and copy_from_user() on the kernel object
>> being copied to/from:
>> - address range doesn't wrap around
>> - address range isn't NULL or zero-allocated (with a non-zero copy size)
>> - if on the slab allocator:
>>   - object size must be less than or equal to copy size (when check is
>>     implemented in the allocator, which appear in subsequent patches)
>> - otherwise, object must not span page allocations
>> - if on the stack
>>   - object must not extend before/after the current process task
>>   - object must be contained by the current stack frame (when there is
>>     arch/build support for identifying stack frames)
>> - object must not overlap with kernel text
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Nice!
>
> I have a few further thoughts, most of which have probably been
> considered before:
>
>> +static inline const char *check_bogus_address(const void *ptr, unsigned long n)
>> +{
>> +     /* Reject if object wraps past end of memory. */
>> +     if (ptr + n < ptr)
>> +             return "<wrapped address>";
>> +
>> +     /* Reject if NULL or ZERO-allocation. */
>> +     if (ZERO_OR_NULL_PTR(ptr))
>> +             return "<null>";
>> +
>> +     return NULL;
>> +}
>
> This checks against address (void*)16, but I guess on most architectures the
> lowest possible kernel address is much higher. While there may not be much
> that to exploit if the expected kernel address points to userland, forbidding
> any obviously incorrect address that is outside of the kernel may be easier.
>
> Even on architectures like s390 that start the kernel memory at (void *)0x0,
> the lowest address to which we may want to do a copy_to_user would be much
> higher than (void*)0x16.

Yeah, that's worth exploring, but given the shenanigans around
set_fs(), I'd like to leave this as-is, and we can add to these checks
as we remove as much of the insane usage of set_fs().

>> +
>> +     /* Allow kernel rodata region (if not marked as Reserved). */
>> +     if (ptr >= (const void *)__start_rodata &&
>> +         end <= (const void *)__end_rodata)
>> +             return NULL;
>
> Should we explicitly forbid writing to rodata, or is it enough to
> rely on page protection here?

Hm, interesting. That's a very small check to add. My knee-jerk is to
just leave it up to page protection. I'm on the fence. :)

>
>> +     /* Allow kernel bss region (if not marked as Reserved). */
>> +     if (ptr >= (const void *)__bss_start &&
>> +         end <= (const void *)__bss_stop)
>> +             return NULL;
>
> accesses to .data/.rodata/.bss are probably not performance critical,
> so we could go further here and check the kallsyms table to ensure
> that we are not spanning multiple symbols here.

Oh, interesting! Yeah, would you be willing to put together that patch
and test it? I wonder if there are any cases where there are
legitimate usercopys across multiple symbols.

> For stuff that is performance critical, should there be a way to
> opt out of the checks, or do we assume it already uses functions
> that avoid the checks? I looked at the file and network I/O path
> briefly and they seem to use kmap_atomic() to get to the user pages
> at least in some of the common cases (but I may well be missing
> important ones).

I don't want to start with an exemption here, so until such a case is
found, I'd rather leave this as-is. That said, the primary protection
here tends to be buggy lengths (which is why put/get_user() is
untouched). For constant-sized copies, some checks could be skipped.
In the second part of this protection (what I named
CONFIG_HARDENED_USERCOPY_WHITELIST in the RFC version of this series),
there are cases where we want to skip the whitelist checking since it
is for a constant-sized copy the code understands is okay to pull out
of an otherwise disallowed allocator object.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[PATCH 1/9] mm: Hardened usercopy

[Kees Cook]

On Thu, Jul 7, 2016 at 12:35 PM, Rik van Riel <riel@redhat.com> wrote:
> On Wed, 2016-07-06 at 15:25 -0700, Kees Cook wrote:
>>
>> +     /* Allow kernel rodata region (if not marked as Reserved).
>> */
>> +     if (ptr >= (const void *)__start_rodata &&
>> +         end <= (const void *)__end_rodata)
>> +             return NULL;
>>
> One comment here.
>
> __check_object_size gets "to_user" as an argument.
>
> It may make sense to pass that to check_heap_object, and
> only allow copy_to_user from rodata, never copy_from_user,
> since that section should be read only.

Well, that's two votes for this extra check, but I'm still not sure
since it may already be allowed by the Reserved check, but I can
reorder things to _reject_ on rodata writes before the Reserved check,
etc.

I'll see what could work here...

-Kees

>
>> +void __check_object_size(const void *ptr, unsigned long n, bool
>> to_user)
>> +{
>>
>
> --
>
> All Rights Reversed.



-- 
Kees Cook
Chrome OS & Brillo Security

[PATCH 1/9] mm: Hardened usercopy

[Baruch Siach]

Hi Kees,

On Thu, Jul 07, 2016 at 01:25:21PM -0400, Kees Cook wrote:
> On Thu, Jul 7, 2016 at 1:37 AM, Baruch Siach <baruch@tkos.co.il> wrote:
> > On Wed, Jul 06, 2016 at 03:25:20PM -0700, Kees Cook wrote:
> >> +#ifdef CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR
> >
> > Should be CONFIG_HARDENED_USERCOPY to match the slab/slub implementation
> > condition.
> >
> >> +const char *__check_heap_object(const void *ptr, unsigned long n,
> >> +                             struct page *page);
> >> +#else
> >> +static inline const char *__check_heap_object(const void *ptr,
> >> +                                           unsigned long n,
> >> +                                           struct page *page)
> >> +{
> >> +     return NULL;
> >> +}
> >> +#endif
> 
> Hmm, I think what I have is correct: if the allocator supports the
> heap object checking, it defines __check_heap_object as existing via
> CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR. If usercopy checking is done
> at all is controlled by CONFIG_HARDENED_USERCOPY.
> 
> I.e. you can have the other usercopy checks even if your allocator
> doesn't support object size checking.

Right. I missed the fact that usercopy.c build also depends on 
CONFIG_HARDENED_USERCOPY. Sorry for the noise.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

[PATCH 1/9] mm: Hardened usercopy

[Thomas Gleixner]

On Thu, 7 Jul 2016, Kees Cook wrote:
> On Thu, Jul 7, 2016 at 3:42 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > I'd rather make that a weak function returning 1 which can be replaced by
> > x86 for CONFIG_FRAME_POINTER=y. That also allows other architectures to
> > implement their specific frame checks.
> 
> Yeah, though I prefer CONFIG-controlled stuff over weak functions, but
> I agree, something like arch_check_stack_frame(...) or similar. I'll
> build something for this on the next revision.

I'm fine with CONFIG_CONTROLLED as long as the ifdeffery is limited to header
files.

Thanks,

	tglx

[PATCH 0/9] mm: Hardened usercopy

[Ingo Molnar]

* Kees Cook <keescook@chromium.org> wrote:

> - I couldn't detect a measurable performance change with these features
>   enabled. Kernel build times were unchanged, hackbench was unchanged,
>   etc. I think we could flip this to "on by default" at some point.

Could you please try to find some syscall workload that does many small user 
copies and thus excercises this code path aggressively?

If that measurement works out fine then I'd prefer to enable these security checks 
by default.

Thaks,

	Ingo

[PATCH 1/9] mm: Hardened usercopy

[Arnd Bergmann]

On Thursday, July 7, 2016 1:37:43 PM CEST Kees Cook wrote:
> >
> >> +     /* Allow kernel bss region (if not marked as Reserved). */
> >> +     if (ptr >= (const void *)__bss_start &&
> >> +         end <= (const void *)__bss_stop)
> >> +             return NULL;
> >
> > accesses to .data/.rodata/.bss are probably not performance critical,
> > so we could go further here and check the kallsyms table to ensure
> > that we are not spanning multiple symbols here.
> 
> Oh, interesting! Yeah, would you be willing to put together that patch
> and test it?

Not at the moment, sorry.

I've given it a closer look and unfortunately realized that kallsyms
today only covers .text and .init.text, so it's currently useless because
those sections are already disallowed.

We could extend kallsyms to also cover all other sections, but doing
that right will likely cause a number of problems (most likely
kallsyms size mismatch) that will have to be debugged first.\

I think it's doable but time-consuming. The check function should
actually be trivial:

static bool usercopy_spans_multiple_symbols(void *ptr, size_t len)
{
	unsigned long size, offset;	

	if (kallsyms_lookup_size_offset((unsigned long)ptr, &size, &offset))
		return 0; /* no symbol found or kallsyms disabled */

	if (size - offset <= len)
		return 0; /* range is within one symbol */

	return 1;
}

This part would also be trivial:

diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
index 1f22a186c18c..e0f37212e2a9 100644
--- a/scripts/kallsyms.c
+++ b/scripts/kallsyms.c
@@ -50,6 +50,11 @@ static struct addr_range text_ranges[] = {
 	{ "_sinittext", "_einittext" },
 	{ "_stext_l1",  "_etext_l1"  },	/* Blackfin on-chip L1 inst SRAM */
 	{ "_stext_l2",  "_etext_l2"  },	/* Blackfin on-chip L2 SRAM */
+#ifdef CONFIG_HARDENED_USERCOPY
+	{ "_sdata",	"_edata"     },
+	{ "__bss_start", "__bss_stop" },
+	{ "__start_rodata", "__end_rodata" },
+#endif
 };
 #define text_range_text     (&text_ranges[0])
 #define text_range_inittext (&text_ranges[1])

but I fear that if you actually try that, things start falling apart
in a big way, so I didn't try ;-)

> I wonder if there are any cases where there are
> legitimate usercopys across multiple symbols.

The only possible use case I can think of is for reading out the entire
kernel memory from /dev/kmem, but your other checks in here already
define that as illegitimate. On that subject, we probably want to
make CONFIG_DEVKMEM mutually exclusive with CONFIG_HARDENED_USERCOPY.

	Arnd

[PATCH 0/9] mm: Hardened usercopy

[Linus Torvalds]

On Fri, Jul 8, 2016 at 1:46 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> Could you please try to find some syscall workload that does many small user
> copies and thus excercises this code path aggressively?

Any stat()-heavy path will hit cp_new_stat() very heavily. Think the
usual kind of "traverse the whole tree looking for something". "git
diff" will do it, just checking that everything is up-to-date.

That said, other things tend to dominate.

                 Linus

[PATCH 0/9] mm: Hardened usercopy

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Fri, Jul 8, 2016 at 1:46 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > Could you please try to find some syscall workload that does many small user
> > copies and thus excercises this code path aggressively?
> 
> Any stat()-heavy path will hit cp_new_stat() very heavily. Think the
> usual kind of "traverse the whole tree looking for something". "git
> diff" will do it, just checking that everything is up-to-date.
> 
> That said, other things tend to dominate.

So I think a cached 'find /usr >/dev/null' might be a good one as well:

 triton:~/tip> strace -c find /usr >/dev/null
 % time     seconds  usecs/call     calls    errors syscall
 ------ ----------- ----------- --------- --------- ----------------
  47.09    0.006518           0    254697           newfstatat
  26.20    0.003627           0    254795           getdents
  14.45    0.002000           0   1147411           fcntl
   7.33    0.001014           0    509811           close
   3.28    0.000454           0    128220         1 openat
   1.52    0.000210           0    128230           fstat
   0.27    0.000016           0     12810           write
   0.00    0.000000           0        10           read

 triton:~/tip> perf stat --repeat 3 -e cycles:u,cycles:k,cycles find /usr >/dev/null

 Performance counter stats for 'find /usr' (3 runs):

     1,594,437,143      cycles:u                                                      ( +-  2.76% )
     2,570,544,009      cycles:k                                                      ( +-  2.50% )
     4,164,981,152      cycles                                                        ( +-  2.59% )

       0.929883686 seconds time elapsed                                          ( +-  2.57% )

... and it's dominated by kernel overhead, with a fair amount of memcpy overhead 
as well:

   1.22%  find     [kernel.kallsyms]   [k] copy_user_enhanced_fast_string                                                                                                            

But maybe there are simple shell commands that are even more user-memcpy intense? 

Thanks,

	Ingo

[PATCH 0/9] mm: Hardened usercopy

[Rik van Riel]

On Fri, 2016-07-08 at 19:22 -0700, Laura Abbott wrote:
>?
> Even with the SLUB fixup I'm still seeing this blow up on my arm64
> system. This is a
> Fedora rawhide kernel + the patches
> 
> [????0.666700] usercopy: kernel memory exposure attempt detected from
> fffffc0008b4dd58 (<kernel text>) (8 bytes)
> [????0.666720] CPU: 2 PID: 79 Comm: modprobe Tainted:
> G????????W???????4.7.0-0.rc6.git1.1.hardenedusercopy.fc25.aarch64 #1
> [????0.666733] Hardware name: AppliedMicro Mustang/Mustang, BIOS
> 1.1.0 Nov 24 2015
> [????0.666744] Call trace:
> [????0.666756] [<fffffc0008088a20>] dump_backtrace+0x0/0x1e8
> [????0.666765] [<fffffc0008088c2c>] show_stack+0x24/0x30
> [????0.666775] [<fffffc0008455344>] dump_stack+0xa4/0xe0
> [????0.666785] [<fffffc000828d874>] __check_object_size+0x6c/0x230
> [????0.666795] [<fffffc00083a5748>] create_elf_tables+0x74/0x420
> [????0.666805] [<fffffc00082fb1f0>] load_elf_binary+0x828/0xb70
> [????0.666814] [<fffffc0008298b4c>] search_binary_handler+0xb4/0x240
> [????0.666823] [<fffffc0008299864>] do_execveat_common+0x63c/0x950
> [????0.666832] [<fffffc0008299bb4>] do_execve+0x3c/0x50
> [????0.666841] [<fffffc00080e3720>]
> call_usermodehelper_exec_async+0xe8/0x148
> [????0.666850] [<fffffc0008084a80>] ret_from_fork+0x10/0x50
> 
> This happens on every call to execve. This seems to be the first
> copy_to_user in
> create_elf_tables. I didn't get a chance to debug and I'm going out
> of town
> all of next week so all I have is the report unfortunately. config
> attached.

That's odd, this should be copying a piece of kernel data (not text)
to userspace.

from fs/binfmt_elf.c

? ? ? ? const char *k_platform = ELF_PLATFORM;

...
? ? ? ? ? ? ? ? size_t len = strlen(k_platform) + 1;
		
? ? ? ? ? ? ? ? u_platform = (elf_addr_t __user *)STACK_ALLOC(p, len);
????????????????if (__copy_to_user(u_platform, k_platform, len))
????????????????????????return -EFAULT;

from arch/arm/include/asm/elf.h:

#define ELF_PLATFORM_SIZE 8
#define ELF_PLATFORM????(elf_platform)

extern char elf_platform[];

from arch/arm/kernel/setup.c:

char elf_platform[ELF_PLATFORM_SIZE];
EXPORT_SYMBOL(elf_platform);

...

????????snprintf(elf_platform, ELF_PLATFORM_SIZE, "%s%c",
?????????????????list->elf_name, ENDIANNESS);

How does that end up in the .text section of the
image, instead of in one of the various data sections?

What kind of linker oddity is going on with ARM?

--?		
All Rights Reversed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160708/488a1f5e/attachment.sig>

[PATCH 0/9] mm: Hardened usercopy

[Ingo Molnar]

* Rik van Riel <riel@redhat.com> wrote:

> On Fri, 2016-07-08 at 19:22 -0700, Laura Abbott wrote:
> >?
> > Even with the SLUB fixup I'm still seeing this blow up on my arm64
> > system. This is a
> > Fedora rawhide kernel + the patches
> > 
> > [????0.666700] usercopy: kernel memory exposure attempt detected from
> > fffffc0008b4dd58 (<kernel text>) (8 bytes)
> > [????0.666720] CPU: 2 PID: 79 Comm: modprobe Tainted:
> > G????????W???????4.7.0-0.rc6.git1.1.hardenedusercopy.fc25.aarch64 #1
> > [????0.666733] Hardware name: AppliedMicro Mustang/Mustang, BIOS
> > 1.1.0 Nov 24 2015
> > [????0.666744] Call trace:
> > [????0.666756] [<fffffc0008088a20>] dump_backtrace+0x0/0x1e8
> > [????0.666765] [<fffffc0008088c2c>] show_stack+0x24/0x30
> > [????0.666775] [<fffffc0008455344>] dump_stack+0xa4/0xe0
> > [????0.666785] [<fffffc000828d874>] __check_object_size+0x6c/0x230
> > [????0.666795] [<fffffc00083a5748>] create_elf_tables+0x74/0x420
> > [????0.666805] [<fffffc00082fb1f0>] load_elf_binary+0x828/0xb70
> > [????0.666814] [<fffffc0008298b4c>] search_binary_handler+0xb4/0x240
> > [????0.666823] [<fffffc0008299864>] do_execveat_common+0x63c/0x950
> > [????0.666832] [<fffffc0008299bb4>] do_execve+0x3c/0x50
> > [????0.666841] [<fffffc00080e3720>]
> > call_usermodehelper_exec_async+0xe8/0x148
> > [????0.666850] [<fffffc0008084a80>] ret_from_fork+0x10/0x50
> > 
> > This happens on every call to execve. This seems to be the first
> > copy_to_user in
> > create_elf_tables. I didn't get a chance to debug and I'm going out
> > of town
> > all of next week so all I have is the report unfortunately. config
> > attached.
> 
> That's odd, this should be copying a piece of kernel data (not text)
> to userspace.
> 
> from fs/binfmt_elf.c
> 
> ? ? ? ? const char *k_platform = ELF_PLATFORM;
> 
> ...
> ? ? ? ? ? ? ? ? size_t len = strlen(k_platform) + 1;
> 		
> ? ? ? ? ? ? ? ? u_platform = (elf_addr_t __user *)STACK_ALLOC(p, len);
> ????????????????if (__copy_to_user(u_platform, k_platform, len))
> ????????????????????????return -EFAULT;
> 
> from arch/arm/include/asm/elf.h:
> 
> #define ELF_PLATFORM_SIZE 8
> #define ELF_PLATFORM????(elf_platform)
> 
> extern char elf_platform[];
> 
> from arch/arm/kernel/setup.c:
> 
> char elf_platform[ELF_PLATFORM_SIZE];
> EXPORT_SYMBOL(elf_platform);
> 
> ...
> 
> ????????snprintf(elf_platform, ELF_PLATFORM_SIZE, "%s%c",
> ?????????????????list->elf_name, ENDIANNESS);
> 
> How does that end up in the .text section of the
> image, instead of in one of the various data sections?
> 
> What kind of linker oddity is going on with ARM?

I think the crash happened on ARM64, not ARM.

Thanks,

	Ingo

[PATCH 0/9] mm: Hardened usercopy

[Ard Biesheuvel]

On 9 July 2016 at 04:22, Laura Abbott <labbott@redhat.com> wrote:
> On 07/06/2016 03:25 PM, Kees Cook wrote:
>>
>> Hi,
>>
>> This is a start of the mainline port of PAX_USERCOPY[1]. After I started
>> writing tests (now in lkdtm in -next) for Casey's earlier port[2], I
>> kept tweaking things further and further until I ended up with a whole
>> new patch series. To that end, I took Rik's feedback and made a number
>> of other changes and clean-ups as well.
>>
>> Based on my understanding, PAX_USERCOPY was designed to catch a few
>> classes of flaws around the use of copy_to_user()/copy_from_user(). These
>> changes don't touch get_user() and put_user(), since these operate on
>> constant sized lengths, and tend to be much less vulnerable. There
>> are effectively three distinct protections in the whole series,
>> each of which I've given a separate CONFIG, though this patch set is
>> only the first of the three intended protections. (Generally speaking,
>> PAX_USERCOPY covers what I'm calling CONFIG_HARDENED_USERCOPY (this) and
>> CONFIG_HARDENED_USERCOPY_WHITELIST (future), and PAX_USERCOPY_SLABS covers
>> CONFIG_HARDENED_USERCOPY_SPLIT_KMALLOC (future).)
>>
>> This series, which adds CONFIG_HARDENED_USERCOPY, checks that objects
>> being copied to/from userspace meet certain criteria:
>> - if address is a heap object, the size must not exceed the object's
>>   allocated size. (This will catch all kinds of heap overflow flaws.)
>> - if address range is in the current process stack, it must be within the
>>   current stack frame (if such checking is possible) or at least entirely
>>   within the current process's stack. (This could catch large lengths that
>>   would have extended beyond the current process stack, or overflows if
>>   their length extends back into the original stack.)
>> - if the address range is part of kernel data, rodata, or bss, allow it.
>> - if address range is page-allocated, that it doesn't span multiple
>>   allocations.
>> - if address is within the kernel text, reject it.
>> - everything else is accepted
>>
>> The patches in the series are:
>> - The core copy_to/from_user() checks, without the slab object checks:
>>         1- mm: Hardened usercopy
>> - Per-arch enablement of the protection:
>>         2- x86/uaccess: Enable hardened usercopy
>>         3- ARM: uaccess: Enable hardened usercopy
>>         4- arm64/uaccess: Enable hardened usercopy
>>         5- ia64/uaccess: Enable hardened usercopy
>>         6- powerpc/uaccess: Enable hardened usercopy
>>         7- sparc/uaccess: Enable hardened usercopy
>> - The heap allocator implementation of object size checking:
>>         8- mm: SLAB hardened usercopy support
>>         9- mm: SLUB hardened usercopy support
>>
>> Some notes:
>>
>> - This is expected to apply on top of -next which contains fixes for the
>>   position of _etext on both arm and arm64.
>>
>> - I couldn't detect a measurable performance change with these features
>>   enabled. Kernel build times were unchanged, hackbench was unchanged,
>>   etc. I think we could flip this to "on by default" at some point.
>>
>> - The SLOB support extracted from grsecurity seems entirely broken. I
>>   have no idea what's going on there, I spent my time testing SLAB and
>>   SLUB. Having someone else look at SLOB would be nice, but this series
>>   doesn't depend on it.
>>
>> Additional features that would be nice, but aren't blocking this series:
>>
>> - Needs more architecture support for stack frame checking (only x86 now).
>>
>>
>
> Even with the SLUB fixup I'm still seeing this blow up on my arm64 system.
> This is a
> Fedora rawhide kernel + the patches
>
> [ 0.666700] usercopy: kernel memory exposure attempt detected from
> fffffc0008b4dd58 (<kernel text>) (8 bytes)
> [ 0.666720] CPU: 2 PID: 79 Comm: modprobe Tainted: G        W
> 4.7.0-0.rc6.git1.1.hardenedusercopy.fc25.aarch64 #1
> [ 0.666733] Hardware name: AppliedMicro Mustang/Mustang, BIOS 1.1.0 Nov 24
> 2015
> [ 0.666744] Call trace:
> [ 0.666756] [<fffffc0008088a20>] dump_backtrace+0x0/0x1e8
> [ 0.666765] [<fffffc0008088c2c>] show_stack+0x24/0x30
> [ 0.666775] [<fffffc0008455344>] dump_stack+0xa4/0xe0
> [ 0.666785] [<fffffc000828d874>] __check_object_size+0x6c/0x230
> [ 0.666795] [<fffffc00083a5748>] create_elf_tables+0x74/0x420
> [ 0.666805] [<fffffc00082fb1f0>] load_elf_binary+0x828/0xb70
> [ 0.666814] [<fffffc0008298b4c>] search_binary_handler+0xb4/0x240
> [ 0.666823] [<fffffc0008299864>] do_execveat_common+0x63c/0x950
> [ 0.666832] [<fffffc0008299bb4>] do_execve+0x3c/0x50
> [ 0.666841] [<fffffc00080e3720>] call_usermodehelper_exec_async+0xe8/0x148
> [ 0.666850] [<fffffc0008084a80>] ret_from_fork+0x10/0x50
>
> This happens on every call to execve. This seems to be the first
> copy_to_user in
> create_elf_tables. I didn't get a chance to debug and I'm going out of town
> all of next week so all I have is the report unfortunately. config attached.
>

This is a known issue, and a fix is already queued for v4.8 in the arm64 tree:

9fdc14c55c arm64: mm: fix location of _etext [0]

which moves _etext up in the linker script so that it does not cover .rodata

ARM was suffering from the same problem, and Kees proposed a fix for
it. I don't know what the status of that patch is, though.

Note that on arm64, we have

  #define ELF_PLATFORM            ("aarch64")

which explains why k_platform points into .rodata in this case. On
ARM, it points to a writable string (as the code quoted by Rik shows),
so there it will likely explode elsewhere without the linker script
fix.

[0] https://git.kernel.org/cgit/linux/kernel/git/arm64/linux.git/commit/?h=for-next/core&id=9fdc14c55c

-- 
Ard.

[PATCH 0/9] mm: Hardened usercopy

[Kees Cook]

On Fri, Jul 8, 2016 at 7:22 PM, Laura Abbott <labbott@redhat.com> wrote:
> On 07/06/2016 03:25 PM, Kees Cook wrote:
>>
>> Hi,
>>
>> This is a start of the mainline port of PAX_USERCOPY[1]. After I started
>> writing tests (now in lkdtm in -next) for Casey's earlier port[2], I
>> kept tweaking things further and further until I ended up with a whole
>> new patch series. To that end, I took Rik's feedback and made a number
>> of other changes and clean-ups as well.
>>
>> Based on my understanding, PAX_USERCOPY was designed to catch a few
>> classes of flaws around the use of copy_to_user()/copy_from_user(). These
>> changes don't touch get_user() and put_user(), since these operate on
>> constant sized lengths, and tend to be much less vulnerable. There
>> are effectively three distinct protections in the whole series,
>> each of which I've given a separate CONFIG, though this patch set is
>> only the first of the three intended protections. (Generally speaking,
>> PAX_USERCOPY covers what I'm calling CONFIG_HARDENED_USERCOPY (this) and
>> CONFIG_HARDENED_USERCOPY_WHITELIST (future), and PAX_USERCOPY_SLABS covers
>> CONFIG_HARDENED_USERCOPY_SPLIT_KMALLOC (future).)
>>
>> This series, which adds CONFIG_HARDENED_USERCOPY, checks that objects
>> being copied to/from userspace meet certain criteria:
>> - if address is a heap object, the size must not exceed the object's
>>   allocated size. (This will catch all kinds of heap overflow flaws.)
>> - if address range is in the current process stack, it must be within the
>>   current stack frame (if such checking is possible) or at least entirely
>>   within the current process's stack. (This could catch large lengths that
>>   would have extended beyond the current process stack, or overflows if
>>   their length extends back into the original stack.)
>> - if the address range is part of kernel data, rodata, or bss, allow it.
>> - if address range is page-allocated, that it doesn't span multiple
>>   allocations.
>> - if address is within the kernel text, reject it.
>> - everything else is accepted
>>
>> The patches in the series are:
>> - The core copy_to/from_user() checks, without the slab object checks:
>>         1- mm: Hardened usercopy
>> - Per-arch enablement of the protection:
>>         2- x86/uaccess: Enable hardened usercopy
>>         3- ARM: uaccess: Enable hardened usercopy
>>         4- arm64/uaccess: Enable hardened usercopy
>>         5- ia64/uaccess: Enable hardened usercopy
>>         6- powerpc/uaccess: Enable hardened usercopy
>>         7- sparc/uaccess: Enable hardened usercopy
>> - The heap allocator implementation of object size checking:
>>         8- mm: SLAB hardened usercopy support
>>         9- mm: SLUB hardened usercopy support
>>
>> Some notes:
>>
>> - This is expected to apply on top of -next which contains fixes for the
>>   position of _etext on both arm and arm64.
>>
>> - I couldn't detect a measurable performance change with these features
>>   enabled. Kernel build times were unchanged, hackbench was unchanged,
>>   etc. I think we could flip this to "on by default" at some point.
>>
>> - The SLOB support extracted from grsecurity seems entirely broken. I
>>   have no idea what's going on there, I spent my time testing SLAB and
>>   SLUB. Having someone else look at SLOB would be nice, but this series
>>   doesn't depend on it.
>>
>> Additional features that would be nice, but aren't blocking this series:
>>
>> - Needs more architecture support for stack frame checking (only x86 now).
>>
>>
>
> Even with the SLUB fixup I'm still seeing this blow up on my arm64 system.
> This is a
> Fedora rawhide kernel + the patches

Is this on top of -next? The recent _etext change ("arm64: mm: fix
location of _etext") is needed to fix the kernel text test for arm64.

-Kees

>
> [    0.666700] usercopy: kernel memory exposure attempt detected from
> fffffc0008b4dd58 (<kernel text>) (8 bytes)
> [    0.666720] CPU: 2 PID: 79 Comm: modprobe Tainted: G        W
> 4.7.0-0.rc6.git1.1.hardenedusercopy.fc25.aarch64 #1
> [    0.666733] Hardware name: AppliedMicro Mustang/Mustang, BIOS 1.1.0 Nov
> 24 2015
> [    0.666744] Call trace:
> [    0.666756] [<fffffc0008088a20>] dump_backtrace+0x0/0x1e8
> [    0.666765] [<fffffc0008088c2c>] show_stack+0x24/0x30
> [    0.666775] [<fffffc0008455344>] dump_stack+0xa4/0xe0
> [    0.666785] [<fffffc000828d874>] __check_object_size+0x6c/0x230
> [    0.666795] [<fffffc00083a5748>] create_elf_tables+0x74/0x420
> [    0.666805] [<fffffc00082fb1f0>] load_elf_binary+0x828/0xb70
> [    0.666814] [<fffffc0008298b4c>] search_binary_handler+0xb4/0x240
> [    0.666823] [<fffffc0008299864>] do_execveat_common+0x63c/0x950
> [    0.666832] [<fffffc0008299bb4>] do_execve+0x3c/0x50
> [    0.666841] [<fffffc00080e3720>]
> call_usermodehelper_exec_async+0xe8/0x148
> [    0.666850] [<fffffc0008084a80>] ret_from_fork+0x10/0x50
>
> This happens on every call to execve. This seems to be the first
> copy_to_user in
> create_elf_tables. I didn't get a chance to debug and I'm going out of town
> all of next week so all I have is the report unfortunately. config attached.
>
> Thanks,
> Laura



-- 
Kees Cook
Chrome OS & Brillo Security

[PATCH 0/9] mm: Hardened usercopy

[Kees Cook]

On Sat, Jul 9, 2016 at 1:25 AM, Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
> On 9 July 2016 at 04:22, Laura Abbott <labbott@redhat.com> wrote:
>> On 07/06/2016 03:25 PM, Kees Cook wrote:
>>>
>>> Hi,
>>>
>>> This is a start of the mainline port of PAX_USERCOPY[1]. After I started
>>> writing tests (now in lkdtm in -next) for Casey's earlier port[2], I
>>> kept tweaking things further and further until I ended up with a whole
>>> new patch series. To that end, I took Rik's feedback and made a number
>>> of other changes and clean-ups as well.
>>>
>>> Based on my understanding, PAX_USERCOPY was designed to catch a few
>>> classes of flaws around the use of copy_to_user()/copy_from_user(). These
>>> changes don't touch get_user() and put_user(), since these operate on
>>> constant sized lengths, and tend to be much less vulnerable. There
>>> are effectively three distinct protections in the whole series,
>>> each of which I've given a separate CONFIG, though this patch set is
>>> only the first of the three intended protections. (Generally speaking,
>>> PAX_USERCOPY covers what I'm calling CONFIG_HARDENED_USERCOPY (this) and
>>> CONFIG_HARDENED_USERCOPY_WHITELIST (future), and PAX_USERCOPY_SLABS covers
>>> CONFIG_HARDENED_USERCOPY_SPLIT_KMALLOC (future).)
>>>
>>> This series, which adds CONFIG_HARDENED_USERCOPY, checks that objects
>>> being copied to/from userspace meet certain criteria:
>>> - if address is a heap object, the size must not exceed the object's
>>>   allocated size. (This will catch all kinds of heap overflow flaws.)
>>> - if address range is in the current process stack, it must be within the
>>>   current stack frame (if such checking is possible) or at least entirely
>>>   within the current process's stack. (This could catch large lengths that
>>>   would have extended beyond the current process stack, or overflows if
>>>   their length extends back into the original stack.)
>>> - if the address range is part of kernel data, rodata, or bss, allow it.
>>> - if address range is page-allocated, that it doesn't span multiple
>>>   allocations.
>>> - if address is within the kernel text, reject it.
>>> - everything else is accepted
>>>
>>> The patches in the series are:
>>> - The core copy_to/from_user() checks, without the slab object checks:
>>>         1- mm: Hardened usercopy
>>> - Per-arch enablement of the protection:
>>>         2- x86/uaccess: Enable hardened usercopy
>>>         3- ARM: uaccess: Enable hardened usercopy
>>>         4- arm64/uaccess: Enable hardened usercopy
>>>         5- ia64/uaccess: Enable hardened usercopy
>>>         6- powerpc/uaccess: Enable hardened usercopy
>>>         7- sparc/uaccess: Enable hardened usercopy
>>> - The heap allocator implementation of object size checking:
>>>         8- mm: SLAB hardened usercopy support
>>>         9- mm: SLUB hardened usercopy support
>>>
>>> Some notes:
>>>
>>> - This is expected to apply on top of -next which contains fixes for the
>>>   position of _etext on both arm and arm64.
>>>
>>> - I couldn't detect a measurable performance change with these features
>>>   enabled. Kernel build times were unchanged, hackbench was unchanged,
>>>   etc. I think we could flip this to "on by default" at some point.
>>>
>>> - The SLOB support extracted from grsecurity seems entirely broken. I
>>>   have no idea what's going on there, I spent my time testing SLAB and
>>>   SLUB. Having someone else look at SLOB would be nice, but this series
>>>   doesn't depend on it.
>>>
>>> Additional features that would be nice, but aren't blocking this series:
>>>
>>> - Needs more architecture support for stack frame checking (only x86 now).
>>>
>>>
>>
>> Even with the SLUB fixup I'm still seeing this blow up on my arm64 system.
>> This is a
>> Fedora rawhide kernel + the patches
>>
>> [ 0.666700] usercopy: kernel memory exposure attempt detected from
>> fffffc0008b4dd58 (<kernel text>) (8 bytes)
>> [ 0.666720] CPU: 2 PID: 79 Comm: modprobe Tainted: G        W
>> 4.7.0-0.rc6.git1.1.hardenedusercopy.fc25.aarch64 #1
>> [ 0.666733] Hardware name: AppliedMicro Mustang/Mustang, BIOS 1.1.0 Nov 24
>> 2015
>> [ 0.666744] Call trace:
>> [ 0.666756] [<fffffc0008088a20>] dump_backtrace+0x0/0x1e8
>> [ 0.666765] [<fffffc0008088c2c>] show_stack+0x24/0x30
>> [ 0.666775] [<fffffc0008455344>] dump_stack+0xa4/0xe0
>> [ 0.666785] [<fffffc000828d874>] __check_object_size+0x6c/0x230
>> [ 0.666795] [<fffffc00083a5748>] create_elf_tables+0x74/0x420
>> [ 0.666805] [<fffffc00082fb1f0>] load_elf_binary+0x828/0xb70
>> [ 0.666814] [<fffffc0008298b4c>] search_binary_handler+0xb4/0x240
>> [ 0.666823] [<fffffc0008299864>] do_execveat_common+0x63c/0x950
>> [ 0.666832] [<fffffc0008299bb4>] do_execve+0x3c/0x50
>> [ 0.666841] [<fffffc00080e3720>] call_usermodehelper_exec_async+0xe8/0x148
>> [ 0.666850] [<fffffc0008084a80>] ret_from_fork+0x10/0x50
>>
>> This happens on every call to execve. This seems to be the first
>> copy_to_user in
>> create_elf_tables. I didn't get a chance to debug and I'm going out of town
>> all of next week so all I have is the report unfortunately. config attached.
>>
>
> This is a known issue, and a fix is already queued for v4.8 in the arm64 tree:
>
> 9fdc14c55c arm64: mm: fix location of _etext [0]
>
> which moves _etext up in the linker script so that it does not cover .rodata

Oops, I missed this reply, sorry for the redundant answer. :)

> ARM was suffering from the same problem, and Kees proposed a fix for
> it. I don't know what the status of that patch is, though.

This is also in -next "ARM: 8583/1: mm: fix location of _etext".

> Note that on arm64, we have
>
>   #define ELF_PLATFORM            ("aarch64")
>
> which explains why k_platform points into .rodata in this case. On
> ARM, it points to a writable string (as the code quoted by Rik shows),
> so there it will likely explode elsewhere without the linker script
> fix.
>
> [0] https://git.kernel.org/cgit/linux/kernel/git/arm64/linux.git/commit/?h=for-next/core&id=9fdc14c55c

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[PATCH 0/9] mm: Hardened usercopy

[Andy Lutomirski]

On Jul 6, 2016 6:25 PM, "Kees Cook" <keescook@chromium.org> wrote:
>
> Hi,
>
> This is a start of the mainline port of PAX_USERCOPY[1]. After I started
> writing tests (now in lkdtm in -next) for Casey's earlier port[2], I
> kept tweaking things further and further until I ended up with a whole
> new patch series. To that end, I took Rik's feedback and made a number
> of other changes and clean-ups as well.
>

I like the series, but I have one minor nit to pick.  The effect of
this series is to harden usercopy, but most of the code is really
about infrastructure to validate that a pointed-to object is valid.
Might it make sense to call the infrastructure part something else?
After all, this could be extended in the future for memcpy or even for
some GCC plugin to check pointers passed to ordinary (non-allocator)
functions.

[PATCH 0/9] mm: Hardened usercopy

[PaX Team]

On 9 Jul 2016 at 14:27, Andy Lutomirski wrote:

> On Jul 6, 2016 6:25 PM, "Kees Cook" <keescook@chromium.org> wrote:
> >
> > Hi,
> >
> > This is a start of the mainline port of PAX_USERCOPY[1]. After I started
> > writing tests (now in lkdtm in -next) for Casey's earlier port[2], I
> > kept tweaking things further and further until I ended up with a whole
> > new patch series. To that end, I took Rik's feedback and made a number
> > of other changes and clean-ups as well.
> >
> 
> I like the series, but I have one minor nit to pick.  The effect of
> this series is to harden usercopy, but most of the code is really
> about infrastructure to validate that a pointed-to object is valid.

actually USERCOPY has never been about validating pointers. its sole purpose
is to validate the *size* argument of copy*user calls, a very specific form
of runtime bounds checking. it's only really relevant for slab objects and the
pointer checks (that one might mistake for being a part of the defense mechanism)
are only there to determine whether the kernel pointer refers to a slab object
or not (the stack part is a small bonus and was never the main goal either).

> Might it make sense to call the infrastructure part something else?

yes, more bikeshedding will surely help, like the renaming of .data..read_only
to .data..ro_after_init which also had nothing to do with init but everything
to do with objects being conceptually read-only...

> After all, this could be extended in the future for memcpy or even for
> some GCC plugin to check pointers passed to ordinary (non-allocator)
> functions.

what kind of checks are you thinking of here? and more fundamentally, against
what kind of threats? as for memcpy, it's the standard mandated memory copying
function, what security related properties can it check on its pointer arguments?

[PATCH 0/9] mm: Hardened usercopy

[Ingo Molnar]

* PaX Team <pageexec@freemail.hu> wrote:

> On 9 Jul 2016 at 14:27, Andy Lutomirski wrote:
> 
> > I like the series, but I have one minor nit to pick.  The effect of this 
> > series is to harden usercopy, but most of the code is really about 
> > infrastructure to validate that a pointed-to object is valid.
> 
> actually USERCOPY has never been about validating pointers. its sole purpose is 
> to validate the *size* argument of copy*user calls, a very specific form of 
> runtime bounds checking.

What this code has been about originally is largely immaterial, unless you can 
formulate it into a technical argument.

There are a number of cheap tests we can do and there are a number of ways how a 
'pointer' can be validated runtime, without any 'size' information:

 - for example if a pointer points into a red zone straight away then we know it's
   bogus.

 - or if a kernel pointer is points outside the valid kernel virtual memory range
   we know it's bogus as well.

So while only doing a bounds check might have been the original purpose of the 
patch set, Andy's point is that it might make sense to treat this facility as a 
more generic 'object validation' code of (pointer,size) object and not limit it to 
'runtime bounds checking'. That kind of extended purpose behind a facility should 
be reflected in the naming.

Confusing names are often the source of misunderstandings and bugs.

The 9-patch series as submitted here is neither just 'bounds checking' nor just 
pure 'pointer checking', it's about validating that a (pointer,size) range of 
memory passed to a (user) memory copy function is fully within a valid object the 
kernel might know about (in an fast to check fashion).

This necessary means:

 - the start of the range points to a valid object to begin with (if known)

 - the range itself does not point beyond the end of the object (if known)

 - even if the kernel does not know anything about the pointed to object it can 
   do a pointer check (for example is it pointing inside kernel virtual memory) 
   and do a bounds check on the size.

Do you disagree with that?

> > Might it make sense to call the infrastructure part something else?
> 
> yes, more bikeshedding will surely help, [...]

Insulting and ridiculing a reviewer who explicitly qualified his comments with 
"one minor nit to pick" sure does not help upstream integration either. (Unless 
the goal is to prevent upstream integration.)

> [...] like the renaming of .data..read_only to .data..ro_after_init which also 
> had nothing to do with init but everything to do with objects being conceptually 
> read-only...

.data..ro_after_init objects get written to during bootup so it's conceptually 
quite confusing to name it "read-only" without any clear qualifiers.

That it's named consistently with its role of "read-write before init and read 
only after init" on the other hand is not confusing at all. Not sure what your 
problem is with the new name.

Names within submitted patches get renamed on a routine basis during review. It's 
often only minor improvements in naming (which you can consider bike shedding), 
but in this particular case the rename was clearly useful in not just improving 
the name but in avoiding an actively confusing name. So I disagree not just with 
the hostile tone of your reply but with your underlying technical point as well.

Thanks,

	Ingo

[PATCH 0/9] mm: Hardened usercopy

[PaX Team]

On 10 Jul 2016 at 11:16, Ingo Molnar wrote:

> * PaX Team <pageexec@freemail.hu> wrote:
> 
> > On 9 Jul 2016 at 14:27, Andy Lutomirski wrote:
> > 
> > > I like the series, but I have one minor nit to pick.  The effect of this 
> > > series is to harden usercopy, but most of the code is really about 
> > > infrastructure to validate that a pointed-to object is valid.
> > 
> > actually USERCOPY has never been about validating pointers. its sole purpose is 
> > to validate the *size* argument of copy*user calls, a very specific form of 
> > runtime bounds checking.
> 
> What this code has been about originally is largely immaterial, unless you can 
> formulate it into a technical argument.

we design defense mechanisms for specific and clear purposes, starting with
a threat model, evaluating defense options based on various criteria, etc.
USERCOPY underwent this same process and taking it out of its original context
means that all you get in the end is cargo cult security (wouldn't be the first
time it has happened (ExecShield, ASLR, etc)).

that said, i actually started that discussion but for some reason you chose
not to respond to that one part of my mail so let me ask it again:

  what kind of checks are you thinking of here? and more fundamentally, against
  what kind of threats?

as far as i'm concerned, a defense mechanism is only as good as its underlying
threat model. by validating pointers (for yet to be stated security related
properties) you're presumably assuming some kind of threat and unless stated
clearly what that threat is (unintended pointer modification through memory
corruption and/or other bugs?) noone can tell whether the proposed defense
mechanism will actually be effective in preventing exploitation. it is the
worst kind of defense that doesn't actually achieve its stated goals, that
way lies false sense of security and i hope noone here is in that business.

i note that this analysis is also missing from this USERCOPY submission except
for stating what Kees assumed about USERCOPY (and apparently noone could be
bothered to read the original Kconfig help of it which clearly states that the
purpose is copy size checking, not some elaborate pointer validation, the latter
is an implementation detail only and is necessary to be able to derive the
underlying slab object's intended size).

> There are a number of cheap tests we can do and there are a number of ways how a 
> 'pointer' can be validated runtime, without any 'size' information:
> 
>  - for example if a pointer points into a red zone straight away then we know it's
>    bogus.

it's not pointer validation but bounds checking: you already know which memory
object the pointer is supposed to point to, you only check its bounds. if it was
an attacker controlled pointer then all this would be a pointless check of course,
trivial for an attacker to circumvent (and this is why it's not part of the
USERCOPY design).

>  - or if a kernel pointer is points outside the valid kernel virtual memory range
>    we know it's bogus as well.

accesses outside of valid virtual memory will cause a page fault ('oops' in linux
terms), there's no need to explicitly check for that.

> So while only doing a bounds check might have been the original purpose of the 
> patch set, Andy's point is that it might make sense to treat this facility as a 
> more generic 'object validation' code of (pointer,size) object and not limit it to 
> 'runtime bounds checking'.

FYI, 'runtime bounds checking' is a terminus technicus and it is about validating
both the pointer and underlying object's size. that's the reason i called USERCOPY
a 'very specific form' of it only since it doesn't validate each part equally well
(or well enough at all, even the size check is not as precise as it could be).

as for what does or doesn't make sense, first you'll have to define a threat
model and evaluate everything else based on that. since noone has solved the
general bounds checking problem with acceptable properties (mostly performance
impact, but also memory overhead, etc), i'm all ears to hear what you guys have
come up with.

> That kind of extended purpose behind a facility should be reflected in the naming.
> Confusing names are often the source of misunderstandings and bugs.

definitely, but before you bikeshed on naming, you should figure out what and why
you want to do, whether it's even feasible, meaningful, useful, etc. answering the
opening question and digging into the details is the first step of any design
process, not its naming.

> The 9-patch series as submitted here is neither just 'bounds checking' nor just 
> pure 'pointer checking', it's about validating that a (pointer,size) range of 
> memory passed to a (user) memory copy function is fully within a valid object the 
> kernel might know about (in an fast to check fashion).
> 
> This necessary means:
> 
>  - the start of the range points to a valid object to begin with (if known)
> 
>  - the range itself does not point beyond the end of the object (if known)
> 
>  - even if the kernel does not know anything about the pointed to object it can 
>    do a pointer check (for example is it pointing inside kernel virtual memory) 
>    and do a bounds check on the size.
> 
> Do you disagree with that?

as i explained above, you're confusing implementation with design: USERCOPY is
about size checking, not pointer validation. if you want to do the latter as well,
you'll have to first define a threat model, etc. so the answer is 'it depends'
but as the current implementation stands, it's circumventible if an attacker
can control the pointer (which has to be assumed otherwise there's no reason
to validate the pointer, right?).

> > > Might it make sense to call the infrastructure part something else?
> > 
> > yes, more bikeshedding will surely help, [...]
> 
> Insulting and ridiculing a reviewer who explicitly qualified his comments with 
> "one minor nit to pick" sure does not help upstream integration either.

sorry Ingo, but calling a spade a spade isn't insulting, at best it's exposing
some painful truth. you yourself used that term several times in the past, were
you insulting and ridiculing people then?

as for the ad hominem that you displayed here and later, i hope that in the
future you will display the same professional conduct that you apparently expect
from others.

> (Unless the goal is to prevent upstream integration.)

not sure how a properly licensed patch can be prevented from such integration
(as long as you comply with the license, e.g., acknowledge our copyright), but
i'll voice my opinion when you guys are about to screw it up (as it happened in
the past and apparently history keeps repeating itself). if you don't want my
opinion then don't ask for it (in that case we'll write a blog at most ;).

> > [...] like the renaming of .data..read_only to .data..ro_after_init which also 
> > had nothing to do with init but everything to do with objects being conceptually 
> > read-only...
> 
> .data..ro_after_init objects get written to during bootup so it's conceptually 
> quite confusing to name it "read-only" without any clear qualifiers.
> 
> That it's named consistently with its role of "read-write before init and read 
> only after init" on the other hand is not confusing at all. Not sure what your 
> problem is with the new name.

the new name reflects a complete misunderstanding of the PaX feature it was based
on (typical case of cargo cult security). in particular, the __read_only facility
in PaX is part of a defense mechanism that attempts to solve a specific problem
(like everything else) and that problem has nothing whatsoever to do with what
happens before/after the kernel init process. enforcing read-ony kernel memory at
the end of kernel initialization is an implementation detail only and wasn't even
true always (and still isn't true for kernel modules for example): in the linux 2.4
days PaX actually enforced read-only kernel memory properties in startup_32 already
but i relaxed that for the 2.6+ port as the maintenance cost (finding out and
handling new exceptional cases) wasn't worth it.

also naming things after their implementation is poor taste and can result in
even bigger problems down the line since as soon as the implementation changes,
you will have a flag day or have to keep a bad name. this is a lesson that the
REFCOUNT submission will learn too since the kernel's atomic*_t types (an
implementation detail) are used extensively for different purposes, instead of
using specialized types (kref is a good example of that). for .data..ro_after_init
the lesson will happen when you try to add back the remaining pieces from PaX,
such as module handling and not-always-const-in-the-C-sense objects and associated
accessors.

cheers,
 PaX Team

[PATCH 0/9] mm: Hardened usercopy

[Andy Lutomirski]

On Sun, Jul 10, 2016 at 5:03 AM, PaX Team <pageexec@freemail.hu> wrote:
> On 10 Jul 2016 at 11:16, Ingo Molnar wrote:
>
>> * PaX Team <pageexec@freemail.hu> wrote:
>>
>> > On 9 Jul 2016 at 14:27, Andy Lutomirski wrote:
>> >
>> > > I like the series, but I have one minor nit to pick.  The effect of this
>> > > series is to harden usercopy, but most of the code is really about
>> > > infrastructure to validate that a pointed-to object is valid.
>> >
>> > actually USERCOPY has never been about validating pointers. its sole purpose is
>> > to validate the *size* argument of copy*user calls, a very specific form of
>> > runtime bounds checking.
>>
>> What this code has been about originally is largely immaterial, unless you can
>> formulate it into a technical argument.
>
> we design defense mechanisms for specific and clear purposes, starting with
> a threat model, evaluating defense options based on various criteria, etc.
> USERCOPY underwent this same process and taking it out of its original context
> means that all you get in the end is cargo cult security (wouldn't be the first
> time it has happened (ExecShield, ASLR, etc)).
>
> that said, i actually started that discussion but for some reason you chose
> not to respond to that one part of my mail so let me ask it again:
>
>   what kind of checks are you thinking of here? and more fundamentally, against
>   what kind of threats?
>
> as far as i'm concerned, a defense mechanism is only as good as its underlying
> threat model. by validating pointers (for yet to be stated security related
> properties) you're presumably assuming some kind of threat and unless stated
> clearly what that threat is (unintended pointer modification through memory
> corruption and/or other bugs?) noone can tell whether the proposed defense
> mechanism will actually be effective in preventing exploitation. it is the
> worst kind of defense that doesn't actually achieve its stated goals, that
> way lies false sense of security and i hope noone here is in that business.

I'm imaging security bugs that involve buffer length corruption but
that don't call copy_to/from_user.  Hardened usercopy shuts
expoitation down if the first use of the corrupt size is
copy_to/from_user or similar.  I bet that a bit better coverage could
be achieved by instrumenting more functions.

To be clear: I'm not objecting to calling the overall feature hardened
usercopy or similar.  I object to
CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR.  That feature is *used* for
hardened usercopy but is not, in and of itself, a usercopy thing.
It's an object / memory range validation thing.  So we'll feel silly
down the road if we use it for something else and the config option
name has nothing to do with the feature.

>> > [...] like the renaming of .data..read_only to .data..ro_after_init which also
>> > had nothing to do with init but everything to do with objects being conceptually
>> > read-only...
>>
>> .data..ro_after_init objects get written to during bootup so it's conceptually
>> quite confusing to name it "read-only" without any clear qualifiers.
>>
>> That it's named consistently with its role of "read-write before init and read
>> only after init" on the other hand is not confusing at all. Not sure what your
>> problem is with the new name.
>
> the new name reflects a complete misunderstanding of the PaX feature it was based
> on (typical case of cargo cult security). in particular, the __read_only facility
> in PaX is part of a defense mechanism that attempts to solve a specific problem
> (like everything else) and that problem has nothing whatsoever to do with what
> happens before/after the kernel init process. enforcing read-ony kernel memory at
> the end of kernel initialization is an implementation detail only and wasn't even
> true always (and still isn't true for kernel modules for example): in the linux 2.4
> days PaX actually enforced read-only kernel memory properties in startup_32 already
> but i relaxed that for the 2.6+ port as the maintenance cost (finding out and
> handling new exceptional cases) wasn't worth it.
>
> also naming things after their implementation is poor taste and can result in
> even bigger problems down the line since as soon as the implementation changes,
> you will have a flag day or have to keep a bad name. this is a lesson that the
> REFCOUNT submission will learn too since the kernel's atomic*_t types (an
> implementation detail) are used extensively for different purposes, instead of
> using specialized types (kref is a good example of that). for .data..ro_after_init
> the lesson will happen when you try to add back the remaining pieces from PaX,
> such as module handling and not-always-const-in-the-C-sense objects and associated
> accessors.

The name is related to how the thing works.  If I understand
correctly, in PaX, the idea is to make some things readonly and use
pax_open_kernel(), etc to write it as needed.  This is a nifty
mechanism, but it's *not* what .data..ro_after_init does upstream.  If
I mark something __ro_after_init, then I can write it freely during
boot, but I can't write it thereafter.  In contrast, if I put
something in .rodata (using 'const', for example), then I must not
write it *at all* unless I use special helpers (kmap, pax_open_kernel,
etc).  So the practical effect from a programer's perspective of
__ro_after_init is quite different from .rodata, and I think the names
should reflect that.

(And yes, the upstream kernel should soon have __ro_after_init working
in modules.  And the not-always-const-in-the-C-sense objects using
accessors will need changes to add those accessors, and we can and
should change the annotation on the object itself at the same time.
But if I mark something __ro_after_init, I can write it using normal C
during init, and there's nothing wrong with that.)

--Andy

[PATCH 0/9] mm: Hardened usercopy

[Kees Cook]

On Sun, Jul 10, 2016 at 8:03 AM, PaX Team <pageexec@freemail.hu> wrote:
> i note that this analysis is also missing from this USERCOPY submission except
> for stating what Kees assumed about USERCOPY (and apparently noone could be
> bothered to read the original Kconfig help of it which clearly states that the
> purpose is copy size checking, not some elaborate pointer validation, the latter
> is an implementation detail only and is necessary to be able to derive the
> underlying slab object's intended size).

I read the Kconfig text, but it's not entirely accurate. While size is
being checked, it's all nonsense without also the address, so it's
really an object checker. The original design intent may have been the
slab size checks, but it grew beyond that (both within PaX and within
Grsecurity which explicitly added the check for pointers into kernel
text).

I'm just trying to explain as fully as possible what the resulting
code does and why.

> it's not pointer validation but bounds checking: you already know which memory
> object the pointer is supposed to point to, you only check its bounds. if it was
> an attacker controlled pointer then all this would be a pointless check of course,
> trivial for an attacker to circumvent (and this is why it's not part of the
> USERCOPY design).

Agreed: but the pointer is being checked to attempt to figure out what
KIND of object is being copied. It is part of the logic. If it helps
people understand it more clearly, I can describe them as separate
steps: identify the object type, then perform bounds checking of the
size on that type.

>> > yes, more bikeshedding will surely help, [...]
>>
>> Insulting and ridiculing a reviewer who explicitly qualified his comments with
>> "one minor nit to pick" sure does not help upstream integration either.
>
> sorry Ingo, but calling a spade a spade isn't insulting, at best it's exposing
> some painful truth. you yourself used that term several times in the past, were
> you insulting and ridiculing people then?
>
> as for the ad hominem that you displayed here and later, i hope that in the
> future you will display the same professional conduct that you apparently expect
> from others.

There's a long history of misunderstanding and miscommunication
(intentional or otherwise) by everyone on these topics. I'd love it if
we can just side-step all of it, and try to stick as closely to the
technical discussions as possible. Everyone involved in these
discussions wants better security, even if we go about it in different
ways. If anyone finds themselves feeling insulted, just try to let it
go, and focus on the places where we can find productive common
ground, remembering that any fighting just distracts from the more
important issues at hand.

> i'll voice my opinion when you guys are about to screw it up (as it happened in
> the past and apparently history keeps repeating itself). if you don't want my
> opinion then don't ask for it (in that case we'll write a blog at most ;).

I am hugely interested in your involvement in these discussions:
you're by far the most knowledgeable about them. You generally give
very productive feedback, and for that I'm thankful. I prefer that to
just saying something is wrong/broken without any actionable
follow-up. :)

>> > [...] like the renaming of .data..read_only to .data..ro_after_init which also
>> > had nothing to do with init but everything to do with objects being conceptually
>> > read-only...
>>
>> .data..ro_after_init objects get written to during bootup so it's conceptually
>> quite confusing to name it "read-only" without any clear qualifiers.
>>
>> That it's named consistently with its role of "read-write before init and read
>> only after init" on the other hand is not confusing at all. Not sure what your
>> problem is with the new name.
>
> the new name reflects a complete misunderstanding of the PaX feature it was based
> on (typical case of cargo cult security). in particular, the __read_only facility
> in PaX is part of a defense mechanism that attempts to solve a specific problem
> (like everything else) and that problem has nothing whatsoever to do with what
> happens before/after the kernel init process. enforcing read-ony kernel memory at
> the end of kernel initialization is an implementation detail only and wasn't even
> true always (and still isn't true for kernel modules for example): in the linux 2.4
> days PaX actually enforced read-only kernel memory properties in startup_32 already
> but i relaxed that for the 2.6+ port as the maintenance cost (finding out and
> handling new exceptional cases) wasn't worth it.

Part of getting protections into upstream is doing them in ways that
make them palatable for incremental work. As it happened, the
read-after-init piece of the larger read-only attack surface reduction
effort was small enough to make it in. As more work is done, we can
continue to build on it.

Making rodata read-only before mark_rodata() is part of my longer goal
since other architectures (e.g. s390) already do this, and is
technically the more correct thing to do: rodata should start its life
read-only. It's a weird hack that it is delayed at all.

> also naming things after their implementation is poor taste and can result in
> even bigger problems down the line since as soon as the implementation changes,

On the surface, I don't disagree, but as upstream is a large-scale
collaborative effort, I tend to focus on what things are specifically
critical, and naming isn't one of them. :)

> you will have a flag day or have to keep a bad name. this is a lesson that the
> REFCOUNT submission will learn too since the kernel's atomic*_t types (an
> implementation detail) are used extensively for different purposes, instead of
> using specialized types (kref is a good example of that).

Right, and I think part of this is a failure of documentation and
examples. As we make progress with REFCOUNT, we can learn about the
best way to approach these kinds of larger tree-wide changes under the
constraints of the existing upstream development process.

> For .data..ro_after_init
> the lesson will happen when you try to add back the remaining pieces from PaX,
> such as module handling and not-always-const-in-the-C-sense objects and associated
> accessors.

Do you mean the rest of the KERNEXEC (hopefully I'm not confusing
implementation names) code that uses pax_open/close_kernel()? I expect
that to be a gradual addition too, and I'd love participation to get
it and the constify plugin into the kernel.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[PATCH 0/9] mm: Hardened usercopy

[Kees Cook]

On Sun, Jul 10, 2016 at 8:38 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Sun, Jul 10, 2016 at 5:03 AM, PaX Team <pageexec@freemail.hu> wrote:
>> On 10 Jul 2016 at 11:16, Ingo Molnar wrote:
>>
>>> * PaX Team <pageexec@freemail.hu> wrote:
>>>
>>> > On 9 Jul 2016 at 14:27, Andy Lutomirski wrote:
>>> >
>>> > > I like the series, but I have one minor nit to pick.  The effect of this
>>> > > series is to harden usercopy, but most of the code is really about
>>> > > infrastructure to validate that a pointed-to object is valid.
>>> >
>>> > actually USERCOPY has never been about validating pointers. its sole purpose is
>>> > to validate the *size* argument of copy*user calls, a very specific form of
>>> > runtime bounds checking.
>>>
>>> What this code has been about originally is largely immaterial, unless you can
>>> formulate it into a technical argument.
>>
>> we design defense mechanisms for specific and clear purposes, starting with
>> a threat model, evaluating defense options based on various criteria, etc.
>> USERCOPY underwent this same process and taking it out of its original context
>> means that all you get in the end is cargo cult security (wouldn't be the first
>> time it has happened (ExecShield, ASLR, etc)).
>>
>> that said, i actually started that discussion but for some reason you chose
>> not to respond to that one part of my mail so let me ask it again:
>>
>>   what kind of checks are you thinking of here? and more fundamentally, against
>>   what kind of threats?
>>
>> as far as i'm concerned, a defense mechanism is only as good as its underlying
>> threat model. by validating pointers (for yet to be stated security related
>> properties) you're presumably assuming some kind of threat and unless stated
>> clearly what that threat is (unintended pointer modification through memory
>> corruption and/or other bugs?) noone can tell whether the proposed defense
>> mechanism will actually be effective in preventing exploitation. it is the
>> worst kind of defense that doesn't actually achieve its stated goals, that
>> way lies false sense of security and i hope noone here is in that business.
>
> I'm imaging security bugs that involve buffer length corruption but
> that don't call copy_to/from_user.  Hardened usercopy shuts
> expoitation down if the first use of the corrupt size is
> copy_to/from_user or similar.  I bet that a bit better coverage could
> be achieved by instrumenting more functions.
>
> To be clear: I'm not objecting to calling the overall feature hardened
> usercopy or similar.  I object to
> CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR.  That feature is *used* for
> hardened usercopy but is not, in and of itself, a usercopy thing.
> It's an object / memory range validation thing.  So we'll feel silly
> down the road if we use it for something else and the config option
> name has nothing to do with the feature.

Well, the CONFIG_HAVE* stuff is almost entirely invisible to the
end-user, and I feel like it's better to be specific about names now,
and when they change their meaning, we can change their names with it.

I intend to extend the HARDENED_USERCOPY logic in similar ways to how
it is extended in Grsecurity: parts can be used for the "is this
destined for a userspace memory buffer?" test when rejecting writing
pointers or other sensitive information during sprintf (see the
HIDESYM work in grsecurity).

But, I don't like to over-think it: right now, it is named for what it
does, and we can adjust as we need to.

>
>>> > [...] like the renaming of .data..read_only to .data..ro_after_init which also
>>> > had nothing to do with init but everything to do with objects being conceptually
>>> > read-only...
>>>
>>> .data..ro_after_init objects get written to during bootup so it's conceptually
>>> quite confusing to name it "read-only" without any clear qualifiers.
>>>
>>> That it's named consistently with its role of "read-write before init and read
>>> only after init" on the other hand is not confusing at all. Not sure what your
>>> problem is with the new name.
>>
>> the new name reflects a complete misunderstanding of the PaX feature it was based
>> on (typical case of cargo cult security). in particular, the __read_only facility
>> in PaX is part of a defense mechanism that attempts to solve a specific problem
>> (like everything else) and that problem has nothing whatsoever to do with what
>> happens before/after the kernel init process. enforcing read-ony kernel memory at
>> the end of kernel initialization is an implementation detail only and wasn't even
>> true always (and still isn't true for kernel modules for example): in the linux 2.4
>> days PaX actually enforced read-only kernel memory properties in startup_32 already
>> but i relaxed that for the 2.6+ port as the maintenance cost (finding out and
>> handling new exceptional cases) wasn't worth it.
>>
>> also naming things after their implementation is poor taste and can result in
>> even bigger problems down the line since as soon as the implementation changes,
>> you will have a flag day or have to keep a bad name. this is a lesson that the
>> REFCOUNT submission will learn too since the kernel's atomic*_t types (an
>> implementation detail) are used extensively for different purposes, instead of
>> using specialized types (kref is a good example of that). for .data..ro_after_init
>> the lesson will happen when you try to add back the remaining pieces from PaX,
>> such as module handling and not-always-const-in-the-C-sense objects and associated
>> accessors.
>
> The name is related to how the thing works.  If I understand
> correctly, in PaX, the idea is to make some things readonly and use
> pax_open_kernel(), etc to write it as needed.  This is a nifty
> mechanism, but it's *not* what .data..ro_after_init does upstream.  If
> I mark something __ro_after_init, then I can write it freely during
> boot, but I can't write it thereafter.  In contrast, if I put
> something in .rodata (using 'const', for example), then I must not
> write it *at all* unless I use special helpers (kmap, pax_open_kernel,
> etc).  So the practical effect from a programer's perspective of
> __ro_after_init is quite different from .rodata, and I think the names
> should reflect that.

I expect that if/when we add the open/close_kernel logic, we'll have a
new section and it will be named accordingly (since it, too, is not
const-in-the-C-sense, and shouldn't live in the standard .rodata
section).

> (And yes, the upstream kernel should soon have __ro_after_init working
> in modules.  And the not-always-const-in-the-C-sense objects using
> accessors will need changes to add those accessors, and we can and
> should change the annotation on the object itself at the same time.
> But if I mark something __ro_after_init, I can write it using normal C
> during init, and there's nothing wrong with that.)

-Kees


-- 
Kees Cook
Chrome OS & Brillo Security