[Qemu-devel] Patch to fix mapping of elf pheaders specifying both .data and .bss segments

[Qemu-devel] Patch to fix mapping of elf pheaders specifying both .data and .bss segments

[Kai Backman]

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

I recently found an issue where an elf pheader mapping both a .data and a
.bss segment would have its MemSiz ignored and only FileSiz bytes would end
up in the memory map. The bug is exhibiting when an elf interpreter isn't
available on the system. I've attached the output of readelf for such a file
and a patch against the git repository that fixes the problem.
Comments on the patch solicited, I'm also unclear on the commit flow for the
project.

 Kai


Section Headers:
  [Nr] Name              Type            Addr     Off    Size   ES Flg Lk
Inf Al
  [ 0]                   NULL            00000000 000000 000000 00      0
0  0
  [ 1] .text             PROGBITS        000082e0 0002e0 006d60 00  AX  0
0  8
  [ 2] .data             PROGBITS        00010000 008000 000c60 00  WA  0
0  8
  [ 3] .bss              NOBITS          00010c60 008c60 00bf00 00  WA  0
0  8

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x000000 0x00008000 0x00008000 0x07040 0x07040 R E 0x1000
  LOAD           0x008000 0x00010000 0x00010000 0x00c60 0x0cb60 RW  0x1000

diff --git a/linux-user/elfload.c b/linux-user/elfload.c index
190ad14..e4e75d5 100644 --- a/linux-user/elfload.c +++
b/linux-user/elfload.c @@ -1211,7 +1211,7 @@ int load_elf_binary(struct
linux_binprm * bprm, struct target_pt_regs * regs, abi_ulong mapped_addr;
struct elf_phdr * elf_ppnt; struct elf_phdr *elf_phdata; - abi_ulong
elf_bss, k, elf_brk; + abi_ulong elf_bss, last_bss, mapped_bss, k, elf_brk;
int retval; char * elf_interpreter; abi_ulong elf_entry, interp_load_addr =
0; @@ -1271,6 +1271,7 @@ int load_elf_binary(struct linux_binprm * bprm,
struct target_pt_regs * regs, elf_ppnt = elf_phdata; elf_bss = 0; + last_bss
= 0; elf_brk = 0; @@ -1495,12 +1496,24 @@ int load_elf_binary(struct
linux_binprm * bprm, struct target_pt_regs * regs, k = elf_ppnt->p_vaddr +
elf_ppnt->p_filesz; if (k > elf_bss) elf_bss = k; + k = elf_ppnt->p_vaddr +
elf_ppnt->p_memsz; + if (k > last_bss) + last_bss = k; if
((elf_ppnt->p_flags & PF_X) && end_code < k) end_code = k; if (end_data < k)
end_data = k; k = elf_ppnt->p_vaddr + elf_ppnt->p_memsz; if (k > elf_brk)
elf_brk = k; + if (!elf_interpreter && last_bss > elf_bss) { +
padzero(elf_bss, last_bss); + mapped_bss = TARGET_ELF_PAGESTART(elf_bss +
qemu_host_page_size - 1); + + /* Map the last of the bss segment */ +
target_mmap(load_bias + mapped_bss, last_bss-mapped_bss, +
PROT_READ|PROT_WRITE|PROT_EXEC, + MAP_FIXED|MAP_PRIVATE|MAP_ANONYMOUS, -1,
0); + } } elf_entry += load_bias;


-- 
Kai Backman, Software Engineer, kaib@google.com

[-- Attachment #2: Type: text/html, Size: 5276 bytes --]

Re: [Qemu-devel] Patch to fix mapping of elf pheaders specifying both .data and .bss segments

[Riku Voipio]

On Wed, Jun 17, 2009 at 08:36:38PM +0300, Kai Backman wrote:
>    I recently found an issue where an elf pheader mapping both a .data and a
>    .bss segment would have its MemSiz ignored and only FileSiz bytes would
>    end up in the memory map. The bug is exhibiting when an elf interpreter
>    isn't available on the system. I've attached the output of readelf for
>    such a file and a patch against the git repository that fixes the problem.
>    Comments on the patch solicited, I'm also unclear on the commit flow for
>    the project.

I presume you are not using a regular linux userland? A testcase would be nice.
The patch attached is totally messed up thou.

>    diff --git a/linux-user/elfload.c b/linux-user/elfload.c index
>    190ad14..e4e75d5 100644 --- a/linux-user/elfload.c +++
>    b/linux-user/elfload.c @@ -1211,7 +1211,7 @@ int load_elf_binary(struct
>    linux_binprm * bprm, struct target_pt_regs * regs, abi_ulong mapped_addr;
>    struct elf_phdr * elf_ppnt; struct elf_phdr *elf_phdata; - abi_ulong
>    elf_bss, k, elf_brk; + abi_ulong elf_bss, last_bss, mapped_bss, k,
>    elf_brk; int retval; char * elf_interpreter; abi_ulong elf_entry,
>    interp_load_addr = 0; @@ -1271,6 +1271,7 @@ int load_elf_binary(struct
>    linux_binprm * bprm, struct target_pt_regs * regs, elf_ppnt = elf_phdata;
>    elf_bss = 0; + last_bss = 0; elf_brk = 0; @@ -1495,12 +1496,24 @@ int
>    load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
>    k = elf_ppnt->p_vaddr + elf_ppnt->p_filesz; if (k > elf_bss) elf_bss = k;
>    + k = elf_ppnt->p_vaddr + elf_ppnt->p_memsz; + if (k > last_bss) +
>    last_bss = k; if ((elf_ppnt->p_flags & PF_X) && end_code < k) end_code =
>    k; if (end_data < k) end_data = k; k = elf_ppnt->p_vaddr +
>    elf_ppnt->p_memsz; if (k > elf_brk) elf_brk = k; + if (!elf_interpreter &&
>    last_bss > elf_bss) { + padzero(elf_bss, last_bss); + mapped_bss =
>    TARGET_ELF_PAGESTART(elf_bss + qemu_host_page_size - 1); + + /* Map the
>    last of the bss segment */ + target_mmap(load_bias + mapped_bss,
>    last_bss-mapped_bss, + PROT_READ|PROT_WRITE|PROT_EXEC, +
>    MAP_FIXED|MAP_PRIVATE|MAP_ANONYMOUS, -1, 0); + } } elf_entry += load_bias;
>    --
>    Kai Backman, Software Engineer, [1]kaib@google.com
> 
> References
> 
>    Visible links
>    1. mailto:kaib@google.com

Re: [Qemu-devel] Patch to fix mapping of elf pheaders specifying both .data and .bss segments

[Kai Backman]

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

On Thu, Jun 18, 2009 at 11:57 AM, Riku Voipio<riku.voipio@iki.fi> wrote:
> The patch attached is totally messed up thou.

Sorry for the delay. I was vacationing in Finland (incidentally)..

To restart the thread, I've re-attached the patch.

 Kai

-- 
Kai Backman, Software Engineer, kaib@google.com

[-- Attachment #2: qemu_bss.patch --]
[-- Type: text/x-diff, Size: 1719 bytes --]

diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 190ad14..e4e75d5 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -1211,7 +1211,7 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
     abi_ulong mapped_addr;
     struct elf_phdr * elf_ppnt;
     struct elf_phdr *elf_phdata;
-    abi_ulong elf_bss, k, elf_brk;
+    abi_ulong elf_bss, last_bss, mapped_bss, k, elf_brk;
     int retval;
     char * elf_interpreter;
     abi_ulong elf_entry, interp_load_addr = 0;
@@ -1271,6 +1271,7 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
     elf_ppnt = elf_phdata;
 
     elf_bss = 0;
+    last_bss = 0;
     elf_brk = 0;
 
 
@@ -1495,12 +1496,24 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
         k = elf_ppnt->p_vaddr + elf_ppnt->p_filesz;
         if (k > elf_bss)
             elf_bss = k;
+        k = elf_ppnt->p_vaddr + elf_ppnt->p_memsz;
+        if (k > last_bss)
+            last_bss = k;
         if ((elf_ppnt->p_flags & PF_X) && end_code <  k)
             end_code = k;
         if (end_data < k)
             end_data = k;
         k = elf_ppnt->p_vaddr + elf_ppnt->p_memsz;
         if (k > elf_brk) elf_brk = k;
+        if (!elf_interpreter && last_bss > elf_bss) {
+      	    padzero(elf_bss, last_bss);
+	    mapped_bss = TARGET_ELF_PAGESTART(elf_bss + qemu_host_page_size - 1);
+
+	    /* Map the last of the bss segment */
+            target_mmap(load_bias + mapped_bss, last_bss-mapped_bss,
+                        PROT_READ|PROT_WRITE|PROT_EXEC,
+                        MAP_FIXED|MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
+        }
     }
 
     elf_entry += load_bias;

Re: [Qemu-devel] Patch to fix mapping of elf pheaders specifying both .data and .bss segments

[Riku Voipio]

On Thu, Aug 06, 2009 at 03:41:02PM -0700, Kai Backman wrote:
> On Thu, Jun 18, 2009 at 11:57 AM, Riku Voipio<riku.voipio@iki.fi> wrote:
> > The patch attached is totally messed up thou.
> 
> Sorry for the delay. I was vacationing in Finland (incidentally)..
> 
> To restart the thread, I've re-attached the patch.

I see Max Filippov has touched the same function for possibly the same
issue?

http://lists.gnu.org/archive/html/qemu-devel/2009-07/msg00902.html

> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 190ad14..e4e75d5 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1211,7 +1211,7 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
>      abi_ulong mapped_addr;
>      struct elf_phdr * elf_ppnt;
>      struct elf_phdr *elf_phdata;
> -    abi_ulong elf_bss, k, elf_brk;
> +    abi_ulong elf_bss, last_bss, mapped_bss, k, elf_brk;
>      int retval;
>      char * elf_interpreter;
>      abi_ulong elf_entry, interp_load_addr = 0;
> @@ -1271,6 +1271,7 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
>      elf_ppnt = elf_phdata;
>  
>      elf_bss = 0;
> +    last_bss = 0;
>      elf_brk = 0;
>  
>  
> @@ -1495,12 +1496,24 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
>          k = elf_ppnt->p_vaddr + elf_ppnt->p_filesz;
>          if (k > elf_bss)
>              elf_bss = k;
> +        k = elf_ppnt->p_vaddr + elf_ppnt->p_memsz;
> +        if (k > last_bss)
> +            last_bss = k;
>          if ((elf_ppnt->p_flags & PF_X) && end_code <  k)
>              end_code = k;
>          if (end_data < k)
>              end_data = k;
>          k = elf_ppnt->p_vaddr + elf_ppnt->p_memsz;
>          if (k > elf_brk) elf_brk = k;
> +        if (!elf_interpreter && last_bss > elf_bss) {
> +      	    padzero(elf_bss, last_bss);
> +	    mapped_bss = TARGET_ELF_PAGESTART(elf_bss + qemu_host_page_size - 1);
> +
> +	    /* Map the last of the bss segment */
> +            target_mmap(load_bias + mapped_bss, last_bss-mapped_bss,
> +                        PROT_READ|PROT_WRITE|PROT_EXEC,
> +                        MAP_FIXED|MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
> +        }
>      }
>  
>      elf_entry += load_bias;

Re: [Qemu-devel] Patch to fix mapping of elf pheaders specifying both .data and .bss segments

[Max Filippov]

Hello.

> On Thu, Aug 06, 2009 at 03:41:02PM -0700, Kai Backman wrote:
> > On Thu, Jun 18, 2009 at 11:57 AM, Riku Voipio<riku.voipio@iki.fi> wrote:
> > > The patch attached is totally messed up thou.
> > 
> > Sorry for the delay. I was vacationing in Finland (incidentally)..
> > 
> > To restart the thread, I've re-attached the patch.
> 
> I see Max Filippov has touched the same function for possibly the same
> issue?
> 
> http://lists.gnu.org/archive/html/qemu-devel/2009-07/msg00902.html
> 

You're right, the same issue. But the patch doesn't fix it for me: elf_interpreter mentioned in the last
hunk is always set to "/lib/ld.so.1". Having this condition removed it works perfectly.

> > diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> > index 190ad14..e4e75d5 100644
> > --- a/linux-user/elfload.c
> > +++ b/linux-user/elfload.c
> > @@ -1211,7 +1211,7 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
> >      abi_ulong mapped_addr;
> >      struct elf_phdr * elf_ppnt;
> >      struct elf_phdr *elf_phdata;
> > -    abi_ulong elf_bss, k, elf_brk;
> > +    abi_ulong elf_bss, last_bss, mapped_bss, k, elf_brk;
> >      int retval;
> >      char * elf_interpreter;
> >      abi_ulong elf_entry, interp_load_addr = 0;
> > @@ -1271,6 +1271,7 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
> >      elf_ppnt = elf_phdata;
> >  
> >      elf_bss = 0;
> > +    last_bss = 0;
> >      elf_brk = 0;
> >  
> >  
> > @@ -1495,12 +1496,24 @@ int load_elf_binary(struct linux_binprm * bprm, struct target_pt_regs * regs,
> >          k = elf_ppnt->p_vaddr + elf_ppnt->p_filesz;
> >          if (k > elf_bss)
> >              elf_bss = k;
> > +        k = elf_ppnt->p_vaddr + elf_ppnt->p_memsz;
> > +        if (k > last_bss)
> > +            last_bss = k;
> >          if ((elf_ppnt->p_flags & PF_X) && end_code <  k)
> >              end_code = k;
> >          if (end_data < k)
> >              end_data = k;
> >          k = elf_ppnt->p_vaddr + elf_ppnt->p_memsz;
> >          if (k > elf_brk) elf_brk = k;
> > +        if (!elf_interpreter && last_bss > elf_bss) {
> > +      	    padzero(elf_bss, last_bss);
> > +	    mapped_bss = TARGET_ELF_PAGESTART(elf_bss + qemu_host_page_size - 1);
> > +
> > +	    /* Map the last of the bss segment */
> > +            target_mmap(load_bias + mapped_bss, last_bss-mapped_bss,
> > +                        PROT_READ|PROT_WRITE|PROT_EXEC,
> > +                        MAP_FIXED|MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
> > +        }
> >      }
> >  
> >      elf_entry += load_bias;
> 
> 

Thanks.
-- Max