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

From: Borislav Petkov <bp@alien8.de>
To: Jann Horn <jannh@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
	x86@kernel.org, Andrey Ryabinin <aryabinin@virtuozzo.com>,
	Alexander Potapenko <glider@google.com>,
	Dmitry Vyukov <dvyukov@google.com>,
	kasan-dev@googlegroups.com, linux-kernel@vger.kernel.org,
	Andrey Konovalov <andreyknvl@google.com>,
	Andy Lutomirski <luto@kernel.org>,
	Sean Christopherson <sean.j.christopherson@intel.com>
Subject: Re: [PATCH v2 2/3] x86/traps: Print non-canonical address on #GP
Date: Mon, 18 Nov 2019 15:21:44 +0100	[thread overview]
Message-ID: <20191118142144.GC6363@zn.tnic> (raw)
In-Reply-To: <20191115191728.87338-2-jannh@google.com>

On Fri, Nov 15, 2019 at 08:17:27PM +0100, Jann Horn wrote:
>  dotraplinkage void
>  do_general_protection(struct pt_regs *regs, long error_code)
>  {
> @@ -547,8 +581,15 @@ 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)
> +			pr_alert("GPF is segment-related (see error code)\n");
> +		else
> +			print_kernel_gp_address(regs);
> +
> +		die(desc, regs, error_code);

Right, this way, those messages appear before the main "general
protection ..." message:

[    2.434372] traps: probably dereferencing non-canonical address 0xdfff000000000001
[    2.442492] general protection fault: 0000 [#1] PREEMPT SMP

Can we glue/merge them together? Or is this going to confuse tools too much:

[    2.542218] general protection fault while derefing a non-canonical address 0xdfff000000000001: 0000 [#1] PREEMPT SMP

(and that sentence could be shorter too:

 	"general protection fault for non-canonical address 0xdfff000000000001"

looks ok to me too.)

Here's a dirty diff together with a reproducer ontop of yours:

---

diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index bf796f8c9998..dab702ba28a6 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -515,7 +515,7 @@ 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, print that address.
  */
-static void print_kernel_gp_address(struct pt_regs *regs)
+static unsigned long get_kernel_gp_address(struct pt_regs *regs)
 {
 #ifdef CONFIG_X86_64
 	u8 insn_bytes[MAX_INSN_SIZE];
@@ -523,7 +523,7 @@ static void print_kernel_gp_address(struct pt_regs *regs)
 	unsigned long addr_ref;
 
 	if (probe_kernel_read(insn_bytes, (void *)regs->ip, MAX_INSN_SIZE))
-		return;
+		return 0;
 
 	kernel_insn_init(&insn, insn_bytes, MAX_INSN_SIZE);
 	insn_get_modrm(&insn);
@@ -532,22 +532,22 @@ static void print_kernel_gp_address(struct pt_regs *regs)
 
 	/* Bail out if insn_get_addr_ref() failed or we got a kernel address. */
 	if (addr_ref >= ~__VIRTUAL_MASK)
-		return;
+		return 0;
 
 	/* Bail out if the entire operand is in the canonical user half. */
 	if (addr_ref + insn.opnd_bytes - 1 <= __VIRTUAL_MASK)
-		return;
+		return 0;
 
-	pr_alert("probably dereferencing non-canonical address 0x%016lx\n",
-		 addr_ref);
+	return addr_ref;
 #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];
 
 	RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
 	cond_local_irq_enable(regs);
@@ -584,12 +584,18 @@ do_general_protection(struct pt_regs *regs, long error_code)
 			       X86_TRAP_GP, SIGSEGV) == NOTIFY_STOP)
 			return;
 
-		if (error_code)
-			pr_alert("GPF is segment-related (see error code)\n");
-		else
-			print_kernel_gp_address(regs);
+		if (error_code) {
+			snprintf(desc, 90, "segment-related " GPFSTR);
+		} else {
+			unsigned long addr_ref = get_kernel_gp_address(regs);
+
+			if (addr_ref)
+				snprintf(desc, 90, GPFSTR " while derefing a non-canonical address 0x%lx", addr_ref);
+			else
+				snprintf(desc, 90, GPFSTR);
+		}
 
-		die(desc, regs, error_code);
+		die((const char *)desc, regs, error_code);
 		return;
 	}
 
diff --git a/init/main.c b/init/main.c
index 91f6ebb30ef0..7acc7e660be9 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1124,6 +1124,9 @@ static int __ref kernel_init(void *unused)
 
 	rcu_end_inkernel_boot();
 
+	asm volatile("mov $0xdfff000000000001, %rax\n\t"
+		     "jmpq *%rax\n\t");
+
 	if (ramdisk_execute_command) {
 		ret = run_init_process(ramdisk_execute_command);
 		if (!ret)

-- 
Regards/Gruss,
    Boris.

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