Re: [Xen-devel] [PATCH for-4.8] libelf: fix symtab/strtab loading for 32bit domains

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.10.16 at 17:11, <roger.pau@xxxxxxxxxx> wrote:
> --- a/xen/common/libelf/libelf-loader.c
> +++ b/xen/common/libelf/libelf-loader.c
> @@ -262,13 +262,14 @@ static void elf_load_bsdsyms(struct elf_binary *elf)
>      } __attribute__((packed)) header;
>  
>      ELF_HANDLE_DECL(elf_ehdr) header_handle;
> -    unsigned long shdr_size;
> +    unsigned long shdr_size, ehdr_size;
>      ELF_HANDLE_DECL(elf_shdr) section_handle;
> -    unsigned int link, rc;
> +    unsigned int link, rc, i;
>      elf_ptrval header_base;
>      elf_ptrval elf_header_base;
>      elf_ptrval symtab_base;
>      elf_ptrval strtab_base;
> +    void *shdr;

I'd appreciate if you moved this into the scope where it's needed.
Also I think it could be const.

> @@ -394,15 +395,40 @@ do {                                                   
>              \
>      header.size = strtab_base + elf_uval(elf, section_handle, sh_size) -
>                    elf_header_base;
>  
> -    /* Load the headers. */
> +    /* Load the size plus elf header. */
> +    ehdr_size = sizeof(header) - sizeof(header.elf_header.section);

I think offsetof(typeof(header), elf_header.section) would be the
safer expression here, removing the dependency on the packed
attribute.

>      rc = elf_load_image(elf, header_base, ELF_REALPTR2PTRVAL(&header),
> -                        sizeof(header), sizeof(header));
> +                        ehdr_size, ehdr_size);
>      if ( rc != 0 )
>      {
>          elf_mark_broken(elf, "unable to load ELF headers into guest memory");
>          return;
>      }
>  
> +    /*
> +     * Load the section headers.
> +     *
> +     * NB: this _must_ be done one by one, and taking the bitness into 
> account,
> +     * so that the guest can treat this as an array of type Elf{32/64}_Shdr.
> +     */
> +    shdr_size = elf_64bit(elf) ? sizeof(Elf64_Shdr) : sizeof(Elf32_Shdr);
> +    for ( i = 0; i < ELF_BSDSYM_SECTIONS; i++ )
> +    {
> +        if ( elf_64bit(elf) )
> +            shdr = &header.elf_header.section[i].e64;
> +        else
> +            shdr = &header.elf_header.section[i].e32;
> +
> +        rc = elf_load_image(elf, header_base + ehdr_size + shdr_size * i,
> +                            ELF_REALPTR2PTRVAL(shdr), shdr_size, shdr_size);

You shouldn't read shdr_size bytes here, but only sizeof() ones.

Which btw also applies to the earlier memset()ing of the SHN_UNDEF
header, which it then also looks like gets overwritten by the loading
done here (irrespective of the change you make).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel