[PATCH v2 1/5] objtool: Deal with relative jump tables correctly

Ard Biesheuvel posted 5 patches 1 month, 2 weeks ago
There is a newer version of this series
[PATCH v2 1/5] objtool: Deal with relative jump tables correctly
Posted by Ard Biesheuvel 1 month, 2 weeks ago
From: Ard Biesheuvel <ardb@kernel.org>

Relative jump tables contain entries that carry the offset between the
target of the jump and the start of the jump table. This permits the use
of the PIC idiom of

    leaq    jump_table(%rip), %tbl
    movslq  (%tbl,%idx,4), %offset
    addq    %offset, %tbl
    jmp     *%tbl

The jump table entries are decorated with PC32 relocations, which record
the offset of the referenced symbol relative to the target of the
relocation, which is the individual entry in the table.  This means that
only the first entry produces the correct value directly; the subsequent
ones need to be corrected to produce the offset relative to the start of
the table, by applying an addend.

Given that the referenced symbols are anonymous, and thus already
expressed in terms of sections and addends, e.g., .text+0x5df9, the
correction is incorporated into the existing addend. The upshot of this
is that chasing the reference to find the target instruction needs to
take this second addend into account as well.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 tools/objtool/arch/x86/special.c    |  8 -------
 tools/objtool/check.c               | 24 +++++++++++++++++---
 tools/objtool/include/objtool/elf.h |  6 +++++
 3 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/tools/objtool/arch/x86/special.c b/tools/objtool/arch/x86/special.c
index 4ea0f9815fda..415e4d035e53 100644
--- a/tools/objtool/arch/x86/special.c
+++ b/tools/objtool/arch/x86/special.c
@@ -150,13 +150,5 @@ struct reloc *arch_find_switch_table(struct objtool_file *file,
 	if (!rodata_reloc)
 		return NULL;
 
-	/*
-	 * Use of RIP-relative switch jumps is quite rare, and
-	 * indicates a rare GCC quirk/bug which can leave dead
-	 * code behind.
-	 */
-	if (reloc_type(text_reloc) == R_X86_64_PC32)
-		file->ignore_unreachables = true;
-
 	return rodata_reloc;
 }
diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 3cb3e9b5ad0b..7f7981a93535 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -2101,6 +2101,8 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
 {
 	struct symbol *pfunc = insn_func(insn)->pfunc;
 	struct reloc *table = insn_jump_table(insn);
+	unsigned int rtype = reloc_type(table);
+	bool pcrel = rtype == R_X86_64_PC32;
 	struct instruction *dest_insn;
 	unsigned int prev_offset = 0;
 	struct reloc *reloc = table;
@@ -2111,13 +2113,18 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
 	 * instruction.
 	 */
 	for_each_reloc_from(table->sec, reloc) {
+		unsigned long addend = reloc_addend(reloc);
 
 		/* Check for the end of the table: */
 		if (reloc != table && reloc == next_table)
 			break;
 
+		/* Each entry in the jump table should use the same relocation type */
+		if (reloc_type(reloc) != rtype)
+			break;
+
 		/* Make sure the table entries are consecutive: */
-		if (prev_offset && reloc_offset(reloc) != prev_offset + 8)
+		if (prev_offset && reloc_offset(reloc) != prev_offset + (pcrel ? 4 : 8))
 			break;
 
 		/* Detect function pointers from contiguous objects: */
@@ -2125,7 +2132,15 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
 		    reloc_addend(reloc) == pfunc->offset)
 			break;
 
-		dest_insn = find_insn(file, reloc->sym->sec, reloc_addend(reloc));
+		/*
+		 * Place-relative jump tables carry offsets relative to the
+		 * start of the jump table, not to the entry itself. So correct
+		 * the addend for the location of the entry in the table.
+		 */
+		if (pcrel)
+			addend -= reloc_offset(reloc) - reloc_offset(table);
+
+		dest_insn = find_insn(file, reloc->sym->sec, addend);
 		if (!dest_insn)
 			break;
 
@@ -2133,6 +2148,9 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
 		if (!insn_func(dest_insn) || insn_func(dest_insn)->pfunc != pfunc)
 			break;
 
+		if (pcrel)
+			reloc->sym_offset = addend;
+
 		alt = malloc(sizeof(*alt));
 		if (!alt) {
 			WARN("malloc failed");
@@ -4536,7 +4554,7 @@ static int validate_ibt_data_reloc(struct objtool_file *file,
 	struct instruction *dest;
 
 	dest = find_insn(file, reloc->sym->sec,
-			 reloc->sym->offset + reloc_addend(reloc));
+			 reloc->sym->offset + reloc_sym_offset(reloc));
 	if (!dest)
 		return 0;
 
diff --git a/tools/objtool/include/objtool/elf.h b/tools/objtool/include/objtool/elf.h
index d7e815c2fd15..f4a6307f4c08 100644
--- a/tools/objtool/include/objtool/elf.h
+++ b/tools/objtool/include/objtool/elf.h
@@ -78,6 +78,7 @@ struct reloc {
 	struct section *sec;
 	struct symbol *sym;
 	struct reloc *sym_next_reloc;
+	s64 sym_offset;
 };
 
 struct elf {
@@ -251,6 +252,11 @@ static inline s64 reloc_addend(struct reloc *reloc)
 	return __get_reloc_field(reloc, r_addend);
 }
 
+static inline s64 reloc_sym_offset(struct reloc *reloc)
+{
+	return reloc->sym_offset ?: reloc_addend(reloc);
+}
+
 static inline void set_reloc_addend(struct elf *elf, struct reloc *reloc, s64 addend)
 {
 	__set_reloc_field(reloc, r_addend, addend);
-- 
2.47.0.rc0.187.ge670bccf7e-goog
Re: [PATCH v2 1/5] objtool: Deal with relative jump tables correctly
Posted by Peter Zijlstra 1 month, 2 weeks ago
On Thu, Oct 10, 2024 at 02:28:03PM +0200, Ard Biesheuvel wrote:
> diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> index 3cb3e9b5ad0b..7f7981a93535 100644
> --- a/tools/objtool/check.c
> +++ b/tools/objtool/check.c
> @@ -2101,6 +2101,8 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
>  {
>  	struct symbol *pfunc = insn_func(insn)->pfunc;
>  	struct reloc *table = insn_jump_table(insn);
> +	unsigned int rtype = reloc_type(table);
> +	bool pcrel = rtype == R_X86_64_PC32;

R_DATA32 or R_TEXT32 please, the budding cross arch stuff has their own
names for all that.
Re: [PATCH v2 1/5] objtool: Deal with relative jump tables correctly
Posted by Ard Biesheuvel 1 month, 2 weeks ago
On Thu, 10 Oct 2024 at 15:26, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Oct 10, 2024 at 02:28:03PM +0200, Ard Biesheuvel wrote:
> > diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> > index 3cb3e9b5ad0b..7f7981a93535 100644
> > --- a/tools/objtool/check.c
> > +++ b/tools/objtool/check.c
> > @@ -2101,6 +2101,8 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
> >  {
> >       struct symbol *pfunc = insn_func(insn)->pfunc;
> >       struct reloc *table = insn_jump_table(insn);
> > +     unsigned int rtype = reloc_type(table);
> > +     bool pcrel = rtype == R_X86_64_PC32;
>
> R_DATA32 or R_TEXT32 please, the budding cross arch stuff has their own
> names for all that.
>

#define R_DATA32        R_X86_64_PC32
#define R_DATA64        R_X86_64_PC32
#define R_TEXT32        R_X86_64_PC32
#define R_TEXT64        R_X86_64_PC32

Clear as mud.

I'd guess we need the '64' variant here, but I'm not sure which one to
use for a .rodata relocation pointing to .text. Any hints?
Re: [PATCH v2 1/5] objtool: Deal with relative jump tables correctly
Posted by Peter Zijlstra 1 month, 2 weeks ago
On Thu, Oct 10, 2024 at 03:59:43PM +0200, Ard Biesheuvel wrote:
> On Thu, 10 Oct 2024 at 15:26, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Thu, Oct 10, 2024 at 02:28:03PM +0200, Ard Biesheuvel wrote:
> > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> > > index 3cb3e9b5ad0b..7f7981a93535 100644
> > > --- a/tools/objtool/check.c
> > > +++ b/tools/objtool/check.c
> > > @@ -2101,6 +2101,8 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
> > >  {
> > >       struct symbol *pfunc = insn_func(insn)->pfunc;
> > >       struct reloc *table = insn_jump_table(insn);
> > > +     unsigned int rtype = reloc_type(table);
> > > +     bool pcrel = rtype == R_X86_64_PC32;
> >
> > R_DATA32 or R_TEXT32 please, the budding cross arch stuff has their own
> > names for all that.
> >
> 
> #define R_DATA32        R_X86_64_PC32
> #define R_DATA64        R_X86_64_PC32
> #define R_TEXT32        R_X86_64_PC32
> #define R_TEXT64        R_X86_64_PC32
> 
> Clear as mud.
> 
> I'd guess we need the '64' variant here, but I'm not sure which one to
> use for a .rodata relocation pointing to .text. Any hints?

Josh? do R_{DATA,TEXT}64 want to be _PC64 ?
Re: [PATCH v2 1/5] objtool: Deal with relative jump tables correctly
Posted by Josh Poimboeuf 1 month, 2 weeks ago
On Thu, Oct 10, 2024 at 04:07:45PM +0200, Peter Zijlstra wrote:
> On Thu, Oct 10, 2024 at 03:59:43PM +0200, Ard Biesheuvel wrote:
> > On Thu, 10 Oct 2024 at 15:26, Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > On Thu, Oct 10, 2024 at 02:28:03PM +0200, Ard Biesheuvel wrote:
> > > > diff --git a/tools/objtool/check.c b/tools/objtool/check.c
> > > > index 3cb3e9b5ad0b..7f7981a93535 100644
> > > > --- a/tools/objtool/check.c
> > > > +++ b/tools/objtool/check.c
> > > > @@ -2101,6 +2101,8 @@ static int add_jump_table(struct objtool_file *file, struct instruction *insn,
> > > >  {
> > > >       struct symbol *pfunc = insn_func(insn)->pfunc;
> > > >       struct reloc *table = insn_jump_table(insn);
> > > > +     unsigned int rtype = reloc_type(table);
> > > > +     bool pcrel = rtype == R_X86_64_PC32;
> > >
> > > R_DATA32 or R_TEXT32 please, the budding cross arch stuff has their own
> > > names for all that.
> > >
> > 
> > #define R_DATA32        R_X86_64_PC32
> > #define R_DATA64        R_X86_64_PC32
> > #define R_TEXT32        R_X86_64_PC32
> > #define R_TEXT64        R_X86_64_PC32
> > 
> > Clear as mud.

Yep... :-/

> > 
> > I'd guess we need the '64' variant here, but I'm not sure which one to
> > use for a .rodata relocation pointing to .text. Any hints?

This should be R_TEXT64.

> Josh? do R_{DATA,TEXT}64 want to be _PC64 ?

Actually, R_{DATA,TEXT}32 should be R_386_PC32.

-- 
Josh