Re: [PATCH] linux-user: Fix loading of BSS segments

[Laurent Vivier]

Le 17/12/2020 à 11:17, Giuseppe Musacchio a écrit :
> Some ELF binaries encode the .bss section as an extension of the data
> ones by setting the segment p_memsz > p_filesz. Some other binaries take
> a different route and encode it as a stand-alone PT_LOAD segment with
> p_filesz = 0 and p_memsz > 0.
> 
> Both the encodings are actually correct per ELF specification but the
> ELF loader had some troubles in handling the former: with the old logic
> it was very likely to get Qemu to crash in zero_bss when trying to
> access unmapped memory.
> 
> zero_bss isn't meant to allocate whole zero-filled segments but to
> "complete" a previously mapped segment with the needed zero bits.
> 
> The fix is pretty simple, if the segment is completely zero-filled we
> simply allocate one or more pages (according to p_memsz) and avoid
> calling zero_bss altogether.

So, the current code manages the bss segment when the memory page has already
been allocated for the data segment by zeroing it:

+----------------------------------+
 PAGE                              |
 ----------+------------+          |
 DATA      |   BSS      |          |

So your patch fixes the case when there is no data segment and thus no page
to complete:

+----------------------------------+
 PAGE                              |
 ----------+                       |
 BSS       |                       |


But could we have a case where the BSS starts in a page allocated for the
data segment but needs more pages?

+----------------------------------+----------------------------------+
 PAGE                              | PAGE                             |
 ------------------------+----------------------------+               |
 DATA                    | BSS                        |               |

In this case we should also allocate the page, and the previous case is only a
special case (data segment = 0) of this one.

so something like (approxymately):

if (eppnt->p_filesz != 0) {
   target_mmap()
   if (vaddr_ef < vaddr_mem) {
       zero_bss(vaddr_ef, MIN(vaddr_mem, vaddr_ps + vaddr_len))
   }
}
if (vaddr_ps + vaddr_len < vaddr_mem) {
  target_mmap(vaddr_ps + vaddr_len, vaddr_ps + vaddr_len - vaddr_mem - 1,
              elf_prot, MAP_PRIVATE | MAP_FIXED | MAP_ANONYMOUS, -1, 0);
}

I think your fix is correct, but I'm wondering if we can have something more
generic, if we can cover an other possible case.

If you think we don't need to introduce more complexity for a case that can't
happen I will queue your patch as is.

Thanks,
Laurent

> Signed-off-by: Giuseppe Musacchio <thatlemon@gmail.com>
> ---
>  linux-user/elfload.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 0b02a92602..a16c240e0f 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -2776,14 +2776,16 @@ static void load_elf_image(const char *image_name, 
> int image_fd,
>              vaddr = load_bias + eppnt->p_vaddr;
>              vaddr_po = TARGET_ELF_PAGEOFFSET(vaddr);
>              vaddr_ps = TARGET_ELF_PAGESTART(vaddr);
> -            vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_filesz + vaddr_po);
> +
> +            vaddr_ef = vaddr + eppnt->p_filesz;
> +            vaddr_em = vaddr + eppnt->p_memsz;
>  
>              /*
> -             * Some segments may be completely empty without any backing file
> -             * segment, in that case just let zero_bss allocate an empty 
> buffer
> -             * for it.
> +             * Some segments may be completely empty, with a non-zero p_memsz
> +             * but no backing file segment.
>               */
>              if (eppnt->p_filesz != 0) {
> +                vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_filesz + 
> vaddr_po);
>                  error = target_mmap(vaddr_ps, vaddr_len, elf_prot,
>                                      MAP_PRIVATE | MAP_FIXED,
>                                      image_fd, eppnt->p_offset - vaddr_po);
> @@ -2791,14 +2793,22 @@ static void load_elf_image(const char *image_name, 
> int image_fd,
>                  if (error == -1) {
>                      goto exit_mmap;
>                  }
> -            }
>  
> -            vaddr_ef = vaddr + eppnt->p_filesz;
> -            vaddr_em = vaddr + eppnt->p_memsz;
> +                /*
> +                 * If the load segment requests extra zeros (e.g. bss), map 
> it.
> +                 */
> +                if (eppnt->p_filesz < eppnt->p_memsz) {
> +                    zero_bss(vaddr_ef, vaddr_em, elf_prot);
> +                }
> +            } else if (eppnt->p_memsz != 0) {
> +                vaddr_len = TARGET_ELF_PAGELENGTH(eppnt->p_memsz + vaddr_po);
> +                error = target_mmap(vaddr_ps, vaddr_len, elf_prot,
> +                                    MAP_PRIVATE | MAP_FIXED | MAP_ANONYMOUS,
> +                                    -1, 0);
>  
> -            /* If the load segment requests extra zeros (e.g. bss), map it.  
> */
> -            if (vaddr_ef < vaddr_em) {
> -                zero_bss(vaddr_ef, vaddr_em, elf_prot);
> +                if (error == -1) {
> +                    goto exit_mmap;
> +                }
>              }
>  
>              /* Find the full program boundaries.  */
>