Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1406498 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/7] x86: uaccess hardening, easy part Andy Lutomirski <luto@kernel.org> - 2016-05-25 00:50 +0200
    [PATCH 5/7] x86/uaccess: Warn on uaccess faults other than #PF Andy Lutomirski <luto@kernel.org> - 2016-05-25 00:50 +0200
      Re: [PATCH 5/7] x86/uaccess: Warn on uaccess faults other than #PF Borislav Petkov <bp@alien8.de> - 2016-05-25 11:50 +0200
        Re: [PATCH 5/7] x86/uaccess: Warn on uaccess faults other than #PF Andy Lutomirski <luto@amacapital.net> - 2016-05-29 18:50 +0200
    [PATCH 1/7] x86/xen: Simplify set_aliased_prot Andy Lutomirski <luto@kernel.org> - 2016-05-25 01:00 +0200
      Re: [PATCH 1/7] x86/xen: Simplify set_aliased_prot Andrew Cooper <andrew.cooper3@citrix.com> - 2016-05-25 11:40 +0200
      Re: [Xen-devel] [PATCH 1/7] x86/xen: Simplify set_aliased_prot David Vrabel <david.vrabel@citrix.com> - 2016-05-25 12:00 +0200
    [PATCH 3/7] x86/uaccess: Give uaccess faults their own handler Andy Lutomirski <luto@kernel.org> - 2016-05-25 01:00 +0200
    [PATCH 2/7] x86/extable: Pass error_code and an extra unsigned long to exhandlers Andy Lutomirski <luto@kernel.org> - 2016-05-25 01:00 +0200
    Re: [PATCH 0/7] x86: uaccess hardening, easy part Brian Gerst <brgerst@gmail.com> - 2016-05-25 06:00 +0200
      Re: [PATCH 0/7] x86: uaccess hardening, easy part Kees Cook <keescook@chromium.org> - 2016-05-25 19:20 +0200
    Re: [PATCH 0/7] x86: uaccess hardening, easy part Kees Cook <keescook@chromium.org> - 2016-05-25 19:40 +0200

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

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

[toc] | [next] | [standalone]

#1406500 — [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.

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1406502 — [PATCH 1/7] x86/xen: Simplify set_aliased_prot

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

[toc] | [prev] | [next] | [standalone]

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

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>

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1406504 — [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.

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

[toc] | [prev] | [next] | [standalone]

#1406507 — [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.

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

[toc] | [prev] | [next] | [standalone]

#1406606

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

[toc] | [prev] | [next] | [standalone]

#1407081

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

[toc] | [prev] | [next] | [standalone]

#1407101

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web