linux-kernel - Re: [PATCH 2/2] x86/ibt: Avoid duplicate ENDBR in __put_user

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]

Date:   Wed, 2 Aug 2023 22:33:22 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     x86@...nel.org
Cc:     linux-kernel@...r.kernel.org, jpoimboe@...nel.org,
        andrew.cooper3@...rix.com, David.Kaplan@....com
Subject: Re: [PATCH 2/2] x86/ibt: Avoid duplicate ENDBR in
 __put_user_nocheck*()

On Wed, Aug 02, 2023 at 12:55:47PM +0200, Peter Zijlstra wrote:
> Commit cb855971d717 ("x86/putuser: Provide room for padding") changed
> __put_user_nocheck_*() into proper functions but failed to note that
> SYM_FUNC_START() already provides ENDBR, rendering the explicit ENDBR
> superfluous.

The below objtool thing reliably detects this pattern:

vmlinux.o: warning: objtool: __put_user_nocheck_1+0x4: duplicate ENDBR
vmlinux.o: warning: objtool: __put_user_nocheck_2+0x4: duplicate ENDBR
vmlinux.o: warning: objtool: __put_user_nocheck_4+0x4: duplicate ENDBR
vmlinux.o: warning: objtool: __put_user_nocheck_8+0x4: duplicate ENDBR

---
 tools/objtool/check.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 8936a05f0e5a..c6ba6e768b03 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -4254,13 +4254,30 @@ static bool noendbr_range(struct objtool_file *file, struct instruction *insn)
 	return insn->offset == sym->offset + sym->len;
 }
 
-static int validate_ibt_insn(struct objtool_file *file, struct instruction *insn)
+static int validate_ibt_insn(struct objtool_file *file,
+			     struct instruction *insn,
+			     struct instruction *prev)
 {
 	struct instruction *dest;
 	struct reloc *reloc;
 	unsigned long off;
 	int warnings = 0;
 
+	if (prev && prev->sym && prev->sym == insn->sym &&
+	    prev->type == INSN_ENDBR && insn->type == INSN_ENDBR) {
+		/*
+		 * ___bpf_prog_run() is a maze of 'label as value' and
+		 * 'computed goto' and GCC gets confused and emits back-to-back
+		 * ENDBR due to failing to merge two labels.
+		 *
+		 * Don't warn about this until GCC is fixed or verbose.
+		 */
+		if (strcmp(insn->sym->name, "___bpf_prog_run") != 0 || opts.verbose) {
+			WARN_INSN(insn, "duplicate ENDBR");
+			warnings++;
+		}
+	}
+
 	/*
 	 * Looking for function pointer load relocations.  Ignore
 	 * direct/indirect branches:
@@ -4382,11 +4399,13 @@ static int validate_ibt(struct objtool_file *file)
 {
 	struct section *sec;
 	struct reloc *reloc;
-	struct instruction *insn;
+	struct instruction *insn, *prev = NULL;
 	int warnings = 0;
 
-	for_each_insn(file, insn)
-		warnings += validate_ibt_insn(file, insn);
+	for_each_insn(file, insn) {
+		warnings += validate_ibt_insn(file, insn, prev);
+		prev = insn;
+	}
 
 	for_each_sec(file, sec) {