[PATCH 1/3] x86/insn-eval: Add support for 64-bit kernel mode

[PATCH 1/3] x86/insn-eval: Add support for 64-bit kernel mode

[Jann Horn]

To support evaluating 64-bit kernel mode instructions:

Replace existing checks for user_64bit_mode() with a new helper that
checks whether code is being executed in either 64-bit kernel mode or
64-bit user mode.

Select the GS base depending on whether the instruction is being
evaluated in kernel mode.

Signed-off-by: Jann Horn <jannh@google.com>
---
 arch/x86/include/asm/ptrace.h | 13 +++++++++++++
 arch/x86/lib/insn-eval.c      | 26 +++++++++++++++-----------
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
index 5057a8ed100b..ac45b06941a5 100644
--- a/arch/x86/include/asm/ptrace.h
+++ b/arch/x86/include/asm/ptrace.h
@@ -159,6 +159,19 @@ static inline bool user_64bit_mode(struct pt_regs *regs)
 #endif
 }
 
+/*
+ * Determine whether the register set came from any context that is running in
+ * 64-bit mode.
+ */
+static inline bool any_64bit_mode(struct pt_regs *regs)
+{
+#ifdef CONFIG_X86_64
+	return !user_mode(regs) || user_64bit_mode(regs);
+#else
+	return false;
+#endif
+}
+
 #ifdef CONFIG_X86_64
 #define current_user_stack_pointer()	current_pt_regs()->sp
 #define compat_user_stack_pointer()	current_pt_regs()->sp
diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
index 306c3a0902ba..31600d851fd8 100644
--- a/arch/x86/lib/insn-eval.c
+++ b/arch/x86/lib/insn-eval.c
@@ -155,7 +155,7 @@ static bool check_seg_overrides(struct insn *insn, int regoff)
  */
 static int resolve_default_seg(struct insn *insn, struct pt_regs *regs, int off)
 {
-	if (user_64bit_mode(regs))
+	if (any_64bit_mode(regs))
 		return INAT_SEG_REG_IGNORE;
 	/*
 	 * Resolve the default segment register as described in Section 3.7.4
@@ -266,7 +266,7 @@ static int resolve_seg_reg(struct insn *insn, struct pt_regs *regs, int regoff)
 	 * which may be invalid at this point.
 	 */
 	if (regoff == offsetof(struct pt_regs, ip)) {
-		if (user_64bit_mode(regs))
+		if (any_64bit_mode(regs))
 			return INAT_SEG_REG_IGNORE;
 		else
 			return INAT_SEG_REG_CS;
@@ -289,7 +289,7 @@ static int resolve_seg_reg(struct insn *insn, struct pt_regs *regs, int regoff)
 	 * In long mode, segment override prefixes are ignored, except for
 	 * overrides for FS and GS.
 	 */
-	if (user_64bit_mode(regs)) {
+	if (any_64bit_mode(regs)) {
 		if (idx != INAT_SEG_REG_FS &&
 		    idx != INAT_SEG_REG_GS)
 			idx = INAT_SEG_REG_IGNORE;
@@ -646,23 +646,27 @@ unsigned long insn_get_seg_base(struct pt_regs *regs, int seg_reg_idx)
 		 */
 		return (unsigned long)(sel << 4);
 
-	if (user_64bit_mode(regs)) {
+	if (any_64bit_mode(regs)) {
 		/*
 		 * Only FS or GS will have a base address, the rest of
 		 * the segments' bases are forced to 0.
 		 */
 		unsigned long base;
 
-		if (seg_reg_idx == INAT_SEG_REG_FS)
+		if (seg_reg_idx == INAT_SEG_REG_FS) {
 			rdmsrl(MSR_FS_BASE, base);
-		else if (seg_reg_idx == INAT_SEG_REG_GS)
+		} else if (seg_reg_idx == INAT_SEG_REG_GS) {
 			/*
 			 * swapgs was called at the kernel entry point. Thus,
 			 * MSR_KERNEL_GS_BASE will have the user-space GS base.
 			 */
-			rdmsrl(MSR_KERNEL_GS_BASE, base);
-		else
+			if (user_mode(regs))
+				rdmsrl(MSR_KERNEL_GS_BASE, base);
+			else
+				rdmsrl(MSR_GS_BASE, base);
+		} else {
 			base = 0;
+		}
 		return base;
 	}
 
@@ -703,7 +707,7 @@ static unsigned long get_seg_limit(struct pt_regs *regs, int seg_reg_idx)
 	if (sel < 0)
 		return 0;
 
-	if (user_64bit_mode(regs) || v8086_mode(regs))
+	if (any_64bit_mode(regs) || v8086_mode(regs))
 		return -1L;
 
 	if (!sel)
@@ -948,7 +952,7 @@ static int get_eff_addr_modrm(struct insn *insn, struct pt_regs *regs,
 	 * following instruction.
 	 */
 	if (*regoff == -EDOM) {
-		if (user_64bit_mode(regs))
+		if (any_64bit_mode(regs))
 			tmp = regs->ip + insn->length;
 		else
 			tmp = 0;
@@ -1250,7 +1254,7 @@ static void __user *get_addr_ref_32(struct insn *insn, struct pt_regs *regs)
 	 * After computed, the effective address is treated as an unsigned
 	 * quantity.
 	 */
-	if (!user_64bit_mode(regs) && ((unsigned int)eff_addr > seg_limit))
+	if (!any_64bit_mode(regs) && ((unsigned int)eff_addr > seg_limit))
 		goto out;
 
 	/*
-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH 2/3] x86/traps: Print non-canonical address on #GP

[Jann Horn]

A frequent cause of #GP exceptions are memory accesses to non-canonical
addresses. Unlike #PF, #GP doesn't come with a fault address in CR2, so
the kernel doesn't currently print the fault address for #GP.
Luckily, we already have the necessary infrastructure for decoding X86
instructions and computing the memory address that is being accessed;
hook it up to the #GP handler so that we can figure out whether the #GP
looks like it was caused by a non-canonical address, and if so, print
that address.

While it is already possible to compute the faulting address manually by
disassembling the opcode dump and evaluating the instruction against the
register dump, this should make it slightly easier to identify crashes
at a glance.

Signed-off-by: Jann Horn <jannh@google.com>
---
 arch/x86/kernel/traps.c | 45 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index c90312146da0..479cfc6e9507 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -56,6 +56,8 @@
 #include <asm/mpx.h>
 #include <asm/vm86.h>
 #include <asm/umip.h>
+#include <asm/insn.h>
+#include <asm/insn-eval.h>
 
 #ifdef CONFIG_X86_64
 #include <asm/x86_init.h>
@@ -509,6 +511,42 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long error_code)
 	do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs, error_code, 0, NULL);
 }
 
+/*
+ * On 64-bit, if an uncaught #GP occurs while dereferencing a non-canonical
+ * address, print that address.
+ */
+static void print_kernel_gp_address(struct pt_regs *regs)
+{
+#ifdef CONFIG_X86_64
+	u8 insn_bytes[MAX_INSN_SIZE];
+	struct insn insn;
+	unsigned long addr_ref;
+
+	if (probe_kernel_read(insn_bytes, (void *)regs->ip, MAX_INSN_SIZE))
+		return;
+
+	kernel_insn_init(&insn, insn_bytes, MAX_INSN_SIZE);
+	insn_get_modrm(&insn);
+	insn_get_sib(&insn);
+	addr_ref = (unsigned long)insn_get_addr_ref(&insn, regs);
+
+	/*
+	 * If insn_get_addr_ref() failed or we got a canonical address in the
+	 * kernel half, bail out.
+	 */
+	if ((addr_ref | __VIRTUAL_MASK) == ~0UL)
+		return;
+	/*
+	 * For the user half, check against TASK_SIZE_MAX; this way, if the
+	 * access crosses the canonical address boundary, we don't miss it.
+	 */
+	if (addr_ref <= TASK_SIZE_MAX)
+		return;
+
+	pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref);
+#endif
+}
+
 dotraplinkage void
 do_general_protection(struct pt_regs *regs, long error_code)
 {
@@ -547,8 +585,11 @@ do_general_protection(struct pt_regs *regs, long error_code)
 			return;
 
 		if (notify_die(DIE_GPF, desc, regs, error_code,
-			       X86_TRAP_GP, SIGSEGV) != NOTIFY_STOP)
-			die(desc, regs, error_code);
+			       X86_TRAP_GP, SIGSEGV) == NOTIFY_STOP)
+			return;
+
+		print_kernel_gp_address(regs);
+		die(desc, regs, error_code);
 		return;
 	}
 
-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH 3/3] x86/kasan: Print original address on #GP

[Jann Horn]

Make #GP exceptions caused by out-of-bounds KASAN shadow accesses easier
to understand by computing the address of the original access and
printing that. More details are in the comments in the patch.

This turns an error like this:

    kasan: CONFIG_KASAN_INLINE enabled
    kasan: GPF could be caused by NULL-ptr deref or user memory access
    traps: dereferencing non-canonical address 0xe017577ddf75b7dd
    general protection fault: 0000 [#1] PREEMPT SMP KASAN PTI

into this:

    traps: dereferencing non-canonical address 0xe017577ddf75b7dd
    kasan: maybe dereferencing invalid pointer in range
            [0x00badbeefbadbee8-0x00badbeefbadbeef]
    general protection fault: 0000 [#3] PREEMPT SMP KASAN PTI
    [...]

Signed-off-by: Jann Horn <jannh@google.com>
---
 arch/x86/include/asm/kasan.h |  6 +++++
 arch/x86/kernel/traps.c      |  2 ++
 arch/x86/mm/kasan_init_64.c  | 52 +++++++++++++++++++++++++-----------
 3 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/kasan.h b/arch/x86/include/asm/kasan.h
index 13e70da38bed..eaf624a758ed 100644
--- a/arch/x86/include/asm/kasan.h
+++ b/arch/x86/include/asm/kasan.h
@@ -25,6 +25,12 @@
 
 #ifndef __ASSEMBLY__
 
+#ifdef CONFIG_KASAN_INLINE
+void kasan_general_protection_hook(unsigned long addr);
+#else
+static inline void kasan_general_protection_hook(unsigned long addr) { }
+#endif
+
 #ifdef CONFIG_KASAN
 void __init kasan_early_init(void);
 void __init kasan_init(void);
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 479cfc6e9507..e271a5a1ddd4 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -58,6 +58,7 @@
 #include <asm/umip.h>
 #include <asm/insn.h>
 #include <asm/insn-eval.h>
+#include <asm/kasan.h>
 
 #ifdef CONFIG_X86_64
 #include <asm/x86_init.h>
@@ -544,6 +545,7 @@ static void print_kernel_gp_address(struct pt_regs *regs)
 		return;
 
 	pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref);
+	kasan_general_protection_hook(addr_ref);
 #endif
 }
 
diff --git a/arch/x86/mm/kasan_init_64.c b/arch/x86/mm/kasan_init_64.c
index 296da58f3013..9ef099309489 100644
--- a/arch/x86/mm/kasan_init_64.c
+++ b/arch/x86/mm/kasan_init_64.c
@@ -246,20 +246,44 @@ static void __init kasan_map_early_shadow(pgd_t *pgd)
 }
 
 #ifdef CONFIG_KASAN_INLINE
-static int kasan_die_handler(struct notifier_block *self,
-			     unsigned long val,
-			     void *data)
+/*
+ * With CONFIG_KASAN_INLINE, accesses to bogus pointers (outside the high
+ * canonical half of the address space) cause out-of-bounds shadow memory reads
+ * before the actual access. For addresses in the low canonical half of the
+ * address space, as well as most non-canonical addresses, that out-of-bounds
+ * shadow memory access lands in the non-canonical part of the address space,
+ * causing #GP to be thrown.
+ * Help the user figure out what the original bogus pointer was.
+ */
+void kasan_general_protection_hook(unsigned long addr)
 {
-	if (val == DIE_GPF) {
-		pr_emerg("CONFIG_KASAN_INLINE enabled\n");
-		pr_emerg("GPF could be caused by NULL-ptr deref or user memory access\n");
-	}
-	return NOTIFY_OK;
-}
+	unsigned long orig_addr;
+	const char *addr_type;
+
+	if (addr < KASAN_SHADOW_OFFSET)
+		return;
 
-static struct notifier_block kasan_die_notifier = {
-	.notifier_call = kasan_die_handler,
-};
+	orig_addr = (addr - KASAN_SHADOW_OFFSET) << KASAN_SHADOW_SCALE_SHIFT;
+	/*
+	 * For faults near the shadow address for NULL, we can be fairly certain
+	 * that this is a KASAN shadow memory access.
+	 * For faults that correspond to shadow for low canonical addresses, we
+	 * can still be pretty sure - that shadow region is a fairly narrow
+	 * chunk of the non-canonical address space.
+	 * But faults that look like shadow for non-canonical addresses are a
+	 * really large chunk of the address space. In that case, we still
+	 * print the decoded address, but make it clear that this is not
+	 * necessarily what's actually going on.
+	 */
+	if (orig_addr < PAGE_SIZE)
+		addr_type = "dereferencing kernel NULL pointer";
+	else if (orig_addr < TASK_SIZE_MAX)
+		addr_type = "probably dereferencing invalid pointer";
+	else
+		addr_type = "maybe dereferencing invalid pointer";
+	pr_alert("%s in range [0x%016lx-0x%016lx]\n", addr_type,
+		 orig_addr, orig_addr + (1 << KASAN_SHADOW_SCALE_SHIFT) - 1);
+}
 #endif
 
 void __init kasan_early_init(void)
@@ -298,10 +322,6 @@ void __init kasan_init(void)
 	int i;
 	void *shadow_cpu_entry_begin, *shadow_cpu_entry_end;
 
-#ifdef CONFIG_KASAN_INLINE
-	register_die_notifier(&kasan_die_notifier);
-#endif
-
 	memcpy(early_top_pgt, init_top_pgt, sizeof(early_top_pgt));
 
 	/*
-- 
2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH 3/3] x86/kasan: Print original address on #GP

[Dmitry Vyukov]

On Tue, Nov 12, 2019 at 10:10 PM 'Jann Horn' via kasan-dev
<kasan-dev@googlegroups.com> wrote:
>
> Make #GP exceptions caused by out-of-bounds KASAN shadow accesses easier
> to understand by computing the address of the original access and
> printing that. More details are in the comments in the patch.
>
> This turns an error like this:
>
>     kasan: CONFIG_KASAN_INLINE enabled
>     kasan: GPF could be caused by NULL-ptr deref or user memory access
>     traps: dereferencing non-canonical address 0xe017577ddf75b7dd
>     general protection fault: 0000 [#1] PREEMPT SMP KASAN PTI
>
> into this:
>
>     traps: dereferencing non-canonical address 0xe017577ddf75b7dd
>     kasan: maybe dereferencing invalid pointer in range
>             [0x00badbeefbadbee8-0x00badbeefbadbeef]
>     general protection fault: 0000 [#3] PREEMPT SMP KASAN PTI
>     [...]

Nice!

+Andrey, do you see any issues for TAGS mode? Or, Jann, did you test
it by any chance?


> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>  arch/x86/include/asm/kasan.h |  6 +++++
>  arch/x86/kernel/traps.c      |  2 ++
>  arch/x86/mm/kasan_init_64.c  | 52 +++++++++++++++++++++++++-----------
>  3 files changed, 44 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/include/asm/kasan.h b/arch/x86/include/asm/kasan.h
> index 13e70da38bed..eaf624a758ed 100644
> --- a/arch/x86/include/asm/kasan.h
> +++ b/arch/x86/include/asm/kasan.h
> @@ -25,6 +25,12 @@
>
>  #ifndef __ASSEMBLY__
>
> +#ifdef CONFIG_KASAN_INLINE
> +void kasan_general_protection_hook(unsigned long addr);
> +#else
> +static inline void kasan_general_protection_hook(unsigned long addr) { }
> +#endif
> +
>  #ifdef CONFIG_KASAN
>  void __init kasan_early_init(void);
>  void __init kasan_init(void);
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 479cfc6e9507..e271a5a1ddd4 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -58,6 +58,7 @@
>  #include <asm/umip.h>
>  #include <asm/insn.h>
>  #include <asm/insn-eval.h>
> +#include <asm/kasan.h>
>
>  #ifdef CONFIG_X86_64
>  #include <asm/x86_init.h>
> @@ -544,6 +545,7 @@ static void print_kernel_gp_address(struct pt_regs *regs)
>                 return;
>
>         pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref);
> +       kasan_general_protection_hook(addr_ref);
>  #endif
>  }
>
> diff --git a/arch/x86/mm/kasan_init_64.c b/arch/x86/mm/kasan_init_64.c
> index 296da58f3013..9ef099309489 100644
> --- a/arch/x86/mm/kasan_init_64.c
> +++ b/arch/x86/mm/kasan_init_64.c
> @@ -246,20 +246,44 @@ static void __init kasan_map_early_shadow(pgd_t *pgd)
>  }
>
>  #ifdef CONFIG_KASAN_INLINE
> -static int kasan_die_handler(struct notifier_block *self,
> -                            unsigned long val,
> -                            void *data)
> +/*
> + * With CONFIG_KASAN_INLINE, accesses to bogus pointers (outside the high
> + * canonical half of the address space) cause out-of-bounds shadow memory reads
> + * before the actual access. For addresses in the low canonical half of the
> + * address space, as well as most non-canonical addresses, that out-of-bounds
> + * shadow memory access lands in the non-canonical part of the address space,
> + * causing #GP to be thrown.
> + * Help the user figure out what the original bogus pointer was.
> + */
> +void kasan_general_protection_hook(unsigned long addr)
>  {
> -       if (val == DIE_GPF) {
> -               pr_emerg("CONFIG_KASAN_INLINE enabled\n");
> -               pr_emerg("GPF could be caused by NULL-ptr deref or user memory access\n");
> -       }
> -       return NOTIFY_OK;
> -}
> +       unsigned long orig_addr;
> +       const char *addr_type;
> +
> +       if (addr < KASAN_SHADOW_OFFSET)
> +               return;

Thinking how much sense it makes to compare addr with KASAN_SHADOW_END...
If the addr is > KASAN_SHADOW_END, we know it's not a KASAN access,
but do we ever get GP on canonical addresses?

>
> -static struct notifier_block kasan_die_notifier = {
> -       .notifier_call = kasan_die_handler,
> -};
> +       orig_addr = (addr - KASAN_SHADOW_OFFSET) << KASAN_SHADOW_SCALE_SHIFT;
> +       /*
> +        * For faults near the shadow address for NULL, we can be fairly certain
> +        * that this is a KASAN shadow memory access.
> +        * For faults that correspond to shadow for low canonical addresses, we
> +        * can still be pretty sure - that shadow region is a fairly narrow
> +        * chunk of the non-canonical address space.
> +        * But faults that look like shadow for non-canonical addresses are a
> +        * really large chunk of the address space. In that case, we still
> +        * print the decoded address, but make it clear that this is not
> +        * necessarily what's actually going on.
> +        */
> +       if (orig_addr < PAGE_SIZE)
> +               addr_type = "dereferencing kernel NULL pointer";
> +       else if (orig_addr < TASK_SIZE_MAX)
> +               addr_type = "probably dereferencing invalid pointer";

This is access to user memory, right? In outline mode we call it
"user-memory-access". We could say about "user" part here as well.

> +       else
> +               addr_type = "maybe dereferencing invalid pointer";
> +       pr_alert("%s in range [0x%016lx-0x%016lx]\n", addr_type,
> +                orig_addr, orig_addr + (1 << KASAN_SHADOW_SCALE_SHIFT) - 1);

"(1 << KASAN_SHADOW_SCALE_SHIFT) - 1)" part may be replaced with
KASAN_SHADOW_MASK.
Overall it can make sense to move this mm/kasan/report.c b/c we are
open-coding a number of things here (e.g. reverse address mapping). If
another arch will do the same, it will need all of this code too (?).

But in general I think it's a very good usability improvement for KASAN.

> +}
>  #endif
>
>  void __init kasan_early_init(void)
> @@ -298,10 +322,6 @@ void __init kasan_init(void)
>         int i;
>         void *shadow_cpu_entry_begin, *shadow_cpu_entry_end;
>
> -#ifdef CONFIG_KASAN_INLINE
> -       register_die_notifier(&kasan_die_notifier);
> -#endif
> -
>         memcpy(early_top_pgt, init_top_pgt, sizeof(early_top_pgt));
>
>         /*
> --
> 2.24.0.432.g9d3f5f5b63-goog
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20191112211002.128278-3-jannh%40google.com.

Re: [PATCH 3/3] x86/kasan: Print original address on #GP

[Andrey Konovalov]

On Wed, Nov 13, 2019 at 11:11 AM 'Dmitry Vyukov' via kasan-dev
<kasan-dev@googlegroups.com> wrote:
>
> On Tue, Nov 12, 2019 at 10:10 PM 'Jann Horn' via kasan-dev
> <kasan-dev@googlegroups.com> wrote:
> >
> > Make #GP exceptions caused by out-of-bounds KASAN shadow accesses easier
> > to understand by computing the address of the original access and
> > printing that. More details are in the comments in the patch.
> >
> > This turns an error like this:
> >
> >     kasan: CONFIG_KASAN_INLINE enabled
> >     kasan: GPF could be caused by NULL-ptr deref or user memory access
> >     traps: dereferencing non-canonical address 0xe017577ddf75b7dd
> >     general protection fault: 0000 [#1] PREEMPT SMP KASAN PTI
> >
> > into this:
> >
> >     traps: dereferencing non-canonical address 0xe017577ddf75b7dd
> >     kasan: maybe dereferencing invalid pointer in range
> >             [0x00badbeefbadbee8-0x00badbeefbadbeef]
> >     general protection fault: 0000 [#3] PREEMPT SMP KASAN PTI
> >     [...]

Would it make sense to use the common "BUG: KASAN: <bug-type>" report
format here? Something like:

BUG: KASAN: invalid-ptr-deref in range ...

Otherwise this looks amazing, distinguishing NULL pointer accesses
from wild memory accesses is much more convenient with this. Thanks
Jann!

>
> Nice!
>
> +Andrey, do you see any issues for TAGS mode? Or, Jann, did you test
> it by any chance?

Hm, this looks like x86-specific change, so I don't think it
interferes with the TAGS mode.

>
>
> > Signed-off-by: Jann Horn <jannh@google.com>
> > ---
> >  arch/x86/include/asm/kasan.h |  6 +++++
> >  arch/x86/kernel/traps.c      |  2 ++
> >  arch/x86/mm/kasan_init_64.c  | 52 +++++++++++++++++++++++++-----------
> >  3 files changed, 44 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/kasan.h b/arch/x86/include/asm/kasan.h
> > index 13e70da38bed..eaf624a758ed 100644
> > --- a/arch/x86/include/asm/kasan.h
> > +++ b/arch/x86/include/asm/kasan.h
> > @@ -25,6 +25,12 @@
> >
> >  #ifndef __ASSEMBLY__
> >
> > +#ifdef CONFIG_KASAN_INLINE
> > +void kasan_general_protection_hook(unsigned long addr);
> > +#else
> > +static inline void kasan_general_protection_hook(unsigned long addr) { }
> > +#endif
> > +
> >  #ifdef CONFIG_KASAN
> >  void __init kasan_early_init(void);
> >  void __init kasan_init(void);
> > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > index 479cfc6e9507..e271a5a1ddd4 100644
> > --- a/arch/x86/kernel/traps.c
> > +++ b/arch/x86/kernel/traps.c
> > @@ -58,6 +58,7 @@
> >  #include <asm/umip.h>
> >  #include <asm/insn.h>
> >  #include <asm/insn-eval.h>
> > +#include <asm/kasan.h>
> >
> >  #ifdef CONFIG_X86_64
> >  #include <asm/x86_init.h>
> > @@ -544,6 +545,7 @@ static void print_kernel_gp_address(struct pt_regs *regs)
> >                 return;
> >
> >         pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref);
> > +       kasan_general_protection_hook(addr_ref);
> >  #endif
> >  }
> >
> > diff --git a/arch/x86/mm/kasan_init_64.c b/arch/x86/mm/kasan_init_64.c
> > index 296da58f3013..9ef099309489 100644
> > --- a/arch/x86/mm/kasan_init_64.c
> > +++ b/arch/x86/mm/kasan_init_64.c
> > @@ -246,20 +246,44 @@ static void __init kasan_map_early_shadow(pgd_t *pgd)
> >  }
> >
> >  #ifdef CONFIG_KASAN_INLINE
> > -static int kasan_die_handler(struct notifier_block *self,
> > -                            unsigned long val,
> > -                            void *data)
> > +/*
> > + * With CONFIG_KASAN_INLINE, accesses to bogus pointers (outside the high
> > + * canonical half of the address space) cause out-of-bounds shadow memory reads
> > + * before the actual access. For addresses in the low canonical half of the
> > + * address space, as well as most non-canonical addresses, that out-of-bounds
> > + * shadow memory access lands in the non-canonical part of the address space,
> > + * causing #GP to be thrown.
> > + * Help the user figure out what the original bogus pointer was.
> > + */
> > +void kasan_general_protection_hook(unsigned long addr)
> >  {
> > -       if (val == DIE_GPF) {
> > -               pr_emerg("CONFIG_KASAN_INLINE enabled\n");
> > -               pr_emerg("GPF could be caused by NULL-ptr deref or user memory access\n");
> > -       }
> > -       return NOTIFY_OK;
> > -}
> > +       unsigned long orig_addr;
> > +       const char *addr_type;
> > +
> > +       if (addr < KASAN_SHADOW_OFFSET)
> > +               return;
>
> Thinking how much sense it makes to compare addr with KASAN_SHADOW_END...
> If the addr is > KASAN_SHADOW_END, we know it's not a KASAN access,
> but do we ever get GP on canonical addresses?
>
> >
> > -static struct notifier_block kasan_die_notifier = {
> > -       .notifier_call = kasan_die_handler,
> > -};
> > +       orig_addr = (addr - KASAN_SHADOW_OFFSET) << KASAN_SHADOW_SCALE_SHIFT;
> > +       /*
> > +        * For faults near the shadow address for NULL, we can be fairly certain
> > +        * that this is a KASAN shadow memory access.
> > +        * For faults that correspond to shadow for low canonical addresses, we
> > +        * can still be pretty sure - that shadow region is a fairly narrow
> > +        * chunk of the non-canonical address space.
> > +        * But faults that look like shadow for non-canonical addresses are a
> > +        * really large chunk of the address space. In that case, we still
> > +        * print the decoded address, but make it clear that this is not
> > +        * necessarily what's actually going on.
> > +        */
> > +       if (orig_addr < PAGE_SIZE)
> > +               addr_type = "dereferencing kernel NULL pointer";
> > +       else if (orig_addr < TASK_SIZE_MAX)
> > +               addr_type = "probably dereferencing invalid pointer";
>
> This is access to user memory, right? In outline mode we call it
> "user-memory-access". We could say about "user" part here as well.

I think we should use the same naming scheme here as in
get_wild_bug_type(): null-ptr-deref, user-memory-access and
wild-memory-access.

>
> > +       else
> > +               addr_type = "maybe dereferencing invalid pointer";
> > +       pr_alert("%s in range [0x%016lx-0x%016lx]\n", addr_type,
> > +                orig_addr, orig_addr + (1 << KASAN_SHADOW_SCALE_SHIFT) - 1);
>
> "(1 << KASAN_SHADOW_SCALE_SHIFT) - 1)" part may be replaced with
> KASAN_SHADOW_MASK.
> Overall it can make sense to move this mm/kasan/report.c b/c we are
> open-coding a number of things here (e.g. reverse address mapping). If
> another arch will do the same, it will need all of this code too (?).
>
> But in general I think it's a very good usability improvement for KASAN.
>
> > +}
> >  #endif
> >
> >  void __init kasan_early_init(void)
> > @@ -298,10 +322,6 @@ void __init kasan_init(void)
> >         int i;
> >         void *shadow_cpu_entry_begin, *shadow_cpu_entry_end;
> >
> > -#ifdef CONFIG_KASAN_INLINE
> > -       register_die_notifier(&kasan_die_notifier);
> > -#endif
> > -
> >         memcpy(early_top_pgt, init_top_pgt, sizeof(early_top_pgt));
> >
> >         /*
> > --
> > 2.24.0.432.g9d3f5f5b63-goog
> >
> > --
> > You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20191112211002.128278-3-jannh%40google.com.
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/CACT4Y%2BaojSsss3%2BY2FB9Rw%3DOPxXgsFrGF0YiAJ9eo2wJM0ruWg%40mail.gmail.com.

Re: [PATCH 3/3] x86/kasan: Print original address on #GP

[Dmitry Vyukov]

On Wed, Nov 13, 2019 at 4:19 PM Andrey Konovalov <andreyknvl@google.com> wrote:
>
> On Wed, Nov 13, 2019 at 11:11 AM 'Dmitry Vyukov' via kasan-dev
> <kasan-dev@googlegroups.com> wrote:
> >
> > On Tue, Nov 12, 2019 at 10:10 PM 'Jann Horn' via kasan-dev
> > <kasan-dev@googlegroups.com> wrote:
> > >
> > > Make #GP exceptions caused by out-of-bounds KASAN shadow accesses easier
> > > to understand by computing the address of the original access and
> > > printing that. More details are in the comments in the patch.
> > >
> > > This turns an error like this:
> > >
> > >     kasan: CONFIG_KASAN_INLINE enabled
> > >     kasan: GPF could be caused by NULL-ptr deref or user memory access
> > >     traps: dereferencing non-canonical address 0xe017577ddf75b7dd
> > >     general protection fault: 0000 [#1] PREEMPT SMP KASAN PTI
> > >
> > > into this:
> > >
> > >     traps: dereferencing non-canonical address 0xe017577ddf75b7dd
> > >     kasan: maybe dereferencing invalid pointer in range
> > >             [0x00badbeefbadbee8-0x00badbeefbadbeef]
> > >     general protection fault: 0000 [#3] PREEMPT SMP KASAN PTI
> > >     [...]
>
> Would it make sense to use the common "BUG: KASAN: <bug-type>" report
> format here? Something like:
>
> BUG: KASAN: invalid-ptr-deref in range ...


Currently this line is not the official bug title. The official bug
title is "general protection fault:" line that follows.
If we add "BUG: KASAN:" before that we need to be super careful wrt
effect on syzbot but parsing/reporting.



> Otherwise this looks amazing, distinguishing NULL pointer accesses
> from wild memory accesses is much more convenient with this. Thanks
> Jann!
>
> >
> > Nice!
> >
> > +Andrey, do you see any issues for TAGS mode? Or, Jann, did you test
> > it by any chance?
>
> Hm, this looks like x86-specific change, so I don't think it
> interferes with the TAGS mode.
>
> >
> >
> > > Signed-off-by: Jann Horn <jannh@google.com>
> > > ---
> > >  arch/x86/include/asm/kasan.h |  6 +++++
> > >  arch/x86/kernel/traps.c      |  2 ++
> > >  arch/x86/mm/kasan_init_64.c  | 52 +++++++++++++++++++++++++-----------
> > >  3 files changed, 44 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/arch/x86/include/asm/kasan.h b/arch/x86/include/asm/kasan.h
> > > index 13e70da38bed..eaf624a758ed 100644
> > > --- a/arch/x86/include/asm/kasan.h
> > > +++ b/arch/x86/include/asm/kasan.h
> > > @@ -25,6 +25,12 @@
> > >
> > >  #ifndef __ASSEMBLY__
> > >
> > > +#ifdef CONFIG_KASAN_INLINE
> > > +void kasan_general_protection_hook(unsigned long addr);
> > > +#else
> > > +static inline void kasan_general_protection_hook(unsigned long addr) { }
> > > +#endif
> > > +
> > >  #ifdef CONFIG_KASAN
> > >  void __init kasan_early_init(void);
> > >  void __init kasan_init(void);
> > > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > > index 479cfc6e9507..e271a5a1ddd4 100644
> > > --- a/arch/x86/kernel/traps.c
> > > +++ b/arch/x86/kernel/traps.c
> > > @@ -58,6 +58,7 @@
> > >  #include <asm/umip.h>
> > >  #include <asm/insn.h>
> > >  #include <asm/insn-eval.h>
> > > +#include <asm/kasan.h>
> > >
> > >  #ifdef CONFIG_X86_64
> > >  #include <asm/x86_init.h>
> > > @@ -544,6 +545,7 @@ static void print_kernel_gp_address(struct pt_regs *regs)
> > >                 return;
> > >
> > >         pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref);
> > > +       kasan_general_protection_hook(addr_ref);
> > >  #endif
> > >  }
> > >
> > > diff --git a/arch/x86/mm/kasan_init_64.c b/arch/x86/mm/kasan_init_64.c
> > > index 296da58f3013..9ef099309489 100644
> > > --- a/arch/x86/mm/kasan_init_64.c
> > > +++ b/arch/x86/mm/kasan_init_64.c
> > > @@ -246,20 +246,44 @@ static void __init kasan_map_early_shadow(pgd_t *pgd)
> > >  }
> > >
> > >  #ifdef CONFIG_KASAN_INLINE
> > > -static int kasan_die_handler(struct notifier_block *self,
> > > -                            unsigned long val,
> > > -                            void *data)
> > > +/*
> > > + * With CONFIG_KASAN_INLINE, accesses to bogus pointers (outside the high
> > > + * canonical half of the address space) cause out-of-bounds shadow memory reads
> > > + * before the actual access. For addresses in the low canonical half of the
> > > + * address space, as well as most non-canonical addresses, that out-of-bounds
> > > + * shadow memory access lands in the non-canonical part of the address space,
> > > + * causing #GP to be thrown.
> > > + * Help the user figure out what the original bogus pointer was.
> > > + */
> > > +void kasan_general_protection_hook(unsigned long addr)
> > >  {
> > > -       if (val == DIE_GPF) {
> > > -               pr_emerg("CONFIG_KASAN_INLINE enabled\n");
> > > -               pr_emerg("GPF could be caused by NULL-ptr deref or user memory access\n");
> > > -       }
> > > -       return NOTIFY_OK;
> > > -}
> > > +       unsigned long orig_addr;
> > > +       const char *addr_type;
> > > +
> > > +       if (addr < KASAN_SHADOW_OFFSET)
> > > +               return;
> >
> > Thinking how much sense it makes to compare addr with KASAN_SHADOW_END...
> > If the addr is > KASAN_SHADOW_END, we know it's not a KASAN access,
> > but do we ever get GP on canonical addresses?
> >
> > >
> > > -static struct notifier_block kasan_die_notifier = {
> > > -       .notifier_call = kasan_die_handler,
> > > -};
> > > +       orig_addr = (addr - KASAN_SHADOW_OFFSET) << KASAN_SHADOW_SCALE_SHIFT;
> > > +       /*
> > > +        * For faults near the shadow address for NULL, we can be fairly certain
> > > +        * that this is a KASAN shadow memory access.
> > > +        * For faults that correspond to shadow for low canonical addresses, we
> > > +        * can still be pretty sure - that shadow region is a fairly narrow
> > > +        * chunk of the non-canonical address space.
> > > +        * But faults that look like shadow for non-canonical addresses are a
> > > +        * really large chunk of the address space. In that case, we still
> > > +        * print the decoded address, but make it clear that this is not
> > > +        * necessarily what's actually going on.
> > > +        */
> > > +       if (orig_addr < PAGE_SIZE)
> > > +               addr_type = "dereferencing kernel NULL pointer";
> > > +       else if (orig_addr < TASK_SIZE_MAX)
> > > +               addr_type = "probably dereferencing invalid pointer";
> >
> > This is access to user memory, right? In outline mode we call it
> > "user-memory-access". We could say about "user" part here as well.
>
> I think we should use the same naming scheme here as in
> get_wild_bug_type(): null-ptr-deref, user-memory-access and
> wild-memory-access.
>
> >
> > > +       else
> > > +               addr_type = "maybe dereferencing invalid pointer";
> > > +       pr_alert("%s in range [0x%016lx-0x%016lx]\n", addr_type,
> > > +                orig_addr, orig_addr + (1 << KASAN_SHADOW_SCALE_SHIFT) - 1);
> >
> > "(1 << KASAN_SHADOW_SCALE_SHIFT) - 1)" part may be replaced with
> > KASAN_SHADOW_MASK.
> > Overall it can make sense to move this mm/kasan/report.c b/c we are
> > open-coding a number of things here (e.g. reverse address mapping). If
> > another arch will do the same, it will need all of this code too (?).
> >
> > But in general I think it's a very good usability improvement for KASAN.
> >
> > > +}
> > >  #endif
> > >
> > >  void __init kasan_early_init(void)
> > > @@ -298,10 +322,6 @@ void __init kasan_init(void)
> > >         int i;
> > >         void *shadow_cpu_entry_begin, *shadow_cpu_entry_end;
> > >
> > > -#ifdef CONFIG_KASAN_INLINE
> > > -       register_die_notifier(&kasan_die_notifier);
> > > -#endif
> > > -
> > >         memcpy(early_top_pgt, init_top_pgt, sizeof(early_top_pgt));
> > >
> > >         /*
> > > --
> > > 2.24.0.432.g9d3f5f5b63-goog
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> > > To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20191112211002.128278-3-jannh%40google.com.
> >
> > --
> > You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/CACT4Y%2BaojSsss3%2BY2FB9Rw%3DOPxXgsFrGF0YiAJ9eo2wJM0ruWg%40mail.gmail.com.

Re: [PATCH 3/3] x86/kasan: Print original address on #GP

[Jann Horn]

On Wed, Nov 13, 2019 at 11:11 AM Dmitry Vyukov <dvyukov@google.com> wrote:
> On Tue, Nov 12, 2019 at 10:10 PM 'Jann Horn' via kasan-dev
> <kasan-dev@googlegroups.com> wrote:
> > Make #GP exceptions caused by out-of-bounds KASAN shadow accesses easier
> > to understand by computing the address of the original access and
> > printing that. More details are in the comments in the patch.
[...]
> +Andrey, do you see any issues for TAGS mode? Or, Jann, did you test
> it by any chance?

No, I didn't - I don't have anything set up for upstream arm64 testing here.

> > +void kasan_general_protection_hook(unsigned long addr)
> >  {
> > -       if (val == DIE_GPF) {
> > -               pr_emerg("CONFIG_KASAN_INLINE enabled\n");
> > -               pr_emerg("GPF could be caused by NULL-ptr deref or user memory access\n");
> > -       }
> > -       return NOTIFY_OK;
> > -}
> > +       unsigned long orig_addr;
> > +       const char *addr_type;
> > +
> > +       if (addr < KASAN_SHADOW_OFFSET)
> > +               return;
>
> Thinking how much sense it makes to compare addr with KASAN_SHADOW_END...
> If the addr is > KASAN_SHADOW_END, we know it's not a KASAN access,
> but do we ever get GP on canonical addresses?

#GP can occur for various reasons, but on x86-64, if it occurs because
of an invalid address, as far as I know it's always non-canonical. The
#GP handler I wrote will check the address and only call the KASAN
hook if the address is noncanonical (because otherwise the #GP
occurred for some other reason).

> > -static struct notifier_block kasan_die_notifier = {
> > -       .notifier_call = kasan_die_handler,
> > -};
> > +       orig_addr = (addr - KASAN_SHADOW_OFFSET) << KASAN_SHADOW_SCALE_SHIFT;
> > +       /*
> > +        * For faults near the shadow address for NULL, we can be fairly certain
> > +        * that this is a KASAN shadow memory access.
> > +        * For faults that correspond to shadow for low canonical addresses, we
> > +        * can still be pretty sure - that shadow region is a fairly narrow
> > +        * chunk of the non-canonical address space.
> > +        * But faults that look like shadow for non-canonical addresses are a
> > +        * really large chunk of the address space. In that case, we still
> > +        * print the decoded address, but make it clear that this is not
> > +        * necessarily what's actually going on.
> > +        */
> > +       if (orig_addr < PAGE_SIZE)
> > +               addr_type = "dereferencing kernel NULL pointer";
> > +       else if (orig_addr < TASK_SIZE_MAX)
> > +               addr_type = "probably dereferencing invalid pointer";
>
> This is access to user memory, right? In outline mode we call it
> "user-memory-access". We could say about "user" part here as well.

Okay, I'll copy that naming.

> > +       else
> > +               addr_type = "maybe dereferencing invalid pointer";
> > +       pr_alert("%s in range [0x%016lx-0x%016lx]\n", addr_type,
> > +                orig_addr, orig_addr + (1 << KASAN_SHADOW_SCALE_SHIFT) - 1);
>
> "(1 << KASAN_SHADOW_SCALE_SHIFT) - 1)" part may be replaced with
> KASAN_SHADOW_MASK.
> Overall it can make sense to move this mm/kasan/report.c b/c we are
> open-coding a number of things here (e.g. reverse address mapping). If
> another arch will do the same, it will need all of this code too (?).

Alright, I'll try to move it over.

Re: [PATCH 2/3] x86/traps: Print non-canonical address on #GP

[Sean Christopherson]

On Tue, Nov 12, 2019 at 10:10:01PM +0100, Jann Horn wrote:
> A frequent cause of #GP exceptions are memory accesses to non-canonical
> addresses. Unlike #PF, #GP doesn't come with a fault address in CR2, so
> the kernel doesn't currently print the fault address for #GP.
> Luckily, we already have the necessary infrastructure for decoding X86
> instructions and computing the memory address that is being accessed;
> hook it up to the #GP handler so that we can figure out whether the #GP
> looks like it was caused by a non-canonical address, and if so, print
> that address.
> 
> While it is already possible to compute the faulting address manually by
> disassembling the opcode dump and evaluating the instruction against the
> register dump, this should make it slightly easier to identify crashes
> at a glance.
> 
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>  arch/x86/kernel/traps.c | 45 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index c90312146da0..479cfc6e9507 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -56,6 +56,8 @@
>  #include <asm/mpx.h>
>  #include <asm/vm86.h>
>  #include <asm/umip.h>
> +#include <asm/insn.h>
> +#include <asm/insn-eval.h>
>  
>  #ifdef CONFIG_X86_64
>  #include <asm/x86_init.h>
> @@ -509,6 +511,42 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long error_code)
>  	do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs, error_code, 0, NULL);
>  }
>  
> +/*
> + * On 64-bit, if an uncaught #GP occurs while dereferencing a non-canonical
> + * address, print that address.
> + */
> +static void print_kernel_gp_address(struct pt_regs *regs)
> +{
> +#ifdef CONFIG_X86_64
> +	u8 insn_bytes[MAX_INSN_SIZE];
> +	struct insn insn;
> +	unsigned long addr_ref;
> +
> +	if (probe_kernel_read(insn_bytes, (void *)regs->ip, MAX_INSN_SIZE))
> +		return;
> +
> +	kernel_insn_init(&insn, insn_bytes, MAX_INSN_SIZE);
> +	insn_get_modrm(&insn);
> +	insn_get_sib(&insn);
> +	addr_ref = (unsigned long)insn_get_addr_ref(&insn, regs);
> +
> +	/*
> +	 * If insn_get_addr_ref() failed or we got a canonical address in the
> +	 * kernel half, bail out.
> +	 */
> +	if ((addr_ref | __VIRTUAL_MASK) == ~0UL)
> +		return;
> +	/*
> +	 * For the user half, check against TASK_SIZE_MAX; this way, if the
> +	 * access crosses the canonical address boundary, we don't miss it.
> +	 */
> +	if (addr_ref <= TASK_SIZE_MAX)

Any objection to open coding the upper bound instead of using
TASK_SIZE_MASK to make the threshold more obvious?

> +		return;
> +
> +	pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref);

Printing the raw address will confuse users in the case where the access
straddles the lower canonical boundary.  Maybe combine this with open
coding the straddle case?  With a rough heuristic to hedge a bit for
instructions whose operand size isn't accurately reflected in opnd_bytes.

	if (addr_ref > __VIRTUAL_MASK)
		pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref);
	else if ((addr_ref + insn->opnd_bytes - 1) > __VIRTUAL_MASK)
		pr_alert("straddling non-canonical boundary 0x%016lx - 0x%016lx\n",
			 addr_ref, addr_ref + insn->opnd_bytes - 1);
	else if ((addr_ref + PAGE_SIZE - 1) > __VIRTUAL_MASK)
		pr_alert("potentially straddling non-canonical boundary 0x%016lx - 0x%016lx\n",
			 addr_ref, addr_ref + PAGE_SIZE - 1);

> +#endif
> +}
> +
>  dotraplinkage void
>  do_general_protection(struct pt_regs *regs, long error_code)
>  {
> @@ -547,8 +585,11 @@ do_general_protection(struct pt_regs *regs, long error_code)
>  			return;
>  
>  		if (notify_die(DIE_GPF, desc, regs, error_code,
> -			       X86_TRAP_GP, SIGSEGV) != NOTIFY_STOP)
> -			die(desc, regs, error_code);
> +			       X86_TRAP_GP, SIGSEGV) == NOTIFY_STOP)
> +			return;
> +
> +		print_kernel_gp_address(regs);

This can be conditional on '!error_code', non-canonical faults on the
direct access always have zero for the error code.  Doubt it will matter
in practice, but far calls and other silly segment instructions can
generate non-zero error codes on #GP in 64-bit mode.

> +		die(desc, regs, error_code);
>  		return;
>  	}
>  
> -- 
> 2.24.0.432.g9d3f5f5b63-goog
> 

Re: [PATCH 2/3] x86/traps: Print non-canonical address on #GP

[Andy Lutomirski]

On Thu, Nov 14, 2019 at 9:46 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Tue, Nov 12, 2019 at 10:10:01PM +0100, Jann Horn wrote:
> > A frequent cause of #GP exceptions are memory accesses to non-canonical
> > addresses. Unlike #PF, #GP doesn't come with a fault address in CR2, so
> > the kernel doesn't currently print the fault address for #GP.
> > Luckily, we already have the necessary infrastructure for decoding X86
> > instructions and computing the memory address that is being accessed;
> > hook it up to the #GP handler so that we can figure out whether the #GP
> > looks like it was caused by a non-canonical address, and if so, print
> > that address.
> >
> > While it is already possible to compute the faulting address manually by
> > disassembling the opcode dump and evaluating the instruction against the
> > register dump, this should make it slightly easier to identify crashes
> > at a glance.
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
> > ---
> >  arch/x86/kernel/traps.c | 45 +++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 43 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > index c90312146da0..479cfc6e9507 100644
> > --- a/arch/x86/kernel/traps.c
> > +++ b/arch/x86/kernel/traps.c
> > @@ -56,6 +56,8 @@
> >  #include <asm/mpx.h>
> >  #include <asm/vm86.h>
> >  #include <asm/umip.h>
> > +#include <asm/insn.h>
> > +#include <asm/insn-eval.h>
> >
> >  #ifdef CONFIG_X86_64
> >  #include <asm/x86_init.h>
> > @@ -509,6 +511,42 @@ dotraplinkage void do_bounds(struct pt_regs *regs, long error_code)
> >       do_trap(X86_TRAP_BR, SIGSEGV, "bounds", regs, error_code, 0, NULL);
> >  }
> >
> > +/*
> > + * On 64-bit, if an uncaught #GP occurs while dereferencing a non-canonical
> > + * address, print that address.
> > + */
> > +static void print_kernel_gp_address(struct pt_regs *regs)
> > +{
> > +#ifdef CONFIG_X86_64
> > +     u8 insn_bytes[MAX_INSN_SIZE];
> > +     struct insn insn;
> > +     unsigned long addr_ref;
> > +
> > +     if (probe_kernel_read(insn_bytes, (void *)regs->ip, MAX_INSN_SIZE))
> > +             return;
> > +
> > +     kernel_insn_init(&insn, insn_bytes, MAX_INSN_SIZE);
> > +     insn_get_modrm(&insn);
> > +     insn_get_sib(&insn);
> > +     addr_ref = (unsigned long)insn_get_addr_ref(&insn, regs);
> > +
> > +     /*
> > +      * If insn_get_addr_ref() failed or we got a canonical address in the
> > +      * kernel half, bail out.
> > +      */
> > +     if ((addr_ref | __VIRTUAL_MASK) == ~0UL)
> > +             return;
> > +     /*
> > +      * For the user half, check against TASK_SIZE_MAX; this way, if the
> > +      * access crosses the canonical address boundary, we don't miss it.
> > +      */
> > +     if (addr_ref <= TASK_SIZE_MAX)
>
> Any objection to open coding the upper bound instead of using
> TASK_SIZE_MASK to make the threshold more obvious?
>
> > +             return;
> > +
> > +     pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref);
>
> Printing the raw address will confuse users in the case where the access
> straddles the lower canonical boundary.  Maybe combine this with open
> coding the straddle case?  With a rough heuristic to hedge a bit for
> instructions whose operand size isn't accurately reflected in opnd_bytes.
>
>         if (addr_ref > __VIRTUAL_MASK)
>                 pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref);
>         else if ((addr_ref + insn->opnd_bytes - 1) > __VIRTUAL_MASK)
>                 pr_alert("straddling non-canonical boundary 0x%016lx - 0x%016lx\n",
>                          addr_ref, addr_ref + insn->opnd_bytes - 1);
>         else if ((addr_ref + PAGE_SIZE - 1) > __VIRTUAL_MASK)
>                 pr_alert("potentially straddling non-canonical boundary 0x%016lx - 0x%016lx\n",
>                          addr_ref, addr_ref + PAGE_SIZE - 1);

This is unnecessarily complicated, and I suspect that Jann had the
right idea but just didn't quite explain it enough.  The secret here
is that TASK_SIZE_MAX is a full page below the canonical boundary
(thanks, Intel, for screwing up SYSRET), so, if we get #GP for an
address above TASK_SIZE_MAX, then it's either a #GP for a different
reason or it's a genuine non-canonical access.

So I think that just a comment about this would be enough.

*However*, the printout should at least hedge a bit and say something
like "probably dereferencing non-canonical address", since there are
plenty of ways to get #GP with an operand that is nominally
non-canonical but where the actual cause of #GP is different.  And I
think this code should be skipped entirely if error_code != 0.

--Andy

Re: [PATCH 2/3] x86/traps: Print non-canonical address on #GP

[Borislav Petkov]

On Thu, Nov 14, 2019 at 10:00:35AM -0800, Andy Lutomirski wrote:
> And I think this code should be skipped entirely if error_code != 0.

... or say that the #GP is happening due to a segment descriptor access.
Would make the figuring out why it happens a bit simpler as to where to
look.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/3] x86/traps: Print non-canonical address on #GP

[Sean Christopherson]

On Thu, Nov 14, 2019 at 10:00:35AM -0800, Andy Lutomirski wrote:
> On Thu, Nov 14, 2019 at 9:46 AM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> > > +     /*
> > > +      * For the user half, check against TASK_SIZE_MAX; this way, if the
> > > +      * access crosses the canonical address boundary, we don't miss it.
> > > +      */
> > > +     if (addr_ref <= TASK_SIZE_MAX)
> >
> > Any objection to open coding the upper bound instead of using
> > TASK_SIZE_MASK to make the threshold more obvious?
> >
> > > +             return;
> > > +
> > > +     pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref);
> >
> > Printing the raw address will confuse users in the case where the access
> > straddles the lower canonical boundary.  Maybe combine this with open
> > coding the straddle case?  With a rough heuristic to hedge a bit for
> > instructions whose operand size isn't accurately reflected in opnd_bytes.
> >
> >         if (addr_ref > __VIRTUAL_MASK)
> >                 pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref);
> >         else if ((addr_ref + insn->opnd_bytes - 1) > __VIRTUAL_MASK)
> >                 pr_alert("straddling non-canonical boundary 0x%016lx - 0x%016lx\n",
> >                          addr_ref, addr_ref + insn->opnd_bytes - 1);
> >         else if ((addr_ref + PAGE_SIZE - 1) > __VIRTUAL_MASK)
> >                 pr_alert("potentially straddling non-canonical boundary 0x%016lx - 0x%016lx\n",
> >                          addr_ref, addr_ref + PAGE_SIZE - 1);
> 
> This is unnecessarily complicated, and I suspect that Jann had the
> right idea but just didn't quite explain it enough.  The secret here
> is that TASK_SIZE_MAX is a full page below the canonical boundary
> (thanks, Intel, for screwing up SYSRET), so, if we get #GP for an
> address above TASK_SIZE_MAX,

Ya, I followed all that.  My point is that if "addr_ref + insn->opnd_bytes"
straddles the boundary then it's extremely likely the #GP is due to a
non-canonical access, i.e. the pr_alert() doesn't have to hedge (as much).

> then it's either a #GP for a different
> reason or it's a genuine non-canonical access.

Heh, "canonical || !canonical" would be the options :-D

> 
> So I think that just a comment about this would be enough.
> 
> *However*, the printout should at least hedge a bit and say something
> like "probably dereferencing non-canonical address", since there are
> plenty of ways to get #GP with an operand that is nominally
> non-canonical but where the actual cause of #GP is different.  And I
> think this code should be skipped entirely if error_code != 0.
> 
> --Andy

Re: [PATCH 2/3] x86/traps: Print non-canonical address on #GP

[Andy Lutomirski]

On Thu, Nov 14, 2019 at 10:20 AM Sean Christopherson
<sean.j.christopherson@intel.com> wrote:
>
> On Thu, Nov 14, 2019 at 10:00:35AM -0800, Andy Lutomirski wrote:
> > On Thu, Nov 14, 2019 at 9:46 AM Sean Christopherson
> > <sean.j.christopherson@intel.com> wrote:
> > > > +     /*
> > > > +      * For the user half, check against TASK_SIZE_MAX; this way, if the
> > > > +      * access crosses the canonical address boundary, we don't miss it.
> > > > +      */
> > > > +     if (addr_ref <= TASK_SIZE_MAX)
> > >
> > > Any objection to open coding the upper bound instead of using
> > > TASK_SIZE_MASK to make the threshold more obvious?
> > >
> > > > +             return;
> > > > +
> > > > +     pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref);
> > >
> > > Printing the raw address will confuse users in the case where the access
> > > straddles the lower canonical boundary.  Maybe combine this with open
> > > coding the straddle case?  With a rough heuristic to hedge a bit for
> > > instructions whose operand size isn't accurately reflected in opnd_bytes.
> > >
> > >         if (addr_ref > __VIRTUAL_MASK)
> > >                 pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref);
> > >         else if ((addr_ref + insn->opnd_bytes - 1) > __VIRTUAL_MASK)
> > >                 pr_alert("straddling non-canonical boundary 0x%016lx - 0x%016lx\n",
> > >                          addr_ref, addr_ref + insn->opnd_bytes - 1);
> > >         else if ((addr_ref + PAGE_SIZE - 1) > __VIRTUAL_MASK)
> > >                 pr_alert("potentially straddling non-canonical boundary 0x%016lx - 0x%016lx\n",
> > >                          addr_ref, addr_ref + PAGE_SIZE - 1);
> >
> > This is unnecessarily complicated, and I suspect that Jann had the
> > right idea but just didn't quite explain it enough.  The secret here
> > is that TASK_SIZE_MAX is a full page below the canonical boundary
> > (thanks, Intel, for screwing up SYSRET), so, if we get #GP for an
> > address above TASK_SIZE_MAX,
>
> Ya, I followed all that.  My point is that if "addr_ref + insn->opnd_bytes"
> straddles the boundary then it's extremely likely the #GP is due to a
> non-canonical access, i.e. the pr_alert() doesn't have to hedge (as much).

