xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5] libelf: rewrite symtab/strtab loading
@ 2016-03-01 11:59 Roger Pau Monne
  2016-03-01 12:50 ` Wei Liu
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Roger Pau Monne @ 2016-03-01 11:59 UTC (permalink / raw)
  To: xen-devel
  Cc: Wei Liu, Ian Jackson, Ian Campbell, Jan Beulich, Roger Pau Monne

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>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
---
Changes since v4:
 - Add a define that contains the number of sections.
 - Improve the comment to describe the memory layout.
 - Check that the sh_link field is 0 < sh_link < e_shnum.
 - Simplify some of the logic, since the SYMTAB section is already
   discovered by elf_init and it's handler stored in elf->sym_tab.
---
 tools/libxc/xc_dom_elfloader.c    | 203 --------------------------
 xen/common/libelf/libelf-loader.c | 300 ++++++++++++++++++++++++++++----------
 2 files changed, 226 insertions(+), 277 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..b98f5d0 100644
--- a/xen/common/libelf/libelf-loader.c
+++ b/xen/common/libelf/libelf-loader.c
@@ -21,6 +21,9 @@
 
 #include "libelf-private.h"
 
+/* Number of section header needed in order to fit the SYMTAB and STRTAB. */
+#define ELF_BSDSYM_SECTIONS 3
+
 /* ------------------------------------------------------------------------ */
 
 elf_errorstatus elf_init(struct elf_binary *elf, const char *image_input, size_t size)
@@ -151,12 +154,14 @@ static elf_errorstatus elf_load_image(struct elf_binary *elf, elf_ptrval dst, el
 /* Calculate the required additional kernel space for the elf image */
 void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t pstart)
 {
-    uint64_t sz;
+    uint64_t sz, sh_link;
     ELF_HANDLE_DECL(elf_shdr) shdr;
-    unsigned i, type;
 
     if ( !ELF_HANDLE_VALID(elf->sym_tab) )
+    {
+        elf_mark_broken(elf, "invalid ELF handle for symtab section");
         return;
+    }
 
     pstart = elf_round_up(elf, pstart);
 
@@ -164,101 +169,248 @@ 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) +
+          ELF_BSDSYM_SECTIONS * elf_uval(elf, elf->ehdr, e_shentsize);
     sz = elf_round_up(elf, sz);
 
+
     /* Space for the symbol and string tables. */
-    for ( i = 0; i < elf_shdr_count(elf); i++ )
+    sh_link = elf_uval(elf, elf->sym_tab, sh_link);
+    if ( sh_link == SHN_UNDEF || sh_link >= elf_shdr_count(elf) )
     {
-        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));
+        elf_mark_broken(elf, "invalid sh_link value (out-of-bounds)");
+        return;
     }
 
+    sz = elf_round_up(elf, sz + elf_uval(elf, elf->sym_tab, sh_size));
+
+    shdr = elf_shdr_by_index(elf, sh_link);
+
+    if ( !elf_access_ok(elf, ELF_HANDLE_PTRVAL(shdr), 1) )
+        /* input has an insane section header count field */
+        return;
+
+    if ( elf_uval(elf, shdr, sh_type) != SHT_STRTAB )
+        /* Invalid symtab -> strtab link */
+        return;
+
+    sz = elf_round_up(elf, sz + elf_uval(elf, shdr, sh_size));
+
     elf->bsd_symtab_pstart = pstart;
     elf->bsd_symtab_pend   = pstart + sz;
 }
 
 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. The layout in memory is the
+     * following:
+     *
+     * +------------------------+
+     * |                        |
+     * | KERNEL                 |
+     * |                        |
+     * +------------------------+ pend
+     * | ALIGNMENT              |
+     * +------------------------+ bsd_symtab_pstart
+     * |                        |
+     * | size                   | bsd_symtab_pend - bsd_symtab_pstart
+     * |                        |
+     * +------------------------+ bsd_symtab_pstart + 4
+     * |                        |
+     * | ELF header             |
+     * |                        |
+     * +------------------------+
+     * |                        |
+     * | ELF section header 0   | Undefined section header
+     * |                        |
+     * +------------------------+
+     * |                        |
+     * | ELF section header 1   | SYMTAB section header
+     * |                        |
+     * +------------------------+
+     * |                        |
+     * | ELF section header 2   | STRTAB section header
+     * |                        |
+     * +------------------------+
+     * |                        |
+     * | SYMTAB                 |
+     * |                        |
+     * +------------------------+
+     * |                        |
+     * | STRTAB                 |
+     * |                        |
+     * +------------------------+ bsd_symtab_pend
+     *
+     * 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 the undefined section).
+     */
+    struct {
+        uint32_t size;
+        struct {
+            elf_ehdr header;
+            elf_shdr section[ELF_BSDSYM_SECTIONS];
+        } __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;
+    unsigned int link, rc;
+    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 */
-
-    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);
-
-    /* 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);
-
-    for ( i = 0; i < elf_shdr_count(elf); i++ )
+#define SYMTAB_INDEX    1
+#define STRTAB_INDEX    2
+
+    /* Allow elf_memcpy_safe to write to symbol_header. */
+    elf->caller_xdest_base = &header;
+    elf->caller_xdest_size = sizeof(header);
+
+    /*
+     * 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, ELF_BSDSYM_SECTIONS);
+
+    /* 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 undefined 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);
+
+    /*
+     * 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]));
+
+    elf_memcpy_safe(elf, ELF_HANDLE_PTRVAL(section_handle),
+                    ELF_HANDLE_PTRVAL(elf->sym_tab),
+                    shdr_size);
+
+    link = elf_uval(elf, section_handle, sh_link);
+    if ( link == SHN_UNDEF || link >= elf_shdr_count(elf) )
+    {
+        elf_mark_broken(elf, "bad link in symtab");
+        return;
+    }
+
+    /* Load symtab into guest memory. */
+    rc = 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));
+    if ( rc != 0 )
     {
-        elf_ptrval old_shdr_p;
-        elf_ptrval new_shdr_p;
+        elf_mark_broken(elf, "unable to load symtab into guest memory");
+        return;
+    }
 
-        type = elf_uval(elf, shdr, sh_type);
-        if ( (type == SHT_STRTAB) || (type == SHT_SYMTAB) )
-        {
-             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);
-        }
-        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 */
-        {
-            elf_mark_broken(elf, "bad section header length");
-            break;
-        }
-        if ( !elf_access_ok(elf, new_shdr_p, 1) ) /* outside image */
-            break;
-        shdr = ELF_MAKE_HANDLE(elf_shdr, new_shdr_p);
+    /* Adjust the sh_offset and sh_link of the copied section header. */
+    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, "strtab not found");
+        return;
+    }
+
+    /* Load strtab into guest memory. */
+    rc = 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));
+    if ( rc != 0 )
+    {
+        elf_mark_broken(elf, "unable to load strtab into guest memory");
+        return;
     }
 
-    /* Write down the actual sym size. */
-    elf_store_val(elf, uint32_t, symbase, maxva - symtab_addr);
+    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;
+
+    /* Load the headers. */
+    rc = elf_load_image(elf, header_base, ELF_REALPTR2PTRVAL(&header),
+                        sizeof(header), sizeof(header));
+    if ( rc != 0 )
+    {
+        elf_mark_broken(elf, "unable to load ELF headers into guest memory");
+        return;
+    }
+
+    /* 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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v5] libelf: rewrite symtab/strtab loading
  2016-03-01 11:59 [PATCH v5] libelf: rewrite symtab/strtab loading Roger Pau Monne
@ 2016-03-01 12:50 ` Wei Liu
  2016-03-01 18:25 ` Ian Jackson
  2016-03-02 16:39 ` Jan Beulich
  2 siblings, 0 replies; 5+ messages in thread
From: Wei Liu @ 2016-03-01 12:50 UTC (permalink / raw)
  To: Roger Pau Monne
  Cc: xen-devel, Ian Jackson, Ian Campbell, Jan Beulich, Wei Liu

On Tue, Mar 01, 2016 at 12:59:50PM +0100, Roger Pau Monne wrote:
> 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>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Jan Beulich <JBeulich@suse.com>

I will defer this to someone who is more familiar with ELF stuff. Let me
know if anyone thinks my review is required.

Wei.

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v5] libelf: rewrite symtab/strtab loading
  2016-03-01 11:59 [PATCH v5] libelf: rewrite symtab/strtab loading Roger Pau Monne
  2016-03-01 12:50 ` Wei Liu
@ 2016-03-01 18:25 ` Ian Jackson
  2016-03-02 11:53   ` Jan Beulich
  2016-03-02 16:39 ` Jan Beulich
  2 siblings, 1 reply; 5+ messages in thread
From: Ian Jackson @ 2016-03-01 18:25 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: xen-devel, Wei Liu, Jan Beulich

Roger Pau Monne writes ("[PATCH v5] libelf: rewrite symtab/strtab loading"):
> 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.

I haven't checked this for correctness.  This symtab/strtab stuff is
for the benefit of BSD and I trust you to have checked that it works
:-).

I have however checked that your new code seems to follow the
conventions in libelf.h which are intended to stop wild pointer
accesses, signed integer overflow, and other UB.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Jan, or other HV folks, do you want to review/ack this or shall I just
commit it ?

Ian.

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v5] libelf: rewrite symtab/strtab loading
  2016-03-01 18:25 ` Ian Jackson
@ 2016-03-02 11:53   ` Jan Beulich
  0 siblings, 0 replies; 5+ messages in thread
From: Jan Beulich @ 2016-03-02 11:53 UTC (permalink / raw)
  To: Ian Jackson; +Cc: xen-devel, Wei Liu, Roger Pau Monne

>>> On 01.03.16 at 19:25, <Ian.Jackson@eu.citrix.com> wrote:
> Roger Pau Monne writes ("[PATCH v5] libelf: rewrite symtab/strtab loading"):
>> 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.
> 
> I haven't checked this for correctness.  This symtab/strtab stuff is
> for the benefit of BSD and I trust you to have checked that it works
> :-).
> 
> I have however checked that your new code seems to follow the
> conventions in libelf.h which are intended to stop wild pointer
> accesses, signed integer overflow, and other UB.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Jan, or other HV folks, do you want to review/ack this or shall I just
> commit it ?

I'd like to take another look, after my review on v4.

Jan


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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v5] libelf: rewrite symtab/strtab loading
  2016-03-01 11:59 [PATCH v5] libelf: rewrite symtab/strtab loading Roger Pau Monne
  2016-03-01 12:50 ` Wei Liu
  2016-03-01 18:25 ` Ian Jackson
@ 2016-03-02 16:39 ` Jan Beulich
  2 siblings, 0 replies; 5+ messages in thread
From: Jan Beulich @ 2016-03-02 16:39 UTC (permalink / raw)
  To: Roger Pau Monne; +Cc: Ian Jackson, Wei Liu, xen-devel

>>> On 01.03.16 at 12:59, <roger.pau@citrix.com> wrote:
> Changes since v4:
>  - Add a define that contains the number of sections.
>  - Improve the comment to describe the memory layout.
>  - Check that the sh_link field is 0 < sh_link < e_shnum.
>  - Simplify some of the logic, since the SYMTAB section is already
>    discovered by elf_init and it's handler stored in elf->sym_tab.

Well, this was a nice idea, but ...

> @@ -164,101 +169,248 @@ 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) +
> +          ELF_BSDSYM_SECTIONS * elf_uval(elf, elf->ehdr, e_shentsize);
>      sz = elf_round_up(elf, sz);
>  
> +
>      /* Space for the symbol and string tables. */
> -    for ( i = 0; i < elf_shdr_count(elf); i++ )
> +    sh_link = elf_uval(elf, elf->sym_tab, sh_link);
> +    if ( sh_link == SHN_UNDEF || sh_link >= elf_shdr_count(elf) )

... this check then really ought to be moved there (as I now see
in the previous version you likely simply copied what was there).

Everything else looks fine to me now.

Jan


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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2016-03-02 16:39 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-01 11:59 [PATCH v5] libelf: rewrite symtab/strtab loading Roger Pau Monne
2016-03-01 12:50 ` Wei Liu
2016-03-01 18:25 ` Ian Jackson
2016-03-02 11:53   ` Jan Beulich
2016-03-02 16:39 ` Jan Beulich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).