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

[Roger Pau Monne <roger.pau@xxxxxxxxxx>]

On Wed, Oct 05, 2016 at 09:51:06AM -0600, Jan Beulich wrote:
> >>> 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.

Done.
 
> > @@ -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.

Right, I was dubious about which one would be better. I've removed the 
packed attribute of elf_header, but the one from header needs to say (so 
that the elf_header is just after the uint32_t). 

> >      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.

Well, shdr_size is just the result of a sizeof.

> 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).

Hm, I'm not sure I follow. Memset()ing of 
header.elf_header.section[SHN_UNDEF] is required, so that we don't copy 
contents of the stack into guest memory. This memset is done to the local 
data, which is then copied into the guest memory space here.

Roger.

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