|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v1 07/11] xsplice: Implement payload loading
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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |