[PATCH 1/2] objtool: Check for gelf_update_rel[a] failures

[PATCH 1/2] objtool: Check for gelf_update_rel[a] failures

[Michael Forney]

Otherwise, if these fail we end up with garbage data in the
.rela.orc_unwind_ip section, leading to errors like

  ld: fs/squashfs/namei.o: bad reloc symbol index (0x7f16 >= 0x12) for offset 0x7f16d5c82cc8 in section `.orc_unwind_ip'

Signed-off-by: Michael Forney <mforney@mforney.org>
---
 tools/objtool/elf.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index d08f5f3670f8..b396ee4ab85d 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -953,7 +953,10 @@ static int elf_rebuild_rel_reloc_section(struct section *sec, int nr)
 	list_for_each_entry(reloc, &sec->reloc_list, list) {
 		reloc->rel.r_offset = reloc->offset;
 		reloc->rel.r_info = GELF_R_INFO(reloc->sym->idx, reloc->type);
-		gelf_update_rel(sec->data, idx, &reloc->rel);
+		if (!gelf_update_rel(sec->data, idx, &reloc->rel)) {
+			WARN_ELF("gelf_update_rel");
+			return -1;
+		}
 		idx++;
 	}
 
@@ -985,7 +988,10 @@ static int elf_rebuild_rela_reloc_section(struct section *sec, int nr)
 		reloc->rela.r_offset = reloc->offset;
 		reloc->rela.r_addend = reloc->addend;
 		reloc->rela.r_info = GELF_R_INFO(reloc->sym->idx, reloc->type);
-		gelf_update_rela(sec->data, idx, &reloc->rela);
+		if (!gelf_update_rela(sec->data, idx, &reloc->rela)) {
+			WARN_ELF("gelf_update_rela");
+			return -1;
+		}
 		idx++;
 	}
 
-- 
2.31.1

[PATCH 2/2] objtool: Update section header before relocations

[Michael Forney]

The libelf implemention from elftoolchain has a safety check in
gelf_update_rel[a] to check that the data corresponds to a section
that has type SHT_REL[A] [0]. If the relocation is updated before
the section header is updated with the proper type, this check
fails.

To fix this, update the section header first, before the relocations.
Previously, the section size was calculated in elf_rebuild_reloc_section
by counting the number of entries in the reloc_list. However, we
now need the size during elf_write so instead keep a running total
and add to it for every new relocation.

[0] https://sourceforge.net/p/elftoolchain/mailman/elftoolchain-developers/thread/CAGw6cBtkZro-8wZMD2ULkwJ39J+tHtTtAWXufMjnd3cQ7XG54g@mail.gmail.com/

Signed-off-by: Michael Forney <mforney@mforney.org>
---
 tools/objtool/elf.c | 46 +++++++++++++++++----------------------------
 1 file changed, 17 insertions(+), 29 deletions(-)

diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index b396ee4ab85d..53d00fefebcf 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -481,6 +481,7 @@ int elf_add_reloc(struct elf *elf, struct section *sec, unsigned long offset,
 	list_add_tail(&reloc->list, &sec->reloc->reloc_list);
 	elf_hash_add(elf->reloc_hash, &reloc->hash, reloc_hash(reloc));
 
+	sec->reloc->sh.sh_size += sec->reloc->sh.sh_entsize;
 	sec->reloc->changed = true;
 
 	return 0;
@@ -929,26 +930,23 @@ static struct section *elf_create_reloc_section(struct elf *elf,
 	}
 }
 
-static int elf_rebuild_rel_reloc_section(struct section *sec, int nr)
+static int elf_rebuild_rel_reloc_section(struct section *sec)
 {
 	struct reloc *reloc;
-	int idx = 0, size;
+	int idx = 0;
 	void *buf;
 
 	/* Allocate a buffer for relocations */
-	size = nr * sizeof(GElf_Rel);
-	buf = malloc(size);
+	buf = malloc(sec->sh.sh_size);
 	if (!buf) {
 		perror("malloc");
 		return -1;
 	}
 
 	sec->data->d_buf = buf;
-	sec->data->d_size = size;
+	sec->data->d_size = sec->sh.sh_size;
 	sec->data->d_type = ELF_T_REL;
 
-	sec->sh.sh_size = size;
-
 	idx = 0;
 	list_for_each_entry(reloc, &sec->reloc_list, list) {
 		reloc->rel.r_offset = reloc->offset;
@@ -963,26 +961,23 @@ static int elf_rebuild_rel_reloc_section(struct section *sec, int nr)
 	return 0;
 }
 
-static int elf_rebuild_rela_reloc_section(struct section *sec, int nr)
+static int elf_rebuild_rela_reloc_section(struct section *sec)
 {
 	struct reloc *reloc;
-	int idx = 0, size;
+	int idx = 0;
 	void *buf;
 
 	/* Allocate a buffer for relocations with addends */
-	size = nr * sizeof(GElf_Rela);
-	buf = malloc(size);
+	buf = malloc(sec->sh.sh_size);
 	if (!buf) {
 		perror("malloc");
 		return -1;
 	}
 
 	sec->data->d_buf = buf;
-	sec->data->d_size = size;
+	sec->data->d_size = sec->sh.sh_size;
 	sec->data->d_type = ELF_T_RELA;
 
-	sec->sh.sh_size = size;
-
 	idx = 0;
 	list_for_each_entry(reloc, &sec->reloc_list, list) {
 		reloc->rela.r_offset = reloc->offset;
@@ -1000,16 +995,9 @@ static int elf_rebuild_rela_reloc_section(struct section *sec, int nr)
 
 static int elf_rebuild_reloc_section(struct elf *elf, struct section *sec)
 {
-	struct reloc *reloc;
-	int nr;
-
-	nr = 0;
-	list_for_each_entry(reloc, &sec->reloc_list, list)
-		nr++;
-
 	switch (sec->sh.sh_type) {
-	case SHT_REL:  return elf_rebuild_rel_reloc_section(sec, nr);
-	case SHT_RELA: return elf_rebuild_rela_reloc_section(sec, nr);
+	case SHT_REL:  return elf_rebuild_rel_reloc_section(sec);
+	case SHT_RELA: return elf_rebuild_rela_reloc_section(sec);
 	default:       return -1;
 	}
 }
@@ -1069,12 +1057,6 @@ int elf_write(struct elf *elf)
 	/* Update changed relocation sections and section headers: */
 	list_for_each_entry(sec, &elf->sections, list) {
 		if (sec->changed) {
-			if (sec->base &&
-			    elf_rebuild_reloc_section(elf, sec)) {
-				WARN("elf_rebuild_reloc_section");
-				return -1;
-			}
-
 			s = elf_getscn(elf->elf, sec->idx);
 			if (!s) {
 				WARN_ELF("elf_getscn");
@@ -1085,6 +1067,12 @@ int elf_write(struct elf *elf)
 				return -1;
 			}
 
+			if (sec->base &&
+			    elf_rebuild_reloc_section(elf, sec)) {
+				WARN("elf_rebuild_reloc_section");
+				return -1;
+			}
+
 			sec->changed = false;
 			elf->changed = true;
 		}
-- 
2.31.1

Re: [PATCH 2/2] objtool: Update section header before relocations

[Miroslav Benes]

On Sat, 8 May 2021, Michael Forney wrote:

> The libelf implemention from elftoolchain has a safety check in
> gelf_update_rel[a] to check that the data corresponds to a section
> that has type SHT_REL[A] [0]. If the relocation is updated before
> the section header is updated with the proper type, this check
> fails.
> 
> To fix this, update the section header first, before the relocations.
> Previously, the section size was calculated in elf_rebuild_reloc_section
> by counting the number of entries in the reloc_list. However, we
> now need the size during elf_write so instead keep a running total
> and add to it for every new relocation.
> 
> [0] https://sourceforge.net/p/elftoolchain/mailman/elftoolchain-developers/thread/CAGw6cBtkZro-8wZMD2ULkwJ39J+tHtTtAWXufMjnd3cQ7XG54g@mail.gmail.com/
> 
> Signed-off-by: Michael Forney <mforney@mforney.org>

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

M

[tip: objtool/urgent] objtool: Update section header before relocations

[tip-bot2 for Michael Forney]

The following commit has been merged into the objtool/urgent branch of tip:

Commit-ID:     86e1e054e0d2105cf32b0266cf1a64e6c26424f7
Gitweb:        https://git.kernel.org/tip/86e1e054e0d2105cf32b0266cf1a64e6c26424f7
Author:        Michael Forney <mforney@mforney.org>
AuthorDate:    Sat, 08 May 2021 17:01:03 -07:00
Committer:     Josh Poimboeuf <jpoimboe@redhat.com>
CommitterDate: Wed, 06 Oct 2021 20:11:57 -07:00

objtool: Update section header before relocations

The libelf implementation from elftoolchain has a safety check in
gelf_update_rel[a] to check that the data corresponds to a section
that has type SHT_REL[A] [0]. If the relocation is updated before
the section header is updated with the proper type, this check
fails.

To fix this, update the section header first, before the relocations.
Previously, the section size was calculated in elf_rebuild_reloc_section
by counting the number of entries in the reloc_list. However, we
now need the size during elf_write so instead keep a running total
and add to it for every new relocation.

[0] https://sourceforge.net/p/elftoolchain/mailman/elftoolchain-developers/thread/CAGw6cBtkZro-8wZMD2ULkwJ39J+tHtTtAWXufMjnd3cQ7XG54g@mail.gmail.com/

Signed-off-by: Michael Forney <mforney@mforney.org>
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Link: https://lore.kernel.org/r/20210509000103.11008-2-mforney@mforney.org
---
 tools/objtool/elf.c | 46 ++++++++++++++++----------------------------
 1 file changed, 17 insertions(+), 29 deletions(-)

diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index d1d4491..fee03b7 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -508,6 +508,7 @@ int elf_add_reloc(struct elf *elf, struct section *sec, unsigned long offset,
 	list_add_tail(&reloc->list, &sec->reloc->reloc_list);
 	elf_hash_add(reloc, &reloc->hash, reloc_hash(reloc));
 
+	sec->reloc->sh.sh_size += sec->reloc->sh.sh_entsize;
 	sec->reloc->changed = true;
 
 	return 0;
@@ -977,26 +978,23 @@ static struct section *elf_create_reloc_section(struct elf *elf,
 	}
 }
 
-static int elf_rebuild_rel_reloc_section(struct section *sec, int nr)
+static int elf_rebuild_rel_reloc_section(struct section *sec)
 {
 	struct reloc *reloc;
-	int idx = 0, size;
+	int idx = 0;
 	void *buf;
 
 	/* Allocate a buffer for relocations */
-	size = nr * sizeof(GElf_Rel);
-	buf = malloc(size);
+	buf = malloc(sec->sh.sh_size);
 	if (!buf) {
 		perror("malloc");
 		return -1;
 	}
 
 	sec->data->d_buf = buf;
-	sec->data->d_size = size;
+	sec->data->d_size = sec->sh.sh_size;
 	sec->data->d_type = ELF_T_REL;
 
-	sec->sh.sh_size = size;
-
 	idx = 0;
 	list_for_each_entry(reloc, &sec->reloc_list, list) {
 		reloc->rel.r_offset = reloc->offset;
@@ -1011,26 +1009,23 @@ static int elf_rebuild_rel_reloc_section(struct section *sec, int nr)
 	return 0;
 }
 
-static int elf_rebuild_rela_reloc_section(struct section *sec, int nr)
+static int elf_rebuild_rela_reloc_section(struct section *sec)
 {
 	struct reloc *reloc;
-	int idx = 0, size;
+	int idx = 0;
 	void *buf;
 
 	/* Allocate a buffer for relocations with addends */
-	size = nr * sizeof(GElf_Rela);
-	buf = malloc(size);
+	buf = malloc(sec->sh.sh_size);
 	if (!buf) {
 		perror("malloc");
 		return -1;
 	}
 
 	sec->data->d_buf = buf;
-	sec->data->d_size = size;
+	sec->data->d_size = sec->sh.sh_size;
 	sec->data->d_type = ELF_T_RELA;
 
-	sec->sh.sh_size = size;
-
 	idx = 0;
 	list_for_each_entry(reloc, &sec->reloc_list, list) {
 		reloc->rela.r_offset = reloc->offset;
@@ -1048,16 +1043,9 @@ static int elf_rebuild_rela_reloc_section(struct section *sec, int nr)
 
 static int elf_rebuild_reloc_section(struct elf *elf, struct section *sec)
 {
-	struct reloc *reloc;
-	int nr;
-
-	nr = 0;
-	list_for_each_entry(reloc, &sec->reloc_list, list)
-		nr++;
-
 	switch (sec->sh.sh_type) {
-	case SHT_REL:  return elf_rebuild_rel_reloc_section(sec, nr);
-	case SHT_RELA: return elf_rebuild_rela_reloc_section(sec, nr);
+	case SHT_REL:  return elf_rebuild_rel_reloc_section(sec);
+	case SHT_RELA: return elf_rebuild_rela_reloc_section(sec);
 	default:       return -1;
 	}
 }
@@ -1117,12 +1105,6 @@ int elf_write(struct elf *elf)
 	/* Update changed relocation sections and section headers: */
 	list_for_each_entry(sec, &elf->sections, list) {
 		if (sec->changed) {
-			if (sec->base &&
-			    elf_rebuild_reloc_section(elf, sec)) {
-				WARN("elf_rebuild_reloc_section");
-				return -1;
-			}
-
 			s = elf_getscn(elf->elf, sec->idx);
 			if (!s) {
 				WARN_ELF("elf_getscn");
@@ -1133,6 +1115,12 @@ int elf_write(struct elf *elf)
 				return -1;
 			}
 
+			if (sec->base &&
+			    elf_rebuild_reloc_section(elf, sec)) {
+				WARN("elf_rebuild_reloc_section");
+				return -1;
+			}
+
 			sec->changed = false;
 			elf->changed = true;
 		}

Re: [PATCH 1/2] objtool: Check for gelf_update_rel[a] failures

[Michael Forney]

On 2021-05-08, Michael Forney <mforney@mforney.org> wrote:
> Otherwise, if these fail we end up with garbage data in the
> .rela.orc_unwind_ip section, leading to errors like
>
>   ld: fs/squashfs/namei.o: bad reloc symbol index (0x7f16 >= 0x12) for
> offset 0x7f16d5c82cc8 in section `.orc_unwind_ip'
>
> Signed-off-by: Michael Forney <mforney@mforney.org>

Ping on these patches.

Re: [PATCH 1/2] objtool: Check for gelf_update_rel[a] failures

[Peter Zijlstra]

On Mon, Jun 28, 2021 at 12:52:07AM -0700, Michael Forney wrote:
> On 2021-05-08, Michael Forney <mforney@mforney.org> wrote:
> > Otherwise, if these fail we end up with garbage data in the
> > .rela.orc_unwind_ip section, leading to errors like
> >
> >   ld: fs/squashfs/namei.o: bad reloc symbol index (0x7f16 >= 0x12) for
> > offset 0x7f16d5c82cc8 in section `.orc_unwind_ip'
> >
> > Signed-off-by: Michael Forney <mforney@mforney.org>
> 
> Ping on these patches.

Josh, I forever forget which libelf versions we're supposed to support,

But these patches do look reasonable to me, wdyt?

Re: [PATCH 1/2] objtool: Check for gelf_update_rel[a] failures

[Josh Poimboeuf]

On Mon, Jun 28, 2021 at 10:24:38AM +0200, Peter Zijlstra wrote:
> On Mon, Jun 28, 2021 at 12:52:07AM -0700, Michael Forney wrote:
> > On 2021-05-08, Michael Forney <mforney@mforney.org> wrote:
> > > Otherwise, if these fail we end up with garbage data in the
> > > .rela.orc_unwind_ip section, leading to errors like
> > >
> > >   ld: fs/squashfs/namei.o: bad reloc symbol index (0x7f16 >= 0x12) for
> > > offset 0x7f16d5c82cc8 in section `.orc_unwind_ip'
> > >
> > > Signed-off-by: Michael Forney <mforney@mforney.org>
> > 
> > Ping on these patches.
> 
> Josh, I forever forget which libelf versions we're supposed to support,
> 
> But these patches do look reasonable to me, wdyt?

Looks ok to me.  Let me run them through some testing.

-- 
Josh

Re: [PATCH 1/2] objtool: Check for gelf_update_rel[a] failures

[Michael Forney]

On 2021-06-29, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> Looks ok to me.  Let me run them through some testing.

Just wanted to check: did the testing go well? I'd really like to see
these patches in 5.15 if possible.

Re: [PATCH 1/2] objtool: Check for gelf_update_rel[a] failures

[Michael Forney]

On 2021-08-28, Michael Forney <mforney@mforney.org> wrote:
> On 2021-06-29, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>> Looks ok to me.  Let me run them through some testing.
>
> Just wanted to check: did the testing go well? I'd really like to see
> these patches in 5.15 if possible.

Ping again. Anything I can do to help this make 5.16?

Re: [PATCH 1/2] objtool: Check for gelf_update_rel[a] failures

[Josh Poimboeuf]

On Wed, Oct 06, 2021 at 09:58:13AM -0700, Michael Forney wrote:
> On 2021-08-28, Michael Forney <mforney@mforney.org> wrote:
> > On 2021-06-29, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> >> Looks ok to me.  Let me run them through some testing.
> >
> > Just wanted to check: did the testing go well? I'd really like to see
> > these patches in 5.15 if possible.
> 
> Ping again. Anything I can do to help this make 5.16?

Hi Michael,

Sorry this got dropped.  I'll try to get it in.

-- 
Josh

Re: [PATCH 1/2] objtool: Check for gelf_update_rel[a] failures

[Miroslav Benes]

On Sat, 8 May 2021, Michael Forney wrote:

> Otherwise, if these fail we end up with garbage data in the
> .rela.orc_unwind_ip section, leading to errors like
> 
>   ld: fs/squashfs/namei.o: bad reloc symbol index (0x7f16 >= 0x12) for offset 0x7f16d5c82cc8 in section `.orc_unwind_ip'
> 
> Signed-off-by: Michael Forney <mforney@mforney.org>

Reviewed-by: Miroslav Benes <mbenes@suse.cz>

M

[tip: objtool/urgent] objtool: Check for gelf_update_rel[a] failures

[tip-bot2 for Michael Forney]

The following commit has been merged into the objtool/urgent branch of tip:

Commit-ID:     b46179d6bb3182c020f2bf9bb4df6ba5463b0495
Gitweb:        https://git.kernel.org/tip/b46179d6bb3182c020f2bf9bb4df6ba5463b0495
Author:        Michael Forney <mforney@mforney.org>
AuthorDate:    Sat, 08 May 2021 17:01:02 -07:00
Committer:     Josh Poimboeuf <jpoimboe@redhat.com>
CommitterDate: Wed, 06 Oct 2021 20:11:53 -07:00

objtool: Check for gelf_update_rel[a] failures

Otherwise, if these fail we end up with garbage data in the
.rela.orc_unwind_ip section, leading to errors like

  ld: fs/squashfs/namei.o: bad reloc symbol index (0x7f16 >= 0x12) for offset 0x7f16d5c82cc8 in section `.orc_unwind_ip'

Signed-off-by: Michael Forney <mforney@mforney.org>
Reviewed-by: Miroslav Benes <mbenes@suse.cz>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Link: https://lore.kernel.org/r/20210509000103.11008-1-mforney@mforney.org
---
 tools/objtool/elf.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/objtool/elf.c b/tools/objtool/elf.c
index b18f005..d1d4491 100644
--- a/tools/objtool/elf.c
+++ b/tools/objtool/elf.c
@@ -1001,7 +1001,10 @@ static int elf_rebuild_rel_reloc_section(struct section *sec, int nr)
 	list_for_each_entry(reloc, &sec->reloc_list, list) {
 		reloc->rel.r_offset = reloc->offset;
 		reloc->rel.r_info = GELF_R_INFO(reloc->sym->idx, reloc->type);
-		gelf_update_rel(sec->data, idx, &reloc->rel);
+		if (!gelf_update_rel(sec->data, idx, &reloc->rel)) {
+			WARN_ELF("gelf_update_rel");
+			return -1;
+		}
 		idx++;
 	}
 
@@ -1033,7 +1036,10 @@ static int elf_rebuild_rela_reloc_section(struct section *sec, int nr)
 		reloc->rela.r_offset = reloc->offset;
 		reloc->rela.r_addend = reloc->addend;
 		reloc->rela.r_info = GELF_R_INFO(reloc->sym->idx, reloc->type);
-		gelf_update_rela(sec->data, idx, &reloc->rela);
+		if (!gelf_update_rela(sec->data, idx, &reloc->rela)) {
+			WARN_ELF("gelf_update_rela");
+			return -1;
+		}
 		idx++;
 	}