Re: [Xen-devel] [PATCH v8.1 11/27] xsplice: Implement payload loading

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 04/14/16 12:04 AM >>>
>--- a/xen/arch/x86/Makefile
>+++ b/xen/arch/x86/Makefile
>@@ -64,6 +64,7 @@ obj-y += vm_event.o
 >obj-y += xstate.o
 >
 >obj-$(crash_debug) += gdbstub.o
>+obj-$(CONFIG_XSPLICE) += xsplice.o
 
 This should be added in the main section, somewhere ahead of the xstate.o
still visible in context.

>+int arch_xsplice_perform_rela(struct xsplice_elf *elf,
>+                              const struct xsplice_elf_sec *base,
>+                              const struct xsplice_elf_sec *rela)
>+{
>+    const Elf_RelA *r;
>+    unsigned int symndx, i;
>+    uint64_t val;
>+    uint8_t *dest;
>+
>+    if ( !rela->sec->sh_entsize || !rela->sec->sh_size ||
>+         rela->sec->sh_entsize != sizeof(Elf_RelA) )

< and again missing the sh_size % sh_entsize check. Plus an empty relocation
section is in no way invalid.

>+    for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
>+    {
>+        r = rela->data + i * rela->sec->sh_entsize;
>+        if ( (unsigned long)r > (unsigned long)(elf->hdr + elf->len) )
>+        {
>+            dprintk(XENLOG_ERR, XSPLICE "%s: Relative entry %u in %s is past 
>end!\n",
>+                    elf->name, i, rela->name);

With sections having got verified already, I don't see the point of this check.

>+        symndx = ELF64_R_SYM(r->r_info);
>+        if ( symndx > elf->nsym )

>=

>+        {
>+            dprintk(XENLOG_ERR, XSPLICE "%s: Relative symbol wants symbol@%u 
>which is past end!\n",

"Relative relocation wants ..."

>+        dest = base->load_addr + r->r_offset;

I'm sure I've commented on this before: What if r_offset points outside the
section being relocated?

>+        val = r->r_addend + elf->sym[symndx].sym->st_value;
>+
>+        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_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 */

Same here, just to repeat an earlier review comment: This second comment isn't
really useful.

>+        case R_X86_64_PC32:
>+            val -= (uint64_t)dest;
>+            *(uint32_t *)dest = val;

int32_t

>+            if ( (s64)val != *(s32 *)dest )

int64_t and int32_t please (for consistency); I actually doubt the cast on the 
left
side is really needed at all.

>+void *arch_xsplice_alloc_payload(unsigned int pages)
>+{
>+    unsigned int i;
>+    void *p;
>+
>+    ASSERT(pages);
>+
>+    p = vmalloc_xen(pages * PAGE_SIZE);
>+    WARN_ON(!p);

Why? If at all, a dprintk() in the else to ...

>+    if ( p )
>+    {
>+        /* By default they are PAGE_HYPERVISOR aka PAGE_HYPERVISOR_RWX.*/
>+        for ( i = 0; i < pages; i++ )
>+            clear_page(p + (i * PAGE_SIZE) );
>+    }

... this.

>+int arch_xsplice_secure(void *va, unsigned int pages, enum va_type type)

const void *va

>+{
>+    unsigned long start = (unsigned long)va;
>+    int flag;

unsigned int flags;

>+
>+    ASSERT(va);
>+    ASSERT(pages);
>+
>+    if ( type == XSPLICE_VA_RX ) /* PAGE_HYPERVISOR_RX */
>+        flag = _PAGE_PRESENT;
>+    else if ( type == XSPLICE_VA_RW ) /* PAGE_HYPERVISOR_RW */
>+        flag = _PAGE_RW | _PAGE_NX | _PAGE_PRESENT;
>+    else /* PAGE_HYPERVISOR_RO */
>+        flag = _PAGE_NX | _PAGE_PRESENT;

According to the most recent version of that patch, shouldn't you use the 
values in
the comments here instead of the open coded variants?

>+void arch_xsplice_free_payload(void *va)
>+{
>+    vfree_xen(va);
>+}

What is the idea behind having this hook (instead of generic code just calling
vfree_xen() [or really just vfree()])?

>@@ -29,6 +30,13 @@ struct payload {
     >uint32_t state;                      /* One of the XSPLICE_STATE_*. */
     >int32_t rc;                          /* 0 or -XEN_EXX. */
     >struct list_head list;               /* Linked to 'payload_list'. */
>+    void *text_addr;                     /* Virtual address of .text. */
>+    size_t text_size;                    /* .. and its size. */
>+    void *rw_addr;                       /* Virtual address of .data. */
>+    size_t rw_size;                      /* .. and its size (if any). */
>+    void *ro_addr;                       /* Virtual address of .rodata. */
>+    size_t ro_size;                      /* .. and its size (if any). */

And again the question: Do these pointers really need to be non-const?

>+ size_t pages; /* Total pages for [text,rw,ro]_addr */

Why size_t and not just unsigned int?

>+static void calc_section(struct xsplice_elf_sec *sec, size_t *size)
>+{
>+    Elf_Shdr *s = sec->sec;
>+    size_t align_size;
>+
>+    align_size = ROUNDUP(*size, s->sh_addralign);
>+    s->sh_entsize = align_size;

So this is one of the places (the only one?) where the section header gets
altered. Are you not expecting problems down the road resulting from
overwriting this field? After all it's used not just in control sections...

>+static int move_payload(struct payload *payload, struct xsplice_elf *elf)
>+{
>+    uint8_t *buf;
>+    unsigned int i;
>+    size_t size = 0;
>+
>+    /* Compute text regions. */
>+    for ( i = 1; i < elf->hdr->e_shnum; i++ )
>+    {
>+        if ( (elf->sec[i].sec->sh_flags & (SHF_ALLOC|SHF_EXECINSTR)) ==
>+             (SHF_ALLOC|SHF_EXECINSTR) )
>+            calc_section(&elf->sec[i], &payload->text_size);
>+    }
>+
>+    /* Compute rw data. */
>+    for ( i = 1; 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) )
>+            calc_section(&elf->sec[i], &payload->rw_size);
>+    }
>+
>+    /* Compute ro data. */
>+    for ( i = 1; 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) )
>+            calc_section(&elf->sec[i], &payload->ro_size);
>+    }
>+
>+    /* Do not accept wx. */
>+    for ( i = 1; 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) )
>+        {
>+            dprintk(XENLOG_ERR, XSPLICE "%s: No WX sections!\n", elf->name);
>+            return -EINVAL;
>+        }

Is there any reason to have four loops here, with quite a bit of redundancy in
the if()s, instead of just one loop with a if/else sequence?

Also it's not really clear whether you really mean to honor non-progbits, non-
nobits sections with SHF_ALLOC set. Perhaps such would better be refused
for the now at least.

>+    for ( i = 1; i < elf->hdr->e_shnum; i++ )
>+    {
>+        if ( elf->sec[i].sec->sh_flags & SHF_ALLOC )
>+        {
>+            if ( (elf->sec[i].sec->sh_flags & SHF_EXECINSTR) )
>+                 buf = payload->text_addr;
>+            else if ( (elf->sec[i].sec->sh_flags & SHF_WRITE) )
>+                buf = payload->rw_addr;
>+             else

Something's wrong with indentation here (not visible above anymore due to
the limitations of this web frontend of our mail system).

>+            /* Don't copy NOBITS - such as BSS. */
>+            if ( elf->sec[i].sec->sh_type != SHT_NOBITS )
>+            {
>+                memcpy(elf->sec[i].load_addr, elf->sec[i].data,
>+                       elf->sec[i].sec->sh_size);
>+                dprintk(XENLOG_DEBUG, XSPLICE "%s: Loaded %s at 0x%p\n",
>+                        elf->name, elf->sec[i].name, elf->sec[i].load_addr);
>+            }

"else memset();" is what I would have expected here. Now I see that the
allocation function clears the pages (in a bogusly open coded way, instead
of using vzalloc()), but why is that so?

>+int xsplice_elf_resolve_symbols(struct xsplice_elf *elf)
>+{
>+    unsigned int i;
>+    int rc = 0;
>+
>+    /*
>+     * The first entry of an ELF symbol table is the "undefined symbol index".
>+     * aka reserved so we skip it.
>+     */
>+    ASSERT(elf->sym);
>+
>+    for ( i = 1; i < elf->nsym; i++ )
>+    {
>+        uint16_t idx = elf->sym[i].sym->st_shndx;

Any reason not to use unsigned int here?

>+        rc = 0;

rc is already zero whenever you get here afaict.

>+        default:
>+            /* SHN_COMMON and SHN_ABS are above. */
>+            if ( idx > SHN_LORESERVE )

>=

>+                rc = -EOPNOTSUPP;
>+            /* SHN_UNDEF (0) above. */
>+            else if ( idx > elf->hdr->e_shnum && idx < SHN_LORESERVE )

>= and the right side of the && seems pointless due to the preceding if().

>+            if ( !(elf->sec[idx].sec->sh_flags & SHF_ALLOC) )
>+                break;

If you really mean to check this, shouldn't this be done earlier, avoiding 
needless
errors on unsupported symbol kinds above?

>+int xsplice_elf_perform_relocs(struct xsplice_elf *elf)
>+{
>+    struct xsplice_elf_sec *rela, *base;
>+    unsigned int i;
>+    int rc = 0;
>+
>+    /*
>+     * The first entry of an ELF symbol table is the "undefined symbol index".
>+     * aka reserved so we skip it.
>+     */

This comment seems at least misplaced, if not pointless.

>+    ASSERT(elf->sym);
>+
>+    for ( i = 1; i < elf->hdr->e_shnum; i++ )
>+    {
>+        rela = &elf->sec[i];
>+
>+        if ( (rela->sec->sh_type != SHT_RELA) &&
>+             (rela->sec->sh_type != SHT_REL) )
>+            continue;

With this the variable name "rela" is bogus (as it suggests RELA only).

>@@ -497,6 +499,8 @@ typedef struct {
 >
 >#define AuxInfo               Aux32Info
 >#elif defined(ELFSIZE) && (ELFSIZE == 64)
>+#define PRIxElfAddr   "lx"

PRIx64?

>--- a/xen/include/xen/xsplice.h
>+++ b/xen/include/xen/xsplice.h
>@@ -6,6 +6,9 @@
 >#ifndef __XEN_XSPLICE_H__
 >#define __XEN_XSPLICE_H__
 >
>+struct xsplice_elf;
>+struct xsplice_elf_sec;
>+struct xsplice_elf_sym;
 >struct xen_sysctl_xsplice_op;
 >
 >#ifdef CONFIG_XSPLICE
>@@ -15,6 +18,32 @@ struct xen_sysctl_xsplice_op;
 >
 >int xsplice_op(struct xen_sysctl_xsplice_op *);
 >
>+/* Arch hooks. */
>+int arch_xsplice_verify_elf(const struct xsplice_elf *elf);
>+int arch_xsplice_perform_rel(struct xsplice_elf *elf,
>+                             const struct xsplice_elf_sec *base,
>+                             const struct xsplice_elf_sec *rela);
>+int arch_xsplice_perform_rela(struct xsplice_elf *elf,
>+                              const struct xsplice_elf_sec *base,
>+                              const struct xsplice_elf_sec *rela);
>+enum va_type {
>+    XSPLICE_VA_RX, /* .text */
>+    XSPLICE_VA_RW, /* .data */
>+    XSPLICE_VA_RO, /* .rodata */
>+};
>+
>+#include <xen/mm.h>

Why at all? And if needed, why placed in the middle?

>@@ -29,8 +31,10 @@ struct xsplice_elf {
     >struct xsplice_elf_sec *sec;         /* Array of sections, allocated by 
us. */
     >struct xsplice_elf_sym *sym;         /* Array of symbols , allocated by 
us. */
     >unsigned int nsym;
>-    const struct xsplice_elf_sec *symtab;/* Pointer to .symtab section - aka 
>to sec[x]. */
>+    const struct xsplice_elf_sec *symtab;/* Pointer to .symtab section - aka 
>to
>+                                            sec[symtab_idx]. */
     >const struct xsplice_elf_sec *strtab;/* Pointer to .strtab section - aka 
to sec[y]. */

With the adjustment above - who is y? (But perhaps this needs taking care of in
an earlier patch.)

Jan


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