[PATCH 0/7] x86: uaccess hardening, easy part

[PATCH 0/7] x86: uaccess hardening, easy part

[Andy Lutomirski]

This series hardens x86's uaccess code a bit.  It adds warnings for
some screwups, adds an OOPS for a major exploitable screwup, and it
improves debuggability a bit by indicating non-default fs in oopses.

It shouldn't cause any new OOPSes except in the particularly
dangerous case where the kernel faults on a kernel address under
USER_DS, which indicates that an access_ok is missing and is likely
to be easily exploitable -- OOPSing will make it harder to exploit.

I have some draft patches to force OOPSes on user address accesses
under KERNEL_DS (which is a big no-no), but I'd rather make those
warn instead of OOPSing, and I don't have a good implementation of
that yet.  Those patches aren't part of this series.

Andy Lutomirski (7):
  x86/xen: Simplify set_aliased_prot
  x86/extable: Pass error_code and an extra unsigned long to exhandlers
  x86/uaccess: Give uaccess faults their own handler
  x86/dumpstack: If addr_limit is non-default, display it
  x86/uaccess: Warn on uaccess faults other than #PF
  x86/uaccess: Don't fix up USER_DS uaccess faults to kernel addresses
  x86/uaccess: OOPS or warn on a fault with KERNEL_DS and
    !pagefault_disabled()

 arch/x86/include/asm/uaccess.h   |  19 ++++---
 arch/x86/kernel/cpu/mcheck/mce.c |   2 +-
 arch/x86/kernel/dumpstack_32.c   |   4 ++
 arch/x86/kernel/dumpstack_64.c   |   5 ++
 arch/x86/kernel/kprobes/core.c   |   6 +-
 arch/x86/kernel/traps.c          |   6 +-
 arch/x86/lib/getuser.S           |  12 ++--
 arch/x86/lib/putuser.S           |  10 ++--
 arch/x86/mm/extable.c            | 120 ++++++++++++++++++++++++++++++++++-----
 arch/x86/mm/fault.c              |   2 +-
 arch/x86/xen/enlighten.c         |   4 +-
 11 files changed, 145 insertions(+), 45 deletions(-)

-- 
2.5.5

[PATCH 1/7] x86/xen: Simplify set_aliased_prot

[Andy Lutomirski]

In aa1acff356bb ("x86/xen: Probe target addresses in
set_aliased_prot() before the hypercall"), I added an explicit probe
to work around a hypercall issue.  The code can be simplified by
using probe_kernel_read.

Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: David Vrabel <dvrabel@cantab.net>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: xen-devel <xen-devel@lists.xen.org>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/xen/enlighten.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 6ab672233ac9..eed696c229ba 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -521,9 +521,7 @@ static void set_aliased_prot(void *v, pgprot_t prot)
 
 	preempt_disable();
 
-	pagefault_disable();	/* Avoid warnings due to being atomic. */
-	__get_user(dummy, (unsigned char __user __force *)v);
-	pagefault_enable();
+	probe_kernel_read(&dummy, (unsigned char *)v, 1);
 
 	if (HYPERVISOR_update_va_mapping((unsigned long)v, pte, 0))
 		BUG();
-- 
2.5.5

[PATCH 2/7] x86/extable: Pass error_code and an extra unsigned long to exhandlers

[Andy Lutomirski]

Exception handlers might want to know the error code and, for some
exceptions, some other auxiliarry info.  Pass in the error code and
an 'extra' parameter.  For page faults, 'extra' is cr2.

The kprobe code is incomprehensible to me.  For kprobe fixups, just
pass zeroes.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/include/asm/uaccess.h   |  3 ++-
 arch/x86/kernel/cpu/mcheck/mce.c |  2 +-
 arch/x86/kernel/kprobes/core.c   |  6 ++++--
 arch/x86/kernel/traps.c          |  6 +++---
 arch/x86/mm/extable.c            | 31 ++++++++++++++++++++-----------
 arch/x86/mm/fault.c              |  2 +-
 6 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index d794fd1f582f..5b65b2110167 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -108,7 +108,8 @@ struct exception_table_entry {
 
 #define ARCH_HAS_RELATIVE_EXTABLE
 
-extern int fixup_exception(struct pt_regs *regs, int trapnr);
+extern int fixup_exception(struct pt_regs *regs, int trapnr,
+			   unsigned long error_code, unsigned long extra);
 extern bool ex_has_fault_handler(unsigned long ip);
 extern void early_fixup_exception(struct pt_regs *regs, int trapnr);
 
diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index f0c921b03e42..e4321f167947 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -1175,7 +1175,7 @@ out:
 		local_irq_disable();
 		ist_end_non_atomic();
 	} else {
-		if (!fixup_exception(regs, X86_TRAP_MC))
+		if (!fixup_exception(regs, X86_TRAP_MC, 0, 0))
 			mce_panic("Failed kernel mode recovery", &m, NULL);
 	}
 
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 38cf7a741250..4ec4de0d79f7 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -988,9 +988,11 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 
 		/*
 		 * In case the user-specified fault handler returned
-		 * zero, try to fix up.
+		 * zero, try to fix up.  (This is called via die notifiers,
+		 * and die notifiers are a mess.  Just pass zero for the
+		 * error_code and extra info.
 		 */
-		if (fixup_exception(regs, trapnr))
+		if (fixup_exception(regs, trapnr, 0, 0))
 			return 1;
 
 		/*
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index d1590486204a..563b72912cbe 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -186,7 +186,7 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 	}
 
 	if (!user_mode(regs)) {
-		if (!fixup_exception(regs, trapnr)) {
+		if (!fixup_exception(regs, trapnr, error_code, 0)) {
 			tsk->thread.error_code = error_code;
 			tsk->thread.trap_nr = trapnr;
 			die(str, regs, error_code);
@@ -438,7 +438,7 @@ do_general_protection(struct pt_regs *regs, long error_code)
 
 	tsk = current;
 	if (!user_mode(regs)) {
-		if (fixup_exception(regs, X86_TRAP_GP))
+		if (fixup_exception(regs, X86_TRAP_GP, error_code, 0))
 			return;
 
 		tsk->thread.error_code = error_code;
@@ -742,7 +742,7 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)
 	cond_local_irq_enable(regs);
 
 	if (!user_mode(regs)) {
-		if (!fixup_exception(regs, trapnr)) {
+		if (!fixup_exception(regs, trapnr, error_code, 0)) {
 			task->thread.error_code = error_code;
 			task->thread.trap_nr = trapnr;
 			die(str, regs, error_code);
diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index 4bb53b89f3c5..c1a25aca0365 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -3,7 +3,8 @@
 #include <asm/traps.h>
 
 typedef bool (*ex_handler_t)(const struct exception_table_entry *,
-			    struct pt_regs *, int);
+			     struct pt_regs *, int,
+			     unsigned long, unsigned long);
 
 static inline unsigned long
 ex_fixup_addr(const struct exception_table_entry *x)
@@ -17,7 +18,8 @@ ex_fixup_handler(const struct exception_table_entry *x)
 }
 
 bool ex_handler_default(const struct exception_table_entry *fixup,
-		       struct pt_regs *regs, int trapnr)
+			struct pt_regs *regs, int trapnr,
+			unsigned long error_code, unsigned long extra)
 {
 	regs->ip = ex_fixup_addr(fixup);
 	return true;
@@ -25,7 +27,8 @@ bool ex_handler_default(const struct exception_table_entry *fixup,
 EXPORT_SYMBOL(ex_handler_default);
 
 bool ex_handler_fault(const struct exception_table_entry *fixup,
-		     struct pt_regs *regs, int trapnr)
+		      struct pt_regs *regs, int trapnr,
+		      unsigned long error_code, unsigned long extra)
 {
 	regs->ip = ex_fixup_addr(fixup);
 	regs->ax = trapnr;
@@ -34,7 +37,8 @@ bool ex_handler_fault(const struct exception_table_entry *fixup,
 EXPORT_SYMBOL_GPL(ex_handler_fault);
 
 bool ex_handler_ext(const struct exception_table_entry *fixup,
-		   struct pt_regs *regs, int trapnr)
+		    struct pt_regs *regs, int trapnr,
+		    unsigned long error_code, unsigned long extra)
 {
 	/* Special hack for uaccess_err */
 	current_thread_info()->uaccess_err = 1;
@@ -44,7 +48,8 @@ bool ex_handler_ext(const struct exception_table_entry *fixup,
 EXPORT_SYMBOL(ex_handler_ext);
 
 bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup,
-			     struct pt_regs *regs, int trapnr)
+			     struct pt_regs *regs, int trapnr,
+			     unsigned long error_code, unsigned long extra)
 {
 	WARN_ONCE(1, "unchecked MSR access error: RDMSR from 0x%x\n",
 		  (unsigned int)regs->cx);
@@ -58,7 +63,8 @@ bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup,
 EXPORT_SYMBOL(ex_handler_rdmsr_unsafe);
 
 bool ex_handler_wrmsr_unsafe(const struct exception_table_entry *fixup,
-			     struct pt_regs *regs, int trapnr)
+			     struct pt_regs *regs, int trapnr,
+			     unsigned long error_code, unsigned long extra)
 {
 	WARN_ONCE(1, "unchecked MSR access error: WRMSR to 0x%x (tried to write 0x%08x%08x)\n",
 		  (unsigned int)regs->cx,
@@ -71,12 +77,13 @@ bool ex_handler_wrmsr_unsafe(const struct exception_table_entry *fixup,
 EXPORT_SYMBOL(ex_handler_wrmsr_unsafe);
 
 bool ex_handler_clear_fs(const struct exception_table_entry *fixup,
-			 struct pt_regs *regs, int trapnr)
+			 struct pt_regs *regs, int trapnr,
+			 unsigned long error_code, unsigned long extra)
 {
 	if (static_cpu_has(X86_BUG_NULL_SEG))
 		asm volatile ("mov %0, %%fs" : : "rm" (__USER_DS));
 	asm volatile ("mov %0, %%fs" : : "rm" (0));
-	return ex_handler_default(fixup, regs, trapnr);
+	return ex_handler_default(fixup, regs, trapnr, error_code, extra);
 }
 EXPORT_SYMBOL(ex_handler_clear_fs);
 
@@ -93,7 +100,8 @@ bool ex_has_fault_handler(unsigned long ip)
 	return handler == ex_handler_fault;
 }
 
-int fixup_exception(struct pt_regs *regs, int trapnr)
+int fixup_exception(struct pt_regs *regs, int trapnr,
+		    unsigned long error_code, unsigned long extra)
 {
 	const struct exception_table_entry *e;
 	ex_handler_t handler;
@@ -117,7 +125,7 @@ int fixup_exception(struct pt_regs *regs, int trapnr)
 		return 0;
 
 	handler = ex_fixup_handler(e);
-	return handler(e, regs, trapnr);
+	return handler(e, regs, trapnr, error_code, extra);
 }
 
 extern unsigned int early_recursion_flag;
@@ -149,7 +157,8 @@ void __init early_fixup_exception(struct pt_regs *regs, int trapnr)
 	 * Keep in mind that not all vectors actually get here.  Early
 	 * fage faults, for example, are special.
 	 */
-	if (fixup_exception(regs, trapnr))
+	if (fixup_exception(regs, trapnr, regs->orig_ax,
+			    (regs->orig_ax == X86_TRAP_PF ? read_cr2() : 0)))
 		return;
 
 fail:
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 5ce1ed02f7e8..3de8dc66fd5c 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -722,7 +722,7 @@ no_context(struct pt_regs *regs, unsigned long error_code,
 	struct vm_area_struct *vma = NULL;
 
 	/* Are we prepared to handle this kernel fault? */
-	if (fixup_exception(regs, X86_TRAP_PF)) {
+	if (fixup_exception(regs, X86_TRAP_PF, error_code, address)) {
 		/*
 		 * Any interrupt that takes a fault gets the fixup. This makes
 		 * the below recursive fault logic only apply to a faults from
-- 
2.5.5

[PATCH 3/7] x86/uaccess: Give uaccess faults their own handler

[Andy Lutomirski]

This will give us a single piece of code to edit to harden our
uaccess fault handling.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/include/asm/uaccess.h | 16 ++++++++--------
 arch/x86/lib/getuser.S         | 12 ++++++------
 arch/x86/lib/putuser.S         | 10 +++++-----
 arch/x86/mm/extable.c          | 27 +++++++++++++++++++++++----
 4 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 5b65b2110167..1bc0d18446d7 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -205,8 +205,8 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
 		     "4:	movl %3,%0\n"				\
 		     "	jmp 3b\n"					\
 		     ".previous\n"					\
-		     _ASM_EXTABLE(1b, 4b)				\
-		     _ASM_EXTABLE(2b, 4b)				\
+		     _ASM_EXTABLE_HANDLE(1b, 4b, ex_handler_uaccess)	\
+		     _ASM_EXTABLE_HANDLE(2b, 4b, ex_handler_uaccess)	\
 		     : "=r" (err)					\
 		     : "A" (x), "r" (addr), "i" (errret), "0" (err))
 
@@ -374,7 +374,7 @@ do {									\
 		     "	xor"itype" %"rtype"1,%"rtype"1\n"		\
 		     "	jmp 2b\n"					\
 		     ".previous\n"					\
-		     _ASM_EXTABLE(1b, 3b)				\
+		     _ASM_EXTABLE_HANDLE(1b, 3b, ex_handler_uaccess)	\
 		     : "=r" (err), ltype(x)				\
 		     : "m" (__m(addr)), "i" (errret), "0" (err))
 
@@ -446,7 +446,7 @@ struct __large_struct { unsigned long buf[100]; };
 		     "3:	mov %3,%0\n"				\
 		     "	jmp 2b\n"					\
 		     ".previous\n"					\
-		     _ASM_EXTABLE(1b, 3b)				\
+		     _ASM_EXTABLE_HANDLE(1b, 3b, ex_handler_uaccess)	\
 		     : "=r"(err)					\
 		     : ltype(x), "m" (__m(addr)), "i" (errret), "0" (err))
 
@@ -574,7 +574,7 @@ extern void __cmpxchg_wrong_size(void)
 			"3:\tmov     %3, %0\n"				\
 			"\tjmp     2b\n"				\
 			"\t.previous\n"					\
-			_ASM_EXTABLE(1b, 3b)				\
+			_ASM_EXTABLE_HANDLE(1b, 3b, ex_handler_uaccess)	\
 			: "+r" (__ret), "=a" (__old), "+m" (*(ptr))	\
 			: "i" (-EFAULT), "q" (__new), "1" (__old)	\
 			: "memory"					\
@@ -590,7 +590,7 @@ extern void __cmpxchg_wrong_size(void)
 			"3:\tmov     %3, %0\n"				\
 			"\tjmp     2b\n"				\
 			"\t.previous\n"					\
-			_ASM_EXTABLE(1b, 3b)				\
+			_ASM_EXTABLE_HANDLE(1b, 3b, ex_handler_uaccess)	\
 			: "+r" (__ret), "=a" (__old), "+m" (*(ptr))	\
 			: "i" (-EFAULT), "r" (__new), "1" (__old)	\
 			: "memory"					\
@@ -606,7 +606,7 @@ extern void __cmpxchg_wrong_size(void)
 			"3:\tmov     %3, %0\n"				\
 			"\tjmp     2b\n"				\
 			"\t.previous\n"					\
-			_ASM_EXTABLE(1b, 3b)				\
+			_ASM_EXTABLE_HANDLE(1b, 3b, ex_handler_uaccess)	\
 			: "+r" (__ret), "=a" (__old), "+m" (*(ptr))	\
 			: "i" (-EFAULT), "r" (__new), "1" (__old)	\
 			: "memory"					\
@@ -625,7 +625,7 @@ extern void __cmpxchg_wrong_size(void)
 			"3:\tmov     %3, %0\n"				\
 			"\tjmp     2b\n"				\
 			"\t.previous\n"					\
-			_ASM_EXTABLE(1b, 3b)				\
+			_ASM_EXTABLE_HANDLE(1b, 3b, ex_handler_uaccess)	\
 			: "+r" (__ret), "=a" (__old), "+m" (*(ptr))	\
 			: "i" (-EFAULT), "r" (__new), "1" (__old)	\
 			: "memory"					\
diff --git a/arch/x86/lib/getuser.S b/arch/x86/lib/getuser.S
index 46668cda4ffd..953b7be58300 100644
--- a/arch/x86/lib/getuser.S
+++ b/arch/x86/lib/getuser.S
@@ -116,12 +116,12 @@ bad_get_user_8:
 END(bad_get_user_8)
 #endif
 
-	_ASM_EXTABLE(1b,bad_get_user)
-	_ASM_EXTABLE(2b,bad_get_user)
-	_ASM_EXTABLE(3b,bad_get_user)
+	_ASM_EXTABLE_HANDLE(1b,bad_get_user, ex_handler_uaccess)
+	_ASM_EXTABLE_HANDLE(2b,bad_get_user, ex_handler_uaccess)
+	_ASM_EXTABLE_HANDLE(3b,bad_get_user, ex_handler_uaccess)
 #ifdef CONFIG_X86_64
-	_ASM_EXTABLE(4b,bad_get_user)
+	_ASM_EXTABLE_HANDLE(4b,bad_get_user, ex_handler_uaccess)
 #else
-	_ASM_EXTABLE(4b,bad_get_user_8)
-	_ASM_EXTABLE(5b,bad_get_user_8)
+	_ASM_EXTABLE_HANDLE(4b,bad_get_user_8, ex_handler_uaccess)
+	_ASM_EXTABLE_HANDLE(5b,bad_get_user_8, ex_handler_uaccess)
 #endif
diff --git a/arch/x86/lib/putuser.S b/arch/x86/lib/putuser.S
index e0817a12d323..16d21d630cc7 100644
--- a/arch/x86/lib/putuser.S
+++ b/arch/x86/lib/putuser.S
@@ -88,10 +88,10 @@ bad_put_user:
 	EXIT
 END(bad_put_user)
 
-	_ASM_EXTABLE(1b,bad_put_user)
-	_ASM_EXTABLE(2b,bad_put_user)
-	_ASM_EXTABLE(3b,bad_put_user)
-	_ASM_EXTABLE(4b,bad_put_user)
+	_ASM_EXTABLE_HANDLE(1b,bad_put_user, ex_handler_uaccess)
+	_ASM_EXTABLE_HANDLE(2b,bad_put_user, ex_handler_uaccess)
+	_ASM_EXTABLE_HANDLE(3b,bad_put_user, ex_handler_uaccess)
+	_ASM_EXTABLE_HANDLE(4b,bad_put_user, ex_handler_uaccess)
 #ifdef CONFIG_X86_32
-	_ASM_EXTABLE(5b,bad_put_user)
+	_ASM_EXTABLE_HANDLE(5b,bad_put_user, ex_handler_uaccess)
 #endif
diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index c1a25aca0365..658292fdee5e 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -1,5 +1,5 @@
 #include <linux/module.h>
-#include <asm/uaccess.h>
+#include <linux/uaccess.h>
 #include <asm/traps.h>
 
 typedef bool (*ex_handler_t)(const struct exception_table_entry *,
@@ -26,6 +26,23 @@ bool ex_handler_default(const struct exception_table_entry *fixup,
 }
 EXPORT_SYMBOL(ex_handler_default);
 
+static bool uaccess_fault_okay(int trapnr, unsigned long error_code,
+			       unsigned long extra)
+{
+	return true;
+}
+
+bool ex_handler_uaccess(const struct exception_table_entry *fixup,
+			struct pt_regs *regs, int trapnr,
+			unsigned long error_code, unsigned long extra)
+{
+	if (!uaccess_fault_okay(trapnr, error_code, extra))
+		return false;
+
+	return ex_handler_default(fixup, regs, trapnr, error_code, extra);
+}
+EXPORT_SYMBOL(ex_handler_uaccess);
+
 bool ex_handler_fault(const struct exception_table_entry *fixup,
 		      struct pt_regs *regs, int trapnr,
 		      unsigned long error_code, unsigned long extra)
@@ -41,9 +58,11 @@ bool ex_handler_ext(const struct exception_table_entry *fixup,
 		    unsigned long error_code, unsigned long extra)
 {
 	/* Special hack for uaccess_err */
-	current_thread_info()->uaccess_err = 1;
-	regs->ip = ex_fixup_addr(fixup);
-	return true;
+	bool ret = ex_handler_uaccess(fixup, regs, trapnr, error_code, extra);
+
+	if (ret)
+		current_thread_info()->uaccess_err = 1;
+	return ret;
 }
 EXPORT_SYMBOL(ex_handler_ext);
 
-- 
2.5.5

[PATCH 4/7] x86/dumpstack: If addr_limit is non-default, display it

[Andy Lutomirski]

This will help debug OOPSes related to USER_DS vs KERNEL_DS.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/kernel/dumpstack_32.c | 4 ++++
 arch/x86/kernel/dumpstack_64.c | 5 +++++
 2 files changed, 9 insertions(+)

diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
index 464ffd69b92e..5dbb08fd8291 100644
--- a/arch/x86/kernel/dumpstack_32.c
+++ b/arch/x86/kernel/dumpstack_32.c
@@ -124,8 +124,12 @@ show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
 void show_regs(struct pt_regs *regs)
 {
 	int i;
+	struct thread_info *ti = current_thread_info();
 
 	show_regs_print_info(KERN_EMERG);
+	if (ti->addr_limit.seg != TASK_SIZE_MAX)
+		printk(KERN_DEFAULT "task.addr_limit: 0x%lx\n",
+		       ti->addr_limit.seg);
 	__show_regs(regs, !user_mode(regs));
 
 	/*
diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index 5f1c6266eb30..2fdeb64dfed0 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -301,9 +301,14 @@ void show_regs(struct pt_regs *regs)
 {
 	int i;
 	unsigned long sp;
+	struct thread_info *ti = current_thread_info();
 
 	sp = regs->sp;
+
 	show_regs_print_info(KERN_DEFAULT);
+	if (ti->addr_limit.seg != TASK_SIZE_MAX)
+		printk(KERN_DEFAULT "task.addr_limit: 0x%lx\n",
+		       ti->addr_limit.seg);
 	__show_regs(regs, 1);
 
 	/*
-- 
2.5.5

[PATCH 5/7] x86/uaccess: Warn on uaccess faults other than #PF

[Andy Lutomirski]

If a uaccess instruction fails due to an8 error other than #PF,
warn.  If the fault is #GP, it most likely indicates access to a
non-canonical address, which means that an access_ok check is
missing, and that's bad.  If the fault is something else (#UD?),
then something is very wrong and we should diagnose it rather
than ignoring it.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/mm/extable.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index 658292fdee5e..c1933471fce7 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -29,6 +29,19 @@ EXPORT_SYMBOL(ex_handler_default);
 static bool uaccess_fault_okay(int trapnr, unsigned long error_code,
 			       unsigned long extra)
 {
+	/*
+	 * For uaccess, only page faults should be fixed up.  I can't see
+	 * any exploit mitigation value in OOPSing on other types of faults,
+	 * so just warn and continue if that happens.  This means that
+	 * uaccess faults to non-canonical addresses will warn.  That's okay
+	 * -- this will only happen if an access_ok is missing, and we want to
+	 * detect that error if it happens.
+	 */
+	if (WARN_ONCE(trapnr != X86_TRAP_PF,
+		      "unexpected uaccess trap %d (may indicate a missing access_ok on a non-canonical address)\n",
+		      trapnr))
+		return true;  /* no good reason to OOPS. */
+
 	return true;
 }
 
-- 
2.5.5

[PATCH 6/7] x86/uaccess: Don't fix up USER_DS uaccess faults to kernel addresses

[Andy Lutomirski]

If a uaccess in USER_DS mode faults on a kernel address, it is
reasonably like to be an exploitable bug.  Make it more obvious in
testing and make it harder to exploit in practice by letting it
OOPS.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/mm/extable.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index c1933471fce7..818cc7ffef79 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -29,6 +29,8 @@ EXPORT_SYMBOL(ex_handler_default);
 static bool uaccess_fault_okay(int trapnr, unsigned long error_code,
 			       unsigned long extra)
 {
+	bool is_user_ds;
+
 	/*
 	 * For uaccess, only page faults should be fixed up.  I can't see
 	 * any exploit mitigation value in OOPSing on other types of faults,
@@ -42,6 +44,22 @@ static bool uaccess_fault_okay(int trapnr, unsigned long error_code,
 		      trapnr))
 		return true;  /* no good reason to OOPS. */
 
+	/*
+	 * This is a page fault, so extra contains the address we failed to
+	 * access.
+	 */
+	is_user_ds = segment_eq(get_fs(), USER_DS);
+
+	if (unlikely(is_user_ds && extra >= TASK_SIZE_MAX)) {
+		/*
+		 * We accessed out out-of-range address with USER_DS.  Force
+		 * an OOPS: if we just warned, then an attacker could trigger
+		 * this repeatedly to learn the kernel memory layout.
+		 */
+		pr_crit("BUG: uaccess to bad address 0x%lx (missing access_ok check)\n", extra);
+		return false;
+	}
+
 	return true;
 }
 
-- 
2.5.5

[PATCH 7/7] x86/uaccess: OOPS or warn on a fault with KERNEL_DS and !pagefault_disabled()

[Andy Lutomirski]

If someone calls set_fs(KERNEL_DS), then they are responsible for
making sure that whatever addresses are accessed are safe.  If they
get it wrong on a kernel address, OOPS.  If they get it wrong on a user
address, warn.

This will make it harder to exploit bugs in which user code controls
a pointer accessed with KERNEL_DS: an attacker will OOPS if they
access an unmapped page, and they'll therefore need luck or a kASLR
bypass in addition.

To keep probe_kernel_read(), probe_kernel_write(), and
probe_kernel_address() working, skip this check if
pagefault_disabled().

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/mm/extable.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index 818cc7ffef79..4bf3ab2b8be1 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -60,6 +60,37 @@ static bool uaccess_fault_okay(int trapnr, unsigned long error_code,
 		return false;
 	}
 
+	/*
+	 * If fs == KERNEL_DS, then all uaccess should be directed to
+	 * known-good kernel addresses.
+	 *
+	 * We still need to support probe_kernel_read and
+	 * probe_kernel_address, which disable page faults. This could be
+	 * tightened up a bit if we explicitly annotated probe_kernel_read(),
+	 * probe_kernel_write() and probe_kernel_address(), perhaps by
+	 * introducing PROBE_KERNEL_DS.
+	 */
+	if (unlikely(!is_user_ds && !pagefault_disabled())) {
+		if (extra < TASK_SIZE_MAX) {
+			/*
+			 * Accessing user address under KERNEL_DS.  This is a
+			 * bug and should be fixed, but OOPSing is not helpful
+			 * for exploit mitigation.
+			 */
+			WARN_ONCE(1, "BUG: uaccess fault at 0x%lx with KERNEL_DS\n",
+				  extra);
+		} else {
+			/*
+			 * If a bug that allows user-controlled KERNEL_DS
+			 * access exists, this will prevent it from being used
+			 * to trivially bypass kASLR.
+			 */
+			pr_crit("BUG: uaccess fault at 0x%lx with KERNEL_DS\n",
+				extra);
+			return false;
+		}
+	}
+
 	return true;
 }
 
-- 
2.5.5

Re: [PATCH 0/7] x86: uaccess hardening, easy part

[Brian Gerst]

On Tue, May 24, 2016 at 6:48 PM, Andy Lutomirski <luto@kernel.org> wrote:
> This series hardens x86's uaccess code a bit.  It adds warnings for
> some screwups, adds an OOPS for a major exploitable screwup, and it
> improves debuggability a bit by indicating non-default fs in oopses.
>
> It shouldn't cause any new OOPSes except in the particularly
> dangerous case where the kernel faults on a kernel address under
> USER_DS, which indicates that an access_ok is missing and is likely
> to be easily exploitable -- OOPSing will make it harder to exploit.
>
> I have some draft patches to force OOPSes on user address accesses
> under KERNEL_DS (which is a big no-no), but I'd rather make those
> warn instead of OOPSing, and I don't have a good implementation of
> that yet.  Those patches aren't part of this series.
>
> Andy Lutomirski (7):
>   x86/xen: Simplify set_aliased_prot
>   x86/extable: Pass error_code and an extra unsigned long to exhandlers
>   x86/uaccess: Give uaccess faults their own handler
>   x86/dumpstack: If addr_limit is non-default, display it
>   x86/uaccess: Warn on uaccess faults other than #PF
>   x86/uaccess: Don't fix up USER_DS uaccess faults to kernel addresses
>   x86/uaccess: OOPS or warn on a fault with KERNEL_DS and
>     !pagefault_disabled()
>
>  arch/x86/include/asm/uaccess.h   |  19 ++++---
>  arch/x86/kernel/cpu/mcheck/mce.c |   2 +-
>  arch/x86/kernel/dumpstack_32.c   |   4 ++
>  arch/x86/kernel/dumpstack_64.c   |   5 ++
>  arch/x86/kernel/kprobes/core.c   |   6 +-
>  arch/x86/kernel/traps.c          |   6 +-
>  arch/x86/lib/getuser.S           |  12 ++--
>  arch/x86/lib/putuser.S           |  10 ++--
>  arch/x86/mm/extable.c            | 120 ++++++++++++++++++++++++++++++++++-----
>  arch/x86/mm/fault.c              |   2 +-
>  arch/x86/xen/enlighten.c         |   4 +-
>  11 files changed, 145 insertions(+), 45 deletions(-)

I'd also like to see the use of set_fs() (which has been grossly
misnamed since ancient versions of Linux) significantly reduced.  Many
of these uses are in compat syscalls, which do:
- read the user memory
- convert it to the native format
- call set_fs(KERNEL_DS)
- pass it to the native syscall which does another user copy.

By separating the core syscall code from the userspace accesses so
that it only touches kernel memory, you can eliminate the set_fs() and
the extra copy from the compat case.

I had started work on this a while back but never finished it.  I'll
look at bringing it up to date.

--
Brian Gerst

Re: [PATCH 1/7] x86/xen: Simplify set_aliased_prot

[Andrew Cooper]

On 24/05/16 23:48, Andy Lutomirski wrote:
> In aa1acff356bb ("x86/xen: Probe target addresses in
> set_aliased_prot() before the hypercall"), I added an explicit probe
> to work around a hypercall issue.  The code can be simplified by
> using probe_kernel_read.
>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: David Vrabel <dvrabel@cantab.net>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: xen-devel <xen-devel@lists.xen.org>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 5/7] x86/uaccess: Warn on uaccess faults other than #PF

[Borislav Petkov]

On Tue, May 24, 2016 at 03:48:42PM -0700, Andy Lutomirski wrote:
> If a uaccess instruction fails due to an8 error other than #PF,
> warn.  If the fault is #GP, it most likely indicates access to a
> non-canonical address, which means that an access_ok check is
> missing, and that's bad.  If the fault is something else (#UD?),
> then something is very wrong and we should diagnose it rather
> than ignoring it.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/mm/extable.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
> index 658292fdee5e..c1933471fce7 100644
> --- a/arch/x86/mm/extable.c
> +++ b/arch/x86/mm/extable.c
> @@ -29,6 +29,19 @@ EXPORT_SYMBOL(ex_handler_default);
>  static bool uaccess_fault_okay(int trapnr, unsigned long error_code,
>  			       unsigned long extra)
>  {
> +	/*
> +	 * For uaccess, only page faults should be fixed up.  I can't see
> +	 * any exploit mitigation value in OOPSing on other types of faults,
> +	 * so just warn and continue if that happens.  This means that
> +	 * uaccess faults to non-canonical addresses will warn.  That's okay
> +	 * -- this will only happen if an access_ok is missing, and we want to
> +	 * detect that error if it happens.
> +	 */
> +	if (WARN_ONCE(trapnr != X86_TRAP_PF,
> +		      "unexpected uaccess trap %d (may indicate a missing access_ok on a non-canonical address)\n",
> +		      trapnr))

Perhaps dump also regs->ip and make the warn message more helpful...

> +		return true;  /* no good reason to OOPS. */

You love those side comments, don'tcha? :-)

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [Xen-devel] [PATCH 1/7] x86/xen: Simplify set_aliased_prot

[David Vrabel]

On 24/05/16 23:48, Andy Lutomirski wrote:
> In aa1acff356bb ("x86/xen: Probe target addresses in
> set_aliased_prot() before the hypercall"), I added an explicit probe
> to work around a hypercall issue.  The code can be simplified by
> using probe_kernel_read.

Acked-by: David Vrabel <david.vrabel@citrix.com>

David

Re: [PATCH 4/7] x86/dumpstack: If addr_limit is non-default, display it

[Borislav Petkov]

On Tue, May 24, 2016 at 03:48:41PM -0700, Andy Lutomirski wrote:
> This will help debug OOPSes related to USER_DS vs KERNEL_DS.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/kernel/dumpstack_32.c | 4 ++++
>  arch/x86/kernel/dumpstack_64.c | 5 +++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
> index 464ffd69b92e..5dbb08fd8291 100644
> --- a/arch/x86/kernel/dumpstack_32.c
> +++ b/arch/x86/kernel/dumpstack_32.c
> @@ -124,8 +124,12 @@ show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
>  void show_regs(struct pt_regs *regs)
>  {
>  	int i;
> +	struct thread_info *ti = current_thread_info();
>  
>  	show_regs_print_info(KERN_EMERG);
> +	if (ti->addr_limit.seg != TASK_SIZE_MAX)
> +		printk(KERN_DEFAULT "task.addr_limit: 0x%lx\n",
> +		       ti->addr_limit.seg);

I guess we can dump that unconditionally just to be consistent and so
that all oopses look the same, i.e., with that line always present.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH 4/7] x86/dumpstack: If addr_limit is non-default, display it

[Borislav Petkov]

On Tue, May 24, 2016 at 03:48:41PM -0700, Andy Lutomirski wrote:
> This will help debug OOPSes related to USER_DS vs KERNEL_DS.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/kernel/dumpstack_32.c | 4 ++++
>  arch/x86/kernel/dumpstack_64.c | 5 +++++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
> index 464ffd69b92e..5dbb08fd8291 100644
> --- a/arch/x86/kernel/dumpstack_32.c
> +++ b/arch/x86/kernel/dumpstack_32.c
> @@ -124,8 +124,12 @@ show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
>  void show_regs(struct pt_regs *regs)
>  {
>  	int i;
> +	struct thread_info *ti = current_thread_info();
>  
>  	show_regs_print_info(KERN_EMERG);
> +	if (ti->addr_limit.seg != TASK_SIZE_MAX)
> +		printk(KERN_DEFAULT "task.addr_limit: 0x%lx\n",
> +		       ti->addr_limit.seg);

And, of course, that printk should be part of the printk in
show_regs_print_info() and not duplicated here and in dumpstack_64.c

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH 7/7] x86/uaccess: OOPS or warn on a fault with KERNEL_DS and !pagefault_disabled()

[Borislav Petkov]

On Tue, May 24, 2016 at 03:48:44PM -0700, Andy Lutomirski wrote:
> +	if (unlikely(!is_user_ds && !pagefault_disabled())) {
> +		if (extra < TASK_SIZE_MAX) {
> +			/*
> +			 * Accessing user address under KERNEL_DS.  This is a
> +			 * bug and should be fixed, but OOPSing is not helpful
> +			 * for exploit mitigation.
> +			 */
> +			WARN_ONCE(1, "BUG: uaccess fault at 0x%lx with KERNEL_DS\n",

			WARN and BUG?

Also, let's have this string and the one below differ for finding out
where we are during debugging.

> +				  extra);
> +		} else {
> +			/*
> +			 * If a bug that allows user-controlled KERNEL_DS
> +			 * access exists, this will prevent it from being used
> +			 * to trivially bypass kASLR.
> +			 */
> +			pr_crit("BUG: uaccess fault at 0x%lx with KERNEL_DS\n",
> +				extra);

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [PATCH 0/7] x86: uaccess hardening, easy part

[Kees Cook]

On Tue, May 24, 2016 at 8:55 PM, Brian Gerst <brgerst@gmail.com> wrote:
> On Tue, May 24, 2016 at 6:48 PM, Andy Lutomirski <luto@kernel.org> wrote:
>> This series hardens x86's uaccess code a bit.  It adds warnings for
>> some screwups, adds an OOPS for a major exploitable screwup, and it
>> improves debuggability a bit by indicating non-default fs in oopses.
>>
>> It shouldn't cause any new OOPSes except in the particularly
>> dangerous case where the kernel faults on a kernel address under
>> USER_DS, which indicates that an access_ok is missing and is likely
>> to be easily exploitable -- OOPSing will make it harder to exploit.
>>
>> I have some draft patches to force OOPSes on user address accesses
>> under KERNEL_DS (which is a big no-no), but I'd rather make those
>> warn instead of OOPSing, and I don't have a good implementation of
>> that yet.  Those patches aren't part of this series.
>>
>> Andy Lutomirski (7):
>>   x86/xen: Simplify set_aliased_prot
>>   x86/extable: Pass error_code and an extra unsigned long to exhandlers
>>   x86/uaccess: Give uaccess faults their own handler
>>   x86/dumpstack: If addr_limit is non-default, display it
>>   x86/uaccess: Warn on uaccess faults other than #PF
>>   x86/uaccess: Don't fix up USER_DS uaccess faults to kernel addresses
>>   x86/uaccess: OOPS or warn on a fault with KERNEL_DS and
>>     !pagefault_disabled()
>>
>>  arch/x86/include/asm/uaccess.h   |  19 ++++---
>>  arch/x86/kernel/cpu/mcheck/mce.c |   2 +-
>>  arch/x86/kernel/dumpstack_32.c   |   4 ++
>>  arch/x86/kernel/dumpstack_64.c   |   5 ++
>>  arch/x86/kernel/kprobes/core.c   |   6 +-
>>  arch/x86/kernel/traps.c          |   6 +-
>>  arch/x86/lib/getuser.S           |  12 ++--
>>  arch/x86/lib/putuser.S           |  10 ++--
>>  arch/x86/mm/extable.c            | 120 ++++++++++++++++++++++++++++++++++-----
>>  arch/x86/mm/fault.c              |   2 +-
>>  arch/x86/xen/enlighten.c         |   4 +-
>>  11 files changed, 145 insertions(+), 45 deletions(-)
>
> I'd also like to see the use of set_fs() (which has been grossly
> misnamed since ancient versions of Linux) significantly reduced.  Many
> of these uses are in compat syscalls, which do:
> - read the user memory
> - convert it to the native format
> - call set_fs(KERNEL_DS)
> - pass it to the native syscall which does another user copy.
>
> By separating the core syscall code from the userspace accesses so
> that it only touches kernel memory, you can eliminate the set_fs() and
> the extra copy from the compat case.
>
> I had started work on this a while back but never finished it.  I'll
> look at bringing it up to date.

Yes, please! This kind of compat handling is very wrong. :( We've had
nothing but problems from having such a large piece of code running
under KERNEL_DS.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH 0/7] x86: uaccess hardening, easy part

[Kees Cook]

On Tue, May 24, 2016 at 3:48 PM, Andy Lutomirski <luto@kernel.org> wrote:
> This series hardens x86's uaccess code a bit.  It adds warnings for
> some screwups, adds an OOPS for a major exploitable screwup, and it
> improves debuggability a bit by indicating non-default fs in oopses.
>
> It shouldn't cause any new OOPSes except in the particularly
> dangerous case where the kernel faults on a kernel address under
> USER_DS, which indicates that an access_ok is missing and is likely
> to be easily exploitable -- OOPSing will make it harder to exploit.
>
> I have some draft patches to force OOPSes on user address accesses
> under KERNEL_DS (which is a big no-no), but I'd rather make those
> warn instead of OOPSing, and I don't have a good implementation of
> that yet.  Those patches aren't part of this series.
>
> Andy Lutomirski (7):
>   x86/xen: Simplify set_aliased_prot
>   x86/extable: Pass error_code and an extra unsigned long to exhandlers
>   x86/uaccess: Give uaccess faults their own handler
>   x86/dumpstack: If addr_limit is non-default, display it
>   x86/uaccess: Warn on uaccess faults other than #PF
>   x86/uaccess: Don't fix up USER_DS uaccess faults to kernel addresses
>   x86/uaccess: OOPS or warn on a fault with KERNEL_DS and
>     !pagefault_disabled()

Reviewed-by: Kees Cook <keescook@chromium.org>

I'm going to see what this does to lib/test_user_copy.c ... I might
have to move it into lkdtm.c if there is an added Oops condition.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH 5/7] x86/uaccess: Warn on uaccess faults other than #PF

[Andy Lutomirski]

On Wed, May 25, 2016 at 2:49 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Tue, May 24, 2016 at 03:48:42PM -0700, Andy Lutomirski wrote:
>> If a uaccess instruction fails due to an8 error other than #PF,
>> warn.  If the fault is #GP, it most likely indicates access to a
>> non-canonical address, which means that an access_ok check is
>> missing, and that's bad.  If the fault is something else (#UD?),
>> then something is very wrong and we should diagnose it rather
>> than ignoring it.
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>  arch/x86/mm/extable.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
>> index 658292fdee5e..c1933471fce7 100644
>> --- a/arch/x86/mm/extable.c
>> +++ b/arch/x86/mm/extable.c
>> @@ -29,6 +29,19 @@ EXPORT_SYMBOL(ex_handler_default);
>>  static bool uaccess_fault_okay(int trapnr, unsigned long error_code,
>>                              unsigned long extra)
>>  {
>> +     /*
>> +      * For uaccess, only page faults should be fixed up.  I can't see
>> +      * any exploit mitigation value in OOPSing on other types of faults,
>> +      * so just warn and continue if that happens.  This means that
>> +      * uaccess faults to non-canonical addresses will warn.  That's okay
>> +      * -- this will only happen if an access_ok is missing, and we want to
>> +      * detect that error if it happens.
>> +      */
>> +     if (WARN_ONCE(trapnr != X86_TRAP_PF,
>> +                   "unexpected uaccess trap %d (may indicate a missing access_ok on a non-canonical address)\n",
>> +                   trapnr))
>
> Perhaps dump also regs->ip and make the warn message more helpful...
>

The stack trace will show it, and I'm not convinced that regs->ip by
itself will be all that helpful -- depending on what gets inlined,  it
could just point to __copy_from_user or similar.

>> +             return true;  /* no good reason to OOPS. */
>
> You love those side comments, don'tcha? :-)

ECO tip #102: avoid silly newlines :-p

--Andy

Re: [PATCH 4/7] x86/dumpstack: If addr_limit is non-default, display it

[Andy Lutomirski]

On Wed, May 25, 2016 at 4:32 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Tue, May 24, 2016 at 03:48:41PM -0700, Andy Lutomirski wrote:
>> This will help debug OOPSes related to USER_DS vs KERNEL_DS.
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>  arch/x86/kernel/dumpstack_32.c | 4 ++++
>>  arch/x86/kernel/dumpstack_64.c | 5 +++++
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
>> index 464ffd69b92e..5dbb08fd8291 100644
>> --- a/arch/x86/kernel/dumpstack_32.c
>> +++ b/arch/x86/kernel/dumpstack_32.c
>> @@ -124,8 +124,12 @@ show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
>>  void show_regs(struct pt_regs *regs)
>>  {
>>       int i;
>> +     struct thread_info *ti = current_thread_info();
>>
>>       show_regs_print_info(KERN_EMERG);
>> +     if (ti->addr_limit.seg != TASK_SIZE_MAX)
>> +             printk(KERN_DEFAULT "task.addr_limit: 0x%lx\n",
>> +                    ti->addr_limit.seg);
>
> I guess we can dump that unconditionally just to be consistent and so
> that all oopses look the same, i.e., with that line always present.
>

I thought about doing that, but I always hate when things scroll off
the screen, and 99% of the time the addr_limit will be TASK_SIZE_MAX
and the line won't be interesting.

--Andy

Re: [PATCH 4/7] x86/dumpstack: If addr_limit is non-default, display it

[Andy Lutomirski]

On Wed, May 25, 2016 at 4:39 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Tue, May 24, 2016 at 03:48:41PM -0700, Andy Lutomirski wrote:
>> This will help debug OOPSes related to USER_DS vs KERNEL_DS.
>>
>> Signed-off-by: Andy Lutomirski <luto@kernel.org>
>> ---
>>  arch/x86/kernel/dumpstack_32.c | 4 ++++
>>  arch/x86/kernel/dumpstack_64.c | 5 +++++
>>  2 files changed, 9 insertions(+)
>>
>> diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
>> index 464ffd69b92e..5dbb08fd8291 100644
>> --- a/arch/x86/kernel/dumpstack_32.c
>> +++ b/arch/x86/kernel/dumpstack_32.c
>> @@ -124,8 +124,12 @@ show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs,
>>  void show_regs(struct pt_regs *regs)
>>  {
>>       int i;
>> +     struct thread_info *ti = current_thread_info();
>>
>>       show_regs_print_info(KERN_EMERG);
>> +     if (ti->addr_limit.seg != TASK_SIZE_MAX)
>> +             printk(KERN_DEFAULT "task.addr_limit: 0x%lx\n",
>> +                    ti->addr_limit.seg);
>
> And, of course, that printk should be part of the printk in
> show_regs_print_info() and not duplicated here and in dumpstack_64.c

Easier said than done.  struct thread_info doesn't have addr_limit on
sensible architectures (e.g. sparc), and I'd rather not stick a bunch
of ifdefs in generic code.

--Andy

Re: [PATCH 7/7] x86/uaccess: OOPS or warn on a fault with KERNEL_DS and !pagefault_disabled()

[Andy Lutomirski]

On Wed, May 25, 2016 at 8:33 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Tue, May 24, 2016 at 03:48:44PM -0700, Andy Lutomirski wrote:
>> +     if (unlikely(!is_user_ds && !pagefault_disabled())) {
>> +             if (extra < TASK_SIZE_MAX) {
>> +                     /*
>> +                      * Accessing user address under KERNEL_DS.  This is a
>> +                      * bug and should be fixed, but OOPSing is not helpful
>> +                      * for exploit mitigation.
>> +                      */
>> +                     WARN_ONCE(1, "BUG: uaccess fault at 0x%lx with KERNEL_DS\n",
>
>                         WARN and BUG?
>
> Also, let's have this string and the one below differ for finding out
> where we are during debugging.

Done.

Re: [PATCH 4/7] x86/dumpstack: If addr_limit is non-default, display it

[Boris Petkov]

Andy Lutomirski <luto@amacapital.net> wrote:
>Easier said than done.  struct thread_info doesn't have addr_limit on
>sensible architectures (e.g. sparc), and I'd rather not stick a bunch
>of ifdefs in generic code.

It's not like it doesn't have an actual address limit though - I'm guessing it is something like the max userspace address on the arch... UINT_MAX or somesuch.
-- 
Sent from a small device: formatting sux and brevity is inevitable. 

Re: [PATCH 4/7] x86/dumpstack: If addr_limit is non-default, display it

[Andy Lutomirski]

On May 29, 2016 11:42 AM, "Boris Petkov" <bp@alien8.de> wrote:
>
> Andy Lutomirski <luto@amacapital.net> wrote:
> >Easier said than done.  struct thread_info doesn't have addr_limit on
> >sensible architectures (e.g. sparc), and I'd rather not stick a bunch
> >of ifdefs in generic code.
>
> It's not like it doesn't have an actual address limit though - I'm guessing it is something like the max userspace address on the arch... UINT_MAX or somesuch.

Sure, but how do I implement that?  There's no "does this arch have
addr_limit in thread_info" general flag that I know of.

--Andy

Re: [PATCH 4/7] x86/dumpstack: If addr_limit is non-default, display it

[Borislav Petkov]

On Sun, May 29, 2016 at 12:08:29PM -0700, Andy Lutomirski wrote:
> Sure, but how do I implement that?  There's no "does this arch have
> addr_limit in thread_info" general flag that I know of.

What about get_fs()? Looks like all arches implement that and on sparc
it is something called thread_info.current_ds...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Re: [Xen-devel] [PATCH 1/7] x86/xen: Simplify set_aliased_prot

[Andy Lutomirski]

On Wed, May 25, 2016 at 2:50 AM, David Vrabel <david.vrabel@citrix.com> wrote:
> On 24/05/16 23:48, Andy Lutomirski wrote:
>> In aa1acff356bb ("x86/xen: Probe target addresses in
>> set_aliased_prot() before the hypercall"), I added an explicit probe
>> to work around a hypercall issue.  The code can be simplified by
>> using probe_kernel_read.
>
> Acked-by: David Vrabel <david.vrabel@citrix.com>

Ingo, can you apply this one patch directly to x86/asm?  The rest of
the series is stalled pending my fixing Borislav's review comments
and, more importantly, fixing the bugs that testing it has shaken
loose.  This patch is a nice cleanup all by itself, though.

--Andy

Re: [Xen-devel] [PATCH 1/7] x86/xen: Simplify set_aliased_prot

[Ingo Molnar]

* Andy Lutomirski <luto@amacapital.net> wrote:

> On Wed, May 25, 2016 at 2:50 AM, David Vrabel <david.vrabel@citrix.com> wrote:
> > On 24/05/16 23:48, Andy Lutomirski wrote:
> >> In aa1acff356bb ("x86/xen: Probe target addresses in
> >> set_aliased_prot() before the hypercall"), I added an explicit probe
> >> to work around a hypercall issue.  The code can be simplified by
> >> using probe_kernel_read.
> >
> > Acked-by: David Vrabel <david.vrabel@citrix.com>
> 
> Ingo, can you apply this one patch directly to x86/asm?  The rest of
> the series is stalled pending my fixing Borislav's review comments
> and, more importantly, fixing the bugs that testing it has shaken
> loose.  This patch is a nice cleanup all by itself, though.

Ok, agreed, done.

Note that I simplified it some more:

-       probe_kernel_read(&dummy, (unsigned char *)v, 1);
+       probe_kernel_read(&dummy, v, 1);

... because 'v' already has a 'void *' natural type.

Thanks,

	Ingo

[tip:x86/asm] x86/xen: Simplify set_aliased_prot()

[tip-bot for Andy Lutomirski]

Commit-ID:  99158f10e91768d34c5004c40c42f802b719bcae
Gitweb:     http://git.kernel.org/tip/99158f10e91768d34c5004c40c42f802b719bcae
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Tue, 24 May 2016 15:48:38 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Sat, 11 Jun 2016 11:28:11 +0200

x86/xen: Simplify set_aliased_prot()

A year ago, via the following commit:

  aa1acff356bb ("x86/xen: Probe target addresses in set_aliased_prot() before the hypercall")

I added an explicit probe to work around a hypercall issue.  The code can
be simplified by using probe_kernel_read().

No change in functionality.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: David Vrabel <david.vrabel@citrix.com>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: David Vrabel <dvrabel@cantab.net>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: xen-devel <xen-devel@lists.xen.org>
Link: http://lkml.kernel.org/r/0706f1a2538e481194514197298cca6b5e3f2638.1464129798.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/xen/enlighten.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 760789a..0f87db2 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -521,9 +521,7 @@ static void set_aliased_prot(void *v, pgprot_t prot)
 
 	preempt_disable();
 
-	pagefault_disable();	/* Avoid warnings due to being atomic. */
-	__get_user(dummy, (unsigned char __user __force *)v);
-	pagefault_enable();
+	probe_kernel_read(&dummy, v, 1);
 
 	if (HYPERVISOR_update_va_mapping((unsigned long)v, pte, 0))
 		BUG();