x86: uaccess hardening, easy part

---
 arch/x86/xen/enlighten.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

arch/x86/xen/enlighten.c +1 -3

index 6ab672233ac9..eed696c229ba 100644
@@ -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();

Signature · 7 lines

2.5.5


_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Andy Lutomirski

author maintainer

2016-05-24 22:48 UTC #2 lore raw

PATCH 1/7 x86/xen: Simplify set_aliased_prot

Cc Andrew Cooper <[email protected]>

Cc Boris Ostrovsky <[email protected]>

Cc David Vrabel <[email protected]>

Cc Jan Beulich <[email protected]>

Cc Konrad Rzeszutek Wilk <[email protected]>

Cc xen-devel <[email protected]>

---
 arch/x86/xen/enlighten.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

arch/x86/xen/enlighten.c +1 -3

index 6ab672233ac9..eed696c229ba 100644
@@ -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();

Signature · 1 line

2.5.5

Andrew Cooper

Reviewed-by on v1 1/7

2016-05-25 09:38 UTC #3 lore raw

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

Quoted 5 lines · 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.

Quoted 7 lines

Cc Andrew Cooper <[email protected]>
Cc Boris Ostrovsky <[email protected]>
Cc David Vrabel <[email protected]>
Cc Jan Beulich <[email protected]>
Cc Konrad Rzeszutek Wilk <[email protected]>
Cc xen-devel <[email protected]>
Signed-off-by Andy Lutomirski <[email protected]>

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Andrew Cooper

Reviewed-by on v1 1/7

2016-05-25 09:38 UTC #4 lore raw

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

Quoted 5 lines · 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.

Quoted 7 lines

Cc Andrew Cooper <[email protected]>
Cc Boris Ostrovsky <[email protected]>
Cc David Vrabel <[email protected]>
Cc Jan Beulich <[email protected]>
Cc Konrad Rzeszutek Wilk <[email protected]>
Cc xen-devel <[email protected]>
Signed-off-by Andy Lutomirski <[email protected]>

David Vrabel

Acked-by on v1 1/7

2016-05-25 09:50 UTC #5 lore raw

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

Quoted 4 lines · 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.

David

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

David Vrabel

Acked-by on v1 1/7

2016-05-25 09:51 UTC #6 lore raw

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

Quoted 4 lines · 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.

David

Andy Lutomirski

2016-06-10 22:12 UTC #7 lore raw

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

Quoted 7 lines · David Vrabel

Quoted 4 lines · 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.
Acked-by David Vrabel <[email protected]>

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

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Andy Lutomirski

2016-06-10 22:12 UTC #8 lore raw

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

In reply to David Vrabel

Quoted 7 lines · David Vrabel

Quoted 4 lines · 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.
Acked-by David Vrabel <[email protected]>

--Andy

Ingo Molnar

2016-06-11 09:29 UTC #9 lore raw

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

Quoted 7 lines · Andy Lutomirski

Quoted 6 lines · David Vrabel
Quoted 4 lines · 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.

Quoted 6 lines

Quoted 1 line
Acked-by David Vrabel <[email protected]>
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

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Ingo Molnar

2016-06-11 09:29 UTC #10 lore raw

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

Quoted 7 lines · Andy Lutomirski

Quoted 6 lines · David Vrabel
Quoted 4 lines · 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.

Quoted 6 lines

Quoted 1 line
Acked-by David Vrabel <[email protected]>
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-bot for Andy Lutomirski

merge bot

2016-06-11 09:34 UTC #11 lore raw

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

x86/xen: Simplify set_aliased_prot()

Commit-ID:  99158f10e91768d34c5004c40c42f802b719bcae
Gitweb:     http://git.kernel.org/tip/99158f10e91768d34c5004c40c42f802b719bcae
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Tue, 24 May 2016 15:48:38 -0700
Committer:  Ingo Molnar <[email protected]>
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.

Link http://lkml.kernel.org/r/0706f1a2538e481194514197298cca6b5e3f2638.1464129798.git.luto@kernel.org open here

Cc Andy Lutomirski <[email protected]>

Cc Boris Ostrovsky <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Brian Gerst <[email protected]>

Cc David Vrabel <[email protected]>

Cc Denys Vlasenko <[email protected]>

Cc H. Peter Anvin <[email protected]>

Cc Jan Beulich <[email protected]>

Cc Kees Cook <[email protected]>

Cc Konrad Rzeszutek Wilk <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc xen-devel <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/xen/enlighten.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

arch/x86/xen/enlighten.c +1 -3

index 760789a..0f87db2 100644
@@ -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();

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

tip-bot for Andy Lutomirski

merge bot

2016-06-11 10:31 UTC #12 lore raw

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

x86/xen: Simplify set_aliased_prot()

Commit-ID:  99158f10e91768d34c5004c40c42f802b719bcae
Gitweb:     http://git.kernel.org/tip/99158f10e91768d34c5004c40c42f802b719bcae
Author:     Andy Lutomirski <[email protected]>
AuthorDate: Tue, 24 May 2016 15:48:38 -0700
Committer:  Ingo Molnar <[email protected]>
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.

Link http://lkml.kernel.org/r/0706f1a2538e481194514197298cca6b5e3f2638.1464129798.git.luto@kernel.org open here

Cc Andy Lutomirski <[email protected]>

Cc Boris Ostrovsky <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Brian Gerst <[email protected]>

Cc David Vrabel <[email protected]>

Cc Denys Vlasenko <[email protected]>

Cc H. Peter Anvin <[email protected]>

Cc Jan Beulich <[email protected]>

Cc Kees Cook <[email protected]>

Cc Konrad Rzeszutek Wilk <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc xen-devel <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/xen/enlighten.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

arch/x86/xen/enlighten.c +1 -3

index 760789a..0f87db2 100644
@@ -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();

Andy Lutomirski

author maintainer

2016-05-24 22:48 UTC #13 lore raw

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

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.

---
 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(-)

arch/x86/include/asm/uaccess.h +2 -1

index d794fd1f582f..5b65b2110167 100644
@@ -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);

arch/x86/kernel/cpu/mcheck/mce.c +1 -1

index f0c921b03e42..e4321f167947 100644
@@ -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);
 	}

arch/x86/kernel/kprobes/core.c +4 -2

index 38cf7a741250..4ec4de0d79f7 100644
@@ -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;
 
 		/*

arch/x86/kernel/traps.c +3 -3

index d1590486204a..563b72912cbe 100644
@@ -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);

arch/x86/mm/extable.c +20 ~~-11~~

index 4bb53b89f3c5..c1a25aca0365 100644
@@ -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:

arch/x86/mm/fault.c +1 -1

index 5ce1ed02f7e8..3de8dc66fd5c 100644
@@ -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

Signature · 1 line

2.5.5

Andy Lutomirski

author maintainer

2016-05-24 22:50 UTC #14 lore raw

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

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

---
 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(-)

arch/x86/include/asm/uaccess.h +8 -8

index 5b65b2110167..1bc0d18446d7 100644
@@ -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"					\

arch/x86/lib/getuser.S +6 -6

index 46668cda4ffd..953b7be58300 100644
@@ -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

arch/x86/lib/putuser.S +5 -5

index e0817a12d323..16d21d630cc7 100644
@@ -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

arch/x86/mm/extable.c +23 -4

index c1a25aca0365..658292fdee5e 100644
@@ -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);

Signature · 1 line

2.5.5

Andy Lutomirski

author maintainer

2016-05-24 22:48 UTC #15 lore raw

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

This will help debug OOPSes related to USER_DS vs KERNEL_DS.

---
 arch/x86/kernel/dumpstack_32.c | 4 ++++
 arch/x86/kernel/dumpstack_64.c | 5 +++++
 2 files changed, 9 insertions(+)

arch/x86/kernel/dumpstack_32.c +4 -0

index 464ffd69b92e..5dbb08fd8291 100644
@@ -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));
 
 	/*

arch/x86/kernel/dumpstack_64.c +5 -0

index 5f1c6266eb30..2fdeb64dfed0 100644
@@ -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);
 
 	/*

Signature · 1 line

2.5.5

Borislav Petkov

maintainer

2016-05-25 11:32 UTC #16 lore raw

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

Quoted 13 lines · Andy Lutomirski

This will help debug OOPSes related to USER_DS vs KERNEL_DS.
Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/kernel/dumpstack_32.c | 4 ++++
 arch/x86/kernel/dumpstack_64.c | 5 +++++
 2 files changed, 9 insertions(+)
arch/x86/kernel/dumpstack_32.c +0 -0
index 464ffd69b92e..5dbb08fd8291 100644

arch/x86/kernel/dumpstack_32.c show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs, · line 132

Quoted 9 lines

arch/x86/kernel/dumpstack_32.c +4 -0

@@ -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.

Signature · 4 lines

Regards/Gruss,
    Boris.

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

Andy Lutomirski

2016-05-29 16:44 UTC #17 lore raw

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

Quoted 19 lines · Borislav Petkov

Quoted 18 lines · Andy Lutomirski

This will help debug OOPSes related to USER_DS vs KERNEL_DS.

---
 arch/x86/kernel/dumpstack_32.c | 4 ++++
 arch/x86/kernel/dumpstack_64.c | 5 +++++
 2 files changed, 9 insertions(+)

arch/x86/kernel/dumpstack_32.c +1 -0

index 464ffd69b92e..5dbb08fd8291 100644
@@ -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();

Quoted 8 lines

arch/x86/kernel/dumpstack_32.c show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs, · line 132
Quoted 4 lines
arch/x86/kernel/dumpstack_32.c +3 -0
      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

Borislav Petkov

maintainer

2016-05-25 11:39 UTC #18 lore raw

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

Quoted 13 lines · Andy Lutomirski

This will help debug OOPSes related to USER_DS vs KERNEL_DS.
Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/kernel/dumpstack_32.c | 4 ++++
 arch/x86/kernel/dumpstack_64.c | 5 +++++
 2 files changed, 9 insertions(+)
arch/x86/kernel/dumpstack_32.c +0 -0
index 464ffd69b92e..5dbb08fd8291 100644

arch/x86/kernel/dumpstack_32.c show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs, · line 132

Quoted 9 lines

arch/x86/kernel/dumpstack_32.c +4 -0

@@ -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

Signature · 4 lines

Regards/Gruss,
    Boris.

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

Andy Lutomirski

2016-05-29 16:47 UTC #19 lore raw

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

Quoted 19 lines · Borislav Petkov

Quoted 18 lines · Andy Lutomirski

This will help debug OOPSes related to USER_DS vs KERNEL_DS.

---
 arch/x86/kernel/dumpstack_32.c | 4 ++++
 arch/x86/kernel/dumpstack_64.c | 5 +++++
 2 files changed, 9 insertions(+)

arch/x86/kernel/dumpstack_32.c +1 -0

index 464ffd69b92e..5dbb08fd8291 100644
@@ -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();

Quoted 7 lines

arch/x86/kernel/dumpstack_32.c show_stack_log_lvl(struct task_struct *task, struct pt_regs *regs, · line 132
Quoted 4 lines
arch/x86/kernel/dumpstack_32.c +3 -0
      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

Boris Petkov

maintainer

2016-05-29 18:42 UTC #20 lore raw

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

Quoted 3 lines · Andy Lutomirski

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.

Signature · 1 line

Sent from a small device: formatting sux and brevity is inevitable.

Andy Lutomirski

2016-05-29 19:08 UTC #21 lore raw

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

Quoted 7 lines · Boris Petkov

Quoted 3 lines · Andy Lutomirski
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

Borislav Petkov

maintainer

2016-05-30 07:40 UTC #22 lore raw

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

Quoted 2 lines · Andy Lutomirski

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...

Signature · 4 lines

Regards/Gruss,
    Boris.

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

Andy Lutomirski

author maintainer

2016-05-24 22:49 UTC #23 lore raw

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

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.

---
 arch/x86/mm/extable.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

arch/x86/mm/extable.c +13 -0

index 658292fdee5e..c1933471fce7 100644
@@ -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;
 }

Signature · 1 line

2.5.5

Borislav Petkov

maintainer

2016-05-25 09:49 UTC #24 lore raw

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

Quoted 21 lines · 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 <[email protected]>
---
 arch/x86/mm/extable.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
arch/x86/mm/extable.c +1 -0
index 658292fdee5e..c1933471fce7 100644
@@ -29,6 +29,19 @@ EXPORT_SYMBOL(ex_handler_default);
 static bool uaccess_fault_okay(int trapnr, unsigned long error_code,
 			       unsigned long extra)
 {
+	/*

arch/x86/mm/extable.c EXPORT_SYMBOL(ex_handler_default); · line 42

Quoted 10 lines

arch/x86/mm/extable.c +10 -0

+	 * 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...

arch/x86/mm/extable.c EXPORT_SYMBOL(ex_handler_default); · line 43

Quoted 1 line

arch/x86/mm/extable.c +1 -0

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

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

Signature · 4 lines

Regards/Gruss,
    Boris.

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

Andy Lutomirski

2016-05-29 16:42 UTC #25 lore raw

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

Quoted 26 lines · Borislav Petkov

Quoted 25 lines · 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 <[email protected]>
---
 arch/x86/mm/extable.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
arch/x86/mm/extable.c +5 -0
index 658292fdee5e..c1933471fce7 100644
@@ -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

Quoted 9 lines

arch/x86/mm/extable.c EXPORT_SYMBOL(ex_handler_default); · line 42

Quoted 6 lines

arch/x86/mm/extable.c +6 -0

+      * -- 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.

Quoted 3 lines

arch/x86/mm/extable.c EXPORT_SYMBOL(ex_handler_default); · line 43
Quoted 1 line
arch/x86/mm/extable.c +1 -0
+             return true;  /* no good reason to OOPS. */
You love those side comments, don'tcha? :-)

ECO tip #102: avoid silly newlines :-p

--Andy

Andy Lutomirski

author maintainer

2016-05-24 22:48 UTC #26 lore raw

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

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.

---
 arch/x86/mm/extable.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

arch/x86/mm/extable.c +18 -0

index c1933471fce7..818cc7ffef79 100644
@@ -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;
 }

Signature · 1 line

2.5.5

Andy Lutomirski

author maintainer

2016-05-24 22:49 UTC #27 lore raw

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

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().

---
 arch/x86/mm/extable.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

arch/x86/mm/extable.c +31 -0

index 818cc7ffef79..4bf3ab2b8be1 100644
@@ -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;
 }

Signature · 1 line

2.5.5

Borislav Petkov

maintainer

2016-05-25 15:33 UTC #28 lore raw

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

arch/x86/mm/extable.c static bool uaccess_fault_okay(int trapnr, unsigned long error_code, · line 80

Quoted 8 lines · Andy Lutomirski

arch/x86/mm/extable.c +8 -0

+	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.

Quoted 9 lines

arch/x86/mm/extable.c +9 -0

+				  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);

Signature · 4 lines

Regards/Gruss,
    Boris.

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

Andy Lutomirski

2016-05-29 16:52 UTC #29 lore raw

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

Quoted 10 lines · Borislav Petkov

arch/x86/mm/extable.c static bool uaccess_fault_okay(int trapnr, unsigned long error_code, · line 80

Quoted 8 lines · Andy Lutomirski

arch/x86/mm/extable.c +8 -0

+     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",

Quoted 4 lines

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

Done.

Brian Gerst

2016-05-25 03:55 UTC #30 lore raw

Quoted 24 lines · 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()

Quoted 12 lines

 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

Kees Cook

2016-05-25 17:19 UTC #31 lore raw

Quoted 46 lines · Brian Gerst

Quoted 24 lines · 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()
Quoted 12 lines
 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.

Quoted 6 lines

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

Signature · 2 lines

Kees Cook
Chrome OS & Brillo Security

Kees Cook

Reviewed-by on the v1 series

2016-05-25 17:32 UTC #32 lore raw