linux-kernel - Re: [PATCH v4 03/16] x86/alternatives,jump_label: Provide better text

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191021084802.GA825@gmail.com>
Date:   Mon, 21 Oct 2019 10:48:02 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org, rostedt@...dmis.org,
        mhiramat@...nel.org, bristot@...hat.com, jbaron@...mai.com,
        torvalds@...ux-foundation.org, tglx@...utronix.de,
        namit@...are.com, hpa@...or.com, luto@...nel.org,
        ard.biesheuvel@...aro.org, jpoimboe@...hat.com, jeyu@...nel.org
Subject: Re: [PATCH v4 03/16] x86/alternatives,jump_label: Provide better
 text_poke() batching interface


* Peter Zijlstra <peterz@...radead.org> wrote:

> --- a/arch/x86/kernel/jump_label.c
> +++ b/arch/x86/kernel/jump_label.c
> @@ -35,18 +35,19 @@ static void bug_at(unsigned char *ip, in
>  	BUG();
>  }
>  
> -static void __jump_label_set_jump_code(struct jump_entry *entry,
> -				       enum jump_label_type type,
> -				       union jump_code_union *code,
> -				       int init)
> +static const void *
> +__jump_label_set_jump_code(struct jump_entry *entry, enum jump_label_type type, int init)
>  {
> +	static union jump_code_union code; /* relies on text_mutex */
>  	const unsigned char default_nop[] = { STATIC_KEY_INIT_NOP };
>  	const unsigned char *ideal_nop = ideal_nops[NOP_ATOMIC5];
>  	const void *expect;
>  	int line;
>  
> -	code->jump = 0xe9;
> -	code->offset = jump_entry_target(entry) -
> +	lockdep_assert_held(&text_mutex);
> +
> +	code.jump = JMP32_INSN_OPCODE;
> +	code.offset = jump_entry_target(entry) -
>  		       (jump_entry_code(entry) + JUMP_LABEL_NOP_SIZE);
>  
>  	if (init) {
> @@ -54,23 +55,23 @@ static void __jump_label_set_jump_code(s
>  	} else if (type == JUMP_LABEL_JMP) {
>  		expect = ideal_nop; line = __LINE__;
>  	} else {
> -		expect = code->code; line = __LINE__;
> +		expect = code.code; line = __LINE__;

Side note: the whole 'line' logic looked weird to me and it obsfuscates 
the logic a bit, and I had to look it up to see what it's about: 
improving the debug output of text-patching crashes.

How about something like the below on top of your queue? We have %phD 
that can nicely print instructions in hex.

Totally untested though.

Thanks,

	Ingo

---
 arch/x86/kernel/jump_label.c |   21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

Index: tip/arch/x86/kernel/jump_label.c
===================================================================
--- tip.orig/arch/x86/kernel/jump_label.c
+++ tip/arch/x86/kernel/jump_label.c
@@ -16,14 +16,15 @@
 #include <asm/alternative.h>
 #include <asm/text-patching.h>
 
-static void bug_at(const void *ip, int line)
+static void bug_at(const void *ip, const void *op_expected, const void *op_unexpected)
 {
 	/*
 	 * The location is not an op that we were expecting.
 	 * Something went wrong. Crash the box, as something could be
 	 * corrupting the kernel.
 	 */
-	pr_crit("jump_label: Fatal kernel bug, unexpected op at %pS [%p] (%5ph) %d\n", ip, ip, ip, line);
+	pr_crit("jump_label: Fatal kernel bug, expected op (%*phD), unexpected op (%*phD) at %pS [%p] (%5ph\n",
+		JUMP_LABEL_NOP_SIZE, op_expected, JUMP_LABEL_NOP_SIZE, op_unexpected, ip, ip, ip);
 	BUG();
 }
 
@@ -34,23 +35,21 @@ __jump_label_set_jump_code(struct jump_e
 	const unsigned char *ideal_nop = ideal_nops[NOP_ATOMIC5];
 	const void *expect, *code;
 	const void *addr, *dest;
-	int line;
 
 	addr = (void *)jump_entry_code(entry);
 	dest = (void *)jump_entry_target(entry);
 
 	code = text_gen_insn(JMP32_INSN_OPCODE, addr, dest);
 
-	if (init) {
-		expect = default_nop; line = __LINE__;
-	} else if (type == JUMP_LABEL_JMP) {
-		expect = ideal_nop; line = __LINE__;
-	} else {
-		expect = code; line = __LINE__;
-	}
+	if (init)
+		expect = default_nop;
+	else if (type == JUMP_LABEL_JMP)
+		expect = ideal_nop;
+	else
+		expect = code;
 
 	if (memcmp(addr, expect, JUMP_LABEL_NOP_SIZE))
-		bug_at(addr, line);
+		bug_at(addr, expect, addr);
 
 	if (type == JUMP_LABEL_NOP)
 		code = ideal_nop;