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

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Andy Lutomirski-3
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

Reply | Threaded
Open this post in threaded view
|

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

Andy Lutomirski-3
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 <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|

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

Andy Lutomirski-3
In reply to this post by Andy Lutomirski-3
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 <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|

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

Andy Lutomirski-3
In reply to this post by Andy Lutomirski-3
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 <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|

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

Andy Lutomirski-3
In reply to this post by Andy Lutomirski-3
This will help debug OOPSes related to USER_DS vs KERNEL_DS.

Signed-off-by: Andy Lutomirski <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|

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

Andy Lutomirski-3
In reply to this post by Andy Lutomirski-3
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 <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|

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

Andy Lutomirski-3
In reply to this post by Andy Lutomirski-3
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 <[hidden email]>
Cc: Boris Ostrovsky <[hidden email]>
Cc: David Vrabel <[hidden email]>
Cc: Jan Beulich <[hidden email]>
Cc: Konrad Rzeszutek Wilk <[hidden email]>
Cc: xen-devel <[hidden email]>
Signed-off-by: Andy Lutomirski <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|

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

Andy Lutomirski-3
In reply to this post by Andy Lutomirski-3
This will give us a single piece of code to edit to harden our
uaccess fault handling.

Signed-off-by: Andy Lutomirski <[hidden email]>
---
 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

Reply | Threaded
Open this post in threaded view
|

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

Brian Gerst-2
In reply to this post by Andy Lutomirski-3
On Tue, May 24, 2016 at 6:48 PM, Andy Lutomirski <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Andrew Cooper
In reply to this post by Andy Lutomirski-3
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 <[hidden email]>
> Cc: Boris Ostrovsky <[hidden email]>
> Cc: David Vrabel <[hidden email]>
> Cc: Jan Beulich <[hidden email]>
> Cc: Konrad Rzeszutek Wilk <[hidden email]>
> Cc: xen-devel <[hidden email]>
> Signed-off-by: Andy Lutomirski <[hidden email]>

Reviewed-by: Andrew Cooper <[hidden email]>
Reply | Threaded
Open this post in threaded view
|

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

Borislav Petkov-3
In reply to this post by Andy Lutomirski-3
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 <[hidden email]>
> ---
>  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.
Reply | Threaded
Open this post in threaded view
|

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

David Vrabel
In reply to this post by Andy Lutomirski-3
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 <[hidden email]>

David
Reply | Threaded
Open this post in threaded view
|

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

Borislav Petkov-3
In reply to this post by Andy Lutomirski-3
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 <[hidden email]>
> ---
>  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.
Reply | Threaded
Open this post in threaded view
|

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

Borislav Petkov-3
In reply to this post by Andy Lutomirski-3
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 <[hidden email]>
> ---
>  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.
Reply | Threaded
Open this post in threaded view
|

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

Borislav Petkov-3
In reply to this post by Andy Lutomirski-3
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.
Reply | Threaded
Open this post in threaded view
|

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

Kees Cook
In reply to this post by Brian Gerst-2
On Tue, May 24, 2016 at 8:55 PM, Brian Gerst <[hidden email]> wrote:

> On Tue, May 24, 2016 at 6:48 PM, Andy Lutomirski <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Kees Cook
In reply to this post by Andy Lutomirski-3
On Tue, May 24, 2016 at 3:48 PM, Andy Lutomirski <[hidden email]> 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 <[hidden email]>

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