diff mbox series

[2/2] x86/ibt: Avoid duplicate ENDBR in __put_user_nocheck*()

Message ID 20230802110323.086971726@infradead.org
State New
Headers
Series x86/ibt: Fix some spurious ENDBR instances |

Commit Message

Peter Zijlstra Aug. 2, 2023, 10:55 a.m. UTC 
  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.

Fixes: cb855971d717 ("x86/putuser: Provide room for padding")
Reported-by: David Kaplan <David.Kaplan@amd.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/lib/putuser.S |    4 ----
 1 file changed, 4 deletions(-)

Comments

Peter Zijlstra Aug. 2, 2023, 8:33 p.m. UTC | #1 
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) {
diff mbox series

Patch

--- a/arch/x86/lib/putuser.S
+++ b/arch/x86/lib/putuser.S
@@ -56,7 +56,6 @@  SYM_FUNC_END(__put_user_1)
 EXPORT_SYMBOL(__put_user_1)
 
 SYM_FUNC_START(__put_user_nocheck_1)
-	ENDBR
 	ASM_STAC
 2:	movb %al,(%_ASM_CX)
 	xor %ecx,%ecx
@@ -76,7 +75,6 @@  SYM_FUNC_END(__put_user_2)
 EXPORT_SYMBOL(__put_user_2)
 
 SYM_FUNC_START(__put_user_nocheck_2)
-	ENDBR
 	ASM_STAC
 4:	movw %ax,(%_ASM_CX)
 	xor %ecx,%ecx
@@ -96,7 +94,6 @@  SYM_FUNC_END(__put_user_4)
 EXPORT_SYMBOL(__put_user_4)
 
 SYM_FUNC_START(__put_user_nocheck_4)
-	ENDBR
 	ASM_STAC
 6:	movl %eax,(%_ASM_CX)
 	xor %ecx,%ecx
@@ -119,7 +116,6 @@  SYM_FUNC_END(__put_user_8)
 EXPORT_SYMBOL(__put_user_8)
 
 SYM_FUNC_START(__put_user_nocheck_8)
-	ENDBR
 	ASM_STAC
 9:	mov %_ASM_AX,(%_ASM_CX)
 #ifdef CONFIG_X86_32