I suppose.  But I don't think we have a real epidemic of failed
accesses to user memory between TASK_SIZE_MAX and the actual boundary
that get #GP instead of #PF but fail for a reason other than
non-canonicality :)

I think we should just go back in time and fix x86_64 to either give
#PF or at least give some useful page fault for a non-canonical
address. The only difficulties I'm aware of is that Intel CPUs would
either need to be redesigned better or would have slightly odd
semantics for jumps to non-canonical addresses -- #PF in Intel's model
of "RIP literally *can't* have a non-canonical value" would be a bit
strange.  Also, my time machine is out of commission.

--Andy

Re: [PATCH 2/3] x86/traps: Print non-canonical address on #GP

[Sean Christopherson]

On Thu, Nov 14, 2019 at 10:41:06AM -0800, Andy Lutomirski wrote:
> On Thu, Nov 14, 2019 at 10:20 AM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> >
> > On Thu, Nov 14, 2019 at 10:00:35AM -0800, Andy Lutomirski wrote:
> > > On Thu, Nov 14, 2019 at 9:46 AM Sean Christopherson
> > > <sean.j.christopherson@intel.com> wrote:
> > > > > +     /*
> > > > > +      * For the user half, check against TASK_SIZE_MAX; this way, if the
> > > > > +      * access crosses the canonical address boundary, we don't miss it.
> > > > > +      */
> > > > > +     if (addr_ref <= TASK_SIZE_MAX)
> > > >
> > > > Any objection to open coding the upper bound instead of using
> > > > TASK_SIZE_MASK to make the threshold more obvious?
> > > >
> > > > > +             return;
> > > > > +
> > > > > +     pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref);
> > > >
> > > > Printing the raw address will confuse users in the case where the access
> > > > straddles the lower canonical boundary.  Maybe combine this with open
> > > > coding the straddle case?  With a rough heuristic to hedge a bit for
> > > > instructions whose operand size isn't accurately reflected in opnd_bytes.
> > > >
> > > >         if (addr_ref > __VIRTUAL_MASK)
> > > >                 pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref);
> > > >         else if ((addr_ref + insn->opnd_bytes - 1) > __VIRTUAL_MASK)
> > > >                 pr_alert("straddling non-canonical boundary 0x%016lx - 0x%016lx\n",
> > > >                          addr_ref, addr_ref + insn->opnd_bytes - 1);
> > > >         else if ((addr_ref + PAGE_SIZE - 1) > __VIRTUAL_MASK)
> > > >                 pr_alert("potentially straddling non-canonical boundary 0x%016lx - 0x%016lx\n",
> > > >                          addr_ref, addr_ref + PAGE_SIZE - 1);
> > >
> > > This is unnecessarily complicated, and I suspect that Jann had the
> > > right idea but just didn't quite explain it enough.  The secret here
> > > is that TASK_SIZE_MAX is a full page below the canonical boundary
> > > (thanks, Intel, for screwing up SYSRET), so, if we get #GP for an
> > > address above TASK_SIZE_MAX,
> >
> > Ya, I followed all that.  My point is that if "addr_ref + insn->opnd_bytes"
> > straddles the boundary then it's extremely likely the #GP is due to a
> > non-canonical access, i.e. the pr_alert() doesn't have to hedge (as much).
> 
> I suppose.  But I don't think we have a real epidemic of failed
> accesses to user memory between TASK_SIZE_MAX and the actual boundary
> that get #GP instead of #PF but fail for a reason other than
> non-canonicality :)

No argument there.

> I think we should just go back in time and fix x86_64 to either give
> #PF or at least give some useful page fault for a non-canonical
> address. The only difficulties I'm aware of is that Intel CPUs would
> either need to be redesigned better or would have slightly odd
> semantics for jumps to non-canonical addresses -- #PF in Intel's model
> of "RIP literally *can't* have a non-canonical value" would be a bit
> strange.  Also, my time machine is out of commission.

If you happen to fix your time machine, just go back a bit further and
change protected mode to push the faulting address onto the stack.

Re: [PATCH 2/3] x86/traps: Print non-canonical address on #GP

[Jann Horn]

On Thu, Nov 14, 2019 at 7:00 PM Andy Lutomirski <luto@kernel.org> wrote:
> On Thu, Nov 14, 2019 at 9:46 AM Sean Christopherson
> <sean.j.christopherson@intel.com> wrote:
> > On Tue, Nov 12, 2019 at 10:10:01PM +0100, Jann Horn wrote:
> > > A frequent cause of #GP exceptions are memory accesses to non-canonical
> > > addresses. Unlike #PF, #GP doesn't come with a fault address in CR2, so
> > > the kernel doesn't currently print the fault address for #GP.
> > > Luckily, we already have the necessary infrastructure for decoding X86
> > > instructions and computing the memory address that is being accessed;
> > > hook it up to the #GP handler so that we can figure out whether the #GP
> > > looks like it was caused by a non-canonical address, and if so, print
> > > that address.
[...]
> > > +     /*
> > > +      * If insn_get_addr_ref() failed or we got a canonical address in the
> > > +      * kernel half, bail out.
> > > +      */
> > > +     if ((addr_ref | __VIRTUAL_MASK) == ~0UL)
> > > +             return;
> > > +     /*
> > > +      * For the user half, check against TASK_SIZE_MAX; this way, if the
> > > +      * access crosses the canonical address boundary, we don't miss it.
> > > +      */
> > > +     if (addr_ref <= TASK_SIZE_MAX)
> >
> > Any objection to open coding the upper bound instead of using
> > TASK_SIZE_MASK to make the threshold more obvious?
> >
> > > +             return;
> > > +
> > > +     pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref);
> >
> > Printing the raw address will confuse users in the case where the access
> > straddles the lower canonical boundary.  Maybe combine this with open
> > coding the straddle case?  With a rough heuristic to hedge a bit for
> > instructions whose operand size isn't accurately reflected in opnd_bytes.
> >
> >         if (addr_ref > __VIRTUAL_MASK)
> >                 pr_alert("dereferencing non-canonical address 0x%016lx\n", addr_ref);
> >         else if ((addr_ref + insn->opnd_bytes - 1) > __VIRTUAL_MASK)
> >                 pr_alert("straddling non-canonical boundary 0x%016lx - 0x%016lx\n",
> >                          addr_ref, addr_ref + insn->opnd_bytes - 1);
> >         else if ((addr_ref + PAGE_SIZE - 1) > __VIRTUAL_MASK)
> >                 pr_alert("potentially straddling non-canonical boundary 0x%016lx - 0x%016lx\n",
> >                          addr_ref, addr_ref + PAGE_SIZE - 1);
>
> This is unnecessarily complicated, and I suspect that Jann had the
> right idea but just didn't quite explain it enough.  The secret here
> is that TASK_SIZE_MAX is a full page below the canonical boundary
> (thanks, Intel, for screwing up SYSRET), so, if we get #GP for an
> address above TASK_SIZE_MAX, then it's either a #GP for a different
> reason or it's a genuine non-canonical access.
>
> So I think that just a comment about this would be enough.

Ah, I didn't realize that insn->opnd_bytes exists. Since I already
have that available, I guess using that is cleaner than being clever
with TASK_SIZE_MAX.

> *However*, the printout should at least hedge a bit and say something
> like "probably dereferencing non-canonical address", since there are
> plenty of ways to get #GP with an operand that is nominally
> non-canonical but where the actual cause of #GP is different.

Ah, yeah, I'll change that.

> And I think this code should be skipped entirely if error_code != 0.

Makes sense. As Borislav suggested, I'll add some code to
do_general_protection() to instead print a hint about it being a
segment-related problem.