diff mbox series

objtool: Provide origin hint for elf_init_reloc_text_sym() errors

Message ID 20240430235106.work.680-kees@kernel.org (mailing list archive)
State New
Headers show
Series objtool: Provide origin hint for elf_init_reloc_text_sym() errors | expand

Commit Message

Kees Cook April 30, 2024, 11:51 p.m. UTC 
An error report from elf_init_reloc_text_sym() doesn't say what list of
symbols it is working on. Include this on the caller's side so it can be
reported when pathological conditions are encountered.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
I added this to confirm debugging of
https://lore.kernel.org/lkml/202308301532.d7acf63e-oliver.sang@intel.com/
---
Cc: Josh Poimboeuf <jpoimboe@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Youling Tang <tangyouling@loongson.cn>
Cc: Jinyang He <hejinyang@loongson.cn>
Cc: Huacai Chen <chenhuacai@kernel.org>
Cc: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 tools/objtool/arch/loongarch/orc.c  |  3 ++-
 tools/objtool/arch/x86/orc.c        |  3 ++-
 tools/objtool/check.c               | 21 +++++++++++++--------
 tools/objtool/elf.c                 |  7 ++++---
 tools/objtool/include/objtool/elf.h |  3 ++-
 5 files changed, 23 insertions(+), 14 deletions(-)

Comments

Josh Poimboeuf May 4, 2024, 10:24 p.m. UTC | #1 
On Tue, Apr 30, 2024 at 04:51:07PM -0700, Kees Cook wrote:
> @@ -891,8 +892,8 @@ struct reloc *elf_init_reloc_text_sym(struct elf *elf, struct section *sec,
>  	int addend = insn_off;
>  
>  	if (!(insn_sec->sh.sh_flags & SHF_EXECINSTR)) {
> -		WARN("bad call to %s() for data symbol %s",
> -		     __func__, sym->name);
> +		WARN("bad call to %s() for %s symbol %s",
> +		     __func__, origin, sym->name);
>  		return NULL;

Thanks for the patch.

That warning was already phrased pretty awkwardly which was probably
part of the confusion.  It could be rephrased to make it a little
clearer:

Something like:

  .cfi_sites: unexpected reference to non-executable symbol 'execute_location'

And ".cfi_sites" is already in 'sec->name', so you wouldn't need to add
the new 'origin' arg.
Kees Cook May 6, 2024, 3:51 p.m. UTC | #2 
On Sat, May 04, 2024 at 03:24:02PM -0700, Josh Poimboeuf wrote:
> On Tue, Apr 30, 2024 at 04:51:07PM -0700, Kees Cook wrote:
> > @@ -891,8 +892,8 @@ struct reloc *elf_init_reloc_text_sym(struct elf *elf, struct section *sec,
> >  	int addend = insn_off;
> >  
> >  	if (!(insn_sec->sh.sh_flags & SHF_EXECINSTR)) {
> > -		WARN("bad call to %s() for data symbol %s",
> > -		     __func__, sym->name);
> > +		WARN("bad call to %s() for %s symbol %s",
> > +		     __func__, origin, sym->name);
> >  		return NULL;
> 
> Thanks for the patch.
> 
> That warning was already phrased pretty awkwardly which was probably
> part of the confusion.  It could be rephrased to make it a little
> clearer:
> 
> Something like:
> 
>   .cfi_sites: unexpected reference to non-executable symbol 'execute_location'
> 
> And ".cfi_sites" is already in 'sec->name', so you wouldn't need to add
> the new 'origin' arg.

What's so odd is that "execute_location" wasn't being reported at all.
Just ".rodata": 

vmlinux.o: warning: objtool: bad call to elf_init_reloc_text_sym() for data symbol .rodata

But yes, sec->name has what I want, so I can do this easily:

-               WARN("bad call to %s() for data symbol %s",
-                    __func__, sym->name);
+               WARN("bad call to %s() for %s symbol %s",
+                    __func__, sec->name, sym->name);

vmlinux.o: warning: objtool: bad call to elf_init_reloc_text_sym() for .cfi_sites symbol .rodata

I think the symbol is missing because this is coming from
create_cfi_sections()/elf_create_section_pair().

Regardless, I'll send a v2...

Thanks!
diff mbox series

Patch

diff --git a/tools/objtool/arch/loongarch/orc.c b/tools/objtool/arch/loongarch/orc.c
index 873536d009d9..5ed9c1bb2de3 100644
--- a/tools/objtool/arch/loongarch/orc.c
+++ b/tools/objtool/arch/loongarch/orc.c
@@ -110,7 +110,8 @@  int write_orc_entry(struct elf *elf, struct section *orc_sec,
 	memcpy(orc, o, sizeof(*orc));
 
 	/* populate reloc for ip */
-	if (!elf_init_reloc_text_sym(elf, ip_sec, idx * sizeof(int), idx,
+	if (!elf_init_reloc_text_sym("orc_list", elf, ip_sec,
+				     idx * sizeof(int), idx,
 				     insn_sec, insn_off))
 		return -1;
 
diff --git a/tools/objtool/arch/x86/orc.c b/tools/objtool/arch/x86/orc.c
index b6cd943e87f9..68a78ad0d99e 100644
--- a/tools/objtool/arch/x86/orc.c
+++ b/tools/objtool/arch/x86/orc.c
@@ -111,7 +111,8 @@  int write_orc_entry(struct elf *elf, struct section *orc_sec,
 	orc->bp_offset = bswap_if_needed(elf, orc->bp_offset);
 
 	/* populate reloc for ip */
-	if (!elf_init_reloc_text_sym(elf, ip_sec, idx * sizeof(int), idx,
+	if (!elf_init_reloc_text_sym("orc_list", elf, ip_sec,
+				     idx * sizeof(int), idx,
 				     insn_sec, insn_off))
 		return -1;
 
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 0a33d9195b7a..e7f7300ba88b 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -701,7 +701,7 @@  static int create_static_call_sections(struct objtool_file *file)
 	list_for_each_entry(insn, &file->static_call_list, call_node) {
 
 		/* populate reloc for 'addr' */
-		if (!elf_init_reloc_text_sym(file->elf, sec,
+		if (!elf_init_reloc_text_sym("static_call_list", file->elf, sec,
 					     idx * sizeof(*site), idx * 2,
 					     insn->sec, insn->offset))
 			return -1;
@@ -782,7 +782,8 @@  static int create_retpoline_sites_sections(struct objtool_file *file)
 	idx = 0;
 	list_for_each_entry(insn, &file->retpoline_call_list, call_node) {
 
-		if (!elf_init_reloc_text_sym(file->elf, sec,
+		if (!elf_init_reloc_text_sym("retpoline_call_list",
+					     file->elf, sec,
 					     idx * sizeof(int), idx,
 					     insn->sec, insn->offset))
 			return -1;
@@ -820,7 +821,8 @@  static int create_return_sites_sections(struct objtool_file *file)
 	idx = 0;
 	list_for_each_entry(insn, &file->return_thunk_list, call_node) {
 
-		if (!elf_init_reloc_text_sym(file->elf, sec,
+		if (!elf_init_reloc_text_sym("return_thunk_list",
+					     file->elf, sec,
 					     idx * sizeof(int), idx,
 					     insn->sec, insn->offset))
 			return -1;
@@ -874,7 +876,8 @@  static int create_ibt_endbr_seal_sections(struct objtool_file *file)
 		     !strcmp(sym->name, "cleanup_module")))
 			WARN("%s(): not an indirect call target", sym->name);
 
-		if (!elf_init_reloc_text_sym(file->elf, sec,
+		if (!elf_init_reloc_text_sym("endbr_list",
+					     file->elf, sec,
 					     idx * sizeof(int), idx,
 					     insn->sec, insn->offset))
 			return -1;
@@ -922,7 +925,7 @@  static int create_cfi_sections(struct objtool_file *file)
 		if (strncmp(sym->name, "__cfi_", 6))
 			continue;
 
-		if (!elf_init_reloc_text_sym(file->elf, sec,
+		if (!elf_init_reloc_text_sym(".cfi_sites", file->elf, sec,
 					     idx * sizeof(unsigned int), idx,
 					     sym->sec, sym->offset))
 			return -1;
@@ -966,8 +969,10 @@  static int create_mcount_loc_sections(struct objtool_file *file)
 
 		struct reloc *reloc;
 
-		reloc = elf_init_reloc_text_sym(file->elf, sec, idx * addr_size, idx,
-					       insn->sec, insn->offset);
+		reloc = elf_init_reloc_text_sym("mcount_loc_list",
+						file->elf, sec,
+						idx * addr_size, idx,
+						insn->sec, insn->offset);
 		if (!reloc)
 			return -1;
 
@@ -1007,7 +1012,7 @@  static int create_direct_call_sections(struct objtool_file *file)
 	idx = 0;
 	list_for_each_entry(insn, &file->call_list, call_node) {
 
-		if (!elf_init_reloc_text_sym(file->elf, sec,
+		if (!elf_init_reloc_text_sym("call_list", file->elf, sec,
 					     idx * sizeof(unsigned int), idx,
 					     insn->sec, insn->offset))
 			return -1;
diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index 3d27983dc908..8615672e2ff9 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -881,7 +881,8 @@  static struct reloc *elf_init_reloc(struct elf *elf, struct section *rsec,
 	return reloc;
 }
 
-struct reloc *elf_init_reloc_text_sym(struct elf *elf, struct section *sec,
+struct reloc *elf_init_reloc_text_sym(const char *origin,
+				      struct elf *elf, struct section *sec,
 				      unsigned long offset,
 				      unsigned int reloc_idx,
 				      struct section *insn_sec,
@@ -891,8 +892,8 @@  struct reloc *elf_init_reloc_text_sym(struct elf *elf, struct section *sec,
 	int addend = insn_off;
 
 	if (!(insn_sec->sh.sh_flags & SHF_EXECINSTR)) {
-		WARN("bad call to %s() for data symbol %s",
-		     __func__, sym->name);
+		WARN("bad call to %s() for %s symbol %s",
+		     __func__, origin, sym->name);
 		return NULL;
 	}
 
diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h
index 2b8a69de4db8..d026e707806d 100644
--- a/tools/objtool/include/objtool/elf.h
+++ b/tools/objtool/include/objtool/elf.h
@@ -115,7 +115,8 @@  struct section *elf_create_section_pair(struct elf *elf, const char *name,
 
 struct symbol *elf_create_prefix_symbol(struct elf *elf, struct symbol *orig, long size);
 
-struct reloc *elf_init_reloc_text_sym(struct elf *elf, struct section *sec,
+struct reloc *elf_init_reloc_text_sym(const char *origin,
+				      struct elf *elf, struct section *sec,
 				      unsigned long offset,
 				      unsigned int reloc_idx,
 				      struct section *insn_sec,