Re: [PATCH v1 07/11] xsplice: Implement payload loading

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Nov 03, 2015 at 06:16:04PM +0000, Ross Lagerwall wrote:
> Add support for loading xsplice payloads. This is somewhat similar to
> the Linux kernel module loader, implementing the following steps:
> - Verify the elf file.
> - Parse the elf file.
> - Allocate a region of memory mapped within a free area of
>   [xen_virt_end, XEN_VIRT_END].
> - Copy allocated sections into the new region.
> - Resolve section symbols. All other symbols must be absolute addresses.
> - Perform relocations.
> - Process xsplice specific sections.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  xen/arch/arm/Makefile             |   1 +
>  xen/arch/arm/xsplice.c            |  23 ++++
>  xen/arch/x86/Makefile             |   1 +
>  xen/arch/x86/setup.c              |   7 +
>  xen/arch/x86/xsplice.c            |  90 ++++++++++++
>  xen/common/xsplice.c              | 282 ++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-x86/x86_64/page.h |   2 +
>  xen/include/xen/xsplice.h         |  22 +++
>  8 files changed, 428 insertions(+)
>  create mode 100644 xen/arch/arm/xsplice.c
>  create mode 100644 xen/arch/x86/xsplice.c
> 
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 1ef39f7..f785c07 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -39,6 +39,7 @@ obj-y += device.o
>  obj-y += decode.o
>  obj-y += processor.o
>  obj-y += smc.o
> +obj-y += xsplice.o
>  
>  #obj-bin-y += ....o
>  
> diff --git a/xen/arch/arm/xsplice.c b/xen/arch/arm/xsplice.c
> new file mode 100644
> index 0000000..8d85fa9
> --- /dev/null
> +++ b/xen/arch/arm/xsplice.c
> @@ -0,0 +1,23 @@
> +#include <xen/lib.h>
> +#include <xen/errno.h>
> +#include <xen/xsplice_elf.h>
> +#include <xen/xsplice.h>
> +
> +int xsplice_verify_elf(uint8_t *data, ssize_t len)
> +{
> +    return -ENOSYS;
> +}
> +
> +int xsplice_perform_rel(struct xsplice_elf *elf,
> +                        struct xsplice_elf_sec *base,
> +                        struct xsplice_elf_sec *rela)
> +{
> +    return -ENOSYS;
> +}
> +
> +int xsplice_perform_rela(struct xsplice_elf *elf,
> +                         struct xsplice_elf_sec *base,
> +                         struct xsplice_elf_sec *rela)
> +{
> +    return -ENOSYS;
> +}
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 39a8059..6e05532 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -61,6 +61,7 @@ obj-y += x86_emulate.o
>  obj-y += tboot.o
>  obj-y += hpet.o
>  obj-y += vm_event.o
> +obj-y += xsplice.o
>  obj-y += xstate.o
>  
>  obj-$(crash_debug) += gdbstub.o
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 4ed0110..a79c5e3 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -99,6 +99,9 @@ unsigned long __read_mostly xen_phys_start;
>  
>  unsigned long __read_mostly xen_virt_end;
>  
> +unsigned long __read_mostly module_virt_start;
> +unsigned long __read_mostly module_virt_end;
> +
>  DEFINE_PER_CPU(struct tss_struct, init_tss);
>  
>  char __section(".bss.stack_aligned") cpu0_stack[STACK_SIZE];
> @@ -1145,6 +1148,10 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>                     ~((1UL << L2_PAGETABLE_SHIFT) - 1);
>      destroy_xen_mappings(xen_virt_end, XEN_VIRT_START + BOOTSTRAP_MAP_BASE);
>  
> +    module_virt_start = xen_virt_end;
> +    module_virt_end = XEN_VIRT_END - NR_CPUS * PAGE_SIZE;
> +    BUG_ON(module_virt_end <= module_virt_start);
> +
>      memguard_init();
>  
>      nr_pages = 0;
> diff --git a/xen/arch/x86/xsplice.c b/xen/arch/x86/xsplice.c
> new file mode 100644
> index 0000000..dbff0d5
> --- /dev/null
> +++ b/xen/arch/x86/xsplice.c
> @@ -0,0 +1,90 @@

You would want to put Citrix regular Copyright header here.
> +#include <xen/lib.h>
> +#include <xen/errno.h>
> +#include <xen/xsplice_elf.h>
> +#include <xen/xsplice.h>
> +
> +int xsplice_verify_elf(uint8_t *data, ssize_t len)
> +{
> +
> +    Elf64_Ehdr *hdr = (Elf64_Ehdr *)data;
> +
> +    if ( len < (sizeof *hdr) ||
> +         !IS_ELF(*hdr) ||
> +         hdr->e_ident[EI_CLASS] != ELFCLASS64 ||
> +         hdr->e_ident[EI_DATA] != ELFDATA2LSB ||
> +         hdr->e_machine != EM_X86_64 )
> +    {
> +        printk(XENLOG_ERR "Invalid ELF file\n");

For audit reasons I think we should have at least the name (id) that
the payload was. Could we include that as argument and
print it here?

> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +int xsplice_perform_rel(struct xsplice_elf *elf,
> +                        struct xsplice_elf_sec *base,
> +                        struct xsplice_elf_sec *rela)
> +{
> +    printk(XENLOG_ERR "SHT_REL relocation unsupported\n");

%s: SHR_REL relocation ..\n", elf->name);

> +    return -ENOSYS;
> +}
> +
> +int xsplice_perform_rela(struct xsplice_elf *elf,
> +                         struct xsplice_elf_sec *base,
> +                         struct xsplice_elf_sec *rela)
> +{
> +    Elf64_Rela *r;
> +    int symndx, i;

unsigned int

> +    uint64_t val;
> +    uint8_t *dest;
> +

Can you double check that rela->sec-sh_entsize is not zero first?

> +    for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
> +    {
> +        r = (Elf64_Rela *)(rela->data + i * rela->sec->sh_entsize);

Can you check that that 'r' is not past the memory allocated for 'data'?

> +        symndx = ELF64_R_SYM(r->r_info);
> +        dest = base->load_addr + r->r_offset;
> +        val = r->r_addend + elf->sym[symndx].sym->st_value;

Can you check that 'symndx' is not past the what we allocated for elf->sym?

> +
> +        switch ( ELF64_R_TYPE(r->r_info) )
> +        {
> +            case R_X86_64_NONE:
> +                break;
> +            case R_X86_64_64:
> +                *(uint64_t *)dest = val;
> +                break;
> +            case R_X86_64_32:
> +                *(uint32_t *)dest = val;
> +                if (val != *(uint32_t *)dest)
> +                    goto overflow;
> +                break;
> +            case R_X86_64_32S:
> +                *(int32_t *)dest = val;
> +                if ((int64_t)val != *(int32_t *)dest)
> +                    goto overflow;
> +                break;
> +            case R_X86_64_PLT32:
> +                /*
> +                 * Xen uses -fpic which normally uses PLT relocations
> +                 * except that it sets visibility to hidden which means
> +                 * that they are not used.  However, when gcc cannot
> +                 * inline memcpy it emits memcpy with default visibility
> +                 * which then creates a PLT relocation.  It can just be
> +                 * treated the same as R_X86_64_PC32.
> +                 */
> +                /* Fall through */
> +            case R_X86_64_PC32:
> +                *(uint32_t *)dest = val - (uint64_t)dest;
> +                break;
> +            default:
> +                printk(XENLOG_ERR "Unhandled relocation %lu\n",
> +                       ELF64_R_TYPE(r->r_info));
> +                return -EINVAL;
> +        }
> +    }
> +
> +    return 0;
> +
> + overflow:
> +    printk(XENLOG_ERR "Overflow in relocation %d in %s\n", i, rela->name);
> +    return -EOVERFLOW;
> +}
> diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
> index d984c8a..5e88c55 100644
> --- a/xen/common/xsplice.c
> +++ b/xen/common/xsplice.c
> @@ -12,6 +12,7 @@
>  #include <xen/stdbool.h>
>  #include <xen/sched.h>
>  #include <xen/lib.h>
> +#include <xen/xsplice_elf.h>
>  #include <xen/xsplice.h>
>  #include <public/sysctl.h>
>  
> @@ -29,9 +30,15 @@ struct payload {
>  
>      struct list_head   list;   /* Linked to 'payload_list'. */
>  
> +    void *module_address;
> +    size_t module_pages;
> +
>      char  id[XEN_XSPLICE_NAME_SIZE + 1];          /* Name of it. */
>  };
>  
> +static int load_module(struct payload *payload, uint8_t *raw, ssize_t len);
> +static void free_module(struct payload *payload);
> +
>  static const char *state2str(int32_t state)
>  {
>  #define STATE(x) [XSPLICE_STATE_##x] = #x
> @@ -140,6 +147,7 @@ static void __free_payload(struct payload *data)
>      list_del(&data->list);
>      payload_cnt --;
>      payload_version ++;
> +    free_module(data);
>      xfree(data);
>  }
>  
> @@ -178,6 +186,10 @@ static int xsplice_upload(xen_sysctl_xsplice_upload_t *upload)
>      if ( copy_from_guest(raw_data, upload->payload, upload->size) )
>          goto err_raw;
>  
> +    rc = load_module(data, raw_data, upload->size);
> +    if ( rc )
> +        goto err_raw;
> +
>      data->state = XSPLICE_STATE_LOADED;
>      data->rc = 0;
>      INIT_LIST_HEAD(&data->list);
> @@ -390,6 +402,276 @@ int xsplice_control(xen_sysctl_xsplice_op_t *xsplice)
>      return rc;
>  }
>  
> +
> +/*
> + * The following functions prepare an xSplice module to be executed by
> + * allocating space, loading the allocated sections, resolving symbols,
> + * performing relocations, etc.
> + */
> +#ifdef CONFIG_X86
> +static void *alloc_module(size_t size)

s/module/payload/
> +{
> +    mfn_t *mfn, *mfn_ptr;
> +    size_t pages, i;
> +    struct page_info *pg;
> +    unsigned long hole_start, hole_end, cur;
> +    struct payload *data, *data2;
> +
> +    ASSERT(size);
> +
> +    pages = PFN_UP(size);
> +    mfn = xmalloc_array(mfn_t, pages);
> +    if ( mfn == NULL )
> +        return NULL;
> +
> +    for ( i = 0; i < pages; i++ )
> +    {
> +        pg = alloc_domheap_page(NULL, 0);
> +        if ( pg == NULL )
> +            goto error;
> +        mfn[i] = _mfn(page_to_mfn(pg));
> +    }

This looks like 'vmalloc'. Why not use that?
(That explanation should be part of the commit description probably)

> +
> +    hole_start = (unsigned long)module_virt_start;
> +    hole_end = hole_start + pages * PAGE_SIZE;
> +    spin_lock(&payload_list_lock);
> +    list_for_each_entry ( data, &payload_list, list )
> +    {
> +        list_for_each_entry ( data2, &payload_list, list )
> +        {
> +            unsigned long start, end;
> +
> +            start = (unsigned long)data2->module_address;
> +            end = start + data2->module_pages * PAGE_SIZE;
> +            if ( hole_end > start && hole_start < end )
> +            {
> +                hole_start = end;
> +                hole_end = hole_start + pages * PAGE_SIZE;
> +                break;
> +            }
> +        }
> +        if ( &data2->list == &payload_list )
> +            break;
> +    }
> +    spin_unlock(&payload_list_lock);

This could be made in a nice function. 'find_hole' perhaps?

> +
> +    if ( hole_end >= module_virt_end )
> +        goto error;
> +
> +    for ( cur = hole_start, mfn_ptr = mfn; pages--; ++mfn_ptr, cur += PAGE_SIZE )
> +    {
> +        if ( map_pages_to_xen(cur, mfn_x(*mfn_ptr), 1, PAGE_HYPERVISOR_RWX) )
> +        {
> +            if ( cur != hole_start )
> +                destroy_xen_mappings(hole_start, cur);

I think 'destroy_xen_mappings' is OK handling hole_start == cur.

> +            goto error;
> +        }
> +    }
> +    xfree(mfn);
> +    return (void *)hole_start;
> +
> + error:
> +    while ( i-- )
> +        free_domheap_page(mfn_to_page(mfn_x(mfn[i])));
> +    xfree(mfn);
> +    return NULL;
> +}
> +#else
> +static void *alloc_module(size_t size)

s/module/payload/
> +{
> +    return NULL;
> +}
> +#endif
> +
> +static void free_module(struct payload *payload)
> +{
> +    int i;

unsigned int;

> +    struct page_info *pg;
> +    PAGE_LIST_HEAD(pg_list);
> +    void *va = payload->module_address;
> +    unsigned long addr = (unsigned long)va;
> +
> +    if ( !payload->module_address )
> +        return;

How about 'if ( !addr )
		return;
?

> +
> +    payload->module_address = NULL;
> +
> +    for ( i = 0; i < payload->module_pages; i++ )
> +        page_list_add(vmap_to_page(va + i * PAGE_SIZE), &pg_list);
> +
> +    destroy_xen_mappings(addr, addr + payload->module_pages * PAGE_SIZE);
> +
> +    while ( (pg = page_list_remove_head(&pg_list)) != NULL )
> +        free_domheap_page(pg);
> +
> +    payload->module_pages = 0;
> +}
> +
> +static void alloc_section(struct xsplice_elf_sec *sec, size_t *core_size)

s/alloc/compute/?

> +{
> +    size_t align_size = ROUNDUP(*core_size, sec->sec->sh_addralign);
> +    sec->sec->sh_entsize = align_size;
> +    *core_size = sec->sec->sh_size + align_size;
> +}
> +
> +static int move_module(struct payload *payload, struct xsplice_elf *elf)
> +{
> +    uint8_t *buf;
> +    int i;

unsigned int i;

> +    size_t core_size = 0;
> +
> +    /* Allocate text regions */

s/Allocate/Compute/

> +    for ( i = 0; i < elf->hdr->e_shnum; i++ )
> +    {
> +        if ( (elf->sec[i].sec->sh_flags & (SHF_ALLOC|SHF_EXECINSTR)) ==
> +             (SHF_ALLOC|SHF_EXECINSTR) )
> +            alloc_section(&elf->sec[i], &core_size);
> +    }
> +
> +    /* Allocate rw data */
> +    for ( i = 0; i < elf->hdr->e_shnum; i++ )
> +    {
> +        if ( (elf->sec[i].sec->sh_flags & SHF_ALLOC) &&
> +             !(elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&
> +             (elf->sec[i].sec->sh_flags & SHF_WRITE) )
> +            alloc_section(&elf->sec[i], &core_size);
> +    }
> +
> +    /* Allocate ro data */
> +    for ( i = 0; i < elf->hdr->e_shnum; i++ )
> +    {
> +        if ( (elf->sec[i].sec->sh_flags & SHF_ALLOC) &&
> +             !(elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&
> +             !(elf->sec[i].sec->sh_flags & SHF_WRITE) )
> +            alloc_section(&elf->sec[i], &core_size);
> +    }
> +
> +    buf = alloc_module(core_size);
> +    if ( !buf ) {
> +        printk(XENLOG_ERR "Could not allocate memory for module\n");

(%s: Could not allocate %u memory for payload!\n", elf->name, core_size);

> +        return -ENOMEM;
> +    }
> +    memset(buf, 0, core_size);

Perhaps for fun it ought to be 'ud2' ?

> +
> +    for ( i = 0; i < elf->hdr->e_shnum; i++ )
> +    {
> +        if ( elf->sec[i].sec->sh_flags & SHF_ALLOC )
> +        {
> +            elf->sec[i].load_addr = buf + elf->sec[i].sec->sh_entsize;
> +            memcpy(elf->sec[i].load_addr, elf->sec[i].data,
> +                   elf->sec[i].sec->sh_size);
> +            printk(XENLOG_DEBUG "Loaded %s at 0x%p\n",

Add %s: at the start .. 
> +                   elf->sec[i].name, elf->sec[i].load_addr);

which would be elf->name.

> +        }
> +    }
> +
> +    payload->module_address = buf;
> +    payload->module_pages = PFN_UP(core_size);

Instead of module could we name it payload?

> +
> +    return 0;
> +}
> +
> +static int resolve_symbols(struct xsplice_elf *elf)

s/resolve/check/

> +{
> +    int i;

unsigned int;

> +
> +    for ( i = 1; i < elf->nsym; i++ )

Why 1? Please explain as comment.


> +    {
> +        switch ( elf->sym[i].sym->st_shndx )
> +        {
> +            case SHN_COMMON:
> +                printk(XENLOG_ERR "Unexpected common symbol: %s\n",
> +                       elf->sym[i].name);

Please also include elf->name in the error.

> +                return -EINVAL;
> +                break;
> +            case SHN_UNDEF:
> +                printk(XENLOG_ERR "Unknown symbol: %s\n", elf->sym[i].name);

Ditto.
> +                return -ENOENT;
> +                break;
> +            case SHN_ABS:
> +                printk(XENLOG_DEBUG "Absolute symbol: %s => 0x%p\n",
> +                       elf->sym[i].name, (void *)elf->sym[i].sym->st_value);
> +                break;
> +            default:
> +                if ( elf->sec[elf->sym[i].sym->st_shndx].sec->sh_flags & SHF_ALLOC )
> +                {
> +                    elf->sym[i].sym->st_value +=
> +                        (unsigned long)elf->sec[elf->sym[i].sym->st_shndx].load_addr;
> +                    printk(XENLOG_DEBUG "Symbol resolved: %s => 0x%p\n",

Ditto;
> +                           elf->sym[i].name, (void *)elf->sym[i].sym->st_value);
> +                }
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +static int perform_relocs(struct xsplice_elf *elf)
> +{
> +    struct xsplice_elf_sec *rela, *base;
> +    int i, rc;
> +

unsigned int i;

> +    for ( i = 0; i < elf->hdr->e_shnum; i++ )
> +    {
> +        rela = &elf->sec[i];
> +
> +        /* Is it a valid relocation section? */
> +        if ( rela->sec->sh_info >= elf->hdr->e_shnum )
> +            continue;

Um, don't we want to mark it as invalid or such?
Or overwrite it so we won't use it?

> +
> +        base = &elf->sec[rela->sec->sh_info];
> +
> +        /* Don't relocate non-allocated sections */
> +        if ( !(base->sec->sh_flags & SHF_ALLOC) )
> +            continue;

> +
> +        if ( elf->sec[i].sec->sh_type == SHT_RELA )
> +            rc = xsplice_perform_rela(elf, base, rela);
> +        else if ( elf->sec[i].sec->sh_type == SHT_REL )
> +            rc = xsplice_perform_rel(elf, base, rela);
> +
> +        if ( rc )
> +            return rc;
> +    }
> +
> +    return 0;
> +}
> +
> +static int load_module(struct payload *payload, uint8_t *raw, ssize_t len)
> +{
> +    struct xsplice_elf elf;

Wait a minute? We ditch it after this?

> +    int rc = 0;
> +
> +    rc = xsplice_verify_elf(raw, len);
> +    if ( rc )
> +        return rc;
> +
> +    rc = xsplice_elf_load(&elf, raw, len);
> +    if ( rc )
> +        return rc;
> +
> +    rc = move_module(payload, &elf);
> +    if ( rc )
> +        goto err_elf;
> +
> +    rc = resolve_symbols(&elf);
> +    if ( rc )
> +        goto err_module;
> +
> +    rc = perform_relocs(&elf);
> +    if ( rc )
> +        goto err_module;
> +

Shouldn't you call xsplice_elf_free(&elf) here? Or
hook up the elf to the 'struct payload'?


If not, who is going to clean up elf->sec and elf->sym when the
payload is unloaded?
> +    return 0;
> +
> + err_module:
> +    free_module(payload);
> + err_elf:
> +    xsplice_elf_free(&elf);
> +
> +    return rc;
> +}
> +
>  static int __init xsplice_init(void)
>  {
>      register_keyhandler('x', xsplice_printall, "print xsplicing info", 1);
> diff --git a/xen/include/asm-x86/x86_64/page.h b/xen/include/asm-x86/x86_64/page.h
> index 19ab4d0..e6f08e9 100644
> --- a/xen/include/asm-x86/x86_64/page.h
> +++ b/xen/include/asm-x86/x86_64/page.h
> @@ -38,6 +38,8 @@
>  #include <xen/pdx.h>
>  
>  extern unsigned long xen_virt_end;
> +extern unsigned long module_virt_start;
> +extern unsigned long module_virt_end;
>  
>  #define spage_to_pdx(spg) (((spg) - spage_table)<<(SUPERPAGE_SHIFT-PAGE_SHIFT))
>  #define pdx_to_spage(pdx) (spage_table + ((pdx)>>(SUPERPAGE_SHIFT-PAGE_SHIFT)))
> diff --git a/xen/include/xen/xsplice.h b/xen/include/xen/xsplice.h
> index 41e28da..a3946a3 100644
> --- a/xen/include/xen/xsplice.h
> +++ b/xen/include/xen/xsplice.h
> @@ -1,9 +1,31 @@
>  #ifndef __XEN_XSPLICE_H__
>  #define __XEN_XSPLICE_H__
>  
> +struct xsplice_elf;
> +struct xsplice_elf_sec;
> +struct xsplice_elf_sym;
> +
> +struct xsplice_patch_func {
> +    unsigned long new_addr;
> +    unsigned long new_size;
> +    unsigned long old_addr;
> +    unsigned long old_size;
> +    char *name;
> +    unsigned char undo[8];
> +};

We don't use them in this patch. They could be moved to another patch.
> +
>  struct xen_sysctl_xsplice_op;
>  int xsplice_control(struct xen_sysctl_xsplice_op *);
>  
>  extern void xsplice_printall(unsigned char key);
>  
> +/* Arch hooks */
> +int xsplice_verify_elf(uint8_t *data, ssize_t len);
> +int xsplice_perform_rel(struct xsplice_elf *elf,
> +                        struct xsplice_elf_sec *base,
> +                        struct xsplice_elf_sec *rela);
> +int xsplice_perform_rela(struct xsplice_elf *elf,
> +                         struct xsplice_elf_sec *base,
> +                         struct xsplice_elf_sec *rela);
> +
>  #endif /* __XEN_XSPLICE_H__ */
> -- 
> 2.4.3
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel