[PATCH v4 3/4] libelf: rewrite symtab/strtab loading

[Roger Pau Monne <roger.pau@citrix.com>]

Current implementation of elf_load_bsdsyms is broken when loading inside of
a HVM guest, because it assumes elf_memcpy_safe is able to write into guest
memory space, which it is not.

Take the oportunity to do some cleanup and properly document how
elf_{parse/load}_bsdsyms works. The new implementation uses elf_load_image
when dealing with data that needs to be copied to the guest memory space.
Also reduce the number of section headers copied to the minimum necessary.

This patch also removes the duplication of code found in the libxc ELF
loader, since the libelf symtab/strtab loading code will also handle this
case without having to duplicate it.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxc/xc_dom_elfloader.c    | 203 ----------------------------------
 xen/common/libelf/libelf-loader.c | 223 ++++++++++++++++++++++++++++----------
 2 files changed, 163 insertions(+), 263 deletions(-)

diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
index 5039f3f..62d421a 100644
--- a/tools/libxc/xc_dom_elfloader.c
+++ b/tools/libxc/xc_dom_elfloader.c
@@ -133,204 +133,6 @@ static elf_negerrnoval xc_dom_probe_elf_kernel(struct xc_dom_image *dom)
     return 0;
 }
 
-static elf_errorstatus xc_dom_load_elf_symtab(struct xc_dom_image *dom,
-                                  struct elf_binary *elf, bool load)
-{
-    struct elf_binary syms;
-    ELF_HANDLE_DECL(elf_shdr) shdr; ELF_HANDLE_DECL(elf_shdr) shdr2;
-    xen_vaddr_t symtab, maxaddr;
-    elf_ptrval hdr;
-    size_t size;
-    unsigned h, count, type, i, tables = 0;
-    unsigned long *strtab_referenced = NULL;
-
-    if ( elf_swap(elf) )
-    {
-        DOMPRINTF("%s: non-native byte order, bsd symtab not supported",
-                  __FUNCTION__);
-        return 0;
-    }
-
-    size = elf->bsd_symtab_pend - elf->bsd_symtab_pstart;
-
-    if ( load )
-    {
-        char *hdr_ptr;
-        size_t allow_size;
-
-        if ( !dom->bsd_symtab_start )
-            return 0;
-        hdr_ptr = xc_dom_vaddr_to_ptr(dom, dom->bsd_symtab_start, &allow_size);
-        if ( hdr_ptr == NULL )
-        {
-            DOMPRINTF("%s: xc_dom_vaddr_to_ptr(dom,dom->bsd_symtab_start"
-                      " => NULL", __FUNCTION__);
-            return -1;
-        }
-        elf->caller_xdest_base = hdr_ptr;
-        elf->caller_xdest_size = allow_size;
-        hdr = ELF_REALPTR2PTRVAL(hdr_ptr);
-        elf_store_val(elf, unsigned, hdr, size - sizeof(unsigned));
-    }
-    else
-    {
-        char *hdr_ptr;
-
-        hdr_ptr = xc_dom_malloc(dom, size);
-        if ( hdr_ptr == NULL )
-            return 0;
-        elf->caller_xdest_base = hdr_ptr;
-        elf->caller_xdest_size = size;
-        hdr = ELF_REALPTR2PTRVAL(hdr_ptr);
-        dom->bsd_symtab_start = elf_round_up(elf, dom->kernel_seg.vend);
-        dom->kernel_seg.vend = elf_round_up(elf, dom->bsd_symtab_start + size);
-        return 0;
-    }
-
-    elf_memcpy_safe(elf, hdr + sizeof(unsigned),
-           ELF_IMAGE_BASE(elf),
-           elf_size(elf, elf->ehdr));
-    elf_memcpy_safe(elf, hdr + sizeof(unsigned) + elf_size(elf, elf->ehdr),
-           ELF_IMAGE_BASE(elf) + elf_uval(elf, elf->ehdr, e_shoff),
-           elf_shdr_count(elf) * elf_size(elf, shdr));
-    if ( elf_64bit(elf) )
-    {
-        Elf64_Ehdr *ehdr = (Elf64_Ehdr *)(hdr + sizeof(unsigned));
-        ehdr->e_phoff = 0;
-        ehdr->e_phentsize = 0;
-        ehdr->e_phnum = 0;
-        ehdr->e_shoff = elf_size(elf, elf->ehdr);
-        ehdr->e_shstrndx = SHN_UNDEF;
-    }
-    else
-    {
-        Elf32_Ehdr *ehdr = (Elf32_Ehdr *)(hdr + sizeof(unsigned));
-        ehdr->e_phoff = 0;
-        ehdr->e_phentsize = 0;
-        ehdr->e_phnum = 0;
-        ehdr->e_shoff = elf_size(elf, elf->ehdr);
-        ehdr->e_shstrndx = SHN_UNDEF;
-    }
-    if ( elf->caller_xdest_size < sizeof(unsigned) )
-    {
-        DOMPRINTF("%s: header size %"PRIx64" too small",
-                  __FUNCTION__, (uint64_t)elf->caller_xdest_size);
-        return -1;
-    }
-    if ( elf_init(&syms, elf->caller_xdest_base + sizeof(unsigned),
-                  elf->caller_xdest_size - sizeof(unsigned)) )
-        return -1;
-
-    /*
-     * The caller_xdest_{base,size} and dest_{base,size} need to
-     * remain valid so long as each struct elf_image does.  The
-     * principle we adopt is that these values are set when the
-     * memory is allocated or mapped, and cleared when (and if)
-     * they are unmapped.
-     *
-     * Mappings of the guest are normally undone by xc_dom_unmap_all
-     * (directly or via xc_dom_release).  We do not explicitly clear
-     * these because in fact that happens only at the end of
-     * xc_dom_boot_image, at which time all of these ELF loading
-     * functions have returned.  No relevant struct elf_binary*
-     * escapes this file.
-     */
-
-    xc_elf_set_logfile(dom->xch, &syms, 1);
-
-    symtab = dom->bsd_symtab_start + sizeof(unsigned);
-    maxaddr = elf_round_up(&syms, symtab + elf_size(&syms, syms.ehdr) +
-                           elf_shdr_count(&syms) * elf_size(&syms, shdr));
-
-    DOMPRINTF("%s: bsd_symtab_start=%" PRIx64 ", kernel.end=0x%" PRIx64
-              " -- symtab=0x%" PRIx64 ", maxaddr=0x%" PRIx64 "",
-              __FUNCTION__, dom->bsd_symtab_start, dom->kernel_seg.vend,
-              symtab, maxaddr);
-
-    count = elf_shdr_count(&syms);
-    /* elf_shdr_count guarantees that count is reasonable */
-
-    strtab_referenced = xc_dom_malloc(dom, bitmap_size(count));
-    if ( strtab_referenced == NULL )
-        return -1;
-    bitmap_clear(strtab_referenced, count);
-    /* Note the symtabs @h linked to by any strtab @i. */
-    for ( i = 0; i < count; i++ )
-    {
-        shdr2 = elf_shdr_by_index(&syms, i);
-        if ( elf_uval(&syms, shdr2, sh_type) == SHT_SYMTAB )
-        {
-            h = elf_uval(&syms, shdr2, sh_link);
-            if (h < count)
-                set_bit(h, strtab_referenced);
-        }
-    }
-
-    for ( h = 0; h < count; h++ )
-    {
-        shdr = elf_shdr_by_index(&syms, h);
-        if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
-            /* input has an insane section header count field */
-            break;
-        type = elf_uval(&syms, shdr, sh_type);
-        if ( type == SHT_STRTAB )
-        {
-            /* Skip symtab @h if we found no corresponding strtab @i. */
-            if ( !test_bit(h, strtab_referenced) )
-            {
-                if ( elf_64bit(&syms) )
-                    elf_store_field(elf, shdr, e64.sh_offset, 0);
-                else
-                    elf_store_field(elf, shdr, e32.sh_offset, 0);
-                continue;
-            }
-        }
-
-        if ( (type == SHT_STRTAB) || (type == SHT_SYMTAB) )
-        {
-            /* Mangled to be based on ELF header location. */
-            if ( elf_64bit(&syms) )
-                elf_store_field(elf, shdr, e64.sh_offset, maxaddr - symtab);
-            else
-                elf_store_field(elf, shdr, e32.sh_offset, maxaddr - symtab);
-            size = elf_uval(&syms, shdr, sh_size);
-            maxaddr = elf_round_up(&syms, maxaddr + size);
-            tables++;
-            DOMPRINTF("%s: h=%u %s, size=0x%zx, maxaddr=0x%" PRIx64 "",
-                      __FUNCTION__, h,
-                      type == SHT_SYMTAB ? "symtab" : "strtab",
-                      size, maxaddr);
-
-            shdr2 = elf_shdr_by_index(elf, h);
-            elf_memcpy_safe(elf, elf_section_start(&syms, shdr),
-                   elf_section_start(elf, shdr2),
-                   size);
-        }
-
-        /* Name is NULL. */
-        if ( elf_64bit(&syms) )
-            elf_store_field(elf, shdr, e64.sh_name, 0);
-        else
-            elf_store_field(elf, shdr, e32.sh_name, 0);
-    }
-
-    if ( elf_check_broken(&syms) )
-        DOMPRINTF("%s: symbols ELF broken: %s", __FUNCTION__,
-                  elf_check_broken(&syms));
-    if ( elf_check_broken(elf) )
-        DOMPRINTF("%s: ELF broken: %s", __FUNCTION__,
-                  elf_check_broken(elf));
-
-    if ( tables == 0 )
-    {
-        DOMPRINTF("%s: no symbol table present", __FUNCTION__);
-        dom->bsd_symtab_start = 0;
-        return 0;
-    }
-
-    return 0;
-}
-
 static elf_errorstatus xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
     /*
      * This function sometimes returns -1 for error and sometimes
@@ -385,9 +187,6 @@ static elf_errorstatus xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
     dom->kernel_seg.vstart = dom->parms.virt_kstart;
     dom->kernel_seg.vend   = dom->parms.virt_kend;
 
-    if ( dom->parms.bsd_symtab )
-        xc_dom_load_elf_symtab(dom, elf, 0);
-
     dom->guest_type = xc_dom_guest_type(dom, elf);
     DOMPRINTF("%s: %s: 0x%" PRIx64 " -> 0x%" PRIx64 "",
               __FUNCTION__, dom->guest_type,
@@ -422,8 +221,6 @@ static elf_errorstatus xc_dom_load_elf_kernel(struct xc_dom_image *dom)
         DOMPRINTF("%s: failed to load elf binary", __FUNCTION__);
         return rc;
     }
-    if ( dom->parms.bsd_symtab )
-        xc_dom_load_elf_symtab(dom, elf, 1);
     return 0;
 }
 
diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c
index 6f42bea..5875795 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -153,7 +153,7 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart)
 {
     uint64_t sz;
     ELF_HANDLE_DECL(elf_shdr) shdr;
-    unsigned i, type;
+    unsigned int i;
 
     if ( !ELF_HANDLE_VALID(elf->sym_tab) )
         return;
@@ -164,20 +164,33 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart)
     sz = sizeof(uint32_t);
 
     /* Space for the elf and elf section headers */
-    sz += (elf_uval(elf, elf->ehdr, e_ehsize) +
-           elf_shdr_count(elf) * elf_uval(elf, elf->ehdr, e_shentsize));
+    sz += elf_uval(elf, elf->ehdr, e_ehsize) +
+          3 * elf_uval(elf, elf->ehdr, e_shentsize);
     sz = elf_round_up(elf, sz);
 
-    /* Space for the symbol and string tables. */
+    /* Space for the symbol and string table. */
     for ( i = 0; i < elf_shdr_count(elf); i++ )
     {
         shdr = elf_shdr_by_index(elf, i);
         if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
             /* input has an insane section header count field */
             break;
-        type = elf_uval(elf, shdr, sh_type);
-        if ( (type == SHT_STRTAB) || (type == SHT_SYMTAB) )
-            sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size));
+
+        if ( elf_uval(elf, shdr, sh_type) != SHT_SYMTAB )
+            continue;
+
+        sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size));
+        shdr = elf_shdr_by_index(elf, elf_uval(elf, shdr, sh_link));
+
+        if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
+            /* input has an insane section header count field */
+            break;
+
+        if ( elf_uval(elf, shdr, sh_type) != SHT_STRTAB )
+            /* Invalid symtab -> strtab link */
+            break;
+
+        sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size));
     }
 
     elf->bsd_symtab_pstart = pstart;
@@ -186,79 +199,169 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart)
 
 static void elf_load_bsdsyms(struct elf_binary *elf)
 {
-    ELF_HANDLE_DECL(elf_ehdr) sym_ehdr;
-    unsigned long sz;
-    elf_ptrval maxva;
-    elf_ptrval symbase;
-    elf_ptrval symtab_addr;
-    ELF_HANDLE_DECL(elf_shdr) shdr;
-    unsigned i, type;
+    /*
+     * Header that is placed at the end of the kernel and allows
+     * the OS to find where the symtab and strtab have been loaded.
+     * It mimics a valid ELF file header, although it only contains
+     * a symtab and a strtab section.
+     *
+     * NB: according to the ELF spec there's only ONE symtab per ELF
+     * file, and accordingly we will only load the corresponding
+     * strtab, so we only need three section headers in our fake ELF
+     * header (first section header is always a dummy).
+     */
+    struct {
+        uint32_t size;
+        struct {
+            elf_ehdr header;
+            elf_shdr section[3];
+        } __attribute__((packed)) elf_header;
+    } __attribute__((packed)) header;
+
+    ELF_HANDLE_DECL(elf_ehdr) header_handle;
+    unsigned long shdr_size;
+    ELF_HANDLE_DECL(elf_shdr) section_handle;
+    ELF_HANDLE_DECL(elf_shdr) image_handle;
+    unsigned int i, link;
+    elf_ptrval header_base;
+    elf_ptrval elf_header_base;
+    elf_ptrval symtab_base;
+    elf_ptrval strtab_base;
 
     if ( !elf->bsd_symtab_pstart )
         return;
 
-#define elf_hdr_elm(_elf, _hdr, _elm, _val)     \
-do {                                            \
-    if ( elf_64bit(_elf) )                      \
-        elf_store_field(_elf, _hdr, e64._elm, _val);  \
-    else                                        \
-        elf_store_field(_elf, _hdr, e32._elm, _val);  \
+#define elf_store_field_bitness(_elf, _hdr, _elm, _val)             \
+do {                                                                \
+    if ( elf_64bit(_elf) )                                          \
+        elf_store_field(_elf, _hdr, e64._elm, _val);                \
+    else                                                            \
+        elf_store_field(_elf, _hdr, e32._elm, _val);                \
 } while ( 0 )
 
-    symbase = elf_get_ptr(elf, elf->bsd_symtab_pstart);
-    symtab_addr = maxva = symbase + sizeof(uint32_t);
-
-    /* Set up Elf header. */
-    sym_ehdr = ELF_MAKE_HANDLE(elf_ehdr, symtab_addr);
-    sz = elf_uval(elf, elf->ehdr, e_ehsize);
-    elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(sym_ehdr), ELF_HANDLE_PTRVAL(elf->ehdr), sz);
-    maxva += sz; /* no round up */
+#define SYMTAB_INDEX    1
+#define STRTAB_INDEX    2
 
-    elf_hdr_elm(elf, sym_ehdr, e_phoff, 0);
-    elf_hdr_elm(elf, sym_ehdr, e_shoff, elf_uval(elf, elf->ehdr, e_ehsize));
-    elf_hdr_elm(elf, sym_ehdr, e_phentsize, 0);
-    elf_hdr_elm(elf, sym_ehdr, e_phnum, 0);
+    /* Allow elf_memcpy_safe to write to symbol_header. */
+    elf->caller_xdest_base = &header;
+    elf->caller_xdest_size = sizeof(header);
 
-    /* Copy Elf section headers. */
-    shdr = ELF_MAKE_HANDLE(elf_shdr, maxva);
-    sz = elf_shdr_count(elf) * elf_uval(elf, elf->ehdr, e_shentsize);
-    elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(shdr),
-                    ELF_IMAGE_BASE(elf) + elf_uval(elf, elf->ehdr, e_shoff),
-                    sz);
-    maxva = elf_round_up(elf, (unsigned long)maxva + sz);
+    /*
+     * Calculate the position of the various elements in GUEST MEMORY SPACE.
+     * This addresses MUST only be used with elf_load_image.
+     *
+     * NB: strtab_base cannot be calculated at this point because we don't
+     * know the size of the symtab yet, and the strtab will be placed after it.
+     */
+    header_base = elf_get_ptr(elf, elf->bsd_symtab_pstart);
+    elf_header_base = elf_get_ptr(elf, elf->bsd_symtab_pstart) +
+                      sizeof(uint32_t);
+    symtab_base = elf_round_up(elf, header_base + sizeof(header));
+
+    /* Fill the ELF header, copied from the original ELF header. */
+    header_handle = ELF_MAKE_HANDLE(elf_ehdr,
+                                ELF_REALPTR2PTRVAL(&header.elf_header.header));
+    elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(header_handle),
+                    ELF_HANDLE_PTRVAL(elf->ehdr),
+                    elf_uval(elf, elf->ehdr, e_ehsize));
+
+    /* Set the offset to the shdr array. */
+    elf_store_field_bitness(elf, header_handle, e_shoff,
+                            offsetof(typeof(header.elf_header), section));
+
+    /* Set the right number of section headers. */
+    elf_store_field_bitness(elf, header_handle, e_shnum, 3);
+
+    /* Clear a couple of fields we don't use. */
+    elf_store_field_bitness(elf, header_handle, e_phoff, 0);
+    elf_store_field_bitness(elf, header_handle, e_phentsize, 0);
+    elf_store_field_bitness(elf, header_handle, e_phnum, 0);
+
+    /* Zero the dummy section. */
+    section_handle = ELF_MAKE_HANDLE(elf_shdr,
+                     ELF_REALPTR2PTRVAL(&header.elf_header.section[SHN_UNDEF]));
+    shdr_size = elf_uval(elf, elf->ehdr, e_shentsize);
+    elf_memset_safe(elf, ELF_HANDLE_PTRVAL(section_handle), 0, shdr_size);
 
+    /*
+     * Find the actual symtab and strtab in the ELF.
+     *
+     * The symtab section header is going to reside in section[SYMTAB_INDEX],
+     * while the corresponding strtab is going to be placed in
+     * section[STRTAB_INDEX]. sh_offset is mangled so it points to the offset
+     * where the sections are actually loaded (relative to the ELF header
+     * location).
+     */
+    section_handle = ELF_MAKE_HANDLE(elf_shdr,
+                ELF_REALPTR2PTRVAL(&header.elf_header.section[SYMTAB_INDEX]));
     for ( i = 0; i < elf_shdr_count(elf); i++ )
     {
-        elf_ptrval old_shdr_p;
-        elf_ptrval new_shdr_p;
 
-        type = elf_uval(elf, shdr, sh_type);
-        if ( (type == SHT_STRTAB) || (type == SHT_SYMTAB) )
+        image_handle = elf_shdr_by_index(elf, i);
+        if ( elf_uval(elf, image_handle, sh_type) != SHT_SYMTAB )
+            continue;
+
+        elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(section_handle),
+                        ELF_HANDLE_PTRVAL(image_handle),
+                        shdr_size);
+
+        link = elf_uval(elf, section_handle, sh_link);
+        if ( link == SHN_UNDEF )
         {
-             elf_msg(elf, "%s: shdr %i at 0x%"ELF_PRPTRVAL" -> 0x%"ELF_PRPTRVAL"\n", __func__, i,
-                     elf_section_start(elf, shdr), maxva);
-             sz = elf_uval(elf, shdr, sh_size);
-             elf_memcpy_safe(elf, maxva, elf_section_start(elf, shdr), sz);
-             /* Mangled to be based on ELF header location. */
-             elf_hdr_elm(elf, shdr, sh_offset, maxva - symtab_addr);
-             maxva = elf_round_up(elf, (unsigned long)maxva + sz);
+            elf_mark_broken(elf, "bad link in symtab");
+            break;
         }
-        old_shdr_p = ELF_HANDLE_PTRVAL(shdr);
-        new_shdr_p = old_shdr_p + elf_uval(elf, elf->ehdr, e_shentsize);
-        if ( new_shdr_p <= old_shdr_p ) /* wrapped or stuck */
+
+        /* Load symtab into guest memory. */
+        elf_load_image(elf, symtab_base, elf_section_start(elf, section_handle),
+                       elf_uval(elf, section_handle, sh_size),
+                       elf_uval(elf, section_handle, sh_size));
+        elf_store_field_bitness(elf, section_handle, sh_offset,
+                                symtab_base - elf_header_base);
+        elf_store_field_bitness(elf, section_handle, sh_link,
+                                STRTAB_INDEX);
+
+        /* Calculate the guest address where strtab is loaded. */
+        strtab_base = elf_round_up(elf, symtab_base +
+                                   elf_uval(elf, section_handle, sh_size));
+
+        /* Load strtab section header. */
+        section_handle = ELF_MAKE_HANDLE(elf_shdr,
+                ELF_REALPTR2PTRVAL(&header.elf_header.section[STRTAB_INDEX]));
+        elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(section_handle),
+                        ELF_HANDLE_PTRVAL(elf_shdr_by_index(elf, link)),
+                        shdr_size);
+
+        if ( elf_uval(elf, section_handle, sh_type) != SHT_STRTAB )
         {
-            elf_mark_broken(elf, "bad section header length");
+            elf_mark_broken(elf, "strtab not found");
             break;
         }
-        if ( !elf_access_ok(elf, new_shdr_p, 1) ) /* outside image */
-            break;
-        shdr = ELF_MAKE_HANDLE(elf_shdr, new_shdr_p);
+
+        /* Load strtab into guest memory. */
+        elf_load_image(elf, strtab_base, elf_section_start(elf, section_handle),
+                       elf_uval(elf, section_handle, sh_size),
+                       elf_uval(elf, section_handle, sh_size));
+        elf_store_field_bitness(elf, section_handle, sh_offset,
+                                strtab_base - elf_header_base);
+
+        /* Store the whole size (including headers and loaded sections). */
+        header.size = strtab_base + elf_uval(elf, section_handle, sh_size) -
+                      elf_header_base;
+        break;
     }
 
-    /* Write down the actual sym size. */
-    elf_store_val(elf, uint32_t, symbase, maxva - symtab_addr);
+    /* Load the headers. */
+    elf_load_image(elf, header_base, ELF_REALPTR2PTRVAL(&header),
+                   sizeof(header), sizeof(header));
+
+    /* Remove permissions from elf_memcpy_safe. */
+    elf->caller_xdest_base = NULL;
+    elf->caller_xdest_size = 0;
 
-#undef elf_ehdr_elm
+#undef SYMTAB_INDEX
+#undef STRTAB_INDEX
+#undef elf_store_field_bitness
 }
 
 void elf_parse_binary(struct elf_binary *elf)
-- 
2.5.4 (Apple Git-61)


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