ldconfig: handle .dynstr located in separate segment (bug 25087)

Message ID mvmsgo1q758.fsf@suse.de
State New
Headers show
Series
  • ldconfig: handle .dynstr located in separate segment (bug 25087)
Related show

Commit Message

Andreas Schwab Oct. 10, 2019, 9:30 a.m.
To determine the load offset of the DT_STRTAB section search for the
segment containing it, instead of using the load offset of the first
segment.

	[BZ #25087]
	* elf/readelflib.c (process_elf_file): Use containing segment for
	DT_STRTAB load offset.
---
 elf/readelflib.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

-- 
2.23.0

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Comments

Florian Weimer Oct. 16, 2019, 10:34 a.m. | #1
* Andreas Schwab:

> +		  && dyn_entry->d_un.d_val < segment->p_vaddr + segment->p_filesz)


> +	  dynamic_strings = (char *) (file_contents + dyn_entry->d_un.d_val - loadoff);


I think these lines are too long.  I also suspect that the condition
should be written as

  dyn_entry->d_un.d_val - segment->p_vaddr < segment->p_filesz

But in principle, the change looks fine.

Is it possible to write a test case for bug 25087?

Thanks,
Florian
Andreas Schwab Oct. 16, 2019, 1:25 p.m. | #2
On Okt 16 2019, Florian Weimer <fweimer@redhat.com> wrote:

> * Andreas Schwab:

>

>> +		  && dyn_entry->d_un.d_val < segment->p_vaddr + segment->p_filesz)

>

>> +	  dynamic_strings = (char *) (file_contents + dyn_entry->d_un.d_val - loadoff);

>

> I think these lines are too long.  I also suspect that the condition

> should be written as

>

>   dyn_entry->d_un.d_val - segment->p_vaddr < segment->p_filesz


Ok.

> Is it possible to write a test case for bug 25087?


The broken layout is created by patchelf, but I have no idea how to
replicate it without that.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Florian Weimer Oct. 16, 2019, 1:42 p.m. | #3
* Andreas Schwab:

> On Okt 16 2019, Florian Weimer <fweimer@redhat.com> wrote:

>

>> * Andreas Schwab:

>>

>>> +		  && dyn_entry->d_un.d_val < segment->p_vaddr + segment->p_filesz)

>>

>>> +	  dynamic_strings = (char *) (file_contents + dyn_entry->d_un.d_val - loadoff);

>>

>> I think these lines are too long.  I also suspect that the condition

>> should be written as

>>

>>   dyn_entry->d_un.d_val - segment->p_vaddr < segment->p_filesz

>

> Ok.

>

>> Is it possible to write a test case for bug 25087?

>

> The broken layout is created by patchelf, but I have no idea how to

> replicate it without that.


I guess in this case, checking this in without a regression test is
fine.

Thanks,
Florian

Patch

diff --git a/elf/readelflib.c b/elf/readelflib.c
index 09f5858426..23a045a582 100644
--- a/elf/readelflib.c
+++ b/elf/readelflib.c
@@ -45,7 +45,6 @@  process_elf_file (const char *file_name, const char *lib, int *flag,
 {
   int i;
   unsigned int j;
-  ElfW(Addr) loadaddr;
   unsigned int dynamic_addr;
   size_t dynamic_size;
   char *program_interpreter;
@@ -87,7 +86,6 @@  process_elf_file (const char *file_name, const char *lib, int *flag,
      libc5/libc6.  */
   *flag = FLAG_ELF;
 
-  loadaddr = -1;
   dynamic_addr = 0;
   dynamic_size = 0;
   program_interpreter = NULL;
@@ -98,11 +96,6 @@  process_elf_file (const char *file_name, const char *lib, int *flag,
 
       switch (segment->p_type)
 	{
-	case PT_LOAD:
-	  if (loadaddr == (ElfW(Addr)) -1)
-	    loadaddr = segment->p_vaddr - segment->p_offset;
-	  break;
-
 	case PT_DYNAMIC:
 	  if (dynamic_addr)
 	    error (0, 0, _("more than one dynamic segment\n"));
@@ -176,11 +169,6 @@  process_elf_file (const char *file_name, const char *lib, int *flag,
 	}
 
     }
-  if (loadaddr == (ElfW(Addr)) -1)
-    {
-      /* Very strange. */
-      loadaddr = 0;
-    }
 
   /* Now we can read the dynamic sections.  */
   if (dynamic_size == 0)
@@ -197,7 +185,27 @@  process_elf_file (const char *file_name, const char *lib, int *flag,
       check_ptr (dyn_entry);
       if (dyn_entry->d_tag == DT_STRTAB)
 	{
-	  dynamic_strings = (char *) (file_contents + dyn_entry->d_un.d_val - loadaddr);
+	  /* Find the file offset of the segment containing the dynamic
+	     string table.  */
+	  ElfW(Off) loadoff = -1;
+	  for (i = 0, segment = elf_pheader;
+	       i < elf_header->e_phnum; i++, segment++)
+	    {
+	      if (segment->p_type == PT_LOAD
+		  && dyn_entry->d_un.d_val >= segment->p_vaddr
+		  && dyn_entry->d_un.d_val < segment->p_vaddr + segment->p_filesz)
+		{
+		  loadoff = segment->p_vaddr - segment->p_offset;
+		  break;
+		}
+	    }
+	  if (loadoff == (ElfW(Off)) -1)
+	    {
+	      /* Very strange. */
+	      loadoff = 0;
+	    }
+
+	  dynamic_strings = (char *) (file_contents + dyn_entry->d_un.d_val - loadoff);
 	  check_ptr (dynamic_strings);
 	  break;
 	}