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

[PATCH v3 1/4] 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>
---

Notes:
    v2:
      no changes
    v3:
      no changes

 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 v3 2/4] 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>
---

Notes:
    v2:
     - print different message for segment-related GP (Borislav)
     - rewrite check for non-canonical address (Sean)
     - make it clear we don't know for sure why the GP happened (Andy)
    v3:
     - change message format to one line (Borislav)
    
    I have already sent a patch to syzkaller that relaxes their parsing of GPF
    messages (https://github.com/google/syzkaller/commit/432c7650) such that
    changes like the one in this patch don't break it.
    That patch has already made its way into syzbot's syzkaller instances
    according to <https://syzkaller.appspot.com/upstream>.

 arch/x86/kernel/traps.c | 56 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 53 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index c90312146da0..19afedcd6f4e 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,11 +511,45 @@ 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, return that address.
+ */
+static unsigned long get_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 0;
+
+	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);
+
+	/* Bail out if insn_get_addr_ref() failed or we got a kernel address. */
+	if (addr_ref >= ~__VIRTUAL_MASK)
+		return 0;
+
+	/* Bail out if the entire operand is in the canonical user half. */
+	if (addr_ref + insn.opnd_bytes - 1 <= __VIRTUAL_MASK)
+		return 0;
+
+	return addr_ref;
+#else
+	return 0;
+#endif
+}
+
+#define GPFSTR "general protection fault"
 dotraplinkage void
 do_general_protection(struct pt_regs *regs, long error_code)
 {
-	const char *desc = "general protection fault";
 	struct task_struct *tsk;
+	char desc[90] = GPFSTR;
 
 	RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
 	cond_local_irq_enable(regs);
@@ -531,6 +567,8 @@ do_general_protection(struct pt_regs *regs, long error_code)
 
 	tsk = current;
 	if (!user_mode(regs)) {
+		unsigned long non_canonical_addr = 0;
+
 		if (fixup_exception(regs, X86_TRAP_GP, error_code, 0))
 			return;
 
@@ -547,8 +585,20 @@ 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;
+
+		if (error_code)
+			snprintf(desc, sizeof(desc), "segment-related " GPFSTR);
+		else
+			non_canonical_addr = get_kernel_gp_address(regs);
+
+		if (non_canonical_addr)
+			snprintf(desc, sizeof(desc),
+			    GPFSTR " probably for non-canonical address 0x%lx",
+			    non_canonical_addr);
+
+		die(desc, regs, error_code);
 		return;
 	}
 
-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH v3 3/4] x86/dumpstack: Split out header line printing from __die()

[Jann Horn]

Split __die() into __die_header() and __die_body(). This allows callers to
insert extra information below the header line that initiates the bug
report.

This can e.g. be used by __die() callers to allow KASAN to print additional
information below the header line of the bug report.

Signed-off-by: Jann Horn <jannh@google.com>
---

Notes:
    I think that it's nicer to have KASAN's notes about the
    bug below the first oops line from the kernel.
    This also means that tools that work with kernel oops
    reports can just trigger on the "general protection fault"
    line with the die counter and so on, and just include the
    text from on there, and the KASAN message will automatically
    be included.
    But if you think that the code looks too ugly, I'd be
    happy to change that back and drop this patch from the
    series.
    
    v3:
      new patch

 arch/x86/include/asm/kdebug.h |  3 +++
 arch/x86/kernel/dumpstack.c   | 13 ++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/kdebug.h b/arch/x86/include/asm/kdebug.h
index 75f1e35e7c15..a0050fabce42 100644
--- a/arch/x86/include/asm/kdebug.h
+++ b/arch/x86/include/asm/kdebug.h
@@ -33,6 +33,9 @@ enum show_regs_mode {
 };
 
 extern void die(const char *, struct pt_regs *,long);
+extern void __die_header(const char *str, struct pt_regs *regs, long err);
+extern int __must_check __die_body(const char *str, struct pt_regs *regs,
+				   long err);
 extern int __must_check __die(const char *, struct pt_regs *, long);
 extern void show_stack_regs(struct pt_regs *regs);
 extern void __show_regs(struct pt_regs *regs, enum show_regs_mode);
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index e07424e19274..6436f3f5f803 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -365,7 +365,7 @@ void oops_end(unsigned long flags, struct pt_regs *regs, int signr)
 }
 NOKPROBE_SYMBOL(oops_end);
 
-int __die(const char *str, struct pt_regs *regs, long err)
+void __die_header(const char *str, struct pt_regs *regs, long err)
 {
 	const char *pr = "";
 
@@ -384,7 +384,11 @@ int __die(const char *str, struct pt_regs *regs, long err)
 	       IS_ENABLED(CONFIG_KASAN)   ? " KASAN"           : "",
 	       IS_ENABLED(CONFIG_PAGE_TABLE_ISOLATION) ?
 	       (boot_cpu_has(X86_FEATURE_PTI) ? " PTI" : " NOPTI") : "");
+}
+NOKPROBE_SYMBOL(__die_header);
 
+int __die_body(const char *str, struct pt_regs *regs, long err)
+{
 	show_regs(regs);
 	print_modules();
 
@@ -394,6 +398,13 @@ int __die(const char *str, struct pt_regs *regs, long err)
 
 	return 0;
 }
+NOKPROBE_SYMBOL(__die_body);
+
+int __die(const char *str, struct pt_regs *regs, long err)
+{
+	__die_header(str, regs, err);
+	return __die_body(str, regs, err);
+}
 NOKPROBE_SYMBOL(__die);
 
 /*
-- 
2.24.0.432.g9d3f5f5b63-goog

[PATCH v3 4/4] 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
    general protection fault probably for non-canonical address
        0xe017577ddf75b7dd: 0000 [#1] PREEMPT SMP KASAN PTI

into this:

    general protection fault probably for non-canonical address
        0xe017577ddf75b7dd: 0000 [#1] PREEMPT SMP KASAN PTI
    KASAN: maybe wild-memory-access in range
        [0x00badbeefbadbee8-0x00badbeefbadbeef]

The hook is placed in architecture-independent code, but is currently
only wired up to the X86 exception handler because I'm not sufficiently
familiar with the address space layout and exception handling mechanisms
on other architectures.

Signed-off-by: Jann Horn <jannh@google.com>
---

Notes:
    v2:
     - move to mm/kasan/report.c (Dmitry)
     - change hook name to be more generic
     - use TASK_SIZE instead of TASK_SIZE_MAX for compiling on non-x86
     - don't open-code KASAN_SHADOW_MASK (Dmitry)
     - add "KASAN: " prefix, but not "BUG: " (Andrey, Dmitry)
     - use same naming scheme as get_wild_bug_type (Andrey)
     - this version was "Reviewed-by: Dmitry Vyukov <dvyukov@google.com>"
    v3:
     - adjusted example output in commit message based on
       changes in preceding patch
     - ensure that KASAN output happens after bust_spinlocks(1)
     - moved hook in arch/x86/kernel/traps.c such that output
       appears after the first line of KASAN-independent error report

 arch/x86/kernel/traps.c     | 11 ++++++++++
 arch/x86/mm/kasan_init_64.c | 21 -------------------
 include/linux/kasan.h       |  6 ++++++
 mm/kasan/report.c           | 40 +++++++++++++++++++++++++++++++++++++
 4 files changed, 57 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 19afedcd6f4e..b5baf1114d44 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -37,6 +37,7 @@
 #include <linux/mm.h>
 #include <linux/smp.h>
 #include <linux/io.h>
+#include <linux/kasan.h>
 #include <asm/stacktrace.h>
 #include <asm/processor.h>
 #include <asm/debugreg.h>
@@ -568,6 +569,8 @@ do_general_protection(struct pt_regs *regs, long error_code)
 	tsk = current;
 	if (!user_mode(regs)) {
 		unsigned long non_canonical_addr = 0;
+		unsigned long flags;
+		int sig;
 
 		if (fixup_exception(regs, X86_TRAP_GP, error_code, 0))
 			return;
@@ -598,6 +601,14 @@ do_general_protection(struct pt_regs *regs, long error_code)
 			    GPFSTR " probably for non-canonical address 0x%lx",
 			    non_canonical_addr);
 
+		flags = oops_begin();
+		sig = SIGSEGV;
+		__die_header(desc, regs, error_code);
+		if (non_canonical_addr)
+			kasan_non_canonical_hook(non_canonical_addr);
+		if (__die_body(desc, regs, error_code))
+			sig = 0;
+		oops_end(flags, regs, sig);
 		die(desc, regs, error_code);
 		return;
 	}
diff --git a/arch/x86/mm/kasan_init_64.c b/arch/x86/mm/kasan_init_64.c
index 296da58f3013..69c437fb21cc 100644
--- a/arch/x86/mm/kasan_init_64.c
+++ b/arch/x86/mm/kasan_init_64.c
@@ -245,23 +245,6 @@ static void __init kasan_map_early_shadow(pgd_t *pgd)
 	} while (pgd++, addr = next, addr != end);
 }
 
-#ifdef CONFIG_KASAN_INLINE
-static int kasan_die_handler(struct notifier_block *self,
-			     unsigned long val,
-			     void *data)
-{
-	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;
-}
-
-static struct notifier_block kasan_die_notifier = {
-	.notifier_call = kasan_die_handler,
-};
-#endif
-
 void __init kasan_early_init(void)
 {
 	int i;
@@ -298,10 +281,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));
 
 	/*
diff --git a/include/linux/kasan.h b/include/linux/kasan.h
index cc8a03cc9674..7305024b44e3 100644
--- a/include/linux/kasan.h
+++ b/include/linux/kasan.h
@@ -194,4 +194,10 @@ static inline void *kasan_reset_tag(const void *addr)
 
 #endif /* CONFIG_KASAN_SW_TAGS */
 
+#ifdef CONFIG_KASAN_INLINE
+void kasan_non_canonical_hook(unsigned long addr);
+#else /* CONFIG_KASAN_INLINE */
+static inline void kasan_non_canonical_hook(unsigned long addr) { }
+#endif /* CONFIG_KASAN_INLINE */
+
 #endif /* LINUX_KASAN_H */
diff --git a/mm/kasan/report.c b/mm/kasan/report.c
index 621782100eaa..5ef9f24f566b 100644
--- a/mm/kasan/report.c
+++ b/mm/kasan/report.c
@@ -512,3 +512,43 @@ void __kasan_report(unsigned long addr, size_t size, bool is_write, unsigned lon
 
 	end_report(&flags);
 }
+
+#ifdef CONFIG_KASAN_INLINE
+/*
+ * 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.
+ * Help the user figure out what the original bogus pointer was.
+ */
+void kasan_non_canonical_hook(unsigned long addr)
+{
+	unsigned long orig_addr;
+	const char *bug_type;
+
+	if (addr < KASAN_SHADOW_OFFSET)
+		return;
+
+	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)
+		bug_type = "null-ptr-deref";
+	else if (orig_addr < TASK_SIZE)
+		bug_type = "probably user-memory-access";
+	else
+		bug_type = "maybe wild-memory-access";
+	pr_alert("KASAN: %s in range [0x%016lx-0x%016lx]\n", bug_type,
+		 orig_addr, orig_addr + KASAN_SHADOW_MASK);
+}
+#endif
-- 
2.24.0.432.g9d3f5f5b63-goog

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

[Ingo Molnar]

* Jann Horn <jannh@google.com> 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>
> ---
> 
> Notes:
>     v2:
>      - print different message for segment-related GP (Borislav)
>      - rewrite check for non-canonical address (Sean)
>      - make it clear we don't know for sure why the GP happened (Andy)
>     v3:
>      - change message format to one line (Borislav)
>     
>     I have already sent a patch to syzkaller that relaxes their parsing of GPF
>     messages (https://github.com/google/syzkaller/commit/432c7650) such that
>     changes like the one in this patch don't break it.
>     That patch has already made its way into syzbot's syzkaller instances
>     according to <https://syzkaller.appspot.com/upstream>.
> 
>  arch/x86/kernel/traps.c | 56 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 53 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index c90312146da0..19afedcd6f4e 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,11 +511,45 @@ 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, return that address.
> + */
> +static unsigned long get_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 0;
> +
> +	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);

I had to look twice to realize that the 'insn_bytes' isn't an integer 
that shows the number of bytes in the instruction, but the instruction 
buffer itself.

Could we please do s/insn_bytes/insn_buf or such?

> +
> +	/* Bail out if insn_get_addr_ref() failed or we got a kernel address. */
> +	if (addr_ref >= ~__VIRTUAL_MASK)
> +		return 0;
> +
> +	/* Bail out if the entire operand is in the canonical user half. */
> +	if (addr_ref + insn.opnd_bytes - 1 <= __VIRTUAL_MASK)
> +		return 0;

BTW., it would be nice to split this logic in two: return the faulting 
address to do_general_protection(), and print it out both for 
non-canonical and canonical addresses as well -and use the canonical 
check to *additionally* print out a short note when the operand is 
non-canonical?

> +#define GPFSTR "general protection fault"
>  dotraplinkage void

Please separate macro and function definitions by an additional newline.

>  do_general_protection(struct pt_regs *regs, long error_code)
>  {
> -	const char *desc = "general protection fault";
>  	struct task_struct *tsk;
> +	char desc[90] = GPFSTR;


How was this maximum string length of '90' derived? In what way will that 
have to change if someone changes the message?

Thanks,

	Ingo

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

[Borislav Petkov]

On Wed, Nov 20, 2019 at 12:18:59PM +0100, Ingo Molnar wrote:
> How was this maximum string length of '90' derived? In what way will
> that have to change if someone changes the message?

That was me counting the string length in a dirty patch in a previous
thread. We probably should say why we decided for a certain length and
maybe have a define for it.

Also, I could use your opinion on this here:

https://lkml.kernel.org/r/20191118164407.GH6363@zn.tnic

and the following mail.

I think that marking the splat with its number would *immensely* help us
with the question: was this the first splat or wasn't? A question we've
been asking since I got involved in kernel development. :)

-- 
Regards/Gruss,
    Boris.

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

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

[Jann Horn]

On Wed, Nov 20, 2019 at 12:19 PM Ingo Molnar <mingo@kernel.org> wrote:
> * Jann Horn <jannh@google.com> 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.
[...]
> > +/*
> > + * On 64-bit, if an uncaught #GP occurs while dereferencing a non-canonical
> > + * address, return that address.
> > + */
> > +static unsigned long get_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 0;
> > +
> > +     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);
>
> I had to look twice to realize that the 'insn_bytes' isn't an integer
> that shows the number of bytes in the instruction, but the instruction
> buffer itself.
>
> Could we please do s/insn_bytes/insn_buf or such?

Will change it.

> > +
> > +     /* Bail out if insn_get_addr_ref() failed or we got a kernel address. */
> > +     if (addr_ref >= ~__VIRTUAL_MASK)
> > +             return 0;
> > +
> > +     /* Bail out if the entire operand is in the canonical user half. */
> > +     if (addr_ref + insn.opnd_bytes - 1 <= __VIRTUAL_MASK)
> > +             return 0;
>
> BTW., it would be nice to split this logic in two: return the faulting
> address to do_general_protection(), and print it out both for
> non-canonical and canonical addresses as well -and use the canonical
> check to *additionally* print out a short note when the operand is
> non-canonical?

You mean something like this?

========================
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 9b23c4bda243..16a6bdaccb51 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -516,32 +516,36 @@ dotraplinkage void do_bounds(struct pt_regs
*regs, long error_code)
  * On 64-bit, if an uncaught #GP occurs while dereferencing a non-canonical
  * address, return that address.
  */
-static unsigned long get_kernel_gp_address(struct pt_regs *regs)
+static bool get_kernel_gp_address(struct pt_regs *regs, unsigned long *addr,
+                                          bool *non_canonical)
 {
 #ifdef CONFIG_X86_64
        u8 insn_buf[MAX_INSN_SIZE];
        struct insn insn;
-       unsigned long addr_ref;

        if (probe_kernel_read(insn_buf, (void *)regs->ip, MAX_INSN_SIZE))
-               return 0;
+               return false;

        kernel_insn_init(&insn, insn_buf, MAX_INSN_SIZE);
        insn_get_modrm(&insn);
        insn_get_sib(&insn);
-       addr_ref = (unsigned long)insn_get_addr_ref(&insn, regs);
+       *addr = (unsigned long)insn_get_addr_ref(&insn, regs);

-       /* Bail out if insn_get_addr_ref() failed or we got a kernel address. */
-       if (addr_ref >= ~__VIRTUAL_MASK)
-               return 0;
+       if (*addr == (unsigned long)-1L)
+               return false;

-       /* Bail out if the entire operand is in the canonical user half. */
-       if (addr_ref + insn.opnd_bytes - 1 <= __VIRTUAL_MASK)
-               return 0;
+       /*
+        * Check that:
+        *  - the address is not in the kernel half or -1 (which means the
+        *    decoder failed to decode it)
+        *  - the last byte of the address is not in the user canonical half
+        */
+       *non_canonical = *addr < ~__VIRTUAL_MASK &&
+                        *addr + insn.opnd_bytes - 1 > __VIRTUAL_MASK;

-       return addr_ref;
+       return true;
 #else
-       return 0;
+       return false;
 #endif
 }

@@ -569,8 +573,10 @@ do_general_protection(struct pt_regs *regs, long
error_code)

        tsk = current;
        if (!user_mode(regs)) {
-               unsigned long non_canonical_addr = 0;
+               bool addr_resolved = false;
+               unsigned long gp_addr;
                unsigned long flags;
+               bool non_canonical;
                int sig;

                if (fixup_exception(regs, X86_TRAP_GP, error_code, 0))
@@ -595,18 +601,19 @@ do_general_protection(struct pt_regs *regs, long
error_code)
                if (error_code)
                        snprintf(desc, sizeof(desc), "segment-related " GPFSTR);
                else
-                       non_canonical_addr = get_kernel_gp_address(regs);
+                       addr_resolved = get_kernel_gp_address(regs, &gp_addr,
+                                                             &non_canonical);

-               if (non_canonical_addr)
+               if (addr_resolved)
                        snprintf(desc, sizeof(desc),
-                           GPFSTR " probably for non-canonical address 0x%lx",
-                           non_canonical_addr);
+                           GPFSTR " probably for %saddress 0x%lx",
+                           non_canonical ? "non-canonical " : "", gp_addr);

                flags = oops_begin();
                sig = SIGSEGV;
                __die_header(desc, regs, error_code);
-               if (non_canonical_addr)
-                       kasan_non_canonical_hook(non_canonical_addr);
+               if (addr_resolved && non_canonical)
+                       kasan_non_canonical_hook(gp_addr);
                if (__die_body(desc, regs, error_code))
                        sig = 0;
                oops_end(flags, regs, sig);
========================

I guess that could potentially be useful if a #GP is triggered by
something like an SSE alignment error? I'll add it in unless someone
else complains.

> > +#define GPFSTR "general protection fault"
> >  dotraplinkage void
>
> Please separate macro and function definitions by an additional newline.

Will change it.

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

[Jann Horn]

On Wed, Nov 20, 2019 at 12:24 PM Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Nov 20, 2019 at 12:18:59PM +0100, Ingo Molnar wrote:
> > How was this maximum string length of '90' derived? In what way will
> > that have to change if someone changes the message?
>
> That was me counting the string length in a dirty patch in a previous
> thread. We probably should say why we decided for a certain length and
> maybe have a define for it.

Do you think something like this would be better?

char desc[sizeof(GPFSTR) + 50 + 2*sizeof(unsigned long) + 1] = GPFSTR;

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

[Ingo Molnar]

* Jann Horn <jannh@google.com> wrote:

> You mean something like this?
> 
> ========================
> diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> index 9b23c4bda243..16a6bdaccb51 100644
> --- a/arch/x86/kernel/traps.c
> +++ b/arch/x86/kernel/traps.c
> @@ -516,32 +516,36 @@ dotraplinkage void do_bounds(struct pt_regs
> *regs, long error_code)
>   * On 64-bit, if an uncaught #GP occurs while dereferencing a non-canonical
>   * address, return that address.
>   */
> -static unsigned long get_kernel_gp_address(struct pt_regs *regs)
> +static bool get_kernel_gp_address(struct pt_regs *regs, unsigned long *addr,
> +                                          bool *non_canonical)

Yeah, that's pretty much the perfect end result!

> I guess that could potentially be useful if a #GP is triggered by
> something like an SSE alignment error? I'll add it in unless someone
> else complains.

Yeah - also it's correct information about the context of the fault, so 
it probably cannot *hurt*, and will allow us to debug/validate everything 
in this area faster.

> > > +#define GPFSTR "general protection fault"
> > >  dotraplinkage void
> >
> > Please separate macro and function definitions by an additional newline.
> 
> Will change it.

Thanks!

	Ingo

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

[Borislav Petkov]

On Wed, Nov 20, 2019 at 01:30:58PM +0100, Ingo Molnar wrote:
> 
> * Jann Horn <jannh@google.com> wrote:
> 
> > You mean something like this?
> > 
> > ========================
> > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > index 9b23c4bda243..16a6bdaccb51 100644
> > --- a/arch/x86/kernel/traps.c
> > +++ b/arch/x86/kernel/traps.c
> > @@ -516,32 +516,36 @@ dotraplinkage void do_bounds(struct pt_regs
> > *regs, long error_code)
> >   * On 64-bit, if an uncaught #GP occurs while dereferencing a non-canonical
> >   * address, return that address.
> >   */
> > -static unsigned long get_kernel_gp_address(struct pt_regs *regs)
> > +static bool get_kernel_gp_address(struct pt_regs *regs, unsigned long *addr,
> > +                                          bool *non_canonical)
> 
> Yeah, that's pretty much the perfect end result!

Why do we need the bool thing? Can't we rely on the assumption that an
address of 0 is the error case and use that to determine whether the
resolving succeeded or not?

-- 
Regards/Gruss,
    Boris.

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

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

[Borislav Petkov]

On Wed, Nov 20, 2019 at 01:25:30PM +0100, Jann Horn wrote:
> On Wed, Nov 20, 2019 at 12:24 PM Borislav Petkov <bp@alien8.de> wrote:
> > On Wed, Nov 20, 2019 at 12:18:59PM +0100, Ingo Molnar wrote:
> > > How was this maximum string length of '90' derived? In what way will
> > > that have to change if someone changes the message?
> >
> > That was me counting the string length in a dirty patch in a previous
> > thread. We probably should say why we decided for a certain length and
> > maybe have a define for it.
> 
> Do you think something like this would be better?
> 
> char desc[sizeof(GPFSTR) + 50 + 2*sizeof(unsigned long) + 1] = GPFSTR;

Yap, and the 50 is a sufficiently large number so that all possible
string combinations in this case can fit in the resulting string array.

Thx.

-- 
Regards/Gruss,
    Boris.

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

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

[Jann Horn]

On Wed, Nov 20, 2019 at 1:39 PM Borislav Petkov <bp@alien8.de> wrote:
> On Wed, Nov 20, 2019 at 01:30:58PM +0100, Ingo Molnar wrote:
> > * Jann Horn <jannh@google.com> wrote:
> >
> > > You mean something like this?
> > >
> > > ========================
> > > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > > index 9b23c4bda243..16a6bdaccb51 100644
> > > --- a/arch/x86/kernel/traps.c
> > > +++ b/arch/x86/kernel/traps.c
> > > @@ -516,32 +516,36 @@ dotraplinkage void do_bounds(struct pt_regs
> > > *regs, long error_code)
> > >   * On 64-bit, if an uncaught #GP occurs while dereferencing a non-canonical
> > >   * address, return that address.
> > >   */
> > > -static unsigned long get_kernel_gp_address(struct pt_regs *regs)
> > > +static bool get_kernel_gp_address(struct pt_regs *regs, unsigned long *addr,
> > > +                                          bool *non_canonical)
> >
> > Yeah, that's pretty much the perfect end result!
>
> Why do we need the bool thing? Can't we rely on the assumption that an
> address of 0 is the error case and use that to determine whether the
> resolving succeeded or not?

True, that'd work, too. I didn't really want to special-case an
integer here - especially one that has the same value as NULL - but I
guess it's fine.

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

[Ingo Molnar]

* Jann Horn <jannh@google.com> wrote:

> On Wed, Nov 20, 2019 at 12:24 PM Borislav Petkov <bp@alien8.de> wrote:
> > On Wed, Nov 20, 2019 at 12:18:59PM +0100, Ingo Molnar wrote:
> > > How was this maximum string length of '90' derived? In what way will
> > > that have to change if someone changes the message?
> >
> > That was me counting the string length in a dirty patch in a previous
> > thread. We probably should say why we decided for a certain length and
> > maybe have a define for it.
> 
> Do you think something like this would be better?
> 
> char desc[sizeof(GPFSTR) + 50 + 2*sizeof(unsigned long) + 1] = GPFSTR;

I'd much prefer this for, because it's a big honking warning for people 
to not just assume things but double check the limits.

I.e. this mild obfuscation of the array size *helps* code quality in the 
long run :-)

Thanks,

	Ingo

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

[Jann Horn]

On Wed, Nov 20, 2019 at 2:16 PM Ingo Molnar <mingo@kernel.org> wrote:
> * Jann Horn <jannh@google.com> wrote:
>
> > On Wed, Nov 20, 2019 at 12:24 PM Borislav Petkov <bp@alien8.de> wrote:
> > > On Wed, Nov 20, 2019 at 12:18:59PM +0100, Ingo Molnar wrote:
> > > > How was this maximum string length of '90' derived? In what way will
> > > > that have to change if someone changes the message?
> > >
> > > That was me counting the string length in a dirty patch in a previous
> > > thread. We probably should say why we decided for a certain length and
> > > maybe have a define for it.
> >
> > Do you think something like this would be better?
> >
> > char desc[sizeof(GPFSTR) + 50 + 2*sizeof(unsigned long) + 1] = GPFSTR;
>
> I'd much prefer this for, because it's a big honking warning for people
> to not just assume things but double check the limits.

Sorry, I can't parse the start of this sentence. I _think_ you're
saying you want me to make the change to "char desc[sizeof(GPFSTR) +
50 + 2*sizeof(unsigned long) + 1]"?

> I.e. this mild obfuscation of the array size *helps* code quality in the
> long run :-)

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

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> On Wed, Nov 20, 2019 at 01:30:58PM +0100, Ingo Molnar wrote:
> > 
> > * Jann Horn <jannh@google.com> wrote:
> > 
> > > You mean something like this?
> > > 
> > > ========================
> > > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > > index 9b23c4bda243..16a6bdaccb51 100644
> > > --- a/arch/x86/kernel/traps.c
> > > +++ b/arch/x86/kernel/traps.c
> > > @@ -516,32 +516,36 @@ dotraplinkage void do_bounds(struct pt_regs
> > > *regs, long error_code)
> > >   * On 64-bit, if an uncaught #GP occurs while dereferencing a non-canonical
> > >   * address, return that address.
> > >   */
> > > -static unsigned long get_kernel_gp_address(struct pt_regs *regs)
> > > +static bool get_kernel_gp_address(struct pt_regs *regs, unsigned long *addr,
> > > +                                          bool *non_canonical)
> > 
> > Yeah, that's pretty much the perfect end result!
> 
> Why do we need the bool thing? Can't we rely on the assumption that an
> address of 0 is the error case and use that to determine whether the
> resolving succeeded or not?

I'd rather we not trust the decoder and the execution environment so much 
that it never produces a 0 linear address in a #GP:

in get_addr_ref_32() we could get zero:

	linear_addr = (unsigned long)(eff_addr & 0xffffffff) + seg_base;

in get_addr_ref_16() we could get zero too:

	linear_addr = (unsigned long)(eff_addr & 0xffff) + seg_base;

Or in particularly exotic crashes we could get zero in get_addr_ref_64() 
as well:

        linear_addr = (unsigned long)eff_addr + seg_base;

although it's unlikely I suspect.

But the 32-bit case should be plausible enough?

It's also the simplest, most straightforward printout of the decoder 
state: we either see an error, or an (address,canonical) pair of values.

Thanks,

	Ingo

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

[Borislav Petkov]

On Wed, Nov 20, 2019 at 02:28:30PM +0100, Ingo Molnar wrote:
> I'd rather we not trust the decoder and the execution environment so much 
> that it never produces a 0 linear address in a #GP:

I was just scratching my head whether I could trigger a #GP with address
of 0. But yeah, I agree, let's be really cautious here. I wouldn't want
to debug a #GP with a wrong address reported.

Thx.

-- 
Regards/Gruss,
    Boris.

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

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

[Ingo Molnar]

* Jann Horn <jannh@google.com> wrote:

> On Wed, Nov 20, 2019 at 2:16 PM Ingo Molnar <mingo@kernel.org> wrote:
> > * Jann Horn <jannh@google.com> wrote:
> >
> > > On Wed, Nov 20, 2019 at 12:24 PM Borislav Petkov <bp@alien8.de> wrote:
> > > > On Wed, Nov 20, 2019 at 12:18:59PM +0100, Ingo Molnar wrote:
> > > > > How was this maximum string length of '90' derived? In what way will
> > > > > that have to change if someone changes the message?
> > > >
> > > > That was me counting the string length in a dirty patch in a previous
> > > > thread. We probably should say why we decided for a certain length and
> > > > maybe have a define for it.
> > >
> > > Do you think something like this would be better?
> > >
> > > char desc[sizeof(GPFSTR) + 50 + 2*sizeof(unsigned long) + 1] = GPFSTR;
> >
> > I'd much prefer this for, because it's a big honking warning for people
> > to not just assume things but double check the limits.
> 
> Sorry, I can't parse the start of this sentence. I _think_ you're
> saying you want me to make the change to "char desc[sizeof(GPFSTR) +
> 50 + 2*sizeof(unsigned long) + 1]"?

Yeah, correct. There was an extra 'for' in my first sentence:

> > I'd much prefer this, because it's a big honking warning for people
> > to not just assume things but double check the limits.

Thanks,

	Ingo

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

[Sean Christopherson]

On Wed, Nov 20, 2019 at 02:39:13PM +0100, Borislav Petkov wrote:
> On Wed, Nov 20, 2019 at 02:28:30PM +0100, Ingo Molnar wrote:
> > I'd rather we not trust the decoder and the execution environment so much 
> > that it never produces a 0 linear address in a #GP:
> 
> I was just scratching my head whether I could trigger a #GP with address
> of 0. But yeah, I agree, let's be really cautious here. I wouldn't want
> to debug a #GP with a wrong address reported.

It's definitely possible, there are a handful of non-SIMD instructions that
generate #GP(0) it CPL=0 in 64-bit mode *and* have a memory operand.  Some
of them might even be legitimately encountered in the wild.

  - CMPXCHG16B if it's not supported by the CPU.
  - VMXON if CR4 is misconfigured or VMX isn't enabled in FEATURE_CONTROL.
  - MONITOR if ECX has an invalid hint (although MONITOR hardcodes the
    address in DS:RAX and so doesn't have a ModR/M byte).

Undoudbtedly there are other instructions with similar sources of #GP.

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

[Borislav Petkov]

On Wed, Nov 20, 2019 at 08:21:43AM -0800, Sean Christopherson wrote:
> On Wed, Nov 20, 2019 at 02:39:13PM +0100, Borislav Petkov wrote:
> > On Wed, Nov 20, 2019 at 02:28:30PM +0100, Ingo Molnar wrote:
> > > I'd rather we not trust the decoder and the execution environment so much 
> > > that it never produces a 0 linear address in a #GP:
> > 
> > I was just scratching my head whether I could trigger a #GP with address
> > of 0. But yeah, I agree, let's be really cautious here. I wouldn't want
> > to debug a #GP with a wrong address reported.
> 
> It's definitely possible, there are a handful of non-SIMD instructions that
> generate #GP(0) it CPL=0 in 64-bit mode *and* have a memory operand.  Some
> of them might even be legitimately encountered in the wild.
> 
>   - CMPXCHG16B if it's not supported by the CPU.
>   - VMXON if CR4 is misconfigured or VMX isn't enabled in FEATURE_CONTROL.
>   - MONITOR if ECX has an invalid hint (although MONITOR hardcodes the
>     address in DS:RAX and so doesn't have a ModR/M byte).
> 
> Undoudbtedly there are other instructions with similar sources of #GP.

Right, we currently put our trust in the insn decoder to handle those
correctly too.

-- 
Regards/Gruss,
    Boris.

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