[PATCH] Fix potential wrong-code issue in write_dso

[PATCH] Fix potential wrong-code issue in write_dso

[Tom de Vries]

Hi,

The function write_dso has the responsibility to update:
- the file offset of the sections, and
- the file offset of the section header table.

It does this in two steps:
- it calculates the effects of adding, removing and updating
  debug sections
- it makes sure the file offsets have the required alignment

The second step may or may not update the file offsets, but if it does, it
requires the entries in the section header table to be in file offset
order.

However, if the second step does not update the file offsets, no check on
section header table order is done, which implies that the first step should
be able to handle an unsorted section header table.

That is not the case for this update loop:
...
  for (j = dso->ehdr.e_shstrndx + 1; j < dso->ehdr.e_shnum; ++j)
    dso->shdr[j].sh_offset += len;
...

This loop adds an increase of the size of the section header string table to
sections 'after' the section header string table.  But the after test here is
implemented in terms of order in the section header table, which only works if
if the section header table is sorted.

Fix this by rewriting the after test in terms of sh_offset:
...
  for (j = 1; j < dso->ehdr.e_shnum; ++j)
    if (dso->shdr[j].sh_offset
        > dso->shdr[dso->ehdr.e_shstrndx].sh_offset)
      dso->shdr[j].sh_offset += len;
...

OK for trunk?

Thanks,
- Tom

Fix potential wrong-code issue in write_dso

2019-03-15  Tom de Vries  <tdevries@suse.de>

	* dwz.c (write_dso): Make updating of sections after section header
	string table robust against unsortes section header table.

---
 dwz.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/dwz.c b/dwz.c
index 6b6a33e..5672ede 100644
--- a/dwz.c
+++ b/dwz.c
@@ -10097,8 +10097,10 @@ write_dso (DSO *dso, const char *file, struct stat *st)
 	    dso->shdr[dso->ehdr.e_shstrndx].sh_size += len;
 	    if (dso->shdr[dso->ehdr.e_shstrndx].sh_offset < min_shoff)
 	      min_shoff = dso->shdr[dso->ehdr.e_shstrndx].sh_offset;
-	    for (j = dso->ehdr.e_shstrndx + 1; j < dso->ehdr.e_shnum; ++j)
-	      dso->shdr[j].sh_offset += len;
+	    for (j = 1; j < dso->ehdr.e_shnum; ++j)
+	      if (dso->shdr[j].sh_offset
+		  > dso->shdr[dso->ehdr.e_shstrndx].sh_offset)
+		dso->shdr[j].sh_offset += len;
 	    if (ehdr.e_shoff > dso->shdr[dso->ehdr.e_shstrndx].sh_offset)
 	      ehdr.e_shoff += len;
 	    shstrtabadd += len;

[committed] Make updating sections after section header string table order-independent

[Tom de Vries]

[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]

[ was: [PATCH] Fix potential wrong-code issue in write_dso ]

On 15-03-19 16:12, Tom de Vries wrote:
> Hi,
> 
> The function write_dso has the responsibility to update:
> - the file offset of the sections, and
> - the file offset of the section header table.
> 
> It does this in two steps:
> - it calculates the effects of adding, removing and updating
>   debug sections
> - it makes sure the file offsets have the required alignment
> 
> The second step may or may not update the file offsets, but if it does, it
> requires the entries in the section header table to be in file offset
> order.
> 
> However, if the second step does not update the file offsets, no check on
> section header table order is done, which implies that the first step should
> be able to handle an unsorted section header table.
> 
> That is not the case for this update loop:
> ...
>   for (j = dso->ehdr.e_shstrndx + 1; j < dso->ehdr.e_shnum; ++j)
>     dso->shdr[j].sh_offset += len;
> ...
> 
> This loop adds an increase of the size of the section header string table to
> sections 'after' the section header string table.  But the after test here is
> implemented in terms of order in the section header table, which only works if
> if the section header table is sorted.
> 
> Fix this by rewriting the after test in terms of sh_offset:
> ...
>   for (j = 1; j < dso->ehdr.e_shnum; ++j)
>     if (dso->shdr[j].sh_offset
>         > dso->shdr[dso->ehdr.e_shstrndx].sh_offset)
>       dso->shdr[j].sh_offset += len;
> ...
> 
> OK for trunk?
> 

Cleaned up rationale and committed.

Thanks,
- Tom

[-- Attachment #2: 0001-Make-updating-sections-after-section-header-string-table-order-independent.patch --]
[-- Type: text/x-patch, Size: 2020 bytes --]

Make updating sections after section header string table order-independent

Function write_dso updates the section header table: it adds, removes and
updates sections.  It does this in two phases:
- it calculates the effect of changing sections on the following
  sections sh_offset (as well as on the section header table e_shoff)
- it makes sure sh_offset is sh_addralign aligned (as well as that the section
  header table e_shoff is ELFCLASS-appropriately aligned).

The first phase is independent of the order of the section header table, apart
from this loop:
...
  for (j = dso->ehdr.e_shstrndx + 1; j < dso->ehdr.e_shnum; ++j)
    dso->shdr[j].sh_offset += len;
...
It adds an increase of the size of the section header string table to
sections 'after' the section header string table.  But the after test here is
implemented in terms of order in the section header table, which only works if
if the section header table is sorted.

Make the first phase order-independent by rewriting the after test in terms of
sh_offset.

2019-06-25  Tom de Vries  <tdevries@suse.de>

	* dwz.c (write_dso): Make updating of sections after section header
	string table robust against unsorted section header table.

---
 dwz.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/dwz.c b/dwz.c
index 9e39824..101b6e3 100644
--- a/dwz.c
+++ b/dwz.c
@@ -10405,8 +10405,10 @@ write_dso (DSO *dso, const char *file, struct stat *st)
 	    dso->shdr[dso->ehdr.e_shstrndx].sh_size += len;
 	    if (dso->shdr[dso->ehdr.e_shstrndx].sh_offset < min_shoff)
 	      min_shoff = dso->shdr[dso->ehdr.e_shstrndx].sh_offset;
-	    for (j = dso->ehdr.e_shstrndx + 1; j < dso->ehdr.e_shnum; ++j)
-	      dso->shdr[j].sh_offset += len;
+	    for (j = 1; j < dso->ehdr.e_shnum; ++j)
+	      if (dso->shdr[j].sh_offset
+		  > dso->shdr[dso->ehdr.e_shstrndx].sh_offset)
+		dso->shdr[j].sh_offset += len;
 	    if (ehdr.e_shoff > dso->shdr[dso->ehdr.e_shstrndx].sh_offset)
 	      ehdr.e_shoff += len;
 	    shstrtabadd += len;