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

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

[Roger Pau Monne]

Commit ed04ca introduced a bug in the symtab/strtab loading for 32bit
guests, that corrupted the section headers array due to the padding
introduced by the elf_shdr union.

The Elf section header array on 32bit should be accessible as an array of
Elf32_Shdr elements, and the union with Elf64_Shdr done in elf_shdr was
breaking this due to size differences between Elf32_Shdr and Elf64_Shdr.

Fix this by copying each section header one by one, and using the proper
size depending on the bitness of the guest kernel.

Reported-by: Brian Marcotte <marcotte@panix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Brian Marcotte <marcotte@panix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Should be backported to Xen 4.7 stable branch.
---
 xen/common/libelf/libelf-loader.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index 2626a40..3faff62 100644
--- 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;
 
     if ( !elf->bsd_symtab_pstart )
         return;
@@ -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);
     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);
+        if ( rc != 0 )
+        {
+            elf_mark_broken(elf,
+                        "unable to load ELF section header into guest memory");
+            return;
+        }
+    }
+
     /* Remove permissions from elf_memcpy_safe. */
     elf->caller_xdest_base = NULL;
     elf->caller_xdest_size = 0;
-- 
2.7.4 (Apple Git-66)


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

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

[Jan Beulich]

>>> On 05.10.16 at 17:11, <roger.pau@citrix.com> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Roger Pau Monne]

On Wed, Oct 05, 2016 at 09:51:06AM -0600, Jan Beulich wrote:
> >>> On 05.10.16 at 17:11, <roger.pau@citrix.com> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 05.10.16 at 18:17, <roger.pau@citrix.com> wrote:
> On Wed, Oct 05, 2016 at 09:51:06AM -0600, Jan Beulich wrote:
>> >>> On 05.10.16 at 17:11, <roger.pau@citrix.com> wrote:
>> > +    /*
>> > +     * 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.

Oh, I didn't even spot that - it makes things worse: The value gets
retrieved correctly earlier on.

Jan


